Add rule to block toJSON(secrets) expressions#44922
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Hey However, this PR was submitted directly to the repository, which does not match the contribution process for non-core team members.
If you would like to reformulate this as an agentic plan issue, here is a ready-to-use prompt:
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds validation preventing workflows from serializing the complete secrets context.
Changes:
- Detects and reports
toJSON(secrets)based on strict mode. - Integrates detection before expression allowlist validation.
- Adds validation tests and modifies the auto-upgrade schedule.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/expression_secrets_serialization_validation.go |
Implements detection and neutralization. |
pkg/workflow/expression_secrets_serialization_validation_test.go |
Tests strict, non-strict, and safe patterns. |
pkg/workflow/compiler_validators.go |
Integrates the new validator. |
.github/workflows/agentic-auto-upgrade.yml |
Changes the weekly schedule. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Medium
| return ExpressionPatternDotAll.ReplaceAllStringFunc(content, func(match string) string { | ||
| groups := ExpressionPatternDotAll.FindStringSubmatch(match) | ||
| if len(groups) >= 2 && secretsSerializationPattern.MatchString(groups[1]) { | ||
| return "${{ false }}" |
| // toJSON(secrets.SPECIFIC_KEY) because a dot after "secrets" would be consumed | ||
| // by \s*\) only if there is no further content — the closing \) requires that | ||
| // nothing follows "secrets" except optional whitespace. | ||
| var secretsSerializationPattern = regexp.MustCompile(`(?i)\btoJSON\s*\(\s*secrets\s*\)`) |
| } else { | ||
| assert.NoError(t, err, "expected no error") | ||
| } |
| on: | ||
| schedule: | ||
| - cron: "21 3 * * 5" # Weekly (auto-upgrade) | ||
| - cron: "11 4 * * 6" # Weekly (auto-upgrade) |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (3 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add rule to block toJSON(secrets) expressions
The new rule is well-structured and consistent with existing patterns.
One issue: The wantWarning field in TestValidateSecretsSerializationExpressions is declared and set for three test cases but is never asserted — the test body only checks NoError/Error, never verifying that IncrementWarningCount() was actually called. A regression dropping that call in non-strict mode would pass silently.
Everything else looks good
- Regex correctly excludes
toJSON(secrets.KEY)via the closing)constraint neutralizeSecretsSerializationExpressionscleanly prevents double-errors on the non-strict path- Frontmatter YAML is scanned but the allowlist only scans
MarkdownContent, so no double-error risk - Coverage spans strict/non-strict, markdown/frontmatter, case variants, and safe patterns
- Integration wiring in
validateExpressionsis correct and ordered before the allowlist check
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 34.4 AIC · ⌖ 5.46 AIC · ⊞ 4.8K
| strictMode bool | ||
| wantError bool | ||
| errorContains string | ||
| wantWarning bool |
There was a problem hiding this comment.
The wantWarning field is declared and set in several test cases but is never checked in the assertion block — the test runner never verifies that IncrementWarningCount was actually called.
Consider adding an assertion like:
if tt.wantWarning {
assert.Equal(t, 1, compiler.GetWarningCount(), "expected exactly one warning")
}(adjust the accessor name to whatever the Compiler exposes.)
Without this, a regression that silently skips c.IncrementWarningCount() in non-strict mode would pass all tests.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — commenting with test-coverage gaps to address.
📋 Key Themes & Highlights
Key Themes
- Untested warning assertions:
wantWarningfields in 3 table-driven cases are never checked, so warning regressions pass silently - Missing non-strict integration test:
TestValidateSecretsSerializationViaValidateExpressionsonly exercises the strict error path; a matching non-strict path would catch neutralisation bugs - Minor: regex doc comment could be clearer about the exact mechanism that excludes
secrets.KEYpatterns
Positive Highlights
- ✅ Excellent use of the existing strict/non-strict pattern — consistent with the rest of the codebase
- ✅
neutralizeSecretsSerializationExpressionsis a clean solution for preventing confusing secondary allowlist errors - ✅ Case-insensitive regex is correctly scoped:
toJSON(secrets)flagged,toJSON(secrets.KEY)andtoJSON(steps)safe - ✅ 23 test cases provide solid baseline coverage
- ✅ Check is wired before allowlist validation with a clear comment explaining the ordering
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 47.9 AIC · ⌖ 5.56 AIC · ⊞ 6.6K
Comment /matt to run again
| } | ||
| } else { | ||
| assert.NoError(t, err, "expected no error") | ||
| } |
There was a problem hiding this comment.
[/tdd] wantWarning is declared in every non-strict test case but never asserted — the tests pass vacuously even if no warning is emitted.
💡 Suggested assertion
After calling validateSecretsSerializationExpressions, check the compiler warning count:
if tt.wantWarning {
assert.Equal(t, 1, compiler.WarningCount(), "expected exactly one warning")
}Without this, regressions in the warning path (e.g. accidentally removing IncrementWarningCount) go undetected.
@copilot please address this.
| // secretsSerializationPattern matches function calls that pass the entire secrets | ||
| // context as an argument, e.g. toJSON(secrets). | ||
| // | ||
| // GitHub Actions function names are case-insensitive, so (?i) is used. The |
There was a problem hiding this comment.
[/diagnosing-bugs] The regex comment says "nothing follows 'secrets' except optional whitespace" but that description is slightly misleading: the actual guard is \s*\) which only checks the closing paren. toJSON(secrets .DOTTED) (with a space before the dot) would not be flagged, which is the intended behaviour — but the comment doesn't make the space-before-dot edge case explicit. This could cause confusion for future maintainers.
💡 Suggested comment clarification
// The pattern matches toJSON(secrets) but NOT toJSON(secrets.KEY) because
// the closing \) requires that "secrets" is immediately followed by optional
// whitespace then ')'. Any dot or alphanumeric character after "secrets"
// causes the \s*\) to fail, so specific property accesses are safe.
var secretsSerializationPattern = ...@copilot please address this.
| RawFrontmatter: map[string]any{}, | ||
| } | ||
|
|
||
| err := compiler.validateExpressions(workflowData, "/tmp/test.md") |
There was a problem hiding this comment.
[/tdd] TestValidateSecretsSerializationViaValidateExpressions only covers the strict-mode error path via validateExpressions. A non-strict integration test is missing: it should verify that in non-strict mode the call returns nil and the compiler warning count increments (and that no secondary allowlist error is surfaced).
💡 Suggested additional test
func TestValidateSecretsSerializationNonStrictViaValidateExpressions(t *testing.T) {
compiler := NewCompiler()
compiler.strictMode = false
workflowData := &WorkflowData{
Name: "Test",
MarkdownContent: "Expose everything: ${{ toJSON(secrets) }}",
RawFrontmatter: map[string]any{"strict": false},
}
err := compiler.validateExpressions(workflowData, "/tmp/test.md")
assert.NoError(t, err, "non-strict should not return an error")
assert.Equal(t, 1, compiler.WarningCount(), "expected one warning")
}@copilot please address this.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (379 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
REQUEST_CHANGES — 3 medium issues, 1 blocking testability gap
The security intent is sound and the core regex logic is correct. However there are issues that must be addressed before merge.
🔍 Findings summary
1. wantWarning is never asserted (blocking)
Tests define wantWarning: true for all non-strict cases but the assertion is never made. The entire warning code path — including the content and presence of the warning — is unverified. This is a direct consequence of writing to os.Stderr directly; the fix requires making the output writer injectable.
2. Log reports c.strictMode but enforcement uses effectiveStrictMode (medium)
When frontmatter overrides the compiler default, the log says strictMode=true while the code behaves non-strictly. Actively misleading during debugging.
3. Expression reported in errors is inconsistently normalized (medium)
findSecretsSerializationExpressions trims outer whitespace of the captured group but not inner — the reported expression in error messages is neither the literal source text nor a canonical form. Use match[0] to report the original, or normalize fully.
🔎 Code quality review by PR Code Quality Reviewer · 49.4 AIC · ⌖ 4.77 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/expression_secrets_serialization_validation_test.go:231
wantWarning is defined in the test struct but never asserted — the entire non-strict warning path is untested.
<details>
<summary>💡 Details</summary>
Multiple test cases set wantWarning: true (e.g. "toJSON(secrets) in markdown body — non-strict emits warning") but the test loop body never reads that field. Any regression — missing warning, wrong message, spurious warning — passes silently.
Root cause: fmt.Fprintln(os.Stderr, ...) is not capturable in unit tests without redirecting…
pkg/workflow/expression_secrets_serialization_validation.go:148
Log at line 148 reports c.strictMode but enforcement uses effectiveStrictMode, making debug logs misleading.
<details>
<summary>💡 Details</summary>
// Current (line 148):
expressionSecretsSerializationLog.Printf("Validating secrets serialization expressions (strictMode=%t)", c.strictMode)c.strictMode is the compiler-level default. The actual enforcement path calls c.effectiveStrictMode(workflowData.RawFrontmatter), which can return false even when `c.strictMode == t…
pkg/workflow/expression_secrets_serialization_validation.go:111
Error/warning message reports a normalized version of the expression, not the literal text from source — users may not recognize what triggered the error.
<details>
<summary>💡 Details</summary>
full := "${{ " + strings.TrimSpace(exprContent) + " }}"If a workflow contains ${{ toJSON( secrets ) }} (extra spaces), the error message reports ${{ toJSON( secrets ) }} — the outer whitespace is trimmed but the inner spacing is preserved, producing an inconsistent and potent…
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented pr-finisher pass in commit 808779e: addressed the secrets-serialization validator/test feedback, reverted the unrelated generated workflow schedule change, and ran local validation ( |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Please run the
|
Completed a pr-finisher pass on |
🤖 PR Triage
Rationale: Adds compiler rule blocking
|
|
@copilot please run the
|
Expressions like
${{ toJSON(secrets) }}serialize the entire secrets context, exposing all secrets to the agent. This adds a dedicated compiler rule to catch this pattern.Behaviour
strict: false): warning, compilation proceedsChanges
expression_secrets_serialization_validation.go— new rule:toJSON(secrets)but nottoJSON(secrets.MY_SECRET)(specific refs are fine)validateSecretsSerializationExpressions— errors or warns based on effective strict modeneutralizeSecretsSerializationExpressions— replaces matches with${{ false }}in non-strict mode before the allowlist check runs, preventing a confusing secondary errorcompiler_validators.go— wires the new check intovalidateExpressionsbefore the existing allowlist-basedvalidateExpressionSafetyexpression_secrets_serialization_validation_test.go— 23 cases covering strict/non-strict, markdown body vs frontmatter YAML, case variants, and safe patternsSafe patterns (not flagged)