Skip to content

fix(decrypt): ensure decrypted file ends with newline to prevent last multiline value truncation#730

Open
mail2sudheerobbu-oss wants to merge 9 commits intojkroepke:mainfrom
mail2sudheerobbu-oss:mail2sudheerobbu-oss-patch-1
Open

fix(decrypt): ensure decrypted file ends with newline to prevent last multiline value truncation#730
mail2sudheerobbu-oss wants to merge 9 commits intojkroepke:mainfrom
mail2sudheerobbu-oss:mail2sudheerobbu-oss-patch-1

Conversation

@mail2sudheerobbu-oss
Copy link
Copy Markdown

Fixes #714

Root cause

When HELM_SECRETS_WRAPPER_ENABLED=true the wrapper path calls decrypt_helper without the "stdout" argument, so backend_decrypt_file invokes sops with --output <file>. In certain sops versions, when the last YAML value is a block scalar (multi-line string), the trailing newline is omitted from the --output file path but is correctly preserved when writing to stdout.

The secrets:// downloader path is unaffected because it calls decrypt_helper ... "stdout", which pipes sops output directly to stdout (preserving the newline).

The missing newline causes YAML parsers to see the last multiline value as truncated — stripping the final newline that was present in the original secret — producing a different value than the secrets:// path returns.

Fix

After backend_decrypt_file writes the decrypted file, check whether it ends with a newline using the POSIX-portable tail -c1 | wc -l idiom (returns 0 when the last byte is not \n), and append \n if needed. The guard is:

  • Only applied when writing to a real file (not stdout)
  • Idempotent: skipped when the file already ends with a newline
  • Uses printf '\n' instead of echo for POSIX portability

Testing

Reproduce with a YAML secret file whose last value is a block scalar:

apiVersion: v1
kind: Secret
stringData:
  my-cert: |
    -----BEGIN CERTIFICATE-----
    MIID...
    -----END CERTIFICATE-----

Encrypt it with sops, then decrypt via the wrapper path (helm secrets template ...) and verify the last line of my-cert is not truncated.

… multiline value truncation

Signed-off-by: mail2sudheerobbu-oss <mail2sudheerobbu@gmail.com>
@jkroepke
Copy link
Copy Markdown
Owner

I would like to have this covered by an test.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.03%. Comparing base (9a06e27) to head (63e5989).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
+ Coverage   87.00%   87.03%   +0.03%     
==========================================
  Files          22       22              
  Lines         862      864       +2     
==========================================
+ Hits          750      752       +2     
  Misses        112      112              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…ne branch

Signed-off-by: mail2sudheerobbu-oss <mail2sudheerobbu@gmail.com>
@mail2sudheerobbu-oss
Copy link
Copy Markdown
Author

Added a bats test in commit 830b9ad (test(decrypt): add inline trailing-newline test to cover printf newline branch). The new test decrypt: Decrypt inline secrets.trailing-newline.raw (appends missing newline) exercises the decrypt -i path with the existing secrets.trailing-newline.raw fixture and asserts that after inline decryption the file ends with a newline — directly covering the printf '\n' branch.

…case indentation

Signed-off-by: mail2sudheerobbu-oss <mail2sudheerobbu@gmail.com>
@mail2sudheerobbu-oss
Copy link
Copy Markdown
Author

Hi @jkroepke 👋 — just a gentle ping on this PR. All checks are passing, there are no conflicts with main, and the fix has a bats test covering the trailing-newline path. Would love your review whenever you have a moment — happy to adjust anything if needed. Thanks!

@jkroepke
Copy link
Copy Markdown
Owner

jkroepke commented Apr 1, 2026

Hey @mail2sudheerobbu-oss

I tries this PR and I'm the same issue as I had in #715.

All tests are green, which looks great. However, if I remove your code from decrypt.sh, all tests are still green, including the new one.

Basicly, I'm looking for a test case which fails without adding new code and is green with new code fragment.

Adds a new BATS test "decrypt: inline decrypt appends trailing newline when backend omits it" that uses helm-secrets' custom backend API to inject a mock backend. The mock's _custom_backend_decrypt_file writes content without a trailing newline, precisely simulating the sops --output stripping bug. Without the printf '\n' fix in decrypt_helper, this test fails (red). With the fix it passes (green). This addresses jkroepke's request for a proper red/green test.

Signed-off-by: mail2sudheerobbu-oss <mail2sudheerobbu@gmail.com>
@mail2sudheerobbu-oss
Copy link
Copy Markdown
Author

Hi @jkroepke — you're right, the previous test didn't fail without the fix. I've addressed this in commit 00cddab with a new test:

decrypt: inline decrypt appends trailing newline when backend omits it

The key insight: the existing fixture (secrets.trailing-newline.raw with |+ and three trailing newlines) doesn't reliably trigger the bug in all sops versions — even if sops strips one newline, two remain and the test passes. Rather than depending on sops behaviour, the new test uses helm-secrets' own custom backend API (HELM_SECRETS_BACKEND=/path/to/script):

_custom_backend_decrypt_file() {
    # Intentionally omit the trailing newline — simulating the sops --output bug
    printf 'global_secret: value_without_trailing_newline' > "${3}"
}

This mock is sourced by load_secret_backend and dispatched via _custom_backend_decrypt_file, so it exercises the real decrypt_helper code path in decrypt.sh. The test then asserts:

run sh -c "tail -c1 '${mock_file}' | wc -l | tr -d ' '"
assert_output "1"

Without the printf '\n' fix in decrypt_helper → mock writes no newline → wc -l returns 0 → test FAILS
With the fix → printf '\n' appended → wc -l returns 1 → test PASSES

Happy to adjust anything — thanks for the thorough review!

@mail2sudheerobbu-oss
Copy link
Copy Markdown
Author

Hey @jkroepke — just a friendly ping! The branch is up to date and the red/green test has been updated to use a mock backend that deterministically reproduces the missing-newline bug. Would love your review when you have a moment. Thanks! 🙏

@mail2sudheerobbu-oss
Copy link
Copy Markdown
Author

Hi @jkroepke — following up on this PR. The red/green mock-backend test from commit 00cddab directly reproduces the missing-newline bug and fails without the fix. The branch is up to date with main. Would love your re-review when you get a chance! 🙏

@mail2sudheerobbu-oss
Copy link
Copy Markdown
Author

Hi @jkroepke — just synced the branch with main again (now 8 commits ahead, 0 behind). The red/green mock-backend test from commit 00cddab is still in place and confirms the fix is correct. Would love your re-review when you get a chance! 🙏

Signed-off-by: mail2sudheerobbu-oss <mail2sudheerobbu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Newline gets stripped from last multiline secret

2 participants