Allow secrets in step-level env: bindings under strict mode

[Copilot]

Strict mode blocks all secrets.* in user-defined steps, forcing workflows that need tool credentials (API tokens, OAuth keys) to disable strict mode entirely via strict: false, losing all other protections. Step-level env: bindings are controlled, masked surfaces — functionally equivalent to what the framework's own generated jobs already do.

Important: Secrets in env: bindings must be consumed only within the step itself. Steps that reference $GITHUB_ENV while using env-bound secrets are blocked to prevent leaking secrets to subsequent steps (including the agent step).

Changes

• classifyStepSecrets() — new helper partitions a step's secret refs into unsafeRefs (run, with, etc.) and envRefs (env bindings). Malformed env values (string, slice) are treated as unsafe to prevent bypass. Steps that reference $GITHUB_ENV while having env-bound secrets are reclassified as entirely unsafe.
• validateStepsSectionSecrets() — now only errors on unsafeRefs in strict mode; env-only secrets pass through
• filterBuiltinTokens() — now parses secret names precisely via regex instead of substring matching; secrets.GITHUB_TOKEN_SUFFIX and mixed expressions are no longer wrongly exempt
• stepReferencesGitHubEnv() / valueReferencesGitHubEnv() — new helpers detect $GITHUB_ENV references in non-env step fields to prevent secret leakage
• Error/warning messages — secret refs are sorted for deterministic output; error suggests env: bindings as alternative
• ADR-0002 documents the design decision with RFC 2119 normative spec (renumbered to avoid conflict with existing ADR-0001)
• Tests — unit tests for classification logic (10 cases), filterBuiltinTokens precision (5 cases), stepReferencesGitHubEnv (3 cases), updated validation tests (9 new cases), integration tests for full compilation

Example

Now compiles under strict mode (secret scoped to the step only):

steps:
  - name: Run scan with credentials
    env:
      SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
    run: sonar-scanner

Still blocked (inline secret in run):

steps:
  - name: Inline secret
    run: curl -H 'Authorization: ${{ secrets.API_TOKEN }}' https://example.com

Also blocked (env-bound secret leaked via $GITHUB_ENV):

steps:
  - name: Leaky step
    env:
      TOKEN: ${{ secrets.TOKEN }}
    run: echo "TOKEN=${TOKEN}" >> "$GITHUB_ENV"

[pelikhan]

@copilot steps are not allowed to reference $GITHUB_ENV to avoid leaking secrets in env

[Copilot]

@copilot steps are not allowed to reference $GITHUB_ENV to avoid leaking secrets in env

Fixed in 22cd2b0 — removed all $GITHUB_ENV references from test examples. Steps now use tool invocations (e.g., sonar-scanner, ciam-auth verify) that consume the secret only within the step scope, avoiding leaking secrets to subsequent steps.

[Copilot]

Pull request overview

Enables strict mode workflows to use ${{ secrets.* }} in step-level env: bindings while still blocking secrets in other step fields (e.g., run, with) to reduce accidental secret leakage into the agent job.

Changes:

• Updated strict-mode steps validation to allow secrets referenced only within step env: maps.
• Added classifyStepSecrets() helper to partition secret refs into env-bound vs unsafe refs (with malformed env treated as unsafe).
• Added/updated unit tests and introduced integration tests covering compilation behavior; added an ADR documenting the decision.

Show a summary per file

File	Description
pkg/workflow/strict_mode_steps_validation.go	Implements env-vs-unsafe secret classification and updates strict/non-strict enforcement & messaging.
pkg/workflow/strict_mode_steps_validation_test.go	Updates existing tests and adds unit tests for classifyStepSecrets().
pkg/workflow/strict_mode_steps_secrets_integration_test.go	Adds integration coverage for strict-mode compilation allowing env-bound secrets while blocking unsafe secrets.
docs/adr/0001-allow-secrets-in-step-env-bindings-under-strict-mode.md	Documents the design decision and normative behavior for secret classification/enforcement.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 4/4 changed files
• Comments generated: 5

[Copilot]

Allowing secrets in step-level env under strict mode introduces a new, easy-to-miss leak path: a step can persist an env-bound secret to later steps (including the agent step) via "$GITHUB_ENV" (or equivalent), which defeats the “step-only” scoping described in the PR. Consider adding strict-mode validation that rejects steps which both (a) contain env-bound secrets and (b) reference GITHUB_ENV / write to it in run (and/or documenting this explicitly in the compiler error/warning output).

