fix: classify companion CLI documentation by context - #547
Conversation
d659cb9 to
d5eb8c8
Compare
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Manual Review Needed — changes required on d5eb8c8176121ca58cfa55607d802e5c3fcacfe2
The authenticated GitHub account is also this PR's author, so this cannot serve as an independent approval. On technical merits, the new contextual downgrade predicates have four blocking fail-open cases:
- PE3 accepts imperative token-acquisition clauses.
- PE3 misses adjacent token disclosure using verbs such as ‘paste.’
- RA1 treats protected agent/tool names with CLI suffixes as companions.
- RA1 accepts signed-release evidence from a different logical line.
All hosted checks pass and the head is current with main, but they do not exercise these adversarial cases. Please make the predicates fail closed, add the focused regressions described inline, and have an independent reviewer assess the corrected head.
The actual CrowdStrike/NVCARPS-152 fixture was unavailable in this environment; rerun that downstream fixture before release.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Manual Review Needed — still changes required on 5b6562a095c8af904fea7fa9ee40e8038836e722
Re-review found that this head only merges current main (f161b700d83d1e685f501c525d4cf7a8a49bd82e) into the previously reviewed head (d5eb8c8176121ca58cfa55607d802e5c3fcacfe2). All six PR-owned blobs are byte-for-byte unchanged, so the four existing unresolved review threads remain blocking:
- PE3 accepts imperative token-acquisition clauses.
- PE3 misses adjacent token disclosure using verbs such as ‘paste.’
- RA1 treats protected agent/tool names with CLI suffixes as companions.
- RA1 accepts signed-release evidence from a different logical line.
The authenticated GitHub account is also this PR’s author, so this cannot serve as an independent approval.
The new exact head also lacks required-check results. CI run 34934093664 concluded action_required without starting any jobs, leaving lint, DCO Check, and test-unit absent. This independently blocks merging.
Please address the existing findings, obtain successful checks on the corrected head, rerun the downstream CrowdStrike/NVCARPS-152 fixture before release, and obtain independent review.
|
The new documentation downgrade still applies when the neighbouring line is an instruction to send the token away. Ran Base reports PE3 HIGH at confidence 0.7. This head reports LOW at 0.15, "OAuth Token Documentation", tagged An affirmative benign signal would close that better than a list of bad ones. |
|
Addressed the reported POST-that-value bypass in ee4e271. The bounded transfer veto now recognizes POST of a deictic/returned value as well as explicit tokens, while benign POSTs of a public client ID or unrelated page-field value remain downgraded. The exact reported two-line reproduction now yields PE3 HIGH at 0.7 instead of LOW at 0.15. Validation: focused suite 131 passed; full non-provider/integration suite 4,962 passed; Ruff, format, focused mypy, and diff-check passed. The private NVCARPS-152 attachment remains unavailable; I also checked the canonical security-review Drive folder and used its Finding 11 intent plus this sanitized reproduction. |
|
Exact head ee4e271 is current, mergeable, clean, and all five hosted checks pass. All inline threads remain resolved, and the later POST-that-value reproduction is fixed and covered. Because the authenticated reviewer is also this PR's author, I cannot provide the required independent approval. Requesting an independent maintainer review and approval on this exact head. The private NVCARPS-152 fixture could not be retrieved; the canonical Finding 11 intent and sanitized exact reproduction were validated. |
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
ee4e271 to
465370f
Compare
|
Rebased the PR onto current main at 2e9ae8d (#470) and completed the companion-documentation hardening pass. Key changes:
Validation on the rebased head:
|
Summary
Tracking: NVCARPS-152
Related to #37.
Root cause
Three syntax-only matches were being treated as if they described malicious behavior:
access token; existing contextual tags did not affect risk scoring.self-updatewithout identifying what was being updated or whether signature verification was required.The analyzer result model also only allowed remediation overrides, so contextual cases inherited rule-wide explanations that described different behavior. Together, a sanitized documentation fixture scored 62 / HIGH / DO_NOT_INSTALL.
Implementation
self-updatefollowed only by enabled--verify-signature, with non-negated signed-release wording on the same line. Disabled, embedded, or conflicting flags, extra arguments, wrappers, redirects, shell composition, substitutions, comments, executable scripts, and real self-modification remain HIGH.AnalyzerFinding.explanationis optional and appended to the dataclass, preserving positional compatibility and existing default explanations.The same sanitized fixture now scores 24 / MEDIUM / CAUTION. All three findings remain visible.
Files changed
src/skillspector/models.pysrc/skillspector/nodes/analyzers/static_runner.pyValidation
pytestacross the focused and adjacent analyzer/report suites: 1,111 passedruff check src tests: passedruff format --check src tests: passed (210 files)git diff --check: passedstatic_patterns_supply_chain.pyHosted CI on the reviewed head is fully green: changes, lint, unit tests, DCO, and Docker smoke passed. Docker smoke initially failed during its external
octocat/Hello-Worldscan after the image build and local fixture scan passed; a failed-job-only rerun passed without a code change. The exact downstream private fixtures were unavailable, so the committed cases use sanitized equivalents; the downstream integration must still be rerun before release.Compatibility, risk, and rollback
There are no dependency, configuration, CLI, or public report-schema changes. The main risk is an overly broad contextual downgrade, so the implementation uses same-occurrence ownership and fail-closed vetoes. An independent adversarial review added negative coverage for trailing and cross-line disclosure, attacker-owned token results, redirects, wrapper commands, embedded flags, extra arguments, Markdown substitutions, negated evidence, and warnings associated with a different installer.
Rollback is a single-commit revert; no data migration is required.
Security-review follow-up
Two signed commits close all four P1 review findings and additional adversarial cases:
2e59815keeps imperative or causative OAuth acquisition, adjacent token disclosure, protected CLI subjects, and cross-logical-line signed-release evidence fail closed.796ca91adds modal/directive acquisition variants, boundPut/Pass/Entertransfers without unrelated-prose false positives, stacked protected executable suffixes, and normalized-view source-line coverage.All four review conversations were answered and resolved. Validation on the final local head:
git diff --check: passedAll five hosted checks on final head
796ca91pass: changes, lint, unit tests, DCO, and Docker smoke.