From 130a9ab9f035a8f319502f8d5e044ed05962b2aa Mon Sep 17 00:00:00 2001 From: sxh Date: Sun, 4 Oct 2026 11:00:05 +0800 Subject: [PATCH] fix(skillscan): report secrets bound by unpacking and container or conditional literals (#6247) * fix(skillscan): report secrets bound by unpacking and container literals The AST walk behind secret-env-assignment claims "precision, never less coverage", but it only read a single assignment target and a bare string value, so a credential moved into `host, api_key = endpoint, "..."` or into `api_key = ["..."]` stopped being reported even though the pre-#5648 line sweep still reported the same bytes. Pair unpacking targets with values positionally (fixed head and tail only, so a runtime-sized slice names nothing) and assert on each literal a container or conditional holds, in source order. * fix(skillscan): distinguish mapping labels from credentials --------- Co-authored-by: sxh313 Co-authored-by: Willem Jiang --- README.md | 2 + .../harness/deerflow/skills/AGENTS.md | 1 + .../deerflow/skills/skillscan/orchestrator.py | 149 +++++++++++-- backend/tests/test_skill_review_core.py | 14 ++ backend/tests/test_skillscan_native.py | 200 ++++++++++++++++++ 5 files changed, 348 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 2a46a1197d..0b75733435 100644 --- a/README.md +++ b/README.md @@ -1279,6 +1279,8 @@ If a trusted operator manages the configured skills directory through an externa Skill installs and agent-managed skill edits run through **SkillScan**, a native deterministic safety scanner before the LLM-based skill scanner. Phase 1 runs offline with no Semgrep/OpenGrep dependency, blocks high-confidence `CRITICAL` findings such as private keys or shell execution, and passes warning findings to the LLM scanner for contextual review. Code files (anything under `scripts/`, a script suffix such as `.py`, `.sh`, or `.js`, or an extensionless file starting with `#!`) that are not NUL-free UTF-8 text raise a warning and are still analyzed over a lossy decode, so a single stray byte cannot hide them from `CRITICAL` checks. The moderation adapter normalizes both plain-text model responses and LangChain Responses API text blocks before parsing the required JSON decision. Python instance-client exfiltration checks follow a minimal same-scope evidence chain: a simple name bound to a known client constructor, optional name-to-name aliases, and an actual outbound method or context-manager use supported by that constructor. Constructor roots must be proven imports; bare canonical-looking names are not inferred as modules. Nested scopes do not inherit client handles and inherit only constructor import aliases that are never rebound in the enclosing scope. Comprehensions, walrus-bearing statements, annotations, complex binding targets, unsupported operations, and ambiguous branch flows produce no finding from this signal; skipped constructs conservatively invalidate every name they may bind so stale client state cannot create a finding. A deterministic work budget or recursion limit reached by this best-effort analysis does not discard findings already collected for the file. Set `skill_scan.enabled: false` in `config.yaml` to disable only the deterministic analyzers; safe archive extraction and the LLM scanner still run. +For Python credential mappings, SkillScan checks literal values while treating ordinary dictionary keys as labels. A mapping such as `tokens = {"access_token": os.getenv("ACCESS_TOKEN")}` does not report a hardcoded credential. Keys matching a recognized cloud or API token format are still checked as embedded credentials. + DeerFlow also ships with **skill-reviewer**, a public skill for read-only skill quality review. It uses the built-in `review_skill_package` tool to inspect installed skills, local packages, archives, or pasted `SKILL.md` content without activating the target skill, binding its secrets, executing its scripts, or installing it. The tool returns a compact, tag-neutralized JSON payload to the model context and keeps the full raw review payload in the tool artifact for programmatic consumers. The deterministic review core reuses DeerFlow parsing and SkillScan facts, emits versioned JSON contracts under `contracts/skill_review/`, and can be run from the backend CLI: ```bash diff --git a/backend/packages/harness/deerflow/skills/AGENTS.md b/backend/packages/harness/deerflow/skills/AGENTS.md index 91cbb066a5..723471196e 100644 --- a/backend/packages/harness/deerflow/skills/AGENTS.md +++ b/backend/packages/harness/deerflow/skills/AGENTS.md @@ -15,6 +15,7 @@ - **Installation**: `POST /api/skills/install` extracts .skill ZIP archive to custom/ directory - **Managed integrations**: Lark/Feishu CLI support installs one global official `lark-*` pack as read-only `SkillCategory.INTEGRATION` entries under `/mnt/skills/integrations/lark-cli/...`; enabled flags, app configuration, and OAuth data remain per-user. Install resolves the newest `larksuite/cli` release from GitHub (`releases/latest`) at install time (falling back to a bottom-line pinned version if the lookup fails) rather than hard-coding the pack version; integrity relies on the official host + structural archive guards + a recorded hash of the effective installed tree after shared guidance injection (not a pinned archive-byte SHA, which GitHub does not keep stable). The Gateway image still installs a pinned `@larksuite/cli` binary, so `get_lark_integration_status` surfaces `latest_available_version` and `runtime_version_mismatch` for the UI. AIO installs additionally verify and publish official Linux amd64/arm64 binaries under `{DEER_FLOW_HOME}/integrations/lark-cli/sandbox-cli`, mounted read-only at `/mnt/integrations/lark-cli/runtime`; `/mnt/integrations/lark-cli/config` (app credentials, incl. the long-lived `appSecret`) is mounted **read-only** into the sandbox, its empty `config/locks` subdirectory is over-mounted writable for `lark-cli` coordination files, and `/mnt/integrations/lark-cli/data` (refreshable OAuth tokens) stays writable, all mapping to owner-only per-user directories. **Sandbox trust boundary:** the credential-bearing config and data dirs are still *readable* by arbitrary sandbox processes (the agent's `bash` tool, or code reached via prompt-injection in a tool result), so the app secret and tokens are exposed to sandbox-side code even though they never reach the browser — the read-only config mount only prevents in-sandbox tampering, not read/exfiltration. The sidecar credential-broker (Pattern B, issue #4338) is the fix that removes these plaintext mounts from sandbox execution: set `LARK_CLI_BROKER_IMAGE` on the provisioner (see `docker/lark-cli-broker/`) and the Gateway sends `provision_lark_cli_broker` on sandbox create. The provisioner then runs a `lark-cli-broker` sidecar that owns the per-user `config`/`config/locks`/`data` mounts (mounted into the **sidecar only**, at `/var/lark/{config,config/locks,data}` with only the nested locks mount writable) and serves the `lark-cli` command surface on Pod loopback (`http://127.0.0.1:8788`); a shim init container (`install-shim`) writes a forwarding `lark-cli` into the shared runtime `emptyDir`, so the sandbox gets `DEERFLOW_LARK_BROKER_URL` + a shim on PATH but **no** credential files. The on-PATH `bin/lark-cli` is a `/bin/sh` launcher that resolves a Python 3 interpreter and execs the Python shim body (`bin/lark-cli-shim.py`) beside it, so broker mode does not silently ENOEXEC on a sandbox image without a `#!/usr/bin/env python3`-resolvable interpreter — it fails loudly (exit 127, actionable message) and can be pinned with `DEERFLOW_LARK_BROKER_PYTHON`. The broker runs `lark-cli` in the sidecar's cwd and cannot see the sandbox filesystem, so cwd is intentionally **not** forwarded and file-I/O subcommands relative to the sandbox cwd are unsupported (command surface only). An optional `DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS` denylist (comma-separated command paths, matched in order through interleaved option values, forwarded from the provisioner) lets the broker refuse secret-dumping subcommands before spawning the binary. `lark_cli_env_overlay(broker=True)` therefore omits `LARKSUITE_CLI_CONFIG_DIR`/`DATA_DIR`; `sandbox_lark_broker_active()` (TTL-cached provisioner `/api/capabilities` probe, tight timeout + longer negative caching on the bash hot path) selects broker vs. binary mode for both the bash env overlay and status. `DEER_FLOW_LARK_CLI_SANDBOX_RUNTIME_DIR` supplies a validated, symlink-free pre-staged runtime for air-gapped deployments. For the remote provisioner (K8s), the runtime binary is otherwise provisioned by an optional init container + shared `emptyDir` (Pattern A): set `LARK_CLI_INIT_IMAGE` on the provisioner (see `docker/lark-cli-init/`) and the Gateway sends `provision_lark_cli_runtime` on sandbox create once the pack is installed, so remote installs skip the Gateway-side GitHub download entirely. Broker (Pattern B) supersedes the init-container binary (Pattern A) when both images are configured. `get_lark_integration_status(check_runtime=True)` surfaces `sandbox_runtime_mode` (`none` / `gateway-download` / `init-container` / `broker`) and `sandbox_runtime_ready` (remote modes read the provisioner `GET /api/capabilities`: `lark_cli_init_image` / `lark_cli_broker_image`), plus `sandbox_runtime_probed` separating a confirmed probe result from the conservative unprobed fallback (mutation responses carry a probed status built from a credential snapshot taken under the credential lock), so a green UI can't hide a chat-time `lark-cli: command not found`. Cheap status probes are explicitly not live-verified; users authorize or reconnect through the browser device-flow endpoints instead of running terminal commands. - **SkillScan**: `packages/harness/deerflow/skills/skillscan/` is the native deterministic scanner for `.skill` archives and agent-managed skill writes. It runs offline before the LLM scanner, emits structured findings (`rule_id`, `severity`, `file`, `line`, `message`, `remediation`, redacted `evidence` — category/analyzer are encoded in the `rule_id` prefix), blocks `CRITICAL`, and passes warning findings into `scan_skill_content()`. The moderation adapter must normalize both plain-text responses and LangChain Responses API text blocks before parsing the required JSON decision. `scan_archive_preflight()` / `scan_skill_dir()` are pure sync functions (dispatch off the event loop); `enforce_static_scan()` applies the blocking policy and the `skill_scan.enabled` kill switch. `skills/package_files.py` is the single definition of code files (`scripts/` members, code suffixes, extensionless `#!` files) and executable magic bytes; the installer, export guard, and SkillScan all import it, so do not re-derive either rule locally. Files that fail NUL-free UTF-8 decoding are treated as binaries unless they are code files: interpreters still run those, so they raise `package-undecodable-script` (HIGH) and are analyzed over a lossy decode, keeping one stray byte from hiding a file or downgrading a `CRITICAL` match; one with executable magic skips the text rules, which only misread its string tables. The Python instance-client signal deliberately follows only a one-level, same-scope evidence chain (PR #4265 review): a proven imported constructor bound to a simple name, optional name-to-name alias propagation, rebinding invalidation, and a constructor-supported outbound method or context-manager use; bare canonical-looking names never fall back to module identity. Nested scopes never inherit client handles and inherit only constructor aliases proven stable by a binding-only enclosing-scope prepass. Comprehensions, walrus-bearing statements, annotations, executable expressions inside complex binding targets, unsupported operations, and ambiguous flows produce no finding from this signal; skipped constructs invalidate all names they may bind, while representative false negatives are pinned by `test_python_declared_false_negatives_stay_unreported`. Compound bodies are walked from isolated copies so wrapping code in `if True:` is not a bypass, while copied scope entries, binding-only prepasses, and AST visits consume a deterministic work budget and the walk stops after its first sink. Budget or recursion exhaustion skips only this best-effort signal and retains deterministic findings already collected for the file. Do not add Semgrep/OpenGrep or YAML rule-engine dependencies to the core path; Phase 1 rule specs live in Python constants next to their analyzers in `skillscan/orchestrator.py`. +- **Python secret mappings**: `secret-env-assignment` scans literal container values and conditional branches under secret-like bindings. Dictionary keys are structural labels and contribute candidates only when their folded literal matches the shared `_SECRET_TOKEN_PATTERNS`; they must not inherit credential evidence from the enclosing variable name. Opaque keys without a recognized token format stay unreported. Regressions in `tests/test_skillscan_native.py` cover runtime-loaded mappings, nested labels, credential-value locations, and literal/concatenated token keys; `tests/test_skill_review_core.py` pins the CI CLI gate. - **Skill Review Core**: `packages/harness/deerflow/skills/review/` provides read-only package snapshots, deterministic facts, resource/eval analysis, report rendering, and the CLI (`python -m deerflow.skills.review.cli`). It reuses the shared frontmatter helper and SkillScan; it must not import `app.*`, execute target scripts, install dependencies, or call networks. JSON contracts live in `contracts/skill_review/`. The analyzer hands SkillScan a byte-faithful copy of every non-symlink snapshot file (binary entries carry `content_base64`), so package rules such as `package-executable-binary` cover binaries; only eval-fixture `SKILL.md` samples are withheld, while scripts and other files under `evals/fixtures/` are scanned. Materialization uses exclusive create, so a duplicate archive member or a case-folded name fails the scan closed (`not_assessed: skillscan`) instead of overwriting an earlier file, as does an entry without bytes in an untruncated snapshot. The `review_skill_package` built-in tool labels results with `review_subject_entry` and never `skill_context_entry`, so reviewing a target does not activate it, bind its `required-secrets`, or apply its `allowed-tools`. Its model-visible `ToolMessage.content` is a compact JSON payload with untrusted control tags neutralized; the full raw review payload, including Markdown renders, stays in `ToolMessage.artifact`. CI should run the CLI with `--fail-on error --fail-on-incomplete` so blocker/error findings and truncated/not-assessed packages fail the gate. The public `skills/public/skill-reviewer` skill owns semantic readiness review and suggestions only; mutation and runtime experiments remain owned by `skill-creator`. #### Request-Scoped Secrets (`required-secrets`) diff --git a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py index b6be9466fa..4f957ee1c8 100644 --- a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py +++ b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py @@ -118,6 +118,15 @@ _PLACEHOLDER_VALUES = {"", "x", "xx", "xxx", "xxxx", "changeme", "change-me", "e # analyzed from its AST instead, because a regex cannot tell an annotation from a value. _SECRET_ASSIGNMENT_RE = re.compile(r"(?im)\b(token|password|passwd|api[_-]?key|secret|credential)s?\b\s*[:=]\s*[\"']?([^\"'\s#]+)") _SECRET_ASSIGNMENT_NAME_RE = re.compile(r"(?i)^(?:token|password|passwd|api[_-]?key|secret|credential)s?$") +_SECRET_TOKEN_PATTERNS = tuple( + re.compile(pattern) + for pattern in ( + r"\b(?:AKIA|ASIA)[A-Z0-9]{16}\b", + r"\bgh[pousr]_[A-Za-z0-9_]{20,}\b", + r"\bxox[baprs]-[A-Za-z0-9-]{20,}\b", + r"\bsk-[A-Za-z0-9]{20,}\b", + ) +) _SENSITIVE_PATH_RE = re.compile(r"(~/.ssh|/etc/passwd|/etc/shadow|/var/run/docker\.sock|docker\.sock|169\.254\.169\.254)") _EXTERNAL_HTTP_RE = re.compile(r"http://([A-Za-z0-9.-]+)(?::\d+)?(?:/|\b)") _URL_RE = re.compile(r"https?://[^\s)'\"<>]+") @@ -315,14 +324,8 @@ def _scan_secrets(rel_path: str, text: str) -> list[SecurityFinding]: if private_key: findings.append(_finding_from_match("secret-private-key", rel_path, text, private_key)) - token_patterns = [ - r"\b(?:AKIA|ASIA)[A-Z0-9]{16}\b", - r"\bgh[pousr]_[A-Za-z0-9_]{20,}\b", - r"\bxox[baprs]-[A-Za-z0-9-]{20,}\b", - r"\bsk-[A-Za-z0-9]{20,}\b", - ] - for pattern in token_patterns: - match = re.search(pattern, text) + for pattern in _SECRET_TOKEN_PATTERNS: + match = pattern.search(text) if match and not _looks_like_placeholder(match.group(0)): findings.append(_finding_from_match("secret-cloud-token", rel_path, text, match)) break @@ -355,18 +358,71 @@ def _python_secret_assignment_target(node: ast.expr) -> str | None: return None +def _python_secret_unpacked_span(elts: list[ast.expr]) -> tuple[int, int, bool]: + """Fixed head, fixed tail and star presence of one side of an unpacking. + + Python aligns everything before the first ``*`` and everything after the last one; only + the elements between stars share a runtime-determined slice. A side without a star has + its whole length fixed, so its head is every element and its tail is none. + """ + stars = [index for index, element in enumerate(elts) if isinstance(element, ast.Starred)] + if not stars: + return len(elts), 0, False + return stars[0], len(elts) - stars[-1] - 1, True + + +def _python_secret_unpacked_pairs(target: ast.expr, value: ast.expr) -> list[tuple[ast.expr, ast.expr]] | None: + """Target-and-value pairs of an unpacking assignment, or None when it is not one. + + ``host, api_key = endpoint, "…"`` binds each name to the value written in the same + position, so the two sides have to be read together. Pairs come only from the fixed + head and tail of the two element lists (``_python_secret_unpacked_span``): what falls + between two stars is a runtime slice. An unpacking with no star on either side binds + only when the lengths match, so a mismatch -- which raises before binding anything -- + contributes none. Nested targets align positionally too, so they are read the same way. + """ + if not isinstance(target, (ast.Tuple, ast.List)): + return None + if not isinstance(value, (ast.Tuple, ast.List)): + return [] + target_head, target_tail, target_starred = _python_secret_unpacked_span(target.elts) + value_head, value_tail, value_starred = _python_secret_unpacked_span(value.elts) + if not target_starred and not value_starred: + if len(target.elts) != len(value.elts): + return [] + head, tail = len(target.elts), 0 + else: + # A star-free side is fully fixed, so its tail is whatever its head did not consume. + head = min(target_head, value_head) + tail = min(len(target.elts) - head if not target_starred else target_tail, len(value.elts) - head if not value_starred else value_tail) + pairs = list(zip(target.elts[:head], value.elts[:head], strict=True)) + if tail: + pairs += list(zip(target.elts[len(target.elts) - tail :], value.elts[len(value.elts) - tail :], strict=True)) + expanded: list[tuple[ast.expr, ast.expr]] = [] + for paired_target, paired_value in pairs: + nested = _python_secret_unpacked_pairs(paired_target, paired_value) + expanded.extend(nested or [(paired_target, paired_value)]) + return expanded + + def _python_secret_bindings(tree: ast.AST) -> list[tuple[str | None, ast.expr]]: """Every ``(bound name, value expression)`` pair the tree binds, in walk order. Assignment statements are not the only place a skill can park a credential: a keyword argument, a parameter default and a walrus all read as ``name=value`` to the line-oriented sweep this rule replaced, so a caller - that merely moves the assignment into a call escapes the gate. + that merely moves the assignment into a call escapes the gate. An unpacking + assignment reads the same way once its targets are paired with values. """ bindings: list[tuple[str | None, ast.expr]] = [] for node in ast.walk(tree): if isinstance(node, ast.Assign): - bindings.extend((_python_secret_assignment_target(target), node.value) for target in node.targets) + for target in node.targets: + pairs = _python_secret_unpacked_pairs(target, node.value) + if pairs is None: + bindings.append((_python_secret_assignment_target(target), node.value)) + continue + bindings.extend((_python_secret_assignment_target(paired_target), paired_value) for paired_target, paired_value in pairs) elif isinstance(node, (ast.AnnAssign, ast.NamedExpr)): # A bare annotation binds no value at all, so ``AnnAssign.value`` is None. bindings.append((_python_secret_assignment_target(node.target), node.value)) @@ -441,6 +497,57 @@ def _python_secret_literal_atom(node: ast.expr) -> str | None: return None +def _python_secret_literal_members(expr: ast.expr) -> list[ast.expr]: + """Values held inside a container or a conditional, in the order they are written. + + ``api_key = ["…"]``, ``api_key = {"live": "…"}`` and ``api_key = "…" if prod else + "…"`` are each a finding for the line-oriented sweep this rule replaced: its value + capture stops at the first quote after the name, so it reports the line even when the + token it grabbed is only a bracket. This names the literal instead of the bracket. + Each member is asserted on by itself, because joining a container's members would + describe a string that no Python program ever binds. + + Mapping keys are structural labels, so they need independent evidence from a + recognized token format; the enclosing secret-like name is evidence only for values. + """ + if isinstance(expr, (ast.List, ast.Tuple, ast.Set)): + return list(expr.elts) + if isinstance(expr, ast.Dict): + # A ``{**spread}`` entry carries a None key and names nothing of its own. + members: list[ast.expr] = [] + for key, value in zip(expr.keys, expr.values, strict=True): + if key is not None: + literal = _python_secret_literal(key) + if literal is not None and any(pattern.search(literal) for pattern in _SECRET_TOKEN_PATTERNS): + members.append(key) + members.append(value) + return members + if isinstance(expr, ast.IfExp): + return [expr.body, expr.orelse] + return [] + + +def _python_secret_literal_candidates(expr: ast.expr) -> list[tuple[ast.expr, str]]: + """Every ``(node, literal)`` pair this binding can assert on, in source order. + + A concatenation yields the whole value, because ``"sk-" + "a1b2"`` binds one + string; a container or a conditional yields its members. A work list rather than + recursion, matching ``_python_secret_literal_parts``: an expression tree holds each + node once, so a node queued from its parent is never queued again. + """ + candidates: list[tuple[ast.expr, str]] = [] + pending: list[ast.expr] = [expr] + while pending: + node = pending.pop() + literal = _python_secret_literal(node) + if literal is not None: + candidates.append((node, literal)) + continue + pending.extend(_python_secret_literal_members(node)) + candidates.sort(key=lambda candidate: (candidate[0].lineno, candidate[0].col_offset)) + return candidates + + def _scan_python_secret_assignments(rel_path: str, text: str) -> list[SecurityFinding]: """Report embedded Python secrets from real literal bindings, not from raw text. @@ -449,13 +556,18 @@ def _scan_python_secret_assignments(rel_path: str, text: str) -> list[SecurityFi (``api_key = os.getenv("X")``) from a literal, and it points at an annotated assignment's annotation rather than at its value. - What it gains is precision, never less coverage: every binding form the - sweep reported stays reported, per ``_python_secret_bindings``, and so does - every literal value shape it saw, per ``_python_secret_literal``. + ``_python_secret_bindings`` preserves the sweep's binding forms, and + ``_python_secret_literal_candidates`` covers literal values inside containers + and conditionals. Mapping labels do not inherit credential evidence from the + enclosing name; only keys matching a recognized token format are candidates. A file Python cannot parse falls back to that sweep: the AST is only an improvement, and returning nothing would let one syntax error (or a NUL byte) - silence a HIGH-severity rule for the whole file. + silence a HIGH-severity rule for the whole file. That sweep reads the run of + characters that follows ``name[:=]``, so it never reaches a credential an earlier + element of the same line precedes (``host, api_key = "https://…", "…"``) or a bracket + separates from its name (``[token, version] = ["…", 2]``): those two shapes are + reported while the file parses and go quiet once it carries a syntax error. """ try: tree = ast.parse(text) @@ -463,11 +575,12 @@ def _scan_python_secret_assignments(rel_path: str, text: str) -> list[SecurityFi return _scan_secret_assignments_by_text(rel_path, text) for name, value in _python_secret_bindings(tree): - literal = _python_secret_literal(value) - if literal is None or _looks_like_placeholder(literal): + if not _SECRET_ASSIGNMENT_NAME_RE.match(name or ""): continue - if _SECRET_ASSIGNMENT_NAME_RE.match(name or ""): - return [_finding_for_node("secret-env-assignment", rel_path, value, literal)] + for node, literal in _python_secret_literal_candidates(value): + if _looks_like_placeholder(literal): + continue + return [_finding_for_node("secret-env-assignment", rel_path, node, literal)] return [] diff --git a/backend/tests/test_skill_review_core.py b/backend/tests/test_skill_review_core.py index 7b3f1bacbe..f27f5fa0d6 100644 --- a/backend/tests/test_skill_review_core.py +++ b/backend/tests/test_skill_review_core.py @@ -451,6 +451,20 @@ def test_skillscan_high_findings_are_review_errors(tmp_path): assert finding["skillscan_severity"] == "HIGH" +@pytest.mark.parametrize("hardcoded", [False, True]) +def test_cli_mapping_credentials_gate_on_values(tmp_path, capsys, hardcoded): + _write(tmp_path / "SKILL.md", _valid_skill() + "\nRead [loader](scripts/load.py).\n") + value = '"9f8e7d6c5b4a3210ff"' if hardcoded else 'os.getenv("ACCESS_TOKEN")' + _write(tmp_path / "scripts" / "load.py", f'import os\ntokens = {{"access_token": {value}, "refresh_token": os.getenv("REFRESH_TOKEN")}}\n') + + exit_code = review_cli_main([str(tmp_path), "--format", "json", "--fail-on", "error", "--fail-on-incomplete"]) + report = json.loads(capsys.readouterr().out) + + assert exit_code == (1 if hardcoded else 0) + assert any(finding["rule_id"] == "secret-env-assignment" for finding in report["findings"]) is hardcoded + assert "9f8e7d6c5b4a3210ff" not in repr(report) + + def test_skillscan_ignores_eval_fixture_skill_markdown(tmp_path): _write(tmp_path / "SKILL.md", _valid_skill()) _write( diff --git a/backend/tests/test_skillscan_native.py b/backend/tests/test_skillscan_native.py index 43b06ab8ed..30de784542 100644 --- a/backend/tests/test_skillscan_native.py +++ b/backend/tests/test_skillscan_native.py @@ -1528,6 +1528,206 @@ def test_secret_assignment_still_flags_python_walrus_binding(tmp_path: Path) -> assert _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment")["line"] == 1 +@pytest.mark.parametrize( + "source", + [ + 'host, api_key = "internal", "9f8e7d6c5b4a3210ff"\n', + 'host, api_key = "https://internal.example", "9f8e7d6c5b4a3210ff"\n', + ], +) +def test_secret_assignment_still_flags_python_tuple_unpacking(tmp_path: Path, source: str) -> None: + """``host, api_key = "internal", "…"`` binds the literal to ``api_key``. + + The sweep reported that form (it matched the line and took the first quoted value), + so unpacking was a way to walk out of a HIGH-severity gate; ``_python_secret_assignment_target`` + only understood a single target node. The second case goes further: there the sweep + took the URL as the value, read it as a placeholder and stayed silent too, so neither + implementation saw it. + """ + finding = _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment") + + assert finding["line"] == 1 + assert finding["evidence"] == "[redacted]" + assert "9f8e7d6c5b4a3210ff" not in repr(finding) + + +def test_secret_assignment_still_flags_python_list_target_unpacking(tmp_path: Path) -> None: + """A list target unpacks the same way a tuple target does; the brackets are syntax.""" + source = '[token, version] = ["9f8e7d6c5b4a3210ff", 2]\n' + + assert _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment")["line"] == 1 + + +def test_secret_assignment_ignores_python_unpacking_of_environment_lookups(tmp_path: Path) -> None: + """The precision gain has to survive the unpacked form too. + + ``api_key, url = os.getenv("K"), endpoint`` binds both values at runtime, so neither + is a literal this rule can assert on. + """ + source = 'api_key, url = os.getenv("DEERFLOW_TOKEN"), "https://api.example"\n' + + assert _secret_assignments(_scan_python_sample(tmp_path, source)) == [] + + +def test_secret_assignment_still_flags_python_conditional_literal(tmp_path: Path) -> None: + """``api_key = "…" if prod else "x"`` ships the literal in the file.""" + source = 'api_key = "9f8e7d6c5b4a3210ff" if prod else "x"\n' + + finding = _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment") + + assert finding["line"] == 1 + assert "9f8e7d6c5b4a3210ff" not in repr(finding) + + +@pytest.mark.parametrize( + "source", + [ + 'api_key = ["9f8e7d6c5b4a3210ff"]\n', + 'api_key = ("https://internal.example", "9f8e7d6c5b4a3210ff")\n', + 'api_key = {"live": "9f8e7d6c5b4a3210ff"}\n', + ], +) +def test_secret_assignment_still_flags_python_container_literal(tmp_path: Path, source: str) -> None: + """A credential inside a list, tuple or dict literal is still in the package.""" + finding = _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment") + + assert finding["line"] == 1 + assert "9f8e7d6c5b4a3210ff" not in repr(finding) + + +def test_secret_assignment_ignores_python_container_without_literal(tmp_path: Path) -> None: + """Only values Python resolves from source are asserted on: a container of runtime + data (a call, a variable) binds nothing this rule can name.""" + source = 'api_key = [os.getenv("DEERFLOW_TOKEN"), token_from_config]\n' + + assert _secret_assignments(_scan_python_sample(tmp_path, source)) == [] + + +@pytest.mark.parametrize("key", ['"ghp_a1b2c3d4e5f6g7h8i9j0"', '"ghp_" + "a1b2c3d4e5f6g7h8i9j0"']) +def test_secret_assignment_still_flags_python_dict_key_literal(tmp_path: Path, key: str) -> None: + """A mapping key needs credential evidence of its own: a known token format, + including one spelled as a concatenation of literals. + + The key and the value are put on separate lines so the reported line says which of the + two was asserted on: a value-only reader would name line 3. + """ + source = f'api_key = {{\n {key}:\n "live",\n}}\n' + + finding = _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment") + + assert finding["line"] == 2 + assert finding["evidence"] == "[redacted]" + assert "a1b2c3d4e5f6g7h8i9j0" not in repr(finding) + + +@pytest.mark.parametrize( + "source", + [ + 'tokens = {"access_token": os.getenv("ACCESS_TOKEN"), "refresh_token": os.getenv("REFRESH_TOKEN")}\n', + 'credentials = {"api_key": token_from_config, "password": config["password"]}\n', + 'tokens = {"outer": {"access_token": os.environ["ACCESS_TOKEN"]}}\n', + 'connect(tokens={"access_token": os.getenv("ACCESS_TOKEN")})\n', + 'tokens = {("access_token", "refresh_token"): runtime_tokens}\n', + 'api_key = {123456: os.getenv("API_KEY")}\n', + 'tokens = {"9f8e7d6c5b4a3210ff": runtime_token}\n', + ], +) +def test_secret_assignment_ignores_python_mapping_labels_with_runtime_values(tmp_path: Path, source: str) -> None: + """Mapping labels do not become credentials because the enclosing name is secret-like.""" + findings = _scan_python_sample(tmp_path, source) + + assert [finding for finding in findings if finding["rule_id"].startswith("secret-")] == [] + + +@pytest.mark.parametrize("value", ['"9f8e7d6c5b4a3210ff"', 'os.getenv("ACCESS_TOKEN") if prod else "9f8e7d6c5b4a3210ff"']) +def test_secret_assignment_reports_python_mapping_credential_value(tmp_path: Path, value: str) -> None: + """A label must neither hide a hardcoded value nor take its source location.""" + source = f'tokens = {{\n "access_token":\n {value},\n}}\n' + + finding = _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment") + + assert finding["line"] == 3 + assert finding["severity"] == "HIGH" + assert finding["evidence"] == "[redacted]" + assert "9f8e7d6c5b4a3210ff" not in repr(finding) + + +def test_secret_assignment_reports_python_container_members_in_source_order(tmp_path: Path) -> None: + """Two literals in one container point at the first one written, so the finding's + location does not depend on how the value expression happens to be traversed.""" + source = 'api_key = [\n "9f8e7d6c5b4a3210ff",\n "7d6c5b4a3210ff9e8",\n]\n' + + assert _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment")["line"] == 2 + + +def test_secret_assignment_still_flags_python_starred_target_prefix(tmp_path: Path) -> None: + """``api_key, *rest = "…", "b"`` binds ``api_key`` to the first element: the star only + absorbs the tail, so the prefix keeps its fixed position.""" + finding = _finding_by_rule(_scan_python_sample(tmp_path, 'api_key, *rest = "9f8e7d6c5b4a3210ff", "b"\n'), "secret-env-assignment") + + assert finding["line"] == 1 + assert finding["evidence"] == "[redacted]" + assert "9f8e7d6c5b4a3210ff" not in repr(finding) + + +def test_secret_assignment_starred_element_value_stays_unreported(tmp_path: Path) -> None: + """The mirror case of the one above: here the credential lands in ``rest``, a list + Python builds at runtime, so no name on that line has a fixed position for it. + Pinned so the limitation is checked, not assumed.""" + source = 'mode, *rest = "prod", "9f8e7d6c5b4a3210ff"\n' + + assert _secret_assignments(_scan_python_sample(tmp_path, source)) == [] + + +def test_secret_assignment_still_flags_python_starred_value_suffix(tmp_path: Path) -> None: + """``mode, api_key = *pair, "…"`` aligns from the right: the element after the star is + the last one, so it reaches the last name whatever the starred sequence's length is.""" + finding = _finding_by_rule(_scan_python_sample(tmp_path, 'mode, api_key = *pair, "9f8e7d6c5b4a3210ff"\n'), "secret-env-assignment") + + assert finding["line"] == 1 + assert finding["evidence"] == "[redacted]" + assert "9f8e7d6c5b4a3210ff" not in repr(finding) + + +def test_secret_assignment_star_on_both_sides_stays_unreported(tmp_path: Path) -> None: + """With a star on both sides the head and the tail can be read, but neither name in + ``api_key, *rest`` has a position that matches the literal on the right. Pinned so the + limitation is checked, not assumed.""" + source = 'api_key, *rest = *pair, "9f8e7d6c5b4a3210ff"\n' + + assert _secret_assignments(_scan_python_sample(tmp_path, source)) == [] + + +def test_secret_assignment_value_star_on_the_last_name_stays_unreported(tmp_path: Path) -> None: + """``api_key, mode = *pair, "…"`` does pair the literal, with ``mode``: the credential + reaches a name this rule is not about, so nothing is reported.""" + source = 'api_key, mode = *pair, "9f8e7d6c5b4a3210ff"\n' + + assert _secret_assignments(_scan_python_sample(tmp_path, source)) == [] + + +def test_secret_assignment_still_flags_python_nested_unpacking(tmp_path: Path) -> None: + """A parenthesised target element unpacks positionally too, so the credential reaches + the name written at its own position rather than the tuple around it.""" + finding = _finding_by_rule(_scan_python_sample(tmp_path, 'host, (user, api_key) = "https://internal.example", ("u", "9f8e7d6c5b4a3210ff")\n'), "secret-env-assignment") + + assert finding["line"] == 1 + assert "9f8e7d6c5b4a3210ff" not in repr(finding) + + +def test_secret_assignment_unpacking_with_mismatched_lengths_stays_unreported(tmp_path: Path) -> None: + """``host, api_key = "a", "b", "c"`` cannot run at all, so there is no position to read + the literal from. Pinned together with a second assignment because the pairing builds + tuples elementwise: a mismatch must skip its own line, not raise and cost the file its + other findings. + """ + source = 'host, api_key = "9f8e7d6c5b4a3210ff", "b", "c"\ntoken = "7d6c5b4a3210ff9e8"\n' + + findings = _secret_assignments(_scan_python_sample(tmp_path, source)) + + assert [finding["line"] for finding in findings] == [2] + + def test_secret_assignment_ignores_python_keyword_environment_lookup(tmp_path: Path) -> None: """The precision gain must survive the new binding forms: a keyword whose value is read from the environment is the documented remediation."""