[Feature] Add PPL rest command

[noCharger]

Description

Add a leading rest <endpoint> command that exposes a curated, read-only, fixed-schema set of in-cluster management endpoints as a PPL table, modeled as a system row source bridged through visitRelation (the same seam as describe and the system-index family), so it runs on the Calcite engine without the unsupported table-function path.

• Grammar/AST: REST/TIMEOUT tokens, restCommand rule, RestRelation, AstBuilder visit, query anonymizer.
• Execution: RestSourceTable -> CalciteLogicalRestScan / CalciteEnumerableRestScan; RestEndpointRegistry (read-only allow-list + fixed schema + accepted args); RestEnumerator/RestRequest dispatch via OpenSearchClient (NodeClient in-cluster, RestClient standalone).
• 9 endpoints: cluster health/state/settings, cat indices/nodes/cluster_manager/ plugins/shards, resolve/index.
• Output shaping: numeric type normalization, id-to-name resolution, role-name expansion, structural flattening, graceful null degrade.
• Args: count caps emitted rows; timeout reserved but rejected with 400; get-args applied server-side with per-arg value validation (local on health, health on cat/indices, expand_wildcards on resolve/index). Undeclared arg or out-of-domain value is rejected with a 400. level and include_defaults are deferred to a later release; flat_settings is dropped as redundant.
• Error handling: blank endpoint, negative count, disallowed arg, and uncoercible value all surface clean 400s rather than 500s.

Response redaction, endpoint allow-list, and access control

Two node-level settings (NodeScope, set in opensearch.yml) let an operator harden the command; both default to native behavior, so existing clusters are unchanged. They are intentionally not dynamically updatable, so the hardening cannot be turned off at runtime via _cluster/settings or _plugins/_query/settings once an operator has set it.