[Copilot]

The ordering of secret refs in error/warning messages can be nondeterministic because refs are collected via Go map iteration (stepMap iteration, nested map traversal, and ExtractSecretsFromValue returns a map). This can lead to unstable error strings and flaky tests/assertions downstream. Consider producing deterministic output by sorting the deduplicated ref slices (unsafeSecretRefs / allSecretRefs) before strings.Join.

[Copilot]

This ADR is titled/numbered "ADR-0001" but the repository already contains another ADR-0001 file (docs/adr/0001-conditional-oidc-env-var-forwarding-to-mcp-gateway.md). ADR IDs should be unique; please renumber this document (and its filename/header) to the next available number to avoid ambiguity in references.

[Copilot]

Normative spec mismatch with implementation: the code intentionally treats malformed env: values (string/slice) as unsafe to prevent bypass, but the spec currently says any secret reference in env: MUST be env-bound and MUST NOT be classified as unsafe. Update the spec to explicitly scope “env-bound” to well-formed env mappings (or otherwise reconcile with the malformed-env-as-unsafe behavior).

[Copilot]

filterBuiltinTokens currently drops any ref whose expression string contains the substring secrets.GITHUB_TOKEN. This can unintentionally exempt user secrets like secrets.GITHUB_TOKEN_SUFFIX, and (more critically) can cause other secrets to be ignored when they appear in the same expression as GITHUB_TOKEN (because the entire expression gets filtered). Consider filtering more precisely by parsing secret names from the expression and only removing refs when the expression contains only GITHUB_TOKEN (or filter by secret name before collapsing to expression strings).

[github-actions]

✅ Design Decision Gate — ADR Verified

The implementation in this PR aligns with the stated Architecture Decision Record.

ADR reviewed: ADR-0001: Allow Secrets in Step-Level env: Bindings Under Strict Mode

Verification Summary

Normative Requirement	Status	Evidence
classifyStepSecrets() classifies each ref by field name	✅	strict_mode_steps_validation.go lines 95–114
env: field refs → classified as env-bound	✅	key == "env" + map type check
All other field refs → classified as unsafe	✅	Else branch in field loop
Non-map step → all refs unsafe	✅	First guard in classifyStepSecrets
Strict mode errors only on unsafe refs	✅	if len(unsafeSecretRefs) == 0 { return nil }
Strict mode allows env-bound-only steps	✅	Tests: "env only in strict mode is allowed"
Error message suggests env: bindings	✅	"or use step-level env: bindings instead"
GITHUB_TOKEN filtered from both ref lists	✅	filterBuiltinTokens applied to both slices
Non-strict emits warning for all secrets	✅	allSecretRefs used in warning message
Deduplication before output	✅	sliceutil.Deduplicate called

The code faithfully implements every MUST and MUST NOT requirement in the ADR's normative specification. 🏗️

---

⚠️ One Issue to Address Before Merging

ADR numbering collision: The docs/adr/ directory already contains 0001-conditional-oidc-env-var-forwarding-to-mcp-gateway.md (from a previous change). This PR adds a second 0001-*.md file, which breaks the sequential numbering convention. Please rename this PR's ADR to 0002-allow-secrets-in-step-env-bindings-under-strict-mode.md and update any references.

Additionally, the ADR Status is still Draft (auto-generated by the previous gate run). Once you've reviewed and are satisfied with the content, please change it to Accepted.

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · ● 85.8K · ◷

[github-actions]

Implementation verified: code aligns with the linked Architecture Decision Record (ADR-0001: Allow Secrets in Step-Level env: Bindings Under Strict Mode).

All MUST and MUST NOT normative requirements are satisfied. Two housekeeping items remain before merge:

1. Rename the ADR file to 0002-*.md to resolve the numbering collision with the existing 0001-conditional-oidc-env-var-forwarding-to-mcp-gateway.md.
2. Update the ADR Status from Draft to Accepted.

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 70/100

⚠️ Acceptable — with suggestions

