fix(as3): accept manifest name as current-skill identity - #580
Open
rufushsu9987 wants to merge 3 commits into
Open
rufushsu9987 wants to merge 3 commits into
rufushsu9987 wants to merge 3 commits into
Conversation
Temp git/zip extracts land in directories like `repo` while SKILL.md keeps the real skill name. Requiring path/manifest agreement dropped all identities and false-positived literal self-references as AS3. Treat scan-root basename and manifest name as independent identities so either self-path is suppressed while peer-skill paths still fire. Refs NVIDIA#500 Signed-off-by: HSU Yu Chen <rufushsu9987@users.noreply.github.com>
Signed-off-by: HSU Yu Chen <rufushsu9987@users.noreply.github.com>
SanHsien
added a commit
to SanHsien/SkillSpector
that referenced
this pull request
Sep 17, 2026
Brings the fork up to upstream main c13f70e; the version is still 2.11.2. The fork history was squashed into one commit on 2026-09-13, so it shares no merge-base with upstream and git merge refuses. The range diff was applied with git apply -3 instead. The fork content equals 69dcdfb plus the registered divergences, so conflicts landed only on those seven files; the other 116 applied cleanly. FORK.md now documents this procedure. Divergences, resolved by each row's rule: - static_runner.py takes upstream NVIDIA#522 (SKILLSPECTOR_MAX_STATIC_ANALYSIS_SECONDS_PER_ARTIFACT, default 300s). The fork's SKILLSPECTOR_MAX_STATIC_SECONDS override and its seven tests are removed. Downstream gates must use the upstream name when their pin moves. - test_static_yara.py, test_build_context.py and test_input_handler.py take upstream (NVIDIA#501-NVIDIA#505, NVIDIA#518 fix the same Windows issues); 301 passed on Windows, rows deleted. - test_security_end_to_end.py: upstream's version still fails nine_case on Windows (YARA load and SC8 budgets stay hard-coded), so the relaxation helper is re-applied on top; row kept and rewritten. - .gitignore keeps the fork block; README.md stays Traditional Chinese and the upstream README goes to README.en.md. Two new Windows divergences from new upstream tests: - tests/unit/test_cli.py: a file name containing a backslash is split into two path parts on Windows; skipped by a capability probe added to tests/platform_support.py. - test_json_container_ownership.py: oversized payloads became test ids, which pytest copies into PYTEST_CURRENT_TEST, over Windows' 32,767 character environment limit; short ids added, content unchanged. Triage: 13 of PRs NVIDIA#528-NVIDIA#580 merged into upstream main and arrive here; 29 stay open (including NVIDIA#550, release 2.12.0). Upstream closed this fork's PR NVIDIA#486 on 2026-09-15; NVIDIA#490 builds on it and is open. Verified on Windows in fresh-process batches against this tree: tests/unit 1563 passed, 29 skipped; tests/nodes 3822 passed, 11 skipped, 4 xfailed (plus test_json_container_ownership 71 passed after the id fix); remaining tests 182 passed, 16 skipped; test_security_end_to_end.py 98 passed. ruff check and format clean, check_divergence OK (10 diverging, 10 registered), check_pin_bounds OK. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: SanHsien <34234698+SanHsien@users.noreply.github.com>
rng1995
requested changes
Sep 18, 2026
rng1995
left a comment
rng1995
left a comment
Collaborator
There was a problem hiding this comment.
[SkillSpector Review]
Reviewed exact head 86a740f3ca0f1e74e713ec144c5a67b740a7a06e.
Requesting changes because the new suppression trusts a contributor-controlled manifest name as an independent current-skill identity. A skill can therefore choose the name of a peer and hide an otherwise valid AS3 finding for skills/<peer>/SKILL.md. Preserve the temp-clone false-positive fix without allowing uncorroborated manifest data to suppress the security rule—for example, carry a source-derived repository/archive identity into analyzer state and require it to corroborate the manifest. Add an adversarial mismatched-name regression.
| if path_identifier is not None: | ||
| identifiers.add(path_identifier) | ||
| if manifest_identifier is not None: | ||
| identifiers.add(manifest_identifier) |
Collaborator
There was a problem hiding this comment.
manifest.name comes from the scanned skill, so treating it as an independent trusted identity creates an AS3 false negative: a malicious skill can set name: victim and suppress a literal skills/victim/SKILL.md reference. Please derive the temp-clone identity from trusted input provenance (repository/archive/selected-source metadata), or otherwise require independent corroboration before a manifest name can suppress AS3, and retain a mismatch attack regression.
Temp git/zip roots still use ephemeral basenames like `repo`, but contributor-controlled manifest.name must not suppress AS3 alone. Carry a host/operator-selected repository/archive/path identity into analyzer state and only suppress self-paths that match that trusted identity or a non-ephemeral scan-root basename. Add an adversarial mismatched-name regression. Refs NVIDIA#500 Signed-off-by: HSU Yu Chen <rufushsu9987@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
2 participants
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
nameas independent current-skill identities for AS3 self-reference suppression..../skillspector_*/repo) no longer false-positive literalskills/<manifest-name>/SKILL.mdself-references as Agent Snooping.Why
PR #506 correctly suppressed self-references when path basename and manifest name agree, but deliberately failed closed when they differ. Temporary clone/extraction directories use a generic basename (
repo) while the manifest keeps the real skill name, so those scans still reported AS3 on harmless self-paths.Issue #500 was reopened for exactly this remaining acceptance criterion.
Verification
uv run pytest -q tests/nodes/analyzers/test_static_patterns.py -k 'as3 or AgentSnooping'→ 25 passedskill_pathbasenamerepo, manifestexample-skill):skills/example-skill/SKILL.md→ suppressedskills/other-skill/SKILL.md→ AS3 retainedSecurity notes
Suppression still requires an exact literal
skills/<name>/SKILL.mdpath match against one of the current-skill identities. Transformed/obfuscated peer paths, explicit enumeration, AS1, and AS2 are unchanged.Refs #500