Skip to content

fix: harden secret validation and masking - #2872

Open
Mikhail Shirkov (shirkevich) wants to merge 2 commits into
codex/helm-lifecycle-k3sfrom
codex/secrets-safe-masking
Open

fix: harden secret validation and masking#2872
Mikhail Shirkov (shirkevich) wants to merge 2 commits into
codex/helm-lifecycle-k3sfrom
codex/secrets-safe-masking

Conversation

@shirkevich

@shirkevich Mikhail Shirkov (shirkevich) commented Aug 5, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Collaborator

what

  • Validate every !secret reference against the component declaration registry before taking the masked-inspection fast path.
  • Continue to skip backend retrieval during masked inspection, so validation requires no credentials or network access.
  • Mask multiline secret literals after YAML or other serializers indent continuation lines.
  • Mask long scalar secrets when YAML folding replaces spaces with indented line breaks.
  • Add focused regression coverage for undeclared masked references, provider-call suppression, two-, four-, and six-space indentation, folded values, and existing single-line masking.

This is 1 of 2 in a secret-handling follow-up stack based on #2849:

  1. Secret declaration validation and serialization-safe masking
  2. feat: support explicit raw secret values #2873 — explicit raw secret values and string write/read symmetry

why

Masked inspection previously returned <MASKED> before checking the declaration registry. A misspelled or malformed secret name therefore appeared valid even though execution would later fail.

The masker also searched only for the exact registered literal. Serializers can indent every continuation line of a PEM key or fold a long scalar across lines, so the exact literal no longer appears in the emitted text. That allowed registered secret payload lines to remain visible in command output and CI logs.

behavior and compatibility

  • Correctly declared secrets still produce <MASKED> without contacting their backend.
  • Undeclared references now fail early with the offending name and declaration guidance.
  • Only serializer-introduced whitespace is tolerated while matching; all non-whitespace payload bytes must still match exactly.
  • Existing single-line masking behavior remains unchanged.

validation

  • 242 focused pkg/io and pkg/secrets tests pass.
  • Regression tests use generic values and in-memory mocks only; no cloud credentials or network access are required.
  • git diff --check and a private-name audit pass.

references

Summary by CodeRabbit

  • Bug Fixes

    • Secret references are now validated before masked inspection, preventing undeclared secrets from being accepted.
    • Multiline secrets in indented and folded YAML formats are now masked consistently.
    • Existing behavior for missing secrets, successful resolution, and masking remains supported.

  • Tests

    • Added coverage for undeclared secret references and multiline YAML masking scenarios.

@atmos-pro

atmos-pro Bot commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Tip

Atmos Pro  

No affected stacks workflow was detected for this pull request.
If this is expected, no action is needed.
Learn More. Ask AI.

@github-actions github-actions Bot added the size/m Medium size PR label Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues found.

Scanned Files

None

@shirkevich Mikhail Shirkov (shirkevich) mentioned this pull request Aug 5, 2026
Open
@mergify mergify Bot added the stacked Stacked label Aug 5, 2026
@mergify
mergify Bot temporarily deployed to screengrabs August 5, 2026 14:29 Inactive
@codecov

codecov Bot commented Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.75000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.77%. Comparing base (784d37a) to head (40acdd3).

Files with missing lines Patch % Lines
pkg/io/masker.go 93.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@                     Coverage Diff                      @@
##           codex/helm-lifecycle-k3s    #2872      +/-   ##
============================================================
- Coverage                     82.79%   82.77%   -0.02%     
============================================================
  Files                          1865     1864       -1     
  Lines                        181681   181473     -208     
============================================================
- Hits                         150424   150223     -201     
+ Misses                        23395    23391       -4     
+ Partials                       7862     7859       -3
Flag Coverage Δ
unittests 82.77% <93.75%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/secrets/resolver.go 87.30% <100.00%> (ø)
pkg/io/masker.go 98.70% <93.33%> (-1.30%) ⬇️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@shirkevich
Mikhail Shirkov (shirkevich) marked this pull request as ready for review August 6, 2026 12:14
@shirkevich
Mikhail Shirkov (shirkevich) requested a review from a team as a code owner August 6, 2026 12:14
@shirkevich

Mikhail Shirkov (shirkevich) commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

CodeRabbit (@coderabbitai) review

@coderabbitai

coderabbitai Bot commented Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The change validates secret declarations before mask-only resolution and adds masking for folded and indented YAML multiline literals. Tests cover undeclared secret inspection, backend access, multiline masking, and parameterized secret names.

Changes

Secret handling

Layer / File(s) Summary
Resolver declaration validation
pkg/secrets/resolver.go, pkg/secrets/resolver_test.go		 Resolve checks declaration status before mask-only handling. Tests verify ErrSecretNotDeclared and zero backend calls.
Multiline literal masking
pkg/io/masker.go, pkg/io/masker_test.go		 Literal masking supports folded and indented YAML multiline values. Tests cover private-key and folded long literals.
Component secret inspection tests
internal/exec/describe_stacks_component_processor_test.go		 Table-driven tests select secret names and cover undeclared, masked, missing, and successful resolution cases.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: patch

Suggested reviewers: osterman

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the pull request’s main changes to secret validation and multiline masking.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/secrets-safe-masking

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot reviewed Aug 6, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pkg/secrets/resolver_test.go (1)

81-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Cover the undeclared-secret diagnostic.

require.ErrorIs checks only the sentinel. The PR objective also requires the error to include UNDECLARED_KEY and component's secrets.vars. Assert both details so the test cannot pass after the actionable message is removed.

The PR objective requires the undeclared name and declaration guidance. As per coding guidelines, new behavior needs comprehensive, behavior-focused unit coverage.

Suggested assertions. 
  _, err := Resolve(cfg, "!secret UNDECLARED_KEY", "prod", info)
  require.ErrorIs(t, err, ErrSecretNotDeclared)
+ assert.Contains(t, err.Error(), "UNDECLARED_KEY")
+ assert.Contains(t, err.Error(), "component's secrets.vars")
🤖 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 `@pkg/secrets/resolver_test.go` around lines 81 - 82, Strengthen the
undeclared-secret test around Resolve by retaining the ErrSecretNotDeclared
check and additionally asserting that the returned error message contains both
“UNDECLARED_KEY” and “component's secrets.vars”.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@pkg/secrets/resolver_test.go`:
- Around line 81-82: Strengthen the undeclared-secret test around Resolve by
retaining the ErrSecretNotDeclared check and additionally asserting that the
returned error message contains both “UNDECLARED_KEY” and “component's
secrets.vars”.
ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: aed1f583-0698-4261-9d67-7a7f7a99b189

📥 Commits

Reviewing files that changed from the base of the PR and between 784d37a and 40acdd3.

📒 Files selected for processing (5)
  • internal/exec/describe_stacks_component_processor_test.go
  • pkg/io/masker.go
  • pkg/io/masker_test.go
  • pkg/secrets/resolver.go
  • pkg/secrets/resolver_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] approved these changes

Assignees

No one assigned

Labels

size/m Medium size PR stacked Stacked

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@shirkevich