• plugins.ppl.rest.redaction.enabled (Boolean, default false): when enabled, masks network identifiers in /_cat/* output — IPv4, IPv6, inet[/...] addresses, EC2-style host names (ip-a-b-c-d), and availability-zone names — and availability-zone names in /_cluster/settings values. Default false preserves the native _cat output (real values).
• plugins.ppl.rest.allowed_endpoints (List, default ["*"]): restricts which registered endpoints the command serves. ["*"] allows all (unchanged); an explicit subset allows only those (others return a clear "not enabled on this cluster" 400); [] disables the command entirely. Enforced at a single choke point in OpenSearchStorageEngine#getTable.

Access control. The command is already subject to the security plugin's fine-grained access control with no special integration. Each endpoint dispatches a standard transport action (for example cluster:monitor/nodes/stats, cluster:monitor/state, indices:admin/resolve/index) through the node client under the caller's identity, so the security ActionFilter authorizes every call by action name. A caller lacking the privilege is rejected, exactly as on the native endpoint, and /_resolve/index requires the indices:admin/resolve/index privilege because the command resolves all indices. Document- and field-level security do not apply because the command issues no document searches, which matches the native _cat and _cluster APIs. Consequently the default settings reproduce native behavior 1:1 — real IPs and all endpoints are visible only to callers who already hold the corresponding cluster:monitor/* privilege — and redaction and the allow-list are additional opt-in hardening beyond native for managed or restricted deployments.

Related Issues

Resolves #5597

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit aef4663.

Path	Line	Severity	Description
opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java	359	medium	The /_cluster/settings endpoint exposes all persistent and transient cluster settings via the transport layer. Although a SettingsFilter is applied (and the code correctly fails closed if the filter is uninitialized), only settings explicitly registered with Property.Filtered or a matching filter pattern are redacted. Sensitive operational settings (custom API keys, passwords, access tokens stored in cluster settings by other plugins) that are not marked filtered will be returned verbatim to any caller with cluster-monitor privilege via PPL, matching but also replicating the native REST endpoint's exposure.
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java	254	medium	The /_cat/nodes and /_cat/cluster_manager endpoints expose internal IP addresses (node.getHostAddress()), node names (which may be EC2 hostnames encoding subnet octets), and full resource utilization. The redaction feature (RestResponseRedactor) that would mask these identifiers is off by default (plugins.ppl.rest.redaction.enabled=false), meaning most deployments will expose internal network topology to any caller with cluster-monitor privilege, facilitating lateral-movement mapping.
opensearch/src/main/java/org/opensearch/sql/opensearch/setting/OpenSearchSettings.java	79	low	PPL_REST_ALLOWED_ENDPOINTS_SETTING defaults to List.of("*"), enabling all nine registered endpoints (including /_cluster/settings, /_cat/nodes, /_cat/plugins) without any operator opt-in. Operators must explicitly restrict the list to limit exposure. A secure default (empty list or a minimal subset) would better align with least-privilege, requiring positive opt-in to expose each endpoint.
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java	310	low	The /_cat/plugins endpoint returns the name, component, and version of every installed plugin on every node. Exposing the full plugin inventory to any user with cluster-monitor privilege enables targeted reconnaissance — an attacker can enumerate installed plugins and their exact versions to identify unpatched vulnerabilities or weakly-configured plugin extensions without needing direct node access.

The table above displays the top 10 most important findings.

Total: 4 | Critical: 0 | High: 0 | Medium: 2 | Low: 2

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit aef4663)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Sensitive information exposure: The /_cluster/settings endpoint can expose sensitive configuration values (API keys, passwords, secrets) if the SettingsFilter is not properly applied. The code attempts to mitigate this by requiring a published SettingsFilter (line 458-463 in OpenSearchNodeClient.java) and failing closed if the filter is unavailable. However, the filter is set via a static holder (RestSettingsFilterHolder) in SQLPlugin.getRestHandlers, which is called during plugin initialization. If a query arrives before this initialization completes, or if the holder is inadvertently cleared, the method will throw an IllegalStateException rather than leak secrets, which is the correct fail-safe behavior. The redaction logic for availability zones (line 109-110 in RestEndpointRegistry.java) is a secondary defense but relies on the primary SettingsFilter for comprehensive secret protection. The design is sound, but the reliance on a static holder and the timing of its initialization introduces a narrow window where the system's security posture depends on correct sequencing. Recommendation: ensure RestSettingsFilterHolder.set() is called as early as possible in the plugin lifecycle, and consider adding a startup check that verifies the filter is set before accepting queries.

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue In the toNumber method, parsing an empty string after trimming throws a NumberFormatException with message "empty string". However, the catch block in coerce (lines 379-391) expects IllegalArgumentException or ClassCastException. Since NumberFormatException extends IllegalArgumentException, it will be caught, but the explicit empty-string check at line 402-404 is redundant because Long.parseLong("") and Double.parseDouble("") already throw NumberFormatException for empty strings. The custom exception at line 403 will never be thrown because the parsing methods will throw first. /** Coerce a transport/JSON value to a Number, parsing numeric strings (e.g. the cat JSON API). */ private static Number toNumber(Object value) { if (value instanceof Number n) { return n; } String s = String.valueOf(value).trim(); if (s.isEmpty()) { throw new NumberFormatException("empty string"); } if (s.indexOf('.') >= 0 || s.indexOf('e') >= 0 || s.indexOf('E') >= 0) { return Double.parseDouble(s); } return Long.parseLong(s); } Possible Issue In clusterSettings method, the code retrieves settings from cluster state and then filters them. However, if the SettingsFilter is null (checked at line 460), the method throws an IllegalStateException. The issue is that RestSettingsFilterHolder.get() can return null if the filter was never set or was explicitly set to null via RestSettingsFilterHolder.set(null). While the code fails closed (refusing to return unredacted settings), there's a race condition window: if a query arrives before SQLPlugin.getRestHandlers publishes the filter, the query will fail with an unclear error rather than being rejected earlier in the request lifecycle. This is a narrow scenario but could cause confusion in a rapidly-starting cluster. public List<Map<String, Object>> clusterSettings(Map<String, String> params) { // The transport path has no SettingsFilter of its own; it is published from // SQLPlugin#getRestHandlers at startup. Fail closed before fetching so we never read settings // into memory when we cannot redact them, matching native GET /_cluster/settings. org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( "cluster settings redaction filter is not initialized; refusing to return unredacted" + " settings"); } Possible Issue The IPv4 regex pattern (line 20-21) uses word boundaries \b which may not match IPv4 addresses in all contexts. Specifically, \b requires a word/non-word transition, so an IPv4 at the start of a string or immediately following certain punctuation may not match as expected. For example, the pattern will match "10.0.0.1" in "ip:10.0.0.1" but the boundary behavior with colons and other punctuation should be verified. The tests show it works for common cases, but edge cases like "ip=10.0.0.1" (equals sign) or "[10.0.0.1]" (brackets) may behave unexpectedly depending on regex engine word-boundary rules. private static final String OCTET = "(25[0-5]|2[0-4]\\d|[0-1]?\\d\\d?)"; private static final Pattern IPV4 = Pattern.compile("\\b" + OCTET + "\\." + OCTET + "\\." + OCTET + "\\." + OCTET + "\\b");

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to aef4663

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Validate numeric range before type conversion The coercion logic may produce incorrect results when converting numeric types. Using intValue() on a Number can truncate values without validation. Verify that the numeric value is within the valid range for the target type before conversion to prevent silent data loss. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); } try { if (type == INTEGER) { - return integerValue(toNumber(value).intValue()); + Number num = toNumber(value); + if (num.longValue() < Integer.MIN_VALUE || num.longValue() > Integer.MAX_VALUE) { + throw new IllegalArgumentException("Value out of range for INTEGER"); + } + return integerValue(num.intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } } catch (IllegalArgumentException | ClassCastException e) { throw new IllegalArgumentException( "rest endpoint value for column [" + column + "] could not be coerced to " + type + ": [" + value + "]"); } return stringValue(String.valueOf(value)); } Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a real issue where intValue() truncation could silently lose data when converting numbers outside the INTEGER range. The proposed range check would catch this and throw a clear error. This is a valid improvement for data integrity, though the impact depends on whether the REST endpoints actually return out-of-range values in practice.	Medium
	Add exception handling for cluster state request The method should handle potential exceptions from the cluster state request. If the request fails or times out, the method will throw an uncaught exception. Wrap the actionGet() call in a try-catch block to provide a more informative error message to the caller. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-479] public List<Map<String, Object>> clusterSettings(Map<String, String> params) { org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( "cluster settings redaction filter is not initialized; refusing to return unredacted" + " settings"); } - org.opensearch.action.admin.cluster.state.ClusterStateResponse response = - client - .admin() - .cluster() - .state(new org.opensearch.action.admin.cluster.state.ClusterStateRequest()) - .actionGet(); - List<Map<String, Object>> rows = new java.util.ArrayList<>(); - org.opensearch.common.settings.Settings persistent = - filter.filter(response.getState().metadata().persistentSettings()); - org.opensearch.common.settings.Settings transientSettings = - filter.filter(response.getState().metadata().transientSettings()); - collectSettings(persistent, "persistent", rows); - collectSettings(transientSettings, "transient", rows); - return rows; + try { + org.opensearch.action.admin.cluster.state.ClusterStateResponse response = + client + .admin() + .cluster() + .state(new org.opensearch.action.admin.cluster.state.ClusterStateRequest()) + .actionGet(); + List<Map<String, Object>> rows = new java.util.ArrayList<>(); + org.opensearch.common.settings.Settings persistent = + filter.filter(response.getState().metadata().persistentSettings()); + org.opensearch.common.settings.Settings transientSettings = + filter.filter(response.getState().metadata().transientSettings()); + collectSettings(persistent, "persistent", rows); + collectSettings(transientSettings, "transient", rows); + return rows; + } catch (Exception e) { + throw new IllegalStateException("Failed to retrieve cluster settings", e); + } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the actionGet() call could throw exceptions. However, the existing code already throws IllegalStateException for other error conditions, and uncaught exceptions from actionGet() would propagate as runtime exceptions. The suggestion adds defensive error handling, which is a reasonable improvement but not critical since the method is already designed to fail fast on errors.	Low
	Optimize regex pattern application for redaction The redaction method applies multiple regex patterns sequentially, which can be inefficient for large strings. Consider compiling a single combined pattern or using a more efficient matching strategy to reduce the number of passes over the input text. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestResponseRedactor.java [43-52] public static String redact(String text) { if (text == null || text.isEmpty()) { return text; } - String out = text; + StringBuilder result = new StringBuilder(text.length()); + String remaining = text; for (Mask mask : MASKS) { - out = mask.pattern().matcher(out).replaceAll(mask.replacement()); + remaining = mask.pattern().matcher(remaining).replaceAll(mask.replacement()); } - return out; + return remaining; } Suggestion importance[1-10]: 3 __ Why: The suggestion claims to optimize the redaction method but the improved_code is functionally identical to the existing_code (just uses a different variable name). The suggestion correctly identifies that multiple sequential regex passes could be inefficient, but the proposed code doesn't actually implement any optimization like combining patterns or reducing passes.	Low

