Skip to content

Commit 3956d4b

Browse files
fix(as3): preserve skill identity across input materialization (#580)
* fix(as3): accept manifest name as current-skill identity 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 #500 Signed-off-by: HSU Yu Chen <rufushsu9987@users.noreply.github.com> * style: ruff-format AS3 test assertion for CI format-check Signed-off-by: HSU Yu Chen <rufushsu9987@users.noreply.github.com> * fix(as3): corroborate manifest with selected source identity 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 #500 Signed-off-by: HSU Yu Chen <rufushsu9987@users.noreply.github.com> * fix(as3): derive suppression identity from resolved input Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> --------- Signed-off-by: HSU Yu Chen <rufushsu9987@users.noreply.github.com> Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-authored-by: HSU Yu Chen <rufushsu9987@users.noreply.github.com> Co-authored-by: Narendran Raghavan <nraghavan@nvidia.com>
1 parent 6685a1b commit 3956d4b

8 files changed

Lines changed: 483 additions & 19 deletions

File tree

‎docs/AS3_SELF_REFERENCES.md‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
# AS3 current-skill references
2+
3+
AS3 detects access to other installed skills. A literal reference to the
4+
currently selected skill, such as `skills/example-skill/SKILL.md`, is excluded
5+
when the scanner can establish that skill's identity from the selected input.
6+
7+
Input resolution preserves one identity through temporary materialization:
8+
9+
| Selected input | Identity |
10+
| --- | --- |
11+
| Local skill directory | Resolved directory basename |
12+
| Git repository | Repository basename from the successfully cloned input URL |
13+
| Local `SKILL.md` file | Original file's absolute parent directory basename |
14+
| Archive with one outer skill directory | Preserved outer directory basename |
15+
| Flat local ZIP archive | Selected archive filename without `.zip` |
16+
17+
For example, a flat `example-skill.zip` containing `SKILL.md` with
18+
`name: example-skill` retains that identity after extraction to a temporary
19+
`extracted` directory. A `download.zip` containing an `example-skill/` directory
20+
uses the contained directory name. The archive filename does not become a
21+
second identity for a preserved skill directory.
22+
23+
The manifest name must agree with the selected identity when present. It does
24+
not independently authorize suppression: changing `name` to a peer skill's
25+
name must not hide references to that peer. Case and underscore/hyphen variants
26+
remain distinct. Obfuscated paths, explicit enumeration, AS1, and AS2 retain
27+
their existing detection behavior. Filtering happens before finding budgets
28+
and inspection-ledger emission.
29+
30+
An arbitrary externally renamed staging directory, an ambiguous direct-download
31+
URL, or a flat downloaded archive may lack enough provenance to establish the
32+
skill name. Such inputs retain AS3 findings. URL path segments can represent
33+
repository names or slash-containing Git refs, so they are not guessed as
34+
skill aliases. Integrations should preserve the selected skill-directory name
35+
or supply the original supported input to the scanner.

‎src/skillspector/input_handler.py‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,56 @@ def _validate_zip_member_type(info: zipfile.ZipInfo) -> None:
709709
raise ValueError("Zip directory entry contains file data")
710710

711711

712+
def selected_source_identity_for_input(
713+
input_path: str,
714+
*,
715+
source_type: str,
716+
resolved_path: Path,
717+
temp_dir: Path | None,
718+
) -> str | None:
719+
"""Identify the selected skill without treating materialization names as aliases.
720+
721+
Call only after successful input resolution, using the source type and
722+
temporary directory returned by that handler. A preserved archive root is
723+
stronger evidence than the archive filename; neither introduces a second
724+
identity. Unknown source layouts deliberately have no suppression identity.
725+
"""
726+
if source_type == "directory":
727+
return resolved_path.name or None
728+
729+
if source_type == "git":
730+
text = input_path.strip()
731+
if text.startswith("git@"):
732+
match = re.fullmatch(r"git@[^:]+:(.+)", text)
733+
if match is None:
734+
return None
735+
source_path = match.group(1)
736+
else:
737+
source_path = urlparse(text).path
738+
return source_path.rstrip("/").rsplit("/", 1)[-1].removesuffix(".git") or None
739+
740+
if temp_dir is None:
741+
return None
742+
743+
extraction_root = temp_dir / "extracted"
744+
if source_type in {"zip", "url"} and resolved_path.parent == extraction_root:
745+
# InputHandler preserves a unique outer directory from an archive.
746+
return resolved_path.name or None
747+
if source_type == "zip" and resolved_path == extraction_root:
748+
return Path(input_path.strip()).stem or None
749+
750+
if source_type == "file":
751+
original = Path(input_path.strip())
752+
if original.name not in {"SKILL.md", "skill.md"}:
753+
return None
754+
return Path(os.path.abspath(original)).parent.name or None
755+
756+
# Direct-download URLs cannot reliably distinguish repository, slash-
757+
# containing ref, and skill-directory segments. Keep those identities
758+
# unknown rather than allowing a ref or repository name to hide a peer.
759+
return None
760+
761+
712762
class InputHandler:
713763
"""
714764
Handles input resolution for different source types.

‎src/skillspector/nodes/analyzers/static_patterns_agent_snooping.py‎

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,30 @@ def _normalize_skill_identifier(value: object) -> str | None:
235235
def _current_skill_identifiers(state: SkillspectorState) -> frozenset[str]:
236236
"""Derive a trusted, internally consistent current-skill identity."""
237237

238-
skill_path: object = state.get("skill_path")
238+
# Normal graph scans carry one resolver-owned source identity, including
239+
# an explicit None when the input layout is ambiguous. Direct analyzer
240+
# callers retain the scan-root fallback used before provenance existed.
241+
if "selected_source_identity" in state:
242+
path_identifier = _normalize_skill_identifier(state.get("selected_source_identity"))
243+
else:
244+
path_identifier = _scan_root_identifier(state.get("skill_path"))
245+
246+
manifest = state.get("manifest")
247+
manifest_identifier: str | None = None
248+
if isinstance(manifest, Mapping):
249+
manifest_identifier = _normalize_skill_identifier(manifest.get("name"))
250+
251+
# Manifest data can corroborate the selected source but cannot override a
252+
# disagreement or add an alias that could conceal access to a peer skill.
253+
if path_identifier is None:
254+
return frozenset()
255+
if manifest_identifier is not None and manifest_identifier != path_identifier:
256+
return frozenset()
257+
return frozenset({path_identifier})
258+
259+
260+
def _scan_root_identifier(skill_path: object) -> str | None:
261+
"""Return the root basename for callers without resolved input provenance."""
239262
path_text: str | bytes | None = None
240263
if isinstance(skill_path, str):
241264
path_text = skill_path
@@ -249,19 +272,7 @@ def _current_skill_identifiers(state: SkillspectorState) -> frozenset[str]:
249272
normalized_path = path_text.replace("\\", "/").rstrip("/")
250273
path_identifier = _normalize_skill_identifier(normalized_path.rsplit("/", 1)[-1])
251274

252-
manifest = state.get("manifest")
253-
manifest_identifier: str | None = None
254-
if isinstance(manifest, Mapping):
255-
manifest_identifier = _normalize_skill_identifier(manifest.get("name"))
256-
257-
# The path is the only host-derived identity available here. A manifest
258-
# name is contributor-controlled, so it may corroborate the path but must
259-
# never introduce a second identity or override a disagreement.
260-
if path_identifier is None:
261-
return frozenset()
262-
if manifest_identifier is not None and manifest_identifier != path_identifier:
263-
return frozenset()
264-
return frozenset({path_identifier})
275+
return path_identifier
265276

266277

267278
def _is_current_skill_path_reference(

‎src/skillspector/nodes/resolve_input.py‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
from skillspector.input_handler import (
2828
InputHandler,
2929
TransitiveIngestTruncatedError,
30+
selected_source_identity_for_input,
3031
validate_local_input_path,
3132
)
3233
from skillspector.logging_config import get_logger
@@ -58,11 +59,17 @@ def resolve_input(state: SkillspectorState) -> dict[str, object]:
5859
handler = InputHandler(transitive_budget=workflow_budget)
5960
try:
6061
resolved, source_type = handler.resolve(input_path.strip())
62+
temp_dir = handler.temp_dir_for_cleanup()
6163
update: dict[str, object] = {
6264
"skill_path": str(resolved),
65+
"selected_source_identity": selected_source_identity_for_input(
66+
input_path.strip(),
67+
source_type=source_type,
68+
resolved_path=resolved,
69+
temp_dir=temp_dir,
70+
),
6371
"workflow_resource_budget": workflow_budget,
6472
}
65-
temp_dir = handler.temp_dir_for_cleanup()
6673
if temp_dir is not None:
6774
update["temp_dir_for_cleanup"] = str(temp_dir)
6875
else:
@@ -87,19 +94,22 @@ def resolve_input(state: SkillspectorState) -> dict[str, object]:
8794
resolved = validate_local_input_path(Path(skill_path))
8895
return {
8996
"skill_path": str(resolved),
97+
"selected_source_identity": resolved.name or None,
9098
"temp_dir_for_cleanup": None,
9199
"workflow_resource_budget": workflow_budget,
92100
}
93101
except (OSError, RuntimeError) as e:
94102
logger.warning("Could not resolve skill_path: %s", e)
95103
return {
96104
"skill_path": None,
105+
"selected_source_identity": None,
97106
"temp_dir_for_cleanup": None,
98107
"workflow_resource_budget": workflow_budget,
99108
}
100109

101110
return {
102111
"skill_path": None,
112+
"selected_source_identity": None,
103113
"temp_dir_for_cleanup": None,
104114
"workflow_resource_budget": workflow_budget,
105115
}

‎src/skillspector/state.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,10 @@ class SkillspectorState(TypedDict, total=False):
236236
# Input: resolve_input node consumes input_path or skill_path, sets skill_path
237237
input_path: str | None
238238
skill_path: str | None
239+
# Set/reset by resolve_input from the actual selected source. None means
240+
# materialization could not establish a current-skill identity; analyzers
241+
# must not substitute generated directory names or manifest-only aliases.
242+
selected_source_identity: str | None
239243
# Set by resolve_input when a temp dir was created (git/url/zip/file); caller should clean up
240244
temp_dir_for_cleanup: str | None
241245
zip_bytes: bytes | None

‎tests/nodes/analyzers/test_static_patterns.py‎

Lines changed: 51 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -895,20 +895,67 @@ def test_as3_scan_root_identity_marks_self_reference_when_manifest_is_absent(sel
895895
assert readme_event["emitted_finding_ids"] == []
896896
assert not any(f.rule_id == "AS3" for f in result["findings"])
897897

898-
def test_as3_inconsistent_manifest_identity_fails_closed(self):
899-
"""A contributor-controlled manifest cannot override the scan-root identity."""
898+
def test_as3_selected_source_identity_suppresses_temp_clone_self_reference(self):
899+
"""Temp-clone basename ``repo`` still suppresses when source identity corroborates."""
900900
state = {
901-
"skill_path": "/tmp/checkout-root",
901+
"skill_path": "/tmp/skillspector_abc123/repo",
902+
"selected_source_identity": "example-skill",
902903
"manifest": {"name": "example-skill"},
903904
"components": ["README.md"],
904905
"file_cache": {"README.md": "Root skill: skills/example-skill/SKILL.md"},
905906
}
906907

907908
result = agent_snooping_module.node(state)
908909

910+
readme_event = next(
911+
event for event in result["inspection_ledger"] if event["path"] == "README.md"
912+
)
913+
assert readme_event["outcome"] == "completed"
914+
assert readme_event["emitted_finding_ids"] == []
915+
assert not any(f.rule_id == "AS3" for f in result["findings"])
916+
917+
def test_as3_inconsistent_manifest_identity_fails_closed(self):
918+
"""Conflicting root and manifest identities cannot authorize suppression."""
919+
state = {
920+
"skill_path": "/tmp/checkout-root/example-skill",
921+
"manifest": {"name": "published-name"},
922+
"components": ["README.md"],
923+
"file_cache": {
924+
"README.md": (
925+
"Root skill: skills/example-skill/SKILL.md\n"
926+
"Also: skills/published-name/SKILL.md\n"
927+
"Peer: skills/other-skill/SKILL.md"
928+
)
929+
},
930+
}
931+
932+
result = agent_snooping_module.node(state)
933+
909934
as3_findings = [finding for finding in result["findings"] if finding.rule_id == "AS3"]
910935
assert [finding.matched_text for finding in as3_findings] == [
911-
"skills/example-skill/SKILL.md"
936+
"skills/example-skill/SKILL.md",
937+
"skills/published-name/SKILL.md",
938+
"skills/other-skill/SKILL.md",
939+
]
940+
941+
def test_as3_adversarial_mismatched_manifest_does_not_suppress_peer_path(self):
942+
"""Malicious manifest name unequal to trusted source identity cannot hide AS3."""
943+
state = {
944+
"skill_path": "/tmp/skillspector_abc123/repo",
945+
"selected_source_identity": "evil-skill",
946+
"manifest": {"name": "victim"},
947+
"components": ["README.md"],
948+
"file_cache": {
949+
"README.md": ("Self: skills/evil-skill/SKILL.md\nPeer: skills/victim/SKILL.md")
950+
},
951+
}
952+
953+
result = agent_snooping_module.node(state)
954+
955+
as3_findings = [finding for finding in result["findings"] if finding.rule_id == "AS3"]
956+
assert [finding.matched_text for finding in as3_findings] == [
957+
"skills/evil-skill/SKILL.md",
958+
"skills/victim/SKILL.md",
912959
]
913960

914961
def test_as3_long_current_skill_path_is_not_snooping(self):

0 commit comments

Comments
 (0)