Metric	Value
New/modified tests analyzed	6
✅ Design tests (behavioral contracts)	6 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	4 (67%)
Duplicate test clusters	0
Test inflation detected	YES — strict_mode_steps_validation_test.go: 214 lines added vs 69 in production (3.1:1 ratio)
🚨 Coding-guideline violations	None

---

Test Classification Details

Test	File	Classification	Notes
TestStrictModeStepEnvSecretsAllowed	strict_mode_steps_secrets_integration_test.go	✅ Design	Verifies the core new contract: env-bound secrets compile under strict mode; 5 table rows (single, multi-step, GITHUB_TOKEN, pre-steps, post-steps)
TestStrictModeStepUnsafeSecretsBlocked	strict_mode_steps_secrets_integration_test.go	✅ Design	Verifies rejection contract: run/with secrets still blocked; 3 error-path rows
TestStrictModeStepEnvSecretsErrorSuggestsEnvBindings	strict_mode_steps_secrets_integration_test.go	✅ Design	Error-message quality contract — ensures users get actionable guidance
TestNonStrictModeStepSecretsAllowedWithWarning	strict_mode_steps_secrets_integration_test.go	✅ Design	Behavioral contract for non-strict mode (no regression)
TestValidateStepsSecrets (new rows)	strict_mode_steps_validation_test.go	✅ Design	8 new table rows including malformed env edge cases (string and slice); covers error + allow paths
TestClassifyStepSecrets	strict_mode_steps_validation_test.go	✅ Design	Tests classification function used by the validator; 8 rows covering edge cases (non-map step, malformed string/slice env)

---

Suggestions — Not Blocking

⚠️ TestStrictModeStepEnvSecretsAllowed — Happy-path only

Issue: All 5 table rows expect compilation success; there is no "this specific env input fails" row within this table. The rejection contract is covered in the sibling TestStrictModeStepUnsafeSecretsBlocked function, so this is not a gap in the overall test suite — but adding a comment clarifying the complementary structure would help future readers.

Suggestion: Add a brief doc comment linking to TestStrictModeStepUnsafeSecretsBlocked as the corresponding rejection test, to make the test pair discoverable.

---

⚠️ TestNonStrictModeStepSecretsAllowedWithWarning — Title implies warning, test only checks no error

Issue: The test name says "AllowedWithWarning" but the test only asserts err == nil — it does not verify that a warning is actually emitted to stderr. If the warning is a deliberate user-visible contract, this assertion gap could allow the warning to be silently removed.

Suggested improvement: Capture stderr and assert the warning message is present if non-strict-mode warnings are a documented behavioral contract.

---

i️ Test inflation: strict_mode_steps_validation_test.go

214 test lines added vs 69 production lines (3.1:1 ratio). This exceeds the 2:1 guideline threshold but is well-justified here: the malformed-env edge cases (string env, slice env) and the new TestClassifyStepSecrets function require more test code than production code by design. No action required.

---

Language Support

Tests analyzed:

• 🐹 Go (*_test.go): 6 test functions
  • strict_mode_steps_secrets_integration_test.go — //go:build integration ✅
  • strict_mode_steps_validation_test.go — //go:build !integration ✅
• 🟨 JavaScript (*.test.cjs, *.test.js): 0 tests (none modified in this PR)

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). All 6 test functions verify observable behavioral contracts. Build tags are correct on both files, no mock libraries used.

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 601.6K · ◷

[github-actions]

✅ Test Quality Sentinel: 70/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). All 6 test functions verify observable behavioral contracts, both files have correct build tags, and no forbidden mock libraries are used. Minor suggestions posted in the comment above.

[pelikhan]

@copilot review comments

[Copilot]

@copilot review comments

Addressed all 5 review comments in f47d1f2:

1. GITHUB_ENV leak detection — classifyStepSecrets() now reclassifies env-bound secrets as unsafe when the step references $GITHUB_ENV in any non-env field
2. Deterministic error messages — Secret refs are sort.Strings()-ed before strings.Join()
3. ADR renumbering — Renamed to 0002-allow-secrets-in-step-env-bindings-under-strict-mode.md
4. ADR spec update — Scoped "env-bound" to well-formed mappings; added rule 6 for GITHUB_ENV reclassification
5. Precise GITHUB_TOKEN filtering — filterBuiltinTokens now parses secret names via secretsNamePattern instead of substring matching (GITHUB_TOKEN_SUFFIX is no longer wrongly exempt)