Redact storage paths from physical_plan in profile API responses

[finnegancarroll]

Summary

DataFusion's Display implementation for file-scan nodes (DataSourceExec, ParquetExec) provides the local filesystem paths it is reading from. The profile=true API surfaces this physical_plan text verbatim back to the caller, exposing internal storage details unnecessarily.

Fix

All stage types (SHARD_FRAGMENT, COORDINATOR_REDUCE, LATE_MATERIALIZATION) funnel their physical_plan text through QueryProfileBuilder.parseDataNodePayload().

This change adds a regex redaction there for file_groups={...} to strip the full path.

Before: DataSourceExec: file_groups={1 group: [[/home/user/data/indices/uuid/0/parquet/segment.parquet]]}, ...
After:  DataSourceExec: file_groups={1 group: <redacted>}, ...

Testing

• UTs QueryProfileBuilderRedactionTests
• ITs ExplainApiIT.testProfileReturnsPhysicalPlan

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 6be3271)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 6be3271

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Harden regex against brace characters in paths The regex [^}]* will over-consume if the file paths themselves contain a } character (object-store keys and even filesystem paths can legally contain }), causing the match to terminate early and leak the remainder of the path list. Also, if any subsequent field in the plan uses {...} braces, a missed early termination could leak path content. Consider anchoring on the balanced ]]} terminator that DataFusion always emits for file_groups instead. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/profile/QueryProfileBuilder.java [145] -private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ groups?): [^}]*\\}"); +private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ groups?): \\[.*?\\]\\]\\}", Pattern.DOTALL); Suggestion importance[1-10]: 6 __ Why: Valid concern: paths (especially object-store keys) could theoretically contain }, causing the regex to terminate early and leak path content. The suggested ]]} anchor is more robust, though the improved regex uses .*? with DOTALL which could also over-match across multiple file_groups segments; still, the underlying concern has security relevance.	Low

---

Previous suggestions

Suggestions up to commit 45d07e6

Category	Suggestion	Impact
Security	Harden redaction regex against brace in paths The regex [^}] will fail to redact if DataFusion ever emits a } inside the file list (e.g. object-store keys containing }, or partition values like dt={2024-01-01}), leaving paths unredacted. Consider a more robust match that terminates on the closing ]} sequence of the group list, e.g. \][^\\[\\]]\} anchored, to reduce the chance of a leak via unusual keys. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/profile/QueryProfileBuilder.java [133] -private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ groups?): [^}]*\\}"); +private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ groups?): \\[.*?\\]\\]\\}"); Suggestion importance[1-10]: 6 __ Why: Valid edge-case concern: object-store keys or partition values containing } could bypass the current [^}]* pattern and leak paths. The proposed \[.*?\]\]\} is more robust for security-sensitive redaction, though such keys are uncommon in practice.	Low

Suggestions up to commit 45d07e6

Category	Suggestion	Impact
Security	Make redaction regex more robust The regex [^}]* will stop at the first } character, but object-store URIs or file paths could theoretically contain } (e.g., in URL-encoded form or unusual keys), causing incomplete redaction that leaks the tail of the path. More critically, if DataFusion ever changes the Display format to include nested {} (e.g., statistics blocks) inside file_groups, the pattern will under-match and leak paths. Consider anchoring on the closing ]} sequence that terminates the group list to make the match more robust. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/profile/QueryProfileBuilder.java [133] -private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ groups?): [^}]*\\}"); +private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ groups?): \\[.*?\\]\\]\\}"); Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid theoretical concern about the regex robustness, but the improved regex \[.*?\]\]\} would fail on single-group cases with only one level of ] nesting or greedy matching issues. The current pattern's comment explicitly acknowledges the [^}]* limitation and the existing behavior is reasonable for the known DataFusion format.	Low

Suggestions up to commit 45d07e6

Category	Suggestion	Impact
Security	Make redaction robust to braces in paths The pattern [^}] will fail to match (and thus fail to redact) if any file path or object-store key contains a } character, which is legal in URIs and filesystem paths. Consider using a reluctant match like .?\]\} anchored on the closing ]} of the group list, or otherwise handle } inside paths so redaction cannot be bypassed by a crafted path. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/profile/QueryProfileBuilder.java [133] -private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ groups?): [^}]*\\}"); +private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ groups?): \\[.*?\\]\\}", Pattern.DOTALL); Suggestion importance[1-10]: 6 __ Why: Valid edge case: a } character in a filesystem path or object-store key would break the [^}]* match and bypass redaction, which is a real security consideration since redaction is the whole point. However, such characters are uncommon in practice, and the proposed .*?\]\} pattern is a reasonable hardening.	Low

Suggestions up to commit 45d07e6