---

Previous suggestions

Suggestions up to commit f3a6226

Category	Suggestion	Impact
General	Validate filter before network call The fail-closed check for the settings filter should be performed before any cluster state request is made, not after. Move this validation to the beginning of the method to prevent unnecessary network calls when the filter is unavailable. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [458-464] +org.opensearch.common.settings.SettingsFilter filter = + org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( "cluster settings redaction filter is not initialized; refusing to return unredacted" + " settings"); } +org.opensearch.action.admin.cluster.state.ClusterStateResponse response = + client + .admin() + .cluster() + .state(new org.opensearch.action.admin.cluster.state.ClusterStateRequest()) + .actionGet(); Suggestion importance[1-10]: 7 __ Why: Moving the filter validation before the network call is a good optimization that prevents unnecessary cluster state requests when the filter is unavailable. However, this is a performance improvement rather than a correctness issue, as the current code still fails safely.	Medium
	Prevent potential overflow in truncation The count truncation logic should validate that spec.getCount() is not greater than Integer.MAX_VALUE to prevent potential overflow issues when used with subList. Add a bounds check to ensure safe list operations. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestRequest.java [44-51] @Override public List<ExprValue> search() { List<ExprValue> rows = endpoint.toRows(client, spec, redact); - if (spec.getCount() != null && spec.getCount() >= 0 && rows.size() > spec.getCount()) { - return rows.subList(0, spec.getCount()); + if (spec.getCount() != null && spec.getCount() >= 0) { + int count = Math.min(spec.getCount(), rows.size()); + if (count < rows.size()) { + return rows.subList(0, count); + } } return rows; } Suggestion importance[1-10]: 5 __ Why: The suggestion adds defensive bounds checking using Math.min, which is a good practice. However, the existing validation in RestEndpointRegistry.validate already rejects negative counts, and subList handles the case where count >= rows.size() safely. The improvement is marginal but adds clarity.	Low
	Handle STRING type explicitly The coercion logic should explicitly handle the STRING type case before the fallback. This ensures consistent behavior and makes the code's intent clearer, preventing potential issues if the type system is extended. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); } try { if (type == INTEGER) { return integerValue(toNumber(value).intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } + if (type == STRING) { + return stringValue(String.valueOf(value)); + } } catch (IllegalArgumentException | ClassCastException e) { ... } return stringValue(String.valueOf(value)); } Suggestion importance[1-10]: 3 __ Why: While making the STRING type handling explicit improves code clarity, the existing fallback already handles this case correctly. The suggestion is a minor style improvement that doesn't fix a bug or significantly enhance maintainability.	Low

Suggestions up to commit aef4663

Category	Suggestion	Impact
General	Handle STRING type explicitly The coercion logic does not handle the STRING type explicitly before the fallback. If type == STRING, the method should return stringValue(String.valueOf(value)) directly without entering the try-catch block, avoiding unnecessary exception handling for string columns. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); + } + if (type == STRING) { + return stringValue(String.valueOf(value)); } try { if (type == INTEGER) { return integerValue(toNumber(value).intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } } catch (IllegalArgumentException | ClassCastException e) { throw new IllegalArgumentException( "rest endpoint value for column [" + column + "] could not be coerced to " + type + ": [" + value + "]"); } return stringValue(String.valueOf(value)); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the STRING type should be handled before the try-catch block to avoid unnecessary exception handling. This improves code efficiency and clarity. However, the current code already handles STRING as a fallback after the try-catch, so the impact is moderate rather than critical.	Medium
	Add context to hex decoding errors The fromHex method can throw IllegalArgumentException for malformed hex, but decodeRestSpec does not wrap this exception with context about which token failed. Wrapping the fromHex call in a try-catch and re-throwing with the token name would provide clearer diagnostics when a corrupted token is encountered. core/src/main/java/org/opensearch/sql/utils/SystemIndexUtils.java [78-117] public static RestSpec decodeRestSpec(String indexName) { if (!isRestSource(indexName)) { throw new IllegalArgumentException("not a valid rest source token: " + indexName); } String body = indexName.substring( REST_SOURCE_PREFIX.length(), indexName.length() - REST_SOURCE_SUFFIX.length()); - String decoded = fromHex(body); + String decoded; + try { + decoded = fromHex(body); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("malformed rest source token: " + indexName, e); + } ... if (endpoint == null) { throw new IllegalArgumentException("rest source token is missing the endpoint: " + indexName); } return new RestSpec(endpoint, args, count, timeout); } Suggestion importance[1-10]: 6 __ Why: The suggestion adds better error context when fromHex throws an exception, which improves debugging and error reporting. This is a useful enhancement for maintainability, though it does not fix a critical bug. The suggestion correctly identifies that wrapping the exception would provide clearer diagnostics.	Low
	Improve error message clarity The method fails closed when the filter is null, which is correct. However, the error message could be improved by indicating this is a configuration or initialization issue rather than a runtime state problem, helping operators diagnose the root cause more quickly. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-465] @Override public List<Map<String, Object>> clusterSettings(Map<String, String> params) { org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( - "cluster settings redaction filter is not initialized; refusing to return unredacted" - + " settings"); + "cluster settings redaction filter is not initialized (check plugin initialization); " + + "refusing to return unredacted settings"); } ... } Suggestion importance[1-10]: 5 __ Why: The suggestion improves the error message to help operators diagnose initialization issues more quickly. While this is a valid improvement for maintainability and debugging, it does not address a functional bug or security concern, so the impact is relatively minor.	Low

