diff --git a/src/skillspector/references.py b/src/skillspector/references.py index 6c0d5cd17..dc8f69568 100644 --- a/src/skillspector/references.py +++ b/src/skillspector/references.py @@ -23,7 +23,12 @@ MAX_REFERENCE_RECORDS = 1024 MAX_REFERENCE_RUNTIME_SECONDS = 2.0 _MAX_EVIDENCE = 160 -_MARKDOWN_DESTINATION = re.compile(r"\[[^\]\n]{1,200}\]\(([^)\n]{1,512})\)") +_MAX_MARKDOWN_DESTINATION_CHARS = 512 +_MARKDOWN_REFERENCE_START = re.compile(r"\[[^\]\n]{1,200}\]\(|^[ \t]{0,3}\[[^\]\n]{1,200}\]:[ \t]*") +_MARKDOWN_TITLE = r"""(?:"(?:\\.|[^"\\\r\n])*"|'(?:\\.|[^'\\\r\n])*'|\((?:\\.|[^)\\\r\n])*\))""" +_MARKDOWN_INLINE_END = re.compile(r"[ \t]*(?:" + _MARKDOWN_TITLE + r")?[ \t]*\)") +_MARKDOWN_DEFINITION_END = re.compile(r"[ \t]*(?:" + _MARKDOWN_TITLE + r")?[ \t]*(?:\r?\n)?\Z") +_MARKDOWN_STRUCTURAL_ESCAPE = re.compile(r"\\([\\()<>])") _QUOTED_OR_CODE_PATH = re.compile( r"(?:`|'|\")((?:\./)?(?:[A-Za-z0-9_.-]+/)*[A-Za-z0-9_.-]+\.[A-Za-z0-9]{1,12})(?:`|'|\")" ) @@ -57,6 +62,90 @@ def _evidence(cleaned_line: str, column: int) -> str: return cleaned_line[start : start + _MAX_EVIDENCE] +@dataclass(frozen=True) +class _ReferenceCandidate: + raw: str + start: int + end: int + + +def _markdown_candidates( + line: str, *, deadline: float, clock: Callable[[], float] +) -> Iterator[_ReferenceCandidate]: + """Read bounded destinations without splitting spaces or balanced parentheses. + + Explicit reference definitions use the same destination grammar as inline + links. Recognizing them separately keeps slash-separated prose excluded. + """ + consumed_until = 0 + for opening in _MARKDOWN_REFERENCE_START.finditer(line): + # Malformed openings may never yield a candidate. Bound their work too. + if clock() >= deadline: + return + if opening.start() < consumed_until: + continue + start = opening.end() + while start < len(line) and line[start] in " \t": + start += 1 + if start >= len(line): + continue + limit = min(len(line), start + _MAX_MARKDOWN_DESTINATION_CHARS + 2) + end = start + if line[start] == "<": + start += 1 + end = start + while end < limit and line[end] not in "<>\r\n": + if line[end] == "\\" and end + 1 < limit: + end += 1 + end += 1 + if end >= limit or line[end] != ">": + continue + raw = line[start:end] + destination_end = end + 1 + else: + depth = 0 + while end < limit and not line[end].isspace(): + char = line[end] + if char == "\\" and end + 1 < limit: + end += 2 + continue + if char == "(": + depth += 1 + elif char == ")": + if depth == 0: + break + depth -= 1 + elif char in "<>": + break + end += 1 + if depth or end == start or end - start > _MAX_MARKDOWN_DESTINATION_CHARS: + continue + raw = line[start:end] + destination_end = end + if len(raw) > _MAX_MARKDOWN_DESTINATION_CHARS: + continue + if opening.group().endswith("("): + if not _MARKDOWN_INLINE_END.match( + line, destination_end, min(len(line), destination_end + 512) + ): + continue + elif len(line) - destination_end > 512 or not _MARKDOWN_DEFINITION_END.fullmatch( + line, destination_end + ): + # A prose line such as "[status]: All checks passed." is not a + # reference definition: a destination can only be followed by a title. + continue + consumed_until = destination_end + yield _ReferenceCandidate( + _MARKDOWN_STRUCTURAL_ESCAPE.sub(r"\1", raw), start, destination_end + ) + + +def _pattern_candidates(pattern: re.Pattern[str], line: str) -> Iterator[_ReferenceCandidate]: + for match in pattern.finditer(line): + yield _ReferenceCandidate(match.group(1), match.start(1), match.end(1)) + + def _candidate_strings( text: str, *, @@ -65,43 +154,52 @@ def _candidate_strings( ) -> tuple[list[tuple[str, int, int, str]], tuple[str, ...]]: """Extract path-like strings without materializing all matches or lines. - Each regular expression contributes at most one pending match to a small + Each candidate iterator contributes at most one pending match to a small merge heap. This preserves source ordering while ensuring a dense, attacker-controlled line cannot be fully enumerated and sorted before the candidate and time ceilings are enforced. """ candidates: list[tuple[str, int, int, str]] = [] seen: set[tuple[int, int, str]] = set() - patterns = (_MARKDOWN_DESTINATION, _QUOTED_OR_CODE_PATH, _PLAIN_RELATIVE_PATH) for line_number, line in enumerate(StringIO(text), 1): if clock() >= deadline: return candidates, ("runtime",) cleaned_line = " ".join(line.strip().split()) - iterators: list[Iterator[re.Match[str]]] = [pattern.finditer(line) for pattern in patterns] - pending: list[tuple[int, int, int, re.Match[str]]] = [] + iterators = [ + _markdown_candidates(line, deadline=deadline, clock=clock), + _pattern_candidates(_QUOTED_OR_CODE_PATH, line), + _pattern_candidates(_PLAIN_RELATIVE_PATH, line), + ] + pending: list[tuple[int, int, int, _ReferenceCandidate]] = [] + markdown_destination_end = 0 for pattern_index, iterator in enumerate(iterators): match = next(iterator, None) if match is not None: heapq.heappush( pending, - (match.start(1), match.end(1), pattern_index, match), + (match.start, pattern_index, match.end, match), ) if clock() >= deadline: return candidates, ("runtime",) while pending: if clock() >= deadline: return candidates, ("runtime",) - _, _, pattern_index, match = heapq.heappop(pending) - raw = match.group(1).strip().split(maxsplit=1)[0] - key = (line_number, match.start(1), raw) - if key not in seen: + _, pattern_index, _, match = heapq.heappop(pending) + raw = match.raw + key = (line_number, match.start, raw) + # A plain/quoted substring of an explicit Markdown destination is + # not a second reference (e.g. ). + inside_destination = pattern_index != 0 and match.start < markdown_destination_end + if pattern_index == 0: + markdown_destination_end = max(markdown_destination_end, match.end) + if not inside_destination and key not in seen: seen.add(key) candidates.append( ( raw, line_number, - match.start(1) + 1, - _evidence(cleaned_line, match.start(1) + 1), + match.start + 1, + _evidence(cleaned_line, match.start + 1), ) ) if len(candidates) >= MAX_RAW_REFERENCE_CANDIDATES: @@ -111,9 +209,9 @@ def _candidate_strings( heapq.heappush( pending, ( - next_match.start(1), - next_match.end(1), + next_match.start, pattern_index, + next_match.end, next_match, ), ) @@ -122,12 +220,14 @@ def _candidate_strings( def _normalize_candidate(raw: str, source_path: str) -> str | None: """Return a contained relative POSIX candidate, or None when unsupported.""" - raw = unquote(raw.strip().strip("<>")) + raw = raw.strip() split = urlsplit(raw) if split.scheme or split.netloc or raw.startswith(("/", "\\", "#")): return None - path_part = split.path.replace("\\", "/") - if not path_part: + # Split URI syntax before decoding so %23/%3F remain filename characters. + # Decode exactly once, then apply containment checks to the decoded path. + path_part = unquote(split.path).replace("\\", "/") + if not path_part or path_part.startswith("/"): return None if len(path_part) >= 2 and path_part[1] == ":": return None @@ -206,7 +306,7 @@ def resolve_bundle_references_with_metadata( resolved_target = target status = "resolved" disposition = ArtifactDisposition.ANALYZED - elif "/" not in raw.replace("\\", "/"): + elif "/" not in unquote(raw).replace("\\", "/"): matches = basename_index.get(PurePosixPath(target).name, []) if len(matches) == 1: resolved_target = matches[0] diff --git a/tests/test_reference_destinations.py b/tests/test_reference_destinations.py new file mode 100644 index 000000000..867040dc4 --- /dev/null +++ b/tests/test_reference_destinations.py @@ -0,0 +1,224 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Resolve Markdown destinations without weakening missing-reference coverage.""" + +import base64 +import json +import os +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from skillspector import references as references_module +from skillspector.cli import app +from skillspector.mcp_server import run_scan +from skillspector.references import resolve_bundle_references_with_metadata + + +@pytest.mark.parametrize( + ("source", "target"), + [ + ("Read [guide]().", "docs/user guide.md"), + (r'Read [guide]( "A \"quoted\" title").', "docs/user guide.md"), + (r"Read [guide]( 'A \'quoted\' title').", "docs/user guide.md"), + (r"Read [guide]( (A \(quoted\) title)).", "docs/user guide.md"), + (r"Read [guide](<\.md>).", ".md"), + (r"Read [manual](\).", ""), + ("Read [guide]().", "docs/user.md guide.md"), + ("Read [guide](docs/guide(v1).md).", "docs/guide(v1).md"), + (r"Read [guide](docs/guide\(v1\).md).", "docs/guide(v1).md"), + ("Read [guide](docs/guide(a(b(c))).md).", "docs/guide(a(b(c))).md"), + ('Read [guide](docs/guide.md "Guide title").', "docs/guide.md"), + ("Read [guide][manual].\n\n[manual]: docs/user%20guide.md", "docs/user guide.md"), + ("Read [guide][manual].\n\n[manual]: ", "docs/user guide.md"), + ("Read [guide][manual].\n\n[manual]: docs/guide(v1).md", "docs/guide(v1).md"), + ("Read [guide](docs/part%23one.md#summary).", "docs/part#one.md"), + ("Read [guide](docs/part%3Fone.md?view=1).", "docs/part?one.md"), + ("Read [guide](docs/part%252Fone.md).", "docs/part%2Fone.md"), + ("Read [guide](docs/caf%C3%A9.md).", "docs/café.md"), + ("Read [manual](tool.1).", "tool.1"), + ("Read `tool.1`.", "tool.1"), + ], +) +@pytest.mark.parametrize("present", [True, False]) +def test_markdown_destinations_preserve_present_and_missing_targets( + tmp_path: Path, source: str, target: str, present: bool +) -> None: + result = resolve_bundle_references_with_metadata( + tmp_path, + source_path="SKILL.md", + source_text=source, + known_paths=["SKILL.md", target] if present else ["SKILL.md"], + ) + assert result.complete is True # Extraction completed; resolution may be missing. + assert result.records + assert {record["status"] for record in result.records} == {"resolved" if present else "missing"} + assert {record["target_path"] for record in result.records} == {target if present else None} + + +@pytest.mark.parametrize( + "target", + ["../outside.md", "%2e%2e/outside.md", "%2Foutside.md", "%5Coutside.md", "C%3A/file.md"], +) +def test_decoded_destination_cannot_escape_bundle(tmp_path: Path, target: str) -> None: + result = resolve_bundle_references_with_metadata( + tmp_path, + source_path="SKILL.md", + source_text=f"Read [guide]({target}).", + known_paths=["SKILL.md"], + ) + assert result.complete is True + assert result.records + assert all(record["status"] == "rejected" for record in result.records) + + +def test_reference_definitions_do_not_restore_slash_prose_false_positives(tmp_path: Path) -> None: + result = resolve_bundle_references_with_metadata( + tmp_path, + source_path="SKILL.md", + source_text="Compare process I/O, reads/writes, and environment/profile settings.", + known_paths=["SKILL.md"], + ) + assert result.complete is True + assert result.records == [] + + +@pytest.mark.parametrize("channel", ["cli", "mcp"]) +@pytest.mark.parametrize("present", [True, False]) +@pytest.mark.parametrize( + ("body", "target"), + [ + ("Read [guide][manual].\n\n[manual]: docs/user%20guide.md", "docs/user guide.md"), + ("Read [guide]().", "docs/user.md guide.md"), + (r'Read [guide]( "A \"quoted\" title").', "docs/user guide.md"), + (r"Read [guide]( 'A \'quoted\' title').", "docs/user guide.md"), + (r"Read [guide]( (A \(quoted\) title)).", "docs/user guide.md"), + (r"Read [guide](<\.md>).", ".md"), + (r"Read [manual](\).", ""), + ("Read [guide](docs/guide(v1).md).", "docs/guide(v1).md"), + ("Read [guide](docs/part%23one.md#summary).", "docs/part#one.md"), + ], +) +async def test_cli_and_mcp_reference_completeness_agree( + tmp_path: Path, body: str, target: str, present: bool, channel: str +) -> None: + if "<" in target and os.name == "nt": + pytest.skip("Literal angle filenames are unsupported on Windows") + (tmp_path / "SKILL.md").write_text( + f"---\nname: reference-control\ndescription: Summarize the guide.\n---\n{body}\n", + encoding="utf-8", + ) + if present: + path = tmp_path / target + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("# Guide\nSummarize the supplied text.\n", encoding="utf-8") + if channel == "cli": + result = CliRunner().invoke( + app, ["scan", str(tmp_path), "--no-llm", "--format", "json", "--fail-on-incomplete"] + ) + assert result.exit_code == (0 if present else 1), result.output + report = json.loads(result.stdout) + else: + result = await run_scan(str(tmp_path), use_llm=False, output_format="json") + assert result["safe_to_install"] is present + report = json.loads(result["report"]) + assert report["execution_successful"] is True + assert report["analysis_completeness"]["is_complete"] is present + assert report["risk_assessment"]["recommendation"] == ("SAFE" if present else "CAUTION") + assert {r["status"] for r in report["analysis_completeness"]["references"]} == { + "resolved" if present else "missing" + } + + +@pytest.mark.parametrize( + ("body", "target"), + [("Read [manual](tool.1).", "tool.1"), (r"Read [manual](\).", "")], +) +async def test_numeric_extension_reference_retains_uninspected_artifact_gap( + tmp_path: Path, body: str, target: str +) -> None: + if "<" in target and os.name == "nt": + pytest.skip("Literal angle filenames are unsupported on Windows") + (tmp_path / "SKILL.md").write_text(f"# Guide\n{body}\n", encoding="utf-8") + if target != "tool.1": + (tmp_path / "tool.1").write_text( + "# Safe decoy\nSummarize supplied text.\n", encoding="utf-8" + ) + # A real raster artifact, independent of its man-page-like filename. + (tmp_path / target).write_bytes( + base64.b64decode( + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAusB9Wl6" + "SAAAAABJRU5ErkJggg==" + ) + ) + result = await run_scan(str(tmp_path), use_llm=False, output_format="json") + report = json.loads(result["report"]) + assert result["safe_to_install"] is False + assert report["analysis_completeness"]["is_complete"] is False + assert any(r["target_path"] == target for r in report["analysis_completeness"]["references"]) + assert any(finding["id"] == "AE1" for finding in report["issues"]) + + +@pytest.mark.parametrize("separator", ["%2F", "%5C"]) +def test_encoded_directory_does_not_fall_back_to_another_basename( + tmp_path: Path, separator: str +) -> None: + result = resolve_bundle_references_with_metadata( + tmp_path, + source_path="SKILL.md", + source_text=f"Read [guide](missing{separator}guide.md).", + known_paths=["SKILL.md", "other/guide.md"], + ) + assert result.complete is True + assert len(result.records) == 1 + assert result.records[0]["status"] == "missing" + assert result.records[0]["target_path"] is None + + +@pytest.mark.parametrize("body", ["[status]: All checks passed.", "[note]: Read the guide."]) +def test_prose_after_bracket_label_is_not_a_reference(tmp_path: Path, body: str) -> None: + result = resolve_bundle_references_with_metadata( + tmp_path, source_path="SKILL.md", source_text=body, known_paths=["SKILL.md"] + ) + assert result.complete is True + assert result.records == [] + + +def test_escaped_angle_filename_does_not_resolve_different_file(tmp_path: Path) -> None: + result = resolve_bundle_references_with_metadata( + tmp_path, + source_path="SKILL.md", + source_text=r"Read [manual](\).", + known_paths=["SKILL.md", "tool.1"], + ) + assert len(result.records) == 1 + assert result.records[0]["status"] == "missing" + assert result.records[0]["target_path"] is None + + +def test_malformed_markdown_openings_observe_deadline_before_yield( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + original = references_module._MARKDOWN_REFERENCE_START + observed = 0 + + class CountedStarts: + def finditer(self, line: str): + nonlocal observed + for match in original.finditer(line): + observed += 1 + yield match + + monkeypatch.setattr(references_module, "_MARKDOWN_REFERENCE_START", CountedStarts()) + result = resolve_bundle_references_with_metadata( + tmp_path, + source_path="SKILL.md", + source_text="[x](" * 5000, + known_paths=["SKILL.md"], + clock=lambda: observed / 1000, + ) + assert "runtime" in result.limitations + assert result.complete is False + assert observed <= 2001