Category	Suggestion	Impact
Security	Ensure redaction covers all plan surfaces Redaction is only applied to the physical_plan field parsed here, but if any other field or nested structure surfaced from data nodes also carries a plan string (e.g., sub-plans, explain output), it would bypass redaction. Consider centralizing redaction at the point the plan is serialized into the response, or verify no other paths carry storage locations. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/profile/QueryProfileBuilder.java [151-157] +for (Map.Entry<String, Object> entry : raw.entrySet()) { + if ("physical_plan".equals(entry.getKey()) && entry.getValue() instanceof String s) { + physicalPlan = redactPhysicalPlan(s); + } else if (entry.getValue() instanceof Number n) { + metrics.put(entry.getKey(), n.longValue()); + } +} - Suggestion importance[1-10]: 3 __ Why: The existing_code and improved_code are identical; the suggestion only asks the author to verify no other paths leak storage locations, which is a low-impact verification request.	Low
General	Tighten regex to bracket-delimited path list The regex \d+ groups? will fail to match the singular form 1 group since groups? requires the literal s to be optional but the group stem must be present — wait, it does match. However, the pattern uses [^}]* which could over-match if the plan contains other } structures before the closing brace; more importantly, it won't match if any file path itself contains a } character (rare but possible in object-store keys). Consider anchoring more strictly or documenting this constraint. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/profile/QueryProfileBuilder.java [133] -private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ groups?): [^}]*\\}"); +private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ group(?:s)?): \\[\\[[^}]*\\]\\]\\}"); Suggestion importance[1-10]: 2 __ Why: The suggestion is self-contradictory (acknowledges the current regex works) and the proposed improved_code uses [^}]* which has the same potential issue. Marginal value.	Low

Suggestions up to commit 45d07e6

Category	Suggestion	Impact
Security	Harden redaction regex against nested braces The regex [^}] will fail to redact if DataFusion ever emits nested {} inside the file_groups segment (e.g., statistics or partition metadata), silently leaking paths. Consider using a more robust matcher that consumes balanced brackets, e.g., matching [[.?]] inside file_groups={...}, or use a reluctant quantifier anchored on the known closing pattern ]]}. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/profile/QueryProfileBuilder.java [133] -private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ groups?): [^}]*\\}"); +private static final Pattern FILE_GROUPS_PATTERN = Pattern.compile("file_groups=\\{(\\d+ groups?): \\[\\[.*?\\]\\]\\}"); Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid defensive concern about future-proofing the regex if DataFusion changes its Display format to include nested braces. However, the PR's Javadoc explicitly notes the current format has no nested {}, and the proposed reluctant quantifier .*? with \[\[.*?\]\]\} may not handle all current cases (e.g., single-file groups formatting). Moderate impact given it's speculative.	Low

[github-actions]

✅ Gradle check result for 18af1e8: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.59%. Comparing base (2914470) to head (6be3271).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22747      +/-   ##
============================================
+ Coverage     71.57%   71.59%   +0.01%     
- Complexity    77269    77357      +88     
============================================
  Files          6170     6170              
  Lines        359774   359870      +96     
  Branches      52478    52487       +9     
============================================
+ Hits         257504   257632     +128     
+ Misses        81801    81773      -28     
+ Partials      20469    20465       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit 45d07e6

[github-actions]

✅ Gradle check result for 45d07e6: SUCCESS

[finnegancarroll]

One flaky test failing:

testV2SnapshotIncludesWarmNonDFAIndex

[github-actions]

Persistent review updated to latest commit 45d07e6

[github-actions]

✅ Gradle check result for 45d07e6: SUCCESS

[finnegancarroll]

Flaky tests:

failures:
    tiered_storage_integration_tests::datafusion_query_succeeds_from_cache_after_local_file_deleted
    tiered_storage_integration_tests::small_file_warmup_persists_every_range_to_metadata_tier

[github-actions]

Persistent review updated to latest commit 45d07e6

[github-actions]

✅ Gradle check result for 45d07e6: SUCCESS

[finnegancarroll]

Another flaky test:

testMultiIndexConcurrentRecovery

[github-actions]

Persistent review updated to latest commit 45d07e6

[github-actions]

✅ Gradle check result for 45d07e6: SUCCESS

[github-actions]

Persistent review updated to latest commit 45d07e6

[github-actions]

❕ Gradle check result for 45d07e6: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions]

PR Code Analyzer ❗

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

⛔ Hard block: Issues at Medium severity or above will block this PR from merging.

'Diff too large, requires skip by maintainers after manual review'

---

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]

Persistent review updated to latest commit 6be3271

[github-actions]

✅ Gradle check result for 6be3271: SUCCESS