fix(engine): mask declared secrets in dry-run previews and param-default logs - #36
Merged
Merged
Conversation
…ult logs Env vars listed under the workflow's top-level 'secrets:' key had their raw values echoed by the dry-run step-input preview and the param-defaults log line. Record the declared values at validation time and replace them with '***' in both surfaces (masking runs before the 80-char preview truncation so partial values cannot leak). Tests cover dry-run masking, scoping (undeclared env vars stay unmasked), and the param-default log line via the workflow log file.
There was a problem hiding this comment.
Pull request overview
This PR prevents accidental exposure of environment-variable secrets declared under a workflow’s top-level secrets: key by masking their values (***) in two user-visible surfaces: engine init param-default logging and dry-run step input previews.
Changes:
- Extend
_validate_secrets()to capture current values of declared secret env vars (sorted longest-first for safer overlapping replacements). - Add
_mask_env_values()helper and apply it to:- engine init “Default parameters loaded” log lines
- dry-run step input preview output (masking before the 80-char truncation)
- Add focused tests covering dry-run preview masking, scoping (only declared env vars), and param-default log masking via log file inspection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/yaml_workflow/engine.py |
Records declared secret env values and masks them in default-param logs and dry-run input previews. |
tests/test_secrets.py |
Adds regression tests ensuring declared env values are masked in both affected display surfaces and non-declared env values remain visible. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…equired_env Main's #35 renamed the original secrets helper to _workflow_with_required_env for CodeQL; the masking tests' helper of the same name shadowed it after merge, breaking test_secrets_not_present and test_secrets_invalid_format. Rename it to _masking_workflow.
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
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.
Summary
Env vars listed under a workflow's top-level
secrets:key had their raw values echoed by two display surfaces:execute_step)This PR masks those values as
***in both places.Changes
_validate_secrets()now also records the current values of the declared env vars (longest-first, so overlapping/nested values mask fully)._mask_env_values()helper does value-based replacement, so the secret is caught wherever it surfaces — templated via{{ env.X }}or embedded in a programmatically built workflow dict.Tests
secrets:remain unmasked.Test identifiers/env names avoid the word "secret" where values flow into the engine, to keep CodeQL cleartext-logging name heuristics quiet.
Verification
black --check/isort --check: cleanKnown limitation
Masking is scoped to these two display surfaces; a step that itself echoes the value at real execution time (e.g. a shell task printing it) is unchanged.