Security fixes - #393
Merged
Merged
Conversation
rng1995
force-pushed
the
naren/security-fixes-2.9.6
branch
from
August 18, 2026 12:58
3c13abe to
1e4a9d8
Compare
rng1995
force-pushed
the
naren/security-fixes-2.9.6
branch
4 times, most recently
from
August 21, 2026 04:17
d9bcbdd to
0dfe315
Compare
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
rng1995
force-pushed
the
naren/security-fixes-2.9.6
branch
from
August 21, 2026 04:36
0dfe315 to
2090b16
Compare
Collaborator
Author
|
@Spectorian confirmed that PR is good to merge! |
benedictkwok
pushed a commit
to benedictkwok/SkillSpector
that referenced
this pull request
Aug 22, 2026
main's Security fixes (NVIDIA#393) changed PE2-PE5's doc-context suppression from hard-dropping the finding to tagging it contextual-triage / likely-benign-context and keeping it. Update the two new reference-dir tests added in this branch to assert on the tag instead of absence, matching the pattern NVIDIA#393 already applied to the equivalent negated-credential-access test. Signed-off-by: Benedict Kwok <bkwok.oracle@gmail.com>
rodrigorjsf
added a commit
to rodrigorjsf/SkillSpector-Polyglot
that referenced
this pull request
Aug 22, 2026
* fix(cli): count only the transitive findings the report carries
`transitive_finding_count` was a third expression of "which findings does a
finished Scan report", beside `suppression.effective_findings` and
`report.reported_findings`, and the only one that subtracted nothing: it counted
every `filtered_findings` entry carrying a `source_url` straight out of the
report node's return. The merge path now selects through `effective_findings`
first and filters on `source_url` second, so a Baseline that accepts a
transitive Finding lowers the count and the report's list together.
Measured, and it moves the issue's premise: upstream `73dd1f1` ("Security fixes
(NVIDIA#393)", which arrived in the 2.9.6 sync) narrowed what the report node writes
into `filtered_findings` from the pre-partition population to the kept side
alone. The over-count #130 describes -- 2 published against 1 reported -- no
longer reproduces end to end on this tree, and a `--transitive` scan whose
`--baseline` fingerprints a dependency Finding already answers 1 beside a report
listing one. `test_scan_transitive_count_agrees_with_the_report_it_publishes`
pins that as the control.
`test_scan_transitive_count_subtracts_the_baseline_suppressed_partition` pins
the contract instead of the current key's contents: it asserts the count over a
`filtered_findings` holding one kept and one Baseline-suppressed transitive
Finding -- the shape the report node wrote before `73dd1f1` -- where the old
expression answered 2. Restore that expression and it fails `assert 2 == 1`.
Refs #130
Claude-Session: https://claude.ai/code/session_01NsjXzHBDr49KjMJPnLyLCJ
Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
* refactor(report): let one function select a finished scan's findings
`report.reported_findings` and `suppression.effective_findings` were written for
one defect on the two sides of a fork that had not synced, and the 2.9.6 sync
kept both. They agree on selecting `filtered_findings` by presence rather than
truthiness and on subtracting the `suppressed_findings` partition, and diverge
on one input: with `filtered_findings` absent or malformed, the fork's
subtracted that partition from the raw `findings` list and upstream's
deliberately did not, because raw findings are not the population that produced
it.
Upstream's semantics win and the fork's function becomes a one-line delegation,
so the CLI's two summary tables, its advisory count, `skillspector baseline` and
the MCP verdict answer the same question as the recursive public-record budget
and the transitive count. The name stays: five CLI call sites and the MCP server
read findings through it, and the report node is where a reader looks for what a
report contains.
The five CLI stream tests that go through this reader were re-read against the
adopted semantics before the change and none of them moves. Three assert shapes
the two functions already agreed on -- `filtered_findings` empty, absent with
nothing suppressed, and present beside a suppressed partition -- and two run a
real Scan, where the report node always writes the key. No expectation changed.
Both docstrings asserted that `filtered_findings` is the pre-partition
population. Upstream `73dd1f1` falsified that: the report node writes the kept
side alone. The survivor now says what the key holds, why the subtraction is
retained anyway -- the two keys are one partition wherever they are written
together, and mid-graph and hand-assembled results still carry the wider shape
-- and why the raw-`findings` fallback does not subtract. That stale premise was
also a CLAUDE.md Applied Learning bullet and a test docstring, and
`docs/DEVELOPMENT.md` described the key as a projection of
`effective_finding_ids`, a name the report node never reads; all three are
corrected here.
`TestOneRecursiveScanCountsItsFindingsOnce` pins the agreement across every
shape either reader can be handed, including the four a real Scan cannot
produce, and asserts that the recursive summary's `finding_count` and the
recursive public-record budget select identically on the shape they used to
disagree about.
`test_report_writes_only_the_kept_side_into_filtered_findings` pins the fact
all of that prose rests on. Nothing asserted it before: the Behavior Snapshot
corpus cannot, because `tests/behavior/COVERAGE_LIMITS.md` records that
`filtered_findings` is byte-identical to `findings` in every fixture, no fixture
exercising a Baseline at all. Widening the key back now fails a test instead of
making two docstrings and an Applied Learning bullet quietly wrong.
Refs #130
Claude-Session: https://claude.ai/code/session_01NsjXzHBDr49KjMJPnLyLCJ
Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
* docs: correct the partition premise upstream 73dd1f1 falsified
Three places still asserted that `filtered_findings` is the kept and suppressed
findings together. Upstream `73dd1f1` narrowed what the report node writes to
the kept side alone, and the branch corrected the claim everywhere except here.
The state-key row also stated a rule the tree does not keep. "Read it through
`suppression.effective_findings`, never directly" is contradicted by three
legitimate mid-graph reads, and the row described only what the report node
writes while the transitive merge writes the pre-partition merged population
into the same key beforehand. Qualify both: the row says what the key holds at
each point, and the rule binds the consumer asking which findings a finished
Scan reports rather than the merge path, which reads it before anything has
been partitioned.
Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
* test(cli): assert the summary count through the entry builder, not a second reader
The assertion re-expressed the production selection verbatim, so it was true of
itself: replacing the combined entry's `finding_count` with an inlined read of
`filtered_findings` -- exactly the regression this issue exists to prevent --
left it green. Route the comparison through `_combined_skill_entry` so the call
site is what is asserted, and derive the suppressed record count from the state
instead of hard-coding it.
The neighbouring docstring in the same class asserted the partition premise
upstream `73dd1f1` falsified, two screens below the class docstring that
already corrected it.
Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
---------
Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
rng1995
added a commit
that referenced
this pull request
Aug 24, 2026
* fix(pe3): env lookbehind and singular reference dir Two independent PE3 false-positive fixes, both confirmed against the official anthropics/skills repo (mcp-builder): - The .env pattern had no lookbehind, so it matched Python attribute access (self.env, args.env) as if it were a dotenv file reference. Added (?<!\w) so it only fires when .env is not part of an identifier. - _PE3_TOKEN_DOCUMENTATION_DIRS only recognized the plural "references", so the existing OAuth access-token exemption silently failed to apply under a singular reference/ directory (mcp-builder/reference/). Added singular forms for docs/procedures/references/examples/guides. Both fixes verified against the existing test suite for _is_env_file_reference_in_docs and _is_pe3_documentation_example, and new regression tests added for the attribute-access and singular-directory cases, including negative-space tests confirming real credential-theft instructions with the same vocabulary still fire. Closes #406, #407 Signed-off-by: Benedict Kwok <bkwok.oracle@gmail.com> * test(pe3): match #393's contextual-triage tagging contract main's Security fixes (#393) changed PE2-PE5's doc-context suppression from hard-dropping the finding to tagging it contextual-triage / likely-benign-context and keeping it. Update the two new reference-dir tests added in this branch to assert on the tag instead of absence, matching the pattern #393 already applied to the equivalent negated-credential-access test. Signed-off-by: Benedict Kwok <bkwok.oracle@gmail.com> --------- Signed-off-by: Benedict Kwok <bkwok.oracle@gmail.com> Co-authored-by: Benedict Kwok <bkwok.oracle@gmail.com> Co-authored-by: Narendran Raghavan <32655573+rng1995@users.noreply.github.com>
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
1 participant
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
Validation
No changelog entry is included.