[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/setting/OpenSearchSettings.java	75	medium	PPL_REST_ALLOWED_ENDPOINTS_SETTING defaults to List.of("*"), enabling all registry-listed endpoints on every fresh installation without operator opt-in. Any user with cluster:monitor privilege can immediately query node IPs, plugin inventory, shard topology, and cluster settings.
opensearch/src/main/java/org/opensearch/sql/opensearch/setting/OpenSearchSettings.java	71	medium	PPL_REST_REDACTION_ENABLED_SETTING defaults to false. Node IPs, EC2 hostnames, IPv6 addresses, and AZ names are returned in plain text by default. A user with cluster:monitor can extract full network topology via PPL queries without any administrator action.
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java	254	medium	/_cat/nodes is allow-listed and exposes ip, heap_percent, ram_percent, and cpu per node. Combined with the wide-open default allow-list and redaction-off default, any monitor-privileged PPL user can enumerate the full cluster network topology and resource state.
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestSettingsFilterHolder.java	35	low	Static volatile SettingsFilter holder is set once in getRestHandlers(). In test or non-standard plugin loading contexts where getRestHandlers() is never called, clusterSettings() fails closed with IllegalStateException. Not malicious, but the static mutable global is a fragile bridge.
opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchRestClient.java	339	low	catJson() uses the low-level REST client to perform raw HTTP GET requests (/_cat/nodes, /_cat/cluster_manager, etc.) rather than typed transport actions. This bypasses the security plugin's transport-layer ActionFilter, so the security context enforcement documented in RestCommandSecurityIT may not apply on the standalone-client (non-node-client) code path.

The table above displays the top 10 most important findings.

Total: 5 | Critical: 0 | High: 0 | Medium: 3 | 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

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The toNumber method attempts to parse numeric strings but does not handle negative numbers correctly. The check if (s.indexOf('.') >= 0 || s.indexOf('e') >= 0 || s.indexOf('E') >= 0) will parse negative integers as doubles (e.g., "-5" will be parsed as a double instead of a long). This can lead to precision loss or incorrect type coercion when the schema expects an integer or long. /** 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, if the SettingsFilter is null, the method throws an IllegalStateException. However, the filter is retrieved from a static holder that may not be initialized in all execution paths. If RestSettingsFilterHolder.get() returns null during normal operation (e.g., in tests or certain startup sequences), this will cause a runtime failure. The fail-closed approach is correct for security, but the error message should clarify when this is expected vs. a configuration issue. 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 allows octets from 0-255, but the pattern (25[0-5]|2[0-4]\\d|[0-1]?\\d\\d?) will match invalid octets like "256" in certain contexts due to the [0-1]? part. For example, "256.1.1.1" should not match, but the regex might partially match "56.1.1.1" within it. The test at line 26 expects "256.1.1.1" to remain unmasked, but the regex behavior should be verified to ensure it does not inadvertently mask valid non-IP text. 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"); Possible Issue In registerNonDynamicSettings, the method now checks if (clusterSettings.get(setting) != null) before putting the setting into latestSettings. This change means that if a non-dynamic setting is not explicitly set in the cluster (i.e., it uses its default value), it will not be added to latestSettings. This could cause getSettingValue to return null for settings that have a default value but are not explicitly configured, breaking code that expects the default to be available. Settings.Key key, Setting setting) { settingBuilder.put(key, setting); if (clusterSettings.get(setting) != null) { latestSettings.put(key, clusterSettings.get(setting)); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to aef4663

Explore these optional code suggestions:

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

---

Previous suggestions

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

Suggestions up to commit 06ff820

Category	Suggestion	Impact
Possible issue	Validate endpoint is non-empty The decodeRestSpec method should validate that the decoded endpoint is non-empty before returning. An empty endpoint could bypass validation checks and cause issues downstream, even though it should be caught during encoding. 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); ... - if (endpoint == null) { + if (endpoint == null || endpoint.isEmpty()) { throw new IllegalArgumentException("rest source token is missing the endpoint: " + indexName); } return new RestSpec(endpoint, args, count, timeout); } Suggestion importance[1-10]: 6 __ Why: Adding an empty check for the endpoint field provides defense-in-depth validation. While the encoding process should prevent empty endpoints, this check ensures robustness against potential edge cases or future changes.	Low
General	Handle STRING type explicitly Add explicit handling for STRING type before the default fallback. The current code implicitly treats all non-numeric/non-boolean types as strings, but this should be made explicit for clarity and to prevent potential issues if new types are added. 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)); + throw new IllegalArgumentException("Unsupported type: " + type); } Suggestion importance[1-10]: 5 __ Why: The suggestion improves code clarity by making the STRING type handling explicit, but the current implicit fallback is functionally correct. The improvement is minor and primarily affects maintainability.	Low
	Add overflow handling for numeric parsing The toNumber method should handle potential overflow when parsing large numeric strings. A string representing a number larger than Long.MAX_VALUE will throw a NumberFormatException, but this should be caught and wrapped with context about the column being processed. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [396-409] 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); + try { + if (s.indexOf('.') >= 0 || s.indexOf('e') >= 0 || s.indexOf('E') >= 0) { + return Double.parseDouble(s); + } + return Long.parseLong(s); + } catch (NumberFormatException e) { + throw new NumberFormatException("Cannot parse numeric value: " + s); } - return Long.parseLong(s); } Suggestion importance[1-10]: 3 __ Why: The suggestion wraps NumberFormatException with a more descriptive message, but the existing exception already provides sufficient context through the calling coerce method's catch block. The improvement is marginal.	Low
Security	Improve security error message clarity The fail-closed check for the SettingsFilter is critical for security. However, the error message could be improved to indicate this is a configuration issue rather than a runtime error, helping operators diagnose the problem more quickly. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-464] @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 issue. Refusing to return unredacted settings"); } ... } Suggestion importance[1-10]: 4 __ Why: The suggestion slightly improves the error message to help operators diagnose initialization issues. However, the existing message already clearly indicates the problem, so the impact is minimal.	Low

Suggestions up to commit 0819158

Category	Suggestion	Impact
General	Make type coercion explicit The coercion logic silently converts all non-matching types to STRING as a fallback. This could mask type mismatches and lead to unexpected behavior. Consider throwing an exception for unsupported type conversions to fail fast and make type handling explicit. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [330-362] 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)); + } + throw new IllegalArgumentException( + "rest endpoint value for column [" + + column + + "] has unsupported type: " + + type); } 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 fallback to STRING is implicit and could mask type mismatches. Making the STRING case explicit and throwing an exception for unsupported types improves code clarity and helps catch unexpected type conversions early. However, this is a code quality improvement rather than a critical bug fix.	Medium
	Handle empty tokens in validation The validation logic for expand_wildcards splits on comma but doesn't handle edge cases like consecutive commas, leading/trailing commas, or empty tokens after trimming. This could allow malformed input to pass validation. Add explicit checks for empty tokens after splitting and trimming. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [292-328] 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())) { - throw new IllegalArgumentException(...); + String trimmed = token.trim(); + if (trimmed.isEmpty() || !EXPAND_WILDCARDS_VALUES.contains(trimmed)) { + throw new IllegalArgumentException( + "rest endpoint [" + + endpoint + + "] arg [expand_wildcards] has an unsupported value [" + + value + + "]. Allowed values: " + + EXPAND_WILDCARDS_VALUES); } } } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that empty tokens after trimming should be explicitly rejected. The current code's trim() call could allow malformed input like "open,,closed" to pass validation with empty tokens. Adding an explicit check for trimmed.isEmpty() improves input validation robustness.	Low
	Validate decoded line format The decoder silently skips malformed lines (empty or without '='). While this provides resilience, it could mask corruption in the encoded token. Consider logging warnings or throwing exceptions for unexpected line formats to detect encoding/decoding issues early. 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); ... for (String line : decoded.split("\n")) { if (line.isEmpty()) { continue; } int eq = line.indexOf('='); if (eq < 0) { - continue; + throw new IllegalArgumentException( + "rest source token contains malformed line without '=': " + indexName); } ... } } Suggestion importance[1-10]: 5 __ Why: The suggestion proposes throwing an exception instead of silently skipping malformed lines without '='. While this would help detect encoding/decoding issues, the current behavior of skipping such lines provides resilience against minor corruption. The trade-off between strictness and resilience is reasonable, making this a moderate improvement rather than a critical fix.	Low
	Improve error message clarity The method relies on a static holder for the SettingsFilter which could be null at runtime. While there's a null check, this creates a tight coupling and potential race condition. Consider injecting the filter as a dependency or using a more robust initialization pattern to ensure the filter is always available. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-479] @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]: 3 __ Why: The suggestion adds a minor clarification to the error message ("This indicates a plugin initialization error"). While this provides slightly more context, the existing error message is already clear about the issue. The suggestion doesn't address the architectural concern about the static holder pattern mentioned in the description.	Low

[github-actions]

Persistent review updated to latest commit a7a8a20

[github-actions]

Persistent review updated to latest commit 8b0a147

[github-actions]

Persistent review updated to latest commit e4c63e0

[github-actions]

Persistent review updated to latest commit 343e93a

[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.

[dai-chen]

Design question: Just wonder is it possible to follow our V2 engine approach to simplify and make this generic, like enable system table in Calcite instead of such specific table type. I recall in V2 we convert SHOW/DESC statement into regular SELECT on system table which is backed by cat index API.

[noCharger]

Design question: Just wonder is it possible to follow our V2 engine approach to simplify and make this generic, like enable system table in Calcite instead of such specific table type. I recall in V2 we convert SHOW/DESC statement into regular SELECT on system table which is backed by cat index API.

Yes, we can introduce an "system table" abstraction to cover both. However, the actual logic are quite diverged so the benefit is minimal. @dai-chen could you please confirm if 0819158 is what you meant or there's a better approach for refactor?

[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