Suggestions up to commit 7d9df84

Category	Suggestion	Impact
General	Handle empty tokens in validation The expand_wildcards validation splits on comma but doesn't handle edge cases like consecutive commas or leading/trailing commas that would produce empty tokens after trim. Add validation to reject malformed comma-separated values before processing to prevent potential bypass of validation logic. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [324-360] private static void validateArgValue(String endpoint, String arg, String value) { Set<String> domain = ARG_VALUE_DOMAINS.get(arg); if (domain != null) { if (value == null || !domain.contains(value.toLowerCase(java.util.Locale.ROOT))) { throw new IllegalArgumentException(...); } } else if ("expand_wildcards".equals(arg)) { if (value == null || value.isBlank()) { throw new IllegalArgumentException(...); } for (String token : value.toLowerCase(java.util.Locale.ROOT).split(",")) { - if (!EXPAND_WILDCARDS_VALUES.contains(token.trim())) { + String trimmed = token.trim(); + if (trimmed.isEmpty() || !EXPAND_WILDCARDS_VALUES.contains(trimmed)) { throw new IllegalArgumentException(...); } } } } Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a potential edge case where empty tokens from consecutive commas or leading/trailing commas could bypass validation. Adding trimmed.isEmpty() check before validating against EXPAND_WILDCARDS_VALUES prevents malformed input from being accepted. This is a valid security/validation improvement that prevents potential bypass scenarios.	Medium
	Add explicit STRING type handling Add explicit handling for STRING type before the fallback to prevent unintended type coercion. The current code falls through to stringValue(String.valueOf(value)) for all non-primitive types, which could mask type mismatches. Check if type == STRING and handle it explicitly to make the type handling more robust and maintainable. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); } try { if (type == INTEGER) { return integerValue(toNumber(value).intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } + if (type == STRING) { + return stringValue(String.valueOf(value)); + } } catch (IllegalArgumentException | ClassCastException e) { ... } - return stringValue(String.valueOf(value)); + throw new IllegalArgumentException("Unsupported type for column [" + column + "]: " + type); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the coerce method falls through to stringValue for all non-primitive types. Adding explicit STRING type handling improves code clarity and maintainability. However, the current fallback behavior is intentional (all unhandled types become strings), so this is a moderate improvement rather than a bug fix.	Low
	Improve error message clarity The fail-closed check for SettingsFilter is critical for security but occurs at runtime. Consider adding a startup validation to ensure the filter is initialized during plugin initialization, preventing queries from reaching this point with a null filter. This provides earlier failure detection and clearer operational feedback. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-465] @Override public List<Map<String, Object>> clusterSettings(Map<String, String> params) { org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( - "cluster settings redaction filter is not initialized; refusing to return unredacted" - + " settings"); + "cluster settings redaction filter is not initialized; this indicates a plugin" + + " initialization error. Contact your cluster administrator."); } ... } Suggestion importance[1-10]: 4 __ Why: The suggestion proposes improving the error message to provide clearer operational feedback. While the enhanced message is slightly more user-friendly, the existing message already clearly states the issue ("filter is not initialized; refusing to return unredacted settings"). The improvement is marginal and doesn't address a functional issue.	Low

