Repository navigation
Fix P08, P10, P11, and P12 from the 2026-09-29 medium-findings audit - #11
Merged
Merged
Conversation
Clean's whole-file path and cleanValues both used tag.IsEncrypted (a bare "ENC[" prefix check) to decide a value was already encrypted and needed no further processing. A plaintext value or field that merely starts with "ENC[" without being a well-formed tag satisfied that check and was committed as plaintext instead of being encrypted. The new tag.LooksEncrypted requires the full ENC[...] delimiter shape and a recognized algorithm name, but deliberately still tolerates a pre-rotation legacy tag missing its "key:" field (a full tag.Parse does not: an earlier attempt using Parse as the gate regressed the existing legacy-tag migration test, since a real legacy tag genuinely fails Parse on the missing key field alone). tag.IsEncrypted itself is unchanged and still correct as smudge's pre-Parse fast path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
reportCheckoutResult printed both categories of problem it can find — a genuine per-file checkout failure (AC-6.9 tamper/corruption) and a file merely waiting on a key version this clone hasn't fetched yet (AC-6.11's intentional, recoverable passthrough) — but always returned nil either way. A script relying on the exit code alone could not tell `nebel init` left unusable content behind. The new ErrCheckoutFailed is returned only when a genuine checkout failure is present. The AC-6.11 case deliberately keeps exiting 0: TestInitVersionRecoversContentFromBeforeRotation already asserts that case succeeds, since it's a documented, expected, recoverable state (`nebel init --version N` fixes it), not a failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Verifies P08-P12 from the 2026-09-29 repository analysis (the five findings the existing high-findings plan explicitly left out of scope), records that P08 and P12 are now fixed, and lays out what's left for P09-P11. Resolves the open design question on P11 (reject multi-document YAML rather than attempt cross-document field resolution). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ejection P10: yamlHandler.Render escaped only backslash and the closing quote. A decrypted string containing a literal control character (a real "\n" byte, not the two-character escape) was spliced into a double-quoted scalar as-is, then silently reinterpreted by YAML's own line-folding rules on the next parse with no error ever raised. The natural fallback for characters with no YAML single-character escape - a "\xHH" hex escape - turned out to be unusable: the goccy/go-yaml parser this codebase depends on has a confirmed bug mistracking a scalar token's source span for any \x, \u, or \U escape (reproduced independent of the escaped value), which would corrupt the next Locate of that same field instead of failing where the problem actually is. Render's signature now returns (string, error); YAML's own single-character escapes (\0 \a \b \t \n \v \f \r \e) are used where they exist, and anything else refuses outright with a new ErrControlCharacterUnsupported rather than risk the broken hex-escape path - the same choice already made for block scalars. P11: Locate and Leaves both operated on file.Docs[0] only, silently ignoring every "---"-separated document after the first. A configured path matching a leaf only in a later document was never located, never encrypted, and never even reported as missing. Both now return a new ErrMultiDocumentUnsupported when more than one document is present, rather than resolve an ambiguity (the same path present in two documents - which one does a single Encrypt entry protect?) this codebase does not attempt to solve. Bundled together per the existing mitigation plan's own note: both touch the same YAML rendering/parsing code path and test harness. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Records the fix, the unplanned goccy/go-yaml hex-escape lexer bug found and worked around while fixing P10, and the tests added for both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MuffinUser
changed the title
MuffinUser
marked this pull request as ready for review
September 29, 2026 14:24
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
1 participant
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
Clean/cleanValuesmistook a plaintext value/field starting withENC[for already-encrypted content (bare prefix check) and left it committed as plaintext. Replaced withtag.LooksEncrypted, which requires the full tag shape (and still tolerates the pre-rotation legacy tag format, unlike a fulltag.Parse, so the existing legacy-migration test keeps passing).nebel init(reportCheckoutResult) always exited 0 even when a managed file's checkout genuinely failed (AC-6.9 tamper/corruption). Now returnsErrCheckoutFailedin that case, while the intentional, documented "still needs a key version" passthrough (AC-6.11) keeps exiting 0 as before.yamlHandler.Renderonly escaped backslash and the closing quote. A decrypted string with a literal control character (e.g. a real newline byte) got spliced into a double-quoted scalar as-is and silently reinterpreted by YAML's line-folding rules on the next parse. Fixed with proper escaping — but the natural\xHHhex-escape fallback turned out to be unusable: found and confirmed a real bug in thegoccy/go-yamllexer that mistracks a scalar token's source span for any\x/\u/\Uescape.Rendernow returns(string, error); YAML's own single-character escapes cover 9 control codes, everything else is refused outright (ErrControlCharacterUnsupported) rather than risk that lexer bug corrupting a laterLocate.Locate/Leavesonly ever looked at the first----separated YAML document, silently ignoring the rest. Now rejected outright withErrMultiDocumentUnsupported(per discussion: reject rather than attempt cross-document field resolution, which raises its own ambiguity).audits/2026-09-29-medium-findings-validation-and-mitigation-plan.md) covering all five of the audit's medium-priority findings (P08–P12); P09 (rotation atomicity / password loss on failure) remains open and is intentionally not included here.specs/05-format-handlers.md.Test plan
go build ./...go vet ./...go test ./...(full suite, unchanged elsewhere)TestCleanEncryptsPlaintextStartingWithEncPrefix,TestCleanLeavesLegacyWholeFileTagAlone,TestCleanValuesEncryptsFieldStartingWithEncPrefix,TestReportCheckoutResultFailsOnGenuineCheckoutFailure,TestReportCheckoutResultSucceedsWhenOnlyMissingKeyVersion,TestRenderEscapesYAMLControlCharacters,TestRenderRejectsUnescapableControlCharacters,TestLocateAndLeavesRejectMultiDocumentYAML,TestLocateAndLeavesAcceptSingleDocumentWithLeadingMarker,TestValueRoundTripPreservesEmbeddedNewline,TestSmudgeValuesRejectsUnrenderableControlCharacter🤖 Generated with Claude Code