[Enhancement] Add structural limits for deserialization filter

[RyanL1997]

Description

Follow-up to #5469. Bounds the size and shape of the object graph produced during deserialization in DeserializationFilterUtil with structural limits (maxdepth, maxrefs, maxbytes), and logs structural-limit rejections (depth/refs/bytes) alongside the existing class-rejection logging.

The limits are exposed as dynamic cluster settings so they can be tuned at runtime without a code change:

Setting	Default
plugins.query.deserialization.max_depth	20
plugins.query.deserialization.max_refs	1000
plugins.query.deserialization.max_bytes	15000

The values are resolved from Settings at the call sites (the pagination path receives Settings through Planner/DefaultImplementor; the script-engine paths receive a lazily-resolved Supplier<Settings> because the script engine is constructed before plugin settings are initialized). createFilter falls back to the defaults when no Settings is available (serialize-only call sites and tests).

Related Issues

Follow-up to #5469

Check List

• New functionality includes testing.
• Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit bc9bc19)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The v2ExpressionScriptEngine field is now instance-scoped instead of static, but the code still uses it as if it were shared. If multiple CompoundedScriptEngine instances are created with different settingsSupplier values, each will have its own v2ExpressionScriptEngine with potentially different settings. This could lead to inconsistent deserialization behavior across script contexts if the plugin creates multiple engine instances. private final ExpressionScriptEngine v2ExpressionScriptEngine; private final CalciteScriptEngine calciteScriptEngine; public CompoundedScriptEngine() { this(null); } public CompoundedScriptEngine(Supplier<Settings> settingsSupplier) { this.v2ExpressionScriptEngine = new ExpressionScriptEngine(new DefaultExpressionSerializer(settingsSupplier));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to bc9bc19

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Apply filter before stream consumption The deserialization filter is applied after readAllBytes() has already consumed the entire stream. This means structural limits (maxdepth, maxrefs, maxbytes) are checked too late to prevent resource exhaustion. Move the filter setup before reading the stream to enforce limits during deserialization. core/src/main/java/org/opensearch/sql/executor/pagination/PlanSerializer.java [99-114] +ObjectInputStream objectInput = new CursorDeserializationStream(gzip); objectInput.setObjectInputFilter( settings == null ? DeserializationFilterUtil.createFilter(additionalPatterns) : DeserializationFilterUtil.createFilter(settings, additionalPatterns)); +return (Serializable) objectInput.readObject(); Suggestion importance[1-10]: 9 __ Why: Critical security issue. The filter is applied after readAllBytes() consumes the stream, making structural limits ineffective against resource exhaustion attacks. The filter must be set before reading to enforce limits during deserialization.	High
	Tighten default structural limits The default structural limits may be too permissive for production environments. Consider lowering DEFAULT_MAX_DEPTH to 10-15 and DEFAULT_MAX_REFS to 500-750 to reduce the attack surface for deserialization exploits while still supporting legitimate use cases. core/src/main/java/org/opensearch/sql/utils/DeserializationFilterUtil.java [54-57] -public static final int DEFAULT_MAX_DEPTH = 20; +public static final int DEFAULT_MAX_DEPTH = 15; -public static final int DEFAULT_MAX_REFS = 1000; +public static final int DEFAULT_MAX_REFS = 750; public static final int DEFAULT_MAX_BYTES = 15000; Suggestion importance[1-10]: 5 __ Why: Valid security consideration, but the suggested values are arbitrary without evidence that current defaults are exploitable. The PR already makes limits configurable, allowing administrators to adjust based on their security requirements.	Low
General	Handle settings supplier exceptions gracefully If settingsSupplier.get() throws an exception (e.g., settings not yet initialized), deserialization will fail without a clear error message. Wrap the supplier invocation in a try-catch block and fall back to default limits with a logged warning to improve resilience. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/serde/DefaultExpressionSerializer.java [54-58] -Settings settings = settingsSupplier == null ? null : settingsSupplier.get(); +Settings settings = null; +try { + settings = settingsSupplier == null ? null : settingsSupplier.get(); +} catch (Exception e) { + // Log warning and fall back to defaults +} objectInput.setObjectInputFilter( settings == null ? DeserializationFilterUtil.createFilter("") : DeserializationFilterUtil.createFilter(settings, "")); Suggestion importance[1-10]: 7 __ Why: Good defensive programming practice. If settingsSupplier.get() throws an exception, the current code will fail without clear context. Adding exception handling with fallback to defaults improves resilience, though the suggestion should include logging.	Medium

---

Previous suggestions

Suggestions up to commit 76943f3

Category	Suggestion	Impact
Security	Validate positive limit values Add validation to ensure the retrieved limit values are positive. Negative or zero values could bypass security restrictions and allow unbounded deserialization, creating a critical vulnerability. core/src/main/java/org/opensearch/sql/utils/DeserializationFilterUtil.java [103-109] private static int limit(Settings settings, Settings.Key key, int defaultValue) { if (settings == null) { return defaultValue; } Integer value = settings.getSettingValue(key); - return value == null ? defaultValue : value; + int result = value == null ? defaultValue : value; + if (result <= 0) { + LOG.warn("Invalid limit value {} for key {}, using default {}", result, key, defaultValue); + return defaultValue; + } + return result; } Suggestion importance[1-10]: 9 __ Why: This is a critical security issue. Without validation, negative or zero values could bypass deserialization restrictions, allowing unbounded object graphs that could lead to DoS attacks or memory exhaustion. The suggestion correctly identifies the vulnerability and provides proper validation with logging.	High

Suggestions up to commit a70d0db

Category	Suggestion	Impact
Security	Validate positive structural limit values Add validation to ensure the returned limit value is positive. Negative or zero values could bypass deserialization protections entirely, creating a critical security vulnerability. Validate that the configured value meets the minimum threshold (>= 1) before returning it. core/src/main/java/org/opensearch/sql/utils/DeserializationFilterUtil.java [103-109] private static int limit(Settings settings, Settings.Key key, int defaultValue) { if (settings == null) { return defaultValue; } Integer value = settings.getSettingValue(key); - return value == null ? defaultValue : value; + int result = value == null ? defaultValue : value; + if (result < 1) { + throw new IllegalArgumentException( + String.format("Setting %s must be at least 1, got: %d", key, result)); + } + return result; } Suggestion importance[1-10]: 9 __ Why: This is a critical security issue. Without validation, negative or zero values could completely disable deserialization protections, allowing unbounded object graphs that could lead to DoS attacks or memory exhaustion. The OpenSearch settings already enforce minimum values of 1, but runtime validation provides defense-in-depth.	High
General	Close ObjectInputStream to prevent leaks Close the ObjectInputStream resource to prevent resource leaks. The stream is opened but never explicitly closed, which can lead to file descriptor exhaustion under high load. Use try-with-resources to ensure proper cleanup. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/serde/DefaultExpressionSerializer.java [50-59] public Expression deserialize(String code) { try { ByteArrayInputStream input = new ByteArrayInputStream(Base64.getDecoder().decode(code)); - ObjectInputStream objectInput = new ObjectInputStream(input); Settings settings = settingsSupplier == null ? null : settingsSupplier.get(); - objectInput.setObjectInputFilter(DeserializationFilterUtil.createFilter(settings, "")); - return (Expression) objectInput.readObject(); + try (ObjectInputStream objectInput = new ObjectInputStream(input)) { + objectInput.setObjectInputFilter(DeserializationFilterUtil.createFilter(settings, "")); + return (Expression) objectInput.readObject(); + } } catch (Exception e) { throw new IllegalStateException("Failed to deserialize expression code: " + code, e); } } Suggestion importance[1-10]: 7 __ Why: The ObjectInputStream should be closed to prevent resource leaks. While ByteArrayInputStream doesn't hold system resources, using try-with-resources is a best practice that ensures proper cleanup and makes the code more maintainable. This could lead to issues under high load.	Medium

Suggestions up to commit 1d89079

Category	Suggestion	Impact
Security	Validate positive limit values Add validation to ensure the returned limit value is positive. Negative or zero values from settings could bypass deserialization protections, creating a security vulnerability. core/src/main/java/org/opensearch/sql/utils/DeserializationFilterUtil.java [103-109] private static int limit(Settings settings, Settings.Key key, int defaultValue) { if (settings == null) { return defaultValue; } Integer value = settings.getSettingValue(key); - return value == null ? defaultValue : value; + int result = value == null ? defaultValue : value; + if (result <= 0) { + LOG.warn("Invalid limit value {} for key {}, using default {}", result, key, defaultValue); + return defaultValue; + } + return result; } Suggestion importance[1-10]: 8 __ Why: This is a critical security improvement. Without validation, negative or zero values from settings could disable deserialization protections entirely, creating a serious vulnerability. The suggestion correctly identifies the limit method and proposes adding validation to ensure only positive values are used.	Medium
General	Close ObjectInputStream to prevent leaks Close the ObjectInputStream resource to prevent resource leaks. Use try-with-resources to ensure proper cleanup even when exceptions occur during deserialization. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/serde/DefaultExpressionSerializer.java [50-59] public Expression deserialize(String code) { try { ByteArrayInputStream input = new ByteArrayInputStream(Base64.getDecoder().decode(code)); - ObjectInputStream objectInput = new ObjectInputStream(input); - Settings settings = settingsSupplier == null ? null : settingsSupplier.get(); - objectInput.setObjectInputFilter(DeserializationFilterUtil.createFilter(settings, "")); - return (Expression) objectInput.readObject(); + try (ObjectInputStream objectInput = new ObjectInputStream(input)) { + Settings settings = settingsSupplier == null ? null : settingsSupplier.get(); + objectInput.setObjectInputFilter(DeserializationFilterUtil.createFilter(settings, "")); + return (Expression) objectInput.readObject(); + } } catch (Exception e) { throw new IllegalStateException("Failed to deserialize expression code: " + code, e); } } Suggestion importance[1-10]: 7 __ Why: This is a valid resource management improvement. The ObjectInputStream should be closed to prevent resource leaks. Using try-with-resources ensures proper cleanup even when exceptions occur. However, the impact is moderate since the streams are relatively small and short-lived.	Medium
	Close streams to prevent leaks Close the GZIPInputStream and ObjectInputStream resources to prevent resource leaks. Use try-with-resources to ensure proper cleanup even when exceptions occur during deserialization. core/src/main/java/org/opensearch/sql/executor/pagination/PlanSerializer.java [97-109] protected Serializable deserialize(String code) { - try { - GZIPInputStream gzip = - new GZIPInputStream(new ByteArrayInputStream(HashCode.fromString(code).asBytes())); - ObjectInputStream objectInput = - new CursorDeserializationStream(new ByteArrayInputStream(gzip.readAllBytes())); + try (GZIPInputStream gzip = + new GZIPInputStream(new ByteArrayInputStream(HashCode.fromString(code).asBytes())); + ObjectInputStream objectInput = + new CursorDeserializationStream(new ByteArrayInputStream(gzip.readAllBytes()))) { objectInput.setObjectInputFilter( DeserializationFilterUtil.createFilter( settings, "org.opensearch.sql.planner.physical.*;" + "org.opensearch.sql.opensearch.storage.scan.*;" + "org.opensearch.sql.opensearch.data.type.*;" + "org.opensearch.sql.executor.pagination.*;" Suggestion importance[1-10]: 7 __ Why: This is a valid resource management improvement similar to suggestion 2. Both GZIPInputStream and ObjectInputStream should be closed to prevent resource leaks. Using try-with-resources ensures proper cleanup. The impact is moderate as these are deserialization operations that should complete quickly.	Medium

Suggestions up to commit 6143db0

Category	Suggestion	Impact
General	Make structural limits configurable The structural limits may be too restrictive for legitimate use cases. Consider making these limits configurable through system properties or configuration files to allow tuning based on deployment requirements without code changes. core/src/main/java/org/opensearch/sql/utils/DeserializationFilterUtil.java [50] -private static final String STRUCTURAL_LIMITS = "maxdepth=20;maxrefs=1000;maxbytes=15000;"; +private static final String STRUCTURAL_LIMITS = + String.format("maxdepth=%s;maxrefs=%s;maxbytes=%s;", + System.getProperty("opensearch.sql.deserialization.maxdepth", "20"), + System.getProperty("opensearch.sql.deserialization.maxrefs", "1000"), + System.getProperty("opensearch.sql.deserialization.maxbytes", "15000")); Suggestion importance[1-10]: 5 __ Why: The suggestion to make structural limits configurable is valid and could improve flexibility. However, the improved_code changes a static final String to be dynamically computed, which may have initialization order implications and doesn't match the original constant declaration pattern. The suggestion addresses a minor enhancement rather than a critical issue.	Low

Suggestions up to commit ec32115

Category	Suggestion	Impact
General	Make structural limits configurable The structural limits are hardcoded as constants. Consider making these configurable through system properties or configuration files to allow administrators to adjust limits based on their security requirements and application needs without code changes. core/src/main/java/org/opensearch/sql/utils/DeserializationFilterUtil.java [50] -private static final String STRUCTURAL_LIMITS = "maxdepth=20;maxrefs=300;maxbytes=15000;"; +private static final int DEFAULT_MAX_DEPTH = 20; +private static final int DEFAULT_MAX_REFS = 300; +private static final int DEFAULT_MAX_BYTES = 15000; +private static final String STRUCTURAL_LIMITS = String.format( + "maxdepth=%d;maxrefs=%d;maxbytes=%d;", + Integer.getInteger("opensearch.sql.deserialization.maxdepth", DEFAULT_MAX_DEPTH), + Integer.getInteger("opensearch.sql.deserialization.maxrefs", DEFAULT_MAX_REFS), + Integer.getInteger("opensearch.sql.deserialization.maxbytes", DEFAULT_MAX_BYTES) +); + Suggestion importance[1-10]: 5 __ Why: While making security limits configurable could be useful for flexibility, this suggestion introduces complexity and potential security risks if misconfigured. The current hardcoded approach is acceptable for security-critical constants. The suggestion is valid but represents a design choice rather than a bug fix.	Low

[github-actions]

Persistent review updated to latest commit ec32115

[github-actions]

Persistent review updated to latest commit 6143db0

[github-actions]

Persistent review updated to latest commit 1d89079

[github-actions]

Persistent review updated to latest commit a70d0db

[github-actions]

Persistent review updated to latest commit 76943f3

[github-actions]

Persistent review updated to latest commit bc9bc19