Suggestions up to commit d43f28c

Category	Suggestion	Impact
Possible issue	Prevent numeric overflow during type coercion The coercion logic does not handle potential overflow when converting numeric values. For example, a Long value exceeding Integer.MAX_VALUE will silently overflow when coerced to INTEGER. Add explicit range checks before narrowing conversions to prevent data corruption and surface clear errors when values exceed the target type's range. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); } try { if (type == INTEGER) { - return integerValue(toNumber(value).intValue()); + Number num = toNumber(value); + long longVal = num.longValue(); + if (longVal < Integer.MIN_VALUE || longVal > Integer.MAX_VALUE) { + throw new IllegalArgumentException("value out of INTEGER range: " + longVal); + } + return integerValue(num.intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } } catch (IllegalArgumentException | ClassCastException e) { ... } return stringValue(String.valueOf(value)); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential overflow issue when converting Long to INTEGER. Adding range checks prevents silent data corruption and provides clear error messages. However, this is a defensive improvement rather than a critical bug fix, as the endpoints typically return values within expected ranges.	Medium
General	Validate decoded token structure completeness The decoder silently skips malformed lines (missing = or empty lines) which could mask encoding bugs or tampering. Consider adding validation to ensure all expected fields are present and throw an exception if the decoded structure is incomplete or contains unexpected content. core/src/main/java/org/opensearch/sql/utils/SystemIndexUtils.java [78-117] public static RestSpec decodeRestSpec(String indexName) { if (!isRestSource(indexName)) { throw new IllegalArgumentException("not a valid rest source token: " + indexName); } String body = indexName.substring( REST_SOURCE_PREFIX.length(), indexName.length() - REST_SOURCE_SUFFIX.length()); String decoded = fromHex(body); ... + boolean foundEndpoint = false; for (String line : decoded.split("\n")) { if (line.isEmpty()) { continue; } int eq = line.indexOf('='); if (eq < 0) { - continue; + throw new IllegalArgumentException("malformed rest source token line: " + line); + } + String k = line.substring(0, eq); + if (k.equals("endpoint")) { + foundEndpoint = true; } ... } + if (!foundEndpoint) { + throw new IllegalArgumentException("rest source token is missing the endpoint: " + indexName); + } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that silently skipping malformed lines could mask encoding bugs. However, the existing code already validates that the endpoint field is present at line 113-115. The suggestion adds validation for malformed lines (missing =), which is a good defensive improvement but not critical since the token is internally generated and validated before encoding.	Low
	Expand availability zone pattern coverage The availability zone regex pattern may not cover all valid AWS region formats, particularly newer regions or future naming conventions. Consider maintaining this pattern as a configurable list or updating it regularly to ensure comprehensive coverage of all AWS regions and availability zones. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestResponseRedactor.java [27-30] private static final Pattern AZ_NAME = Pattern.compile( - "(us(-(gov|iso[a-z]?))?|af|ap|ca|cn|eu|sa|me|il)-(central|(north|south)?(east|west)?)-\\d[a-z]", + "(us(-(gov|iso[a-z]?))?|af|ap|ca|cn|eu|sa|me|il|ap)-(central|(north|south)?(east|west)?|northeast|southeast|southwest|northwest)-\\d[a-z]?", Pattern.CASE_INSENSITIVE); Suggestion importance[1-10]: 4 __ Why: The suggestion to expand the AZ pattern is reasonable for future-proofing, but the current pattern covers the documented AWS regions. The improved_code adds some variations but doesn't fundamentally solve the maintainability concern. This is a minor enhancement rather than a critical issue.	Low
Security	Strengthen fail-closed security check The fail-closed check for the SettingsFilter is critical for security but relies on a static holder that could be cleared or never initialized in edge cases. Consider adding a startup validation that ensures the filter is published before any queries can execute, or use dependency injection to guarantee the filter is always available when the client is constructed. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-465] @Override public List<Map<String, Object>> clusterSettings(Map<String, String> params) { org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( "cluster settings redaction filter is not initialized; refusing to return unredacted" - + " settings"); + + " settings. This indicates a plugin initialization error."); } ... } Suggestion importance[1-10]: 5 __ Why: The suggestion adds a clarifying message to the existing fail-closed check, which is helpful but doesn't fundamentally change the security posture. The existing check already prevents unredacted settings from being returned. The suggestion to use dependency injection is valid but would require architectural changes beyond this PR's scope.	Low

