diff --git a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py index 449999d60..b6be9466f 100644 --- a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py +++ b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py @@ -355,14 +355,104 @@ def _python_secret_assignment_target(node: ast.expr) -> str | None: return None +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. + """ + 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) + 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)) + elif isinstance(node, ast.keyword): + # ``**spread`` carries ``arg=None`` and binds no name of its own. + if node.arg is not None: + bindings.append((node.arg, node.value)) + elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)): + params = [*node.args.posonlyargs, *node.args.args] + if node.args.defaults: + # Positional defaults align with the trailing parameters. + bindings.extend((param.arg, default) for param, default in zip(params[len(params) - len(node.args.defaults) :], node.args.defaults, strict=True)) + bindings.extend((param.arg, default) for param, default in zip(node.args.kwonlyargs, node.args.kw_defaults, strict=True) if default is not None) + return bindings + + +def _python_secret_literal(expr: ast.expr) -> str | None: + """Text of a value Python resolves from source alone, else None. + + The pre-AST sweep reported a hardcoded credential that was spelled as a + concatenation of literals (``API_KEY = "sk-" + "a1b2c3d4"``), because its + line-oriented value capture stopped at the first closing quote. Splitting + the quotes is not obfuscation: the bound value is still the same constant, + so the shapes the sweep saw stay visible here - a literal, an explicit + ``+`` of literals, an implicit (adjacent) literal run, and a placeholder-free + f-string. Anything that needs runtime data - a call, a variable, + ``%``-formatting of a template - is not a literal this rule can assert on. + """ + texts: list[str] = [] + for part in _python_secret_literal_parts(expr): + text = _python_secret_literal_atom(part) + if text is None: + return None + texts.append(text) + return "".join(texts) + + +def _python_secret_literal_parts(expr: ast.expr) -> list[ast.expr]: + """Operands of a concatenation, in source order; a single node for anything else. + + A stack loop rather than recursion: the caller reports a finding from this + value, and a chain deep enough to pass the recursion limit would raise past + the per-file analyzer guard, which drops every other finding for that file. + """ + stack: list[ast.expr] = [expr] + parts: list[ast.expr] = [] + while stack: + node = stack.pop() + if not (isinstance(node, ast.BinOp) and isinstance(node.op, ast.Add)): + parts.append(node) + continue + # Right goes on first so the left operand is collected first: a + # concatenation reads in source order however it is parenthesised. + stack.append(node.right) + stack.append(node.left) + return parts + + +def _python_secret_literal_atom(node: ast.expr) -> str | None: + if isinstance(node, ast.Constant): + value = node.value + if not isinstance(value, (str, bytes, int)): + return None + return value.decode("utf-8", "replace") if isinstance(value, bytes) else str(value) + if isinstance(node, ast.JoinedStr): + parts: list[str] = [] + for part in node.values: + if not isinstance(part, ast.Constant) or not isinstance(part.value, str): + return None + parts.append(part.value) + return "".join(parts) + return None + + def _scan_python_secret_assignments(rel_path: str, text: str) -> list[SecurityFinding]: - """Report embedded Python secrets from real literal assignments, not from raw text. + """Report embedded Python secrets from real literal bindings, not from raw text. A line-oriented sweep cannot tell an annotation (``token: Optional[str]``), a statement colon (``if not api_key:``), or this rule's own remediation (``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``. + 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. @@ -372,20 +462,11 @@ def _scan_python_secret_assignments(rel_path: str, text: str) -> list[SecurityFi except SyntaxError: return _scan_secret_assignments_by_text(rel_path, text) - for node in ast.walk(tree): - if isinstance(node, ast.Assign): - targets, value = list(node.targets), node.value - elif isinstance(node, ast.AnnAssign): - # A bare annotation binds no value at all, so `value` stays None. - targets, value = [node.target], node.value - else: + for name, value in _python_secret_bindings(tree): + literal = _python_secret_literal(value) + if literal is None or _looks_like_placeholder(literal): continue - if not isinstance(value, ast.Constant) or not isinstance(value.value, (str, bytes, int)): - continue - literal = value.value.decode("utf-8", "replace") if isinstance(value.value, bytes) else str(value.value) - if _looks_like_placeholder(literal): - continue - if any(_SECRET_ASSIGNMENT_NAME_RE.match(_python_secret_assignment_target(target) or "") for target in targets): + if _SECRET_ASSIGNMENT_NAME_RE.match(name or ""): return [_finding_for_node("secret-env-assignment", rel_path, value, literal)] return [] diff --git a/backend/tests/test_skillscan_native.py b/backend/tests/test_skillscan_native.py index fedb761c3..43b06ab8e 100644 --- a/backend/tests/test_skillscan_native.py +++ b/backend/tests/test_skillscan_native.py @@ -1475,6 +1475,102 @@ def test_secret_assignment_still_flags_annotated_python_literal(tmp_path: Path) assert _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment")["line"] == 5 +def test_secret_assignment_still_flags_python_keyword_argument(tmp_path: Path) -> None: + """``connect(token="…")`` binds the literal just as firmly as ``token = "…"``. + + The line-oriented sweep this rule replaced reported the keyword form, so the + AST path has to keep reporting it; a caller that only moved the assignment + into a call would otherwise walk out of a HIGH-severity gate. + """ + source = 'client = connect("https://api.example", token="9f8e7d6c5b4a3210ff")\n' + + 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_parameter_default(tmp_path: Path) -> None: + """A credential baked into a parameter default ships inside the skill.""" + source = 'def load(api_key="9f8e7d6c5b4a3210ff"):\n return api_key\n' + + assert _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment")["line"] == 1 + + +def test_secret_assignment_still_flags_python_lambda_parameter_default(tmp_path: Path) -> None: + """A credential baked into a ``lambda`` default binds as firmly as a ``def`` default. + + The line-oriented sweep this rule replaced matched ``name=value`` and so reported the + lambda form too; ``_python_secret_bindings`` walked only ``def``/``async def`` defaults, + so moving the assignment into a lambda walked out of the gate. + """ + source = 'handler = lambda api_key="9f8e7d6c5b4a3210ff": api_key\n' + + 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_lambda_keyword_only_default(tmp_path: Path) -> None: + """The keyword-only lambda spelling binds the same literal and must stay reported.""" + source = 'handler = lambda *, token="9f8e7d6c5b4a3210ff": token\n' + + assert _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment")["line"] == 1 + + +def test_secret_assignment_still_flags_python_walrus_binding(tmp_path: Path) -> None: + """``(token := "…")`` is an assignment written as an expression.""" + source = 'if (secret := "9f8e7d6c5b4a3210ff"):\n use(secret)\n' + + assert _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment")["line"] == 1 + + +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.""" + source = 'import os\n\n\ndef load():\n return connect("https://api.example", token=os.getenv("DEERFLOW_TOKEN"))\n' + + assert _secret_assignments(_scan_python_sample(tmp_path, source)) == [] + + +def test_secret_assignment_still_flags_python_literal_concatenation(tmp_path: Path) -> None: + """``API_KEY = "sk-" + "a1b2c3d4e5f6"`` binds a constant, and the sweep saw it.""" + finding = _finding_by_rule(_scan_python_sample(tmp_path, 'API_KEY = "sk-" + "a1b2c3d4e5f6"\n'), "secret-env-assignment") + + assert finding["line"] == 1 + assert finding["evidence"] == "[redacted]" + assert "a1b2c3d4e5f6" not in repr(finding) + + +def test_secret_assignment_flags_python_literal_chain_that_parses_but_recurses(tmp_path: Path) -> None: + """A long ``+`` chain is valid Python, so folding it must not reach the recursion limit. + + An analyzer exception is caught per file and discards that file's findings, so a + chain deep enough to blow the stack silences every rule for the whole file. + """ + source = 'token = os.getenv("DEERFLOW_TOKEN")\nAPI_KEY = ' + " + ".join(['"a1b2c3d4e5f6"'] * 1000) + "\n" + finding = _finding_by_rule(_scan_python_sample(tmp_path, source), "secret-env-assignment") + + assert finding["line"] == 2 + + +def test_secret_assignment_still_flags_python_placeholder_free_fstring(tmp_path: Path) -> None: + """An f-string with no interpolated field is a literal written oddly.""" + finding = _finding_by_rule(_scan_python_sample(tmp_path, 'password = f"hunter2-literal"\n'), "secret-env-assignment") + + assert finding["line"] == 1 + + +def test_secret_assignment_ignores_python_runtime_composed_value(tmp_path: Path) -> None: + """Only fully constant values fold; half of this one comes from the host.""" + source = 'import os\n\napi_key = os.environ["DEERFLOW_KEY"] + "a1b2c3d4e5f6"\n' + + assert _secret_assignments(_scan_python_sample(tmp_path, source)) == [] + + def test_secret_assignment_still_flags_non_python_text(tmp_path: Path) -> None: """Non-Python text keeps the line-oriented sweep for config and shell files.""" skill_dir = tmp_path / "demo-skill"