fix(output-handling): avoid RegExp.exec false positives - #341
Merged
keshprad merged 6 commits intoAug 4, 2026
Merged
Conversation
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
rng1995
previously requested changes
Aug 4, 2026
rng1995
left a comment
Collaborator
There was a problem hiding this comment.
[Automated SkillSpector Review]
Requesting changes for one false-negative path in the new backward recognizer. The focused TestOutputHandling suite passes (110 tests), and GitHub CI is green, but the adversarial case in the inline comment causes an arbitrary .exec(output) receiver to be classified as a RegExp literal and suppresses OH1.
rng1995
reviewed
Aug 4, 2026
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
chrisknvidia
marked this pull request as ready for review
August 4, 2026 21:21
keshprad
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.exec(...)calls on recognized output aliases as parsing rather than generic OH1 code execution.exec, arbitrary receivers, call-result receivers, division-like syntax, non-built-in property casing, and other ambiguous forms.Context
This issue was reported downstream in NVIDIA/SkillEvaluator#31. SkillEvaluator Tier 1 flagged
/.../.exec(output)in theopenclaw/agent-skillssession-viewerskill as high-severity Unvalidated Output Injection. SkillEvaluator invokes SkillSpector for this security scan, so the fix belongs in SkillSpector.Root cause and scope
The generic output-handling pattern matched
.exec(output)without distinguishingRegExp.prototype.execstring parsing from arbitraryexecsinks.The exemption is intentionally syntactic and narrow. A bounded backward recognizer accepts only an unambiguous RegExp literal receiver and otherwise retains the finding. It does not correlate raw mutation-shaped strings across files: without a JavaScript parser, execution order, and data flow, that correlation can both miss real mutations and invent HIGH findings at safe call sites. Prototype-mutation analysis should be a separate parser-backed rule rather than changing the meaning of this OH1 call-site classification.
Uninspected sibling files remain represented by the existing inspection ledger and completeness status; they do not create an unverified OH1 finding in a different inspected file.
Validation
1800 passed, 13 skipped, 38 deselected, 4 xfailed.319 passed.git diff --checkpass.cd128302andopenclaw/agent-skills2a409d3: Security Scan1/1 PASS, exit0.session-viewerskill: score0/100,SAFE, 12 of 12 components inspected, no security issues.SAFEresult.child_process.exec(output)negative control executed its command, remained a HIGH OH1 finding at the correct line, and caused downstream SkillEvaluator to exit1; the fresh wheel and Docker image retained that result.0/100andSAFE, and SARIF contains zero results.0.34slocally with no OH1 false positives; seven dangerous receiver variants remained OH1 findings.No provider credentials are required because this is the static
--no-llmanalysis path. Semantic LLM analysis was intentionally not exercised.