Suggestions up to commit 06ff820

Category	Suggestion	Impact
Security	Add upper bound for count The count validation checks for negative values but does not prevent integer overflow or extremely large values that could cause resource exhaustion. Add an upper bound check to ensure count is within a reasonable range to prevent potential denial-of-service scenarios. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [278-287] public static void validate(RestSpec spec) { Endpoint endpoint = resolve(spec.getEndpoint()); - if (spec.getCount() != null && spec.getCount() < 0) { - throw new IllegalArgumentException( - "rest endpoint [" - + spec.getEndpoint() - + "] count must be a non-negative integer, got [" - + spec.getCount() - + "]"); + if (spec.getCount() != null) { + if (spec.getCount() < 0) { + throw new IllegalArgumentException( + "rest endpoint [" + + spec.getEndpoint() + + "] count must be a non-negative integer, got [" + + spec.getCount() + + "]"); + } + if (spec.getCount() > 10000) { + throw new IllegalArgumentException( + "rest endpoint [" + + spec.getEndpoint() + + "] count exceeds maximum allowed value of 10000, got [" + + spec.getCount() + + "]"); + } } Suggestion importance[1-10]: 6 __ Why: Adding an upper bound for count is a reasonable security improvement to prevent resource exhaustion, though the specific limit of 10000 is arbitrary and should be configurable or documented.	Low
	Validate filter before cluster request The fail-closed check for the SettingsFilter should occur before any cluster state request is made. Move the null check and exception throw to the very beginning of the method, before any client calls, to prevent potential information leakage if the filter initialization fails after the request starts. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-464] public List<Map<String, Object>> clusterSettings(Map<String, String> params) { - // The transport path has no SettingsFilter of its own; it is published from - // SQLPlugin#getRestHandlers at startup. Fail closed before fetching so we never read settings - // into memory when we cannot redact them, matching native GET /_cluster/settings. org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( "cluster settings redaction filter is not initialized; refusing to return unredacted" + " settings"); } + // The transport path has no SettingsFilter of its own; it is published from + // SQLPlugin#getRestHandlers at startup. Fail closed before fetching so we never read settings + // into memory when we cannot redact them, matching native GET /_cluster/settings. Suggestion importance[1-10]: 3 __ Why: The suggestion to move the filter check earlier is redundant since the check already occurs before any cluster state request. The code structure is already correct and fail-closed.	Low
General	Validate decoded body not empty The decoder validates the token shape and checks for a missing endpoint, but does not validate that the decoded hex body is non-empty before parsing. An empty hex body would result in an empty decoded string, leading to a null endpoint. Add an explicit check after hex decoding to ensure the decoded body is not empty before parsing. core/src/main/java/org/opensearch/sql/utils/SystemIndexUtils.java [78-117] public static RestSpec decodeRestSpec(String indexName) { - // Validate the token shape before slicing it: callers gate on isRestSource today, but a - // public decoder must not assume its precondition, otherwise a malformed token would throw an - // opaque StringIndexOutOfBoundsException from substring rather than a clear input error. if (!isRestSource(indexName)) { throw new IllegalArgumentException("not a valid rest source token: " + indexName); } String body = indexName.substring( REST_SOURCE_PREFIX.length(), indexName.length() - REST_SOURCE_SUFFIX.length()); + if (body.isEmpty()) { + throw new IllegalArgumentException("rest source token has empty hex body: " + indexName); + } String decoded = fromHex(body); + if (decoded.isEmpty()) { + throw new IllegalArgumentException("rest source token decoded to empty string: " + indexName); + } ... - if (endpoint == null) { - throw new IllegalArgumentException("rest source token is missing the endpoint: " + indexName); - } Suggestion importance[1-10]: 5 __ Why: Adding validation for empty hex body and decoded string improves robustness by catching malformed tokens earlier with clearer error messages, though the existing endpoint null check already catches this case.	Low
	Handle STRING type explicitly The coercion logic does not handle the STRING type explicitly in the try block, which means STRING values always fall through to the catch-all return statement. This could mask errors if a STRING type is expected but the value is incompatible. Add an explicit STRING case to ensure type safety and consistent error handling. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); } try { if (type == INTEGER) { return integerValue(toNumber(value).intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } + if (type == STRING) { + return stringValue(String.valueOf(value)); + } } catch (IllegalArgumentException | ClassCastException e) { - ... + throw new IllegalArgumentException( + "rest endpoint value for column [" + + column + + "] could not be coerced to " + + type + + ": [" + + value + + "]"); } return stringValue(String.valueOf(value)); } Suggestion importance[1-10]: 2 __ Why: The suggestion adds an explicit STRING case in the try block, but this is unnecessary since STRING values are already handled by the catch-all return statement at the end. The current design intentionally treats STRING as the default type.	Low

[github-actions]

Persistent review updated to latest commit a7a8a20

[github-actions]

Persistent review updated to latest commit 42f77a5

[ahkcs]

Did verification on a live composite cluster with analytics path, it seems like rest breaks on composite-default clusters.

Repro: composite cluster (cluster.pluggable.dataformat=composite) + analytics stack + this PR, run
| rest '/_cluster/health'

TransportPPLQueryAction Routing PPL query to analytics engine
SemanticCheckException: Table 'REST…__REST_SOURCE' not found
Same query works on a non-composite cluster. testRestCommandUnaffectedByAnalyticsEngine doesn't catch it because its cluster isn't composite-default.

[github-actions]

Persistent review updated to latest commit 2db942f

[dai-chen]

Could you confirm if all exposed API here are allowed in managed service / serverless. Just concerned if any security issue later. Ref: https://docs.aws.amazon.com/opensearch-service/latest/developerguide/supported-operations.html

[songkant-aws]

+1, Is there anyway we could verify it by adding some security ITs? Not quite sure whether it's feasible in current plugin.

[noCharger]

@dai-chen @songkant-aws introduced both allow-listing and column redaction 95f0618

[dai-chen]

I think it's still risky. Managed service customer can toggle the PPL_REST_REDACTION_ENABLED setting via our SQL setting endpoint. Please double check.

[noCharger]

I think it's still risky. Managed service customer can toggle the PPL_REST_REDACTION_ENABLED setting via our SQL setting endpoint. Please double check.

Good catch. Both settings were registered Dynamic, so a managed-service customer could flip PPL_REST_REDACTION_ENABLED off at runtime via _plugins/_query/settings or _cluster/settings. aef4663 updated it by dropping Setting.Property.Dynamic so both are now NodeScope-only (set in opensearch.yml). OpenSearch core rejects runtime updates on both paths.

[noCharger]

Did verification on a live composite cluster with analytics path, it seems like rest breaks on composite-default clusters.

Repro: composite cluster (cluster.pluggable.dataformat=composite) + analytics stack + this PR, run | rest '/_cluster/health'

TransportPPLQueryAction Routing PPL query to analytics engine SemanticCheckException: Table 'REST…__REST_SOURCE' not found Same query works on a non-composite cluster. testRestCommandUnaffectedByAnalyticsEngine doesn't catch it because its cluster isn't composite-default.

Good catch, revised on 2db942f

[github-actions]

Persistent review updated to latest commit 0819158

[github-actions]

Persistent review updated to latest commit 95f0618

[github-actions]

Persistent review updated to latest commit 06ff820

[github-actions]

Persistent review updated to latest commit d43f28c

[github-actions]

Persistent review updated to latest commit 7d9df84

[github-actions]

Persistent review updated to latest commit aef4663

[dai-chen]

np: remove all these?

[noCharger]

np: remove all these?

I tried this and had to revert it. It turns out these can't be removed. OpenSearchClient is also implemented downstream by opensearch-project/sql-cli (client/http5/OpenSearchRestClientImpl), which doesn't implement the rest command methods.

Ref https://github.com/opensearch-project/sql/actions/runs/29349113963/job/87140469074?pr=5599

[dai-chen]

I only see RestResponseRedactor focus on IP address redaction. But in my test (Dev Tools in AOS domain), I can see many other masked values, e.g., version in _cat/plugins, cluster.routing.allocation.awareness.force.zone in _cluster/settings, host in _cat/cluster_manager. Are all these redacted automatically? If not, any better way to avoid such risk, like sending what's in REST command go through the exact same process as a REST request sent to the domain by users?

[noCharger]

I only see RestResponseRedactor focus on IP address redaction. But in my test (Dev Tools in AOS domain), I can see many other masked values, e.g., version in _cat/plugins, cluster.routing.allocation.awareness.force.zone in _cluster/settings, host in _cat/cluster_manager. Are all these redacted automatically? If not, any better way to avoid such risk, like sending what's in REST command go through the exact same process as a REST request sent to the domain by users?

The redaction replicates the AOS-side logic exactly. As called out in the PR description, it masks more than IPv4 addresses — IPv6, inet[/…], EC2-style hostnames (ip-a-b-c-d), and availability-zone names are all covered. Every case is asserted in RestResponseRedactorTest.

[github-actions]

Persistent review updated to latest commit f3a6226

[github-actions]

Persistent review updated to latest commit aef4663

[dai-chen]

we need to update this whenever there is a new region?