feat(runners): redact sensitive task options from serialized/printed state#1232
Merged
Conversation
…state Add a 'sensitive: True' option meta-flag (sibling of 'internal'). Option values flagged sensitive are masked with [REDACTED] everywhere runner state is persisted, serialized, or printed -- toDict() run_opts, the built command string (toDict cmd + print_command), and debug echoes -- while the real value still travels in-flight to the worker and runs in the actual subprocess. This lets users pass their own addon API keys through the existing opts channel (e.g. the ai task's api_key, now marked sensitive) without the secret landing in the Mongo runner doc, API responses, --driver api egress, or logs. - _base.py: Runner.sensitive_opt_names (resolved from the runner's own opts or its config tree's task classes, memoized) + redaction in resolved_opts. - command.py: build a redacted cmd alongside the real one; emit it from toDict() and print_command. No behavior change for tasks without a sensitive opt (cmd_redacted == cmd). - cli_helper.py: drop the 'sensitive' key before building click options. - ai.py: mark api_key sensitive. - tests: cover both leak surfaces + the non-sensitive no-op path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VWZJdNgJKrn1XHkENKqVo7
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a ChangesSensitive Option Redaction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/unit/test_sensitive_opts.py (1)
35-36: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a direct
print_command()redaction assertion.The PR objective includes masking secrets in printed command output, but
_build()forcesprint_cmd=Falseand no test callsprint_command(). That leaves the user-visible logging path unverified.Also applies to: 44-58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_sensitive_opts.py` around lines 35 - 36, The sensitive-options tests only exercise object construction because _build() forces print_cmd=False, so the printed command redaction path is never verified. Update the test helper or add a dedicated test that instantiates the relevant class with printing enabled and calls print_command() directly, then assert that secret-bearing options are masked in the returned/printed command for the symbols _build() and print_command().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@secator/runners/_base.py`:
- Around line 323-328: The sensitive option discovery in the task traversal is
failing open because both broad exception handlers in the collection logic
silently continue, which can leave sensitive_opt_names incomplete and let
resolved_opts expose secrets. Tighten the handling around the
collect(Task.get_task_class(node.name)) flow in the redaction/metadata
resolution code: catch only expected discovery/tree traversal errors, and on
failure either log a clear redaction-warning or abort the redaction path instead
of swallowing the exception and continuing.
- Around line 332-338: resolved_opts only redacts runtime values, but toDict()
still serializes config and opts where sensitive option metadata can leak
through real defaults. Update the serialization path around resolved_opts and
toDict() so sensitive entries are sanitized in config/opts as well as run_opts,
using the existing sensitive_opt_names and REDACTED_OPT_VALUE logic to mask any
default or metadata tied to sensitive keys.
In `@secator/runners/command.py`:
- Around line 660-667: The option debug logging is still exposing secrets
because `_process_opts()` logs raw values before redaction and `opts_display`
only masks the top-level `value`, leaving nested fields like `conf.default`
untouched. Update the logging path around `_process_opts()`,
`self.debug('options', ...)`, and the `cmd_options` redaction logic to use a
single shared sanitizer that recursively redacts every sensitive field in the
option object, including nested config values, before any debug call.
- Around line 653-667: The command emission paths still use the raw command in
`yielder()`-related output, which can expose sensitive options even though
`print_command()` already redacts them. Update the command display logic to use
the same redacted command source everywhere a message is emitted, replacing
direct `self.cmd` usage in dry-run `Info(message=...)` and worker-start
`cmd_str` with `self._display_cmd()` (or the existing redacted command selection
used in `print_command()`), and keep `cmd_display`/`opts_display` consistent
with the redaction rules in `CommandRunner`.
In `@tests/unit/test_sensitive_opts.py`:
- Around line 10-11: Remove the module-level logging initialization in the test
module so importing it no longer mutates global logging state. Drop the
top-level level/setup_logging call pair from test_sensitive_opts, and if logging
is actually needed for any specific case, move it into a test-local setup inside
the relevant test function or fixture rather than module scope.
---
Nitpick comments:
In `@tests/unit/test_sensitive_opts.py`:
- Around line 35-36: The sensitive-options tests only exercise object
construction because _build() forces print_cmd=False, so the printed command
redaction path is never verified. Update the test helper or add a dedicated test
that instantiates the relevant class with printing enabled and calls
print_command() directly, then assert that secret-bearing options are masked in
the returned/printed command for the symbols _build() and print_command().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 388b0a04-514b-4d38-82a7-5a3bc840d8ab
📒 Files selected for processing (5)
secator/cli_helper.pysecator/runners/_base.pysecator/runners/command.pysecator/tasks/ai.pytests/unit/test_sensitive_opts.py
Address valid findings from the PR review: - Serialized config/opts: an option's `default` can itself be a secret -- ai.api_key defaults to CONFIG.addons.ai.api_key, so the *platform* key was landing in every runner doc via toDict()['config'].opts.<opt>.default, even though run_opts was already redacted. Add Runner._redact_sensitive (recursive, operates on a deep copy so the live config is never mutated) and apply it to the serialized config/opts in toDict(). - Command emission paths that bypassed redaction: the dry-run Info(message=cmd) and the IN_WORKER cmd stream used the raw self.cmd. Route every display/emit site through a new Command.cmd_for_display property (execution still uses the real self.cmd). - Debug echoes: print_command's cmd_options dump masked only the top-level value, leaving a secret-bearing conf.default; and _get_opt_value's verbose 'got opt value' debug logged the raw value/values. Both now redacted. - sensitive_opt_names: stop failing open silently -- narrow the per-node catch to ValueError and log a warning if whole-tree discovery fails (an unresolved set would let a secret through). - tests: cover print_command(), the dry-run emission path, nested-default redaction, and the serialized-config platform-default leak. Drop module-level setup_logging (no global logging mutation on import). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VWZJdNgJKrn1XHkENKqVo7
…er secrets) The serialized config/opts scrub from the prior commit guarded a non-issue: an option default must never be a config-sourced secret. The correct convention is an empty default plus a runtime `passed or CONFIG.addons.*` fallback in the task (ai.api_key was fixed this way in a separate PR), so config/opts carry no secret and need no masking. Kept (these redact the *user-supplied* value, a real secret surface): - cmd_for_display on every emit path (dry-run Info, IN_WORKER stream, toDict, print) - run_opts redaction via resolved_opts - _get_opt_value verbose-debug value/values masking - print_command cmd_options value masking (reverted to value-only, no default) - sensitive_opt_names fail-loud (warn instead of silent swallow) Removed: Runner._redact_sensitive + toDict deepcopy/scrub + the two tests that asserted default redaction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VWZJdNgJKrn1XHkENKqVo7
…t (CRITICAL)
secator-api serves task opts (including defaults) to the UI, so setting the
api_key/api_base opt `default` to a CONFIG value leaked the platform's LLM API
key into the Runner Create form for every user. Default these opts to empty and
fall back to CONFIG.addons.ai.{api_key,api_base} at runtime
(`api_key = passed or CONFIG.addons.ai.api_key`). Supersedes the earlier
`internal` approach. No secret is ever a task-option default now.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P5vSjfkBuGAAHdKxHS3ySm
ocervell
added
the
feature:secret-hardening
ocervell
commented
Jul 5, 2026
Comment on lines
+291
to
+334
| @property | ||
| def sensitive_opt_names(self): | ||
| """Names of options flagged ``sensitive: True`` in their definition. | ||
|
|
||
| Sensitive option values are redacted from any serialized/printed runner state | ||
| (``toDict()``, debug echoes, the built command string) so a user-supplied secret | ||
| (e.g. a BYO addon API key) never lands in the Mongo runner doc, the API response, | ||
| ``--driver api`` egress, or logs. The value still travels in-flight to the worker. | ||
|
|
||
| Resolved once and memoized. Sources, unioned defensively: | ||
| - a direct task instance's own ``opts``/``meta_opts`` (Command / PythonRunner), | ||
| - the task classes referenced by this runner's config tree (Task / Workflow / Scan, | ||
| which carry no ``opts`` of their own). | ||
| """ | ||
| cached = getattr(self, '_sensitive_opt_names_cache', None) | ||
| if cached is not None: | ||
| return cached | ||
| names = set() | ||
|
|
||
| def collect(holder): | ||
| for attr in ('opts', 'meta_opts'): | ||
| confs = getattr(holder, attr, None) | ||
| if isinstance(confs, dict): | ||
| names.update(k for k, v in confs.items() if isinstance(v, dict) and v.get('sensitive')) | ||
|
|
||
| collect(self) | ||
| try: | ||
| from secator.runners.task import Task | ||
| from secator.tree import build_runner_tree, get_flat_node_list | ||
| for node in get_flat_node_list(build_runner_tree(self.config)): | ||
| if node.type != 'task': | ||
| continue | ||
| try: | ||
| collect(Task.get_task_class(node.name)) | ||
| except ValueError: | ||
| # Unknown task name in this node: skip it, but keep scanning the rest. | ||
| continue | ||
| except Exception as e: | ||
| # Discovery failed for the whole tree: we can't enumerate sensitive opts, so | ||
| # redaction may be incomplete. Surface it loudly rather than failing silently | ||
| # (a silent miss here would let a secret through into serialized/printed state). | ||
| logger.warning('Could not resolve sensitive option names for %s (redaction may be incomplete): %s', getattr(self, 'unique_name', self), e) # noqa: E501 | ||
| self._sensitive_opt_names_cache = names | ||
| return names |
Contributor
Author
There was a problem hiding this comment.
This code is way too complicated for just getting sensitive option names. Aim to simplify it. I don't think there is a need for a broad try / except here, or caching.
Contributor
Author
There was a problem hiding this comment.
Done in b71f123. Dropped the memoization (discover_tasks() is already @cached, so it was redundant) and both try/except layers. The exception-based get_task_class lookup is replaced by a non-raising registry lookup — unknown task names just skip, and a direct Command/PythonRunner instance is already covered by collect(self). Net −22/+9, and it now fails closed (the old broad except swallowed discovery errors → could return an incomplete set and leak a secret). Tests: test_sensitive_opts + test_runners green.
…ept) Per review: getting sensitive option names was over-engineered. discover_tasks() is already @cache'd so instance-level memoization was redundant, and the broad try/except failed *open* (a swallowed discovery error silently returned an incomplete set, letting a secret through). Replace the exception-based task lookup with a non-raising registry lookup (unknown task names simply skip; a direct Command/PythonRunner instance is already covered by collect(self)), and drop both the memoization and the try/except. Fail-closed and ~half the code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5vSjfkBuGAAHdKxHS3ySm
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5vSjfkBuGAAHdKxHS3ySm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a
sensitive: Trueoption meta-flag (a sibling of the existinginternalflag). An option value flagged sensitive is masked with[REDACTED]everywhere runner state is persisted, serialized, or printed, while the real value still travels in-flight to the worker and runs in the actual subprocess.This lets users pass their own addon API keys through the existing options channel — e.g. the
aitask'sapi_key(now marked sensitive) — without the secret ever landing in the Mongo runner doc, API responses,--driver apiegress, or logs.Why
We want users to bring their own secrets (BYO addon keys, etc.) per run. The opts channel already carries these to the worker and persists runner state — but it persisted the secret in plaintext. Rather than build a separate config-override channel, we keep secrets on the opts channel and make the data model secret-aware. (Design:
docs/superpowers/specs/2026-06-29-sensitive-options-and-per-run-overrides-design.mdingke-admin.)Leak surfaces covered
toDict()run_opts— Mongo runner doc, API responses,--driver apiegress (_base.Runner.resolved_opts).--api-key X) is masked in thecmdthattoDict()andprint_commandexpose, while the executed subprocess keeps the real value.Accepted, non-user-facing exposures (out of scope, by design)
ps); tasks that read the secret from an env var avoid this.Changes
runners/_base.py—Runner.sensitive_opt_names(resolved from the runner's ownopts/meta_optsor, forTask/Workflow/Scan, from its config tree's task classes; memoized) + redaction inresolved_opts. NewREDACTED_OPT_VALUEconstant.runners/command.py— build a redacted command alongside the real one; emit it fromtoDict()andprint_command. No behavior change for tasks without a sensitive opt (cmd_redacted == cmd, flag off).cli_helper.py— drop thesensitivekey before building click options (it's not a click param).tasks/ai.py— markapi_keysensitive: True.tests/unit/test_sensitive_opts.py— both leak surfaces (run_opts + cmd) for a Command task, theTask-runner path (aiapi_key), and the non-sensitive no-op path.Testing
test_sensitive_opts.py: 7 passing.test_command.py,test_python_runner.py,test_ai_task_opts.py: green (33 total).flake8clean on all changed files.secator x ai --helpstill builds the--api-keyoption (thesensitivekey no longer breaks click).toDict()/printed cmd andrun_optsare masked.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes