github/deer-flow
1
0
Fork 0
mirror of https://github.com/bytedance/deer-flow.git synced 2026-09-25 05:56:18 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(skillscan): report secrets bound by kwargs, defaults, walrus and constant folds (#5691)

Browse Source 
* fix(skillscan): keep secret literals bound by kwargs, defaults and walrus in view

#5648 replaced the line-oriented `name[:=]value` sweep with an AST walk that
only inspected Assign/AnnAssign. A keyword argument, a parameter default and a
walrus all still read as `name=value` to that sweep, so moving an embedded
credential into a call silenced a HIGH-severity rule.

* fix(skillscan): fold constant-built secret values back into the AST scan

The pre-#5648 text sweep reported `API_KEY = "sk-" + "a1b2c3d4e5f6"`,
because its value capture stopped at the first closing quote of `"sk-`
and the remainder was never examined. Splitting the quotes does not
change the bound value, but `ast.Constant` alone now requires it to:
`+`, an adjacent literal run, and a placeholder-free f-string all bind
a compile-time constant that the HIGH-severity rule no longer sees,
while the same bytes in a file Python cannot parse still trip the text
fallback. `_python_secret_literal` folds those forms, so the AST pass
stays a precision-only change for values as well as binding forms.
Runtime-composed values (`os.environ[...] + "…"`, `%`-formatting) stay
unreported, which is the precision #5648 was after.

* fix(skillscan): fold a literal chain without reaching the recursion limit

The constant fold recursed once per operand, so a concatenation of ~1000
literals raised RecursionError on input ast.parse accepts. That exception
escaped past the per-file analyzer guard in scan_skill_dir, which drops every
finding collected for the file, so the deep chain cost the file its other
findings as well. The fold is now a stack walk that collects operands in
source order for either nesting, and the file's remaining rules keep reporting.

* fix(skillscan): report credential defaults on lambda parameters

_python_secret_bindings walked parameter defaults only for FunctionDef and
AsyncFunctionDef, so a credential moved into a lambda default (positional or
keyword-only) escaped the secret-env-assignment gate even though the
line-oriented sweep this rule replaced reported it. ast.Lambda exposes the same
ast.arguments structure, so add it to the tuple and pin both spellings.

Addresses review feedback on #5691.

---------

Co-authored-by: sxh313 <sxh313@users.noreply.github.com>
This commit is contained in:
sxh 2026-09-23 10:16:19 +08:00 committed by GitHub
parent 4117a50676
commit cb349da014
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 191 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

109
backend/packages/harness/deerflow/skills/skillscan/orchestrator.py
View File

@ -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 []

96
backend/tests/test_skillscan_native.py
View File

@ -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"