Backport 39 security and correctness fixes to the v0.2 line - #91
Merged
Conversation
40 defects found against main by differential testing and code audit: 12 security, 12 correctness, 5 interop, 11 hygiene. 32 reproduced empirically against the reference age CLI v1.3.1; the 8 that are source-read only say so. Lands first so every fix that follows can cite a section of it.
…ctory ProcessStartInfo.FileName was the bare name "age-plugin-<name>", and .NET resolves a bare FileName from the process's current working directory even with UseShellExecute = false. Any AgeSharp-based tool run inside an untrusted tree — an unpacked archive, a shared CI checkout, ~/Downloads — would execute a planted age-plugin-<name> and hand it the file key. docs/spec/age-plugin.md prohibits this verbatim: "Paths relative to the current working directory MUST NOT be searched, even on platforms or systems where this is the default." PluginLocator now walks PATH itself, skipping empty and non-rooted entries (both of which name the current directory), honouring PATHEXT on Windows, and checking the execute bit on POSIX. Only an absolute path reaches FileName, so there is nothing left for .NET's own fallback to resolve. WorkingDirectory is set to the temp directory as go-age does, so the plugin does not inherit the caller's directory either. A resolution miss keeps the existing "plugin not found: age-plugin-<name>" message; the Win32Exception mapping now says "failed to start plugin", which is what it actually means once the file has already been found. Regression test: PluginConnection_NeverExecutesBinaryFromCurrentDirectory plants an executable in a temp dir, chdirs into it, and asserts the launch fails and no marker file appears. Verified failing on the pre-fix code. (cherry picked from commit 2134d6d2184030343196afeca4f595aa1f8e25c0)
The decoder required every 64-character body line to decode to a full 48 bytes, on the premise that a padded line is always short. It is not: a final chunk of 46 bytes base64-encodes to 15 full groups plus "XX==" — exactly 64 characters — and 47 bytes to 64 characters ending in a single "=". Both are canonical, both are what the reference implementations emit and accept (go-age keys off the decoded length, `n < format.BytesPerLine`; rust-age accepts `(false, ARMORED_COLUMNS_PER_LINE)` unconditionally). The consequence was that ~2 of every 48 ciphertext lengths — about 4% of arbitrary armored age files, including AgeSharp's own armored encrypted identity files — could not be read back, with "non-canonical base64 in armor". main could not even round-trip its own output; the encoder was always right. Three edits, which had to land together because the width guard made the other two vacuous: - drop the width-based rejection; - validate canonical padding wherever padding appears rather than only on short lines, so a padded full-width line with non-zero trailing bits is still rejected; - end the body at the first line that cannot be followed by another — short, or full-width and padded — so a padded line followed by more body still errors. Regression tests: a self round-trip sweep over lengths 0..200 (CLI-free, walks the mod-48 cycle four times), explicit 46/47-byte full-width cases, and both strictness guards; plus an interop sweep 0..220 that encrypts with the reference age CLI and decrypts with AgeSharp. Confirmed all four fail without the fix. (cherry picked from commit a0956569a12caf29385eaf486b30551889bb674a)
BEHAVIOUR CHANGE — call out in the release notes.
NewlineBoundedStream.Dispose unconditionally disposed its inner stream, and
that inner stream is the caller's. Disposing the DearmorStream therefore
cascaded down and closed the caller's ciphertext stream — but only for
armored input; binary input was left open. Every armored entry point was
affected: Decrypt, DecryptReader, AgeHeader.Parse and AgeRandomAccess. The
concrete symptom was that decrypting the same armored MemoryStream twice
threw ObjectDisposedException, while the identical binary sequence worked.
This contradicts the library's own documented contract ("the library never
disposes a caller's stream") and AgeRandomAccess's XML doc ("The caller
retains ownership of the stream."), so it is a fix rather than a
regression — but it does stop closing a stream a caller may have come to
rely on being closed, in a patch release.
The fix is a leaveOpen flag on NewlineBoundedStream, defaulting to false,
with AsciiArmor.Dearmor — its one construction site — passing true. The
StreamReader keeps leaveOpen: false so it still disposes the wrapper the
library created; the existing needsDispose/ownsStream plumbing then reaches
only library-created objects. next-version's whole-cloth dearmor rewrite is
entangled with the v0.3 API and was deliberately not ported.
Regression test: Age.Tests/StreamOwnershipTests.cs asserts non-ownership
across all four decrypt-side entry points plus both encrypt-side ones, each
in binary and armored form, and re-decrypts one stream twice. Confirmed the
five armored rows fail without the fix and the binary rows pass either way —
the split that was the defect. No existing assertion encoded the old
behaviour; nothing anywhere asserted stream ownership at all.
(cherry picked from commit 1e938aa8084967f409ebb0c6b09037420fe8ddc5)
AgeRandomAccess derived PlaintextLength from ciphertext layout arithmetic alone and never decrypted anything at construction. Chunk layout cannot distinguish a truncated file from a shorter one, so a payload cut to exactly its trailing 16-byte tag was accepted and a short plaintext returned with no error (S2), and a payload whose computed length was 0 -- an empty file, or one chopped to a bare tag -- authenticated nothing at all, because ReadAt returns early before any chunk is touched (S3). age v1.3.1 and AgeSharp's own forward-only path reject every one of these inputs. InitializeFromStream now reads the last chunk, decrypts it with the final flag set, rejects an empty final chunk that has predecessors, and derives PlaintextLength from the authenticated result. docs/spec/age.md requires exactly this before a seekable reader may report a length, so Length and Seek(0, SeekOrigin.End) stop being arithmetic over an unverified byte count (C11). The payload key is zeroed if construction fails after deriving it, and the now-false XML remark about truncation being detectable only on read is gone. ComputePlaintextLength is dropped: its unauthenticated result no longer has a caller. Fixes S2, S3, C11. (cherry picked from commit ff460b185b002c913f3c6c4cc376d68f7421439a)
Dispose() zeroes the key material, but the Recipient getter and ToSecretString() had no _disposed guard, so after disposal they derived from an all-zero key/seed and returned a well-formed, publicly derivable recipient and secret string. Every disposed X25519Identity collapsed to age19ljhmg68e43yx9fgm2k9lwefquc0la5y4lzvlshdjzv47kxt8d6qr9vf4p and every disposed MlKem768X25519Identity to its PQ equivalent, silently — anything encrypted to such a recipient is world-readable. Unwrap() on the same instance already threw ObjectDisposedException, so the intended contract was unambiguous; this extends it to the two other members that read the key. ToString() is deliberately not made to throw: it is called by debuggers and logging, so it renders "(disposed)" instead. Internal-only behaviour change on an already-broken call path; no public API surface moves. (cherry picked from commit 7bff355630a3a33b539a5ae511132ab7ced62670)
SendWrapRequest sent "-> extension-labels" in every recipient-v1 phase-1
exchange. Per docs/spec/age-plugin.md that advertisement means the client
will accept a "labels" command and MUST check that all stanzas wrapping a
given file key carry the identical label set. ReadWrapResponse has no
"labels" case, so the reply fell through to "unsupported", and
PluginRecipient.Label is hardcoded null — the plugin's constraint was
discarded in silence.
The visible effect: a post-quantum labelling plugin recipient could be
combined with a classical X25519 recipient wrapping the same file key,
which is exactly what labels exist to prevent. age v1.3.1 refuses that
pair ("can't mix post-quantum and classic recipients"); main encrypted
it happily.
Full label support is not API-neutral here — IRecipient.Label is a single
string?, not a set — so the honest fix is to stop making the promise.
A conforming plugin that sees no extension-labels will not send labels,
and AgeEncrypt's existing label check (which does compare Label across
recipients) is no longer defeated by a plugin that had one.
Test change: PluginRecipient_Wrap_BasicProtocol asserted the stanza IS
sent. That assertion encoded the defect — it pinned an advertisement the
client never implemented — so it is inverted to DoesNotContain, and a
dedicated regression test added alongside it.
(cherry picked from commit ce0307a3d8cf9b014c2fe9c571d89136d4186e67)
Stream.Dispose() carries no idempotence guard and Close() is a documented alias for it, so calling both — or nesting a StreamReader inside a using — is ordinary caller code. DecryptStream, EncryptStream and ArmorStream each returned their rented buffers unconditionally, so a second pass put arrays already on the ArrayPool free list back on it again, and two later unrelated Rent calls were handed the same array. The symptom users would actually report is stranger than the cause: with the pool corrupted, a subsequent decrypt of a perfectly well-formed file fails with "chunk 0 authentication failed". Adds a _disposed guard to all three, and with it C12 — Read() after Dispose() was serving whatever the next renter had written into the returned buffers, so it now throws ObjectDisposedException. AgeRandomAccess and X25519Identity already had this guard; these three were the outliers. Five of the seven new tests fail without the change.
C4 — PluginRecipient's read loop assigned rather than appended, so a plugin answering one wrap-file-key with several recipient-stanzas had all but the last silently discarded, exit code 0. For a share-splitting plugin that means the file key can never be reassembled: the plaintext is gone and nothing warned anyone. The spec's own recipient-v1 example shows a plugin emitting two stanzas for one file index, and go-age appends. next-version fixed this by widening IRecipient.Wrap to return a list, which is a public API break and cannot ship in a patch. Instead: an internal IMultiStanzaRecipient seam that AgeEncrypt prefers when present — Wrap has exactly one call site in the library, which is what makes that work. The public Wrap still returns a single Stanza and now throws when the plugin produced more than one, converting silent permanent data loss into a clean failure for anyone calling it directly. C5 — PluginIdentity numbered each stanza with its own FILE_INDEX. That index identifies the *file*: "Duplicate file indices indicate stanzas that are from the same file header, and wrap the same file key." Presenting one header as N phantom files defeats the spec's same-index invalidation rule and stops a plugin from reassembling a key split across stanzas — main could not decrypt files the reference client decrypts. go-age sends "0" for all of them. PluginTests.PluginIdentity_Unwrap_MultipleStanzas asserted the buggy wire form (indices 0 and 1) and now asserts 0 and 0. Each defect was reintroduced separately to confirm the new tests catch it.
C6 — RedirectStandardError = true created a pipe that nothing ever read. Past the
OS pipe buffer (65536 bytes on macOS and Linux, measured: 65536 fine, 65537 hangs)
the plugin's write blocks while we are blocked reading stdout. Neither side can
progress and there is no timeout, so the call hangs forever. Any plugin with
verbose logging, or one retrying a hardware token in a loop, triggers it.
Drained off-thread via ErrorDataReceived + BeginErrorReadLine, keeping the last
20 lines so a failure can quote what the plugin actually said — which the spec
asks for on encryption failure and which was previously impossible.
C7 — two spots let a misbehaving plugin throw raw BCL exceptions out of methods
documented to throw AgePluginException:
- stanza bodies went through Base64Unpadded.Decode unguarded, so malformed,
padded or non-canonical base64 surfaced as FormatException
- ReadStanza never validated the stanza charset the way Stanza.Parse does, so
the strings reached `new Stanza(...)` and came back as ArgumentException.
Two consecutive spaces in a plugin's stanza line were enough — no exotic
bytes needed.
Both now throw AgePluginException, so the documented `catch (AgeException)`
actually holds. DecodeOptionLabel next door already did this correctly; this
makes the feature internally consistent. Validating inside ReadStanza covers
PluginRecipient and PluginIdentity with a single guard.
All five new tests fail without the C7 guards.
I4 — neither direction validated the file index. We send exactly one file key, so 0 is the only valid value, but a recipient-stanza addressed to file 7 was accepted straight into the header and a file-key for phantom index 42 was honoured. A second file-key silently replaced the first, leaving the discarded key material unzeroed. go-age rejects all three. Only meaningful now that C5 makes us send 0 consistently, which is why it ships alongside it. I5 — a confirm with no arguments was answered with an invented "yes" label. The spec's form is (confirm, Base64(YES_STRING) [Base64(NO_STRING)]; MESSAGE), so the label is mandatory: the user saw a prompt whose affirmative button text the library made up, and a malformed command was answered as if well formed. Fixed in both verbatim copies, in PluginRecipient and PluginIdentity. H7 — Dispose waited 5 s for a plugin that had been asked to exit and then simply abandoned it, leaking the process and its hold on any hardware token for the lifetime of the host. Now kills the tree if the grace period expires. Three existing tests encoded the old behaviour and are corrected: - ConfirmWithoutLabels_UsesDefaults -> _Throws; the name said what the defect was - PluginIdentity_Unwrap_MultipleStanzas used file-key index 1 - FileKeyMissingIndex_Throws asserted the old message text
… S8) Eight agreement sites across five files, of which five had no all-zero guard and no try/catch. BouncyCastle rejects low-order and identity points itself, so no zero shared secret was ever used and the spec's MUST was satisfied — this is defence in depth plus a consistent exception type, not the closing of an exploitable hole. Worth being precise about, because the user-visible symptom looked far worse than the cryptography was: $ Age.Cli -d -i keys/id_ed25519 tampered.age age: internal error: X25519 agreement failed This is a bug. Please report it at .../issues A merely malformed input file reported as a library bug, and a caller doing the documented catch (AgeException) got an unhandled InvalidOperationException instead. The reachable site was SshEd25519Identity.Unwrap, where the ephemeral share comes straight from the stanza and is fully attacker-controlled. C9 — XWing.Encaps guarded neither its agreement nor MLKemPublicKeyParameters .FromEncoding, while Decaps in the same file guarded both. Since MlKem768X25519Recipient.Parse validates only HRP, length and case, a hostile age1pq1… string reached both calls and surfaced as ArgumentException out of the public Encrypt. S8 — HkdfDerive handed BouncyCastle an ikm.ToArray() copy of every input key — file keys, shared secrets — and never cleared it. Now in a finally. Kept main's byte[] return rather than next-version's span-filling signature, which would cascade into 22 call sites for no benefit here. All 8 sites now call CryptoHelper.X25519Agree; grep for CalculateAgreement outside CryptoHelper returns nothing. Unrouting any one site fails the five end-to-end tamper tests.
…, S12, H1, H4) The zeroization cluster, shipped together as the survey recommends. S7 — the entire post-quantum path cleared nothing: grep -c ZeroMemory returned 0 for both XWing.cs and HpkeHelper.cs. Now cleared: the ML-KEM private seed (d,z) and the X25519 scalar from ExpandSeed (both call sites discarded the seed with `_`, so no reference survived to clear it by), both shared-secret halves on encaps and decaps, the HPKE PRK, key and base nonce, and the labeled_ikm buffer that holds a verbatim copy of the X-Wing shared secret. MlKem768X25519Recipient .Wrap passed fileKey.ToArray() inline as an argument — an uncleared heap copy of the file key itself, with nothing to clear it by; now named and cleared. S9 — five reachable throw sites abandoned the file key. Fixed at the source rather than per call site: BuildHeaderAndFileKey and UnwrapHeaderFromReader now clear on their own throw paths, so EncryptDetached and DecryptDetached inherit it without changing their call shape. The routine cases matter most — VerifyMac failing on a tampered header, a plugin declining a touch prompt, a file truncated inside the payload nonce. S10 — DecryptIdentityFile produced three uncleared copies of the AGE-SECRET-KEY lines. Two are cleared; the string cannot be, because ParseIdentityFile takes a string and is shipped public API. Documented as reducing exposure, not removing it. S11 — Ed25519Converter dropped the full SHA-512 expansion of the SSH seed. Bytes 0-32 are the X25519 key SshEd25519Identity.Dispose carefully zeroes; 32-64 are the signing nonce prefix. Dispose was clearing one copy while a complete second copy sat in freed memory. S12 — Bech32.Decode left a trivially invertible 5-bit image of the same AGE-SECRET-KEY payload that both Parse methods take care to zero. ConvertBits also grew from an empty List<byte>, so reallocation scattered a non-deterministic number of stale generations; it now writes into an exactly-sized array, which also drops the ToArray copy. H1 — Header.ComputeMac left a file-key-derived MAC key on the heap on every encrypt and decrypt. The one derived key on main that nobody owned. H4 — X25519Identity.Unwrap allocated the shared secret outside its try. The old window had no reachable exposure (on every path that throws inside it the secret is all-zero by definition), so this is tidiness, not a vulnerability. Honest caveat: BouncyCastle keeps its own copies of key material, so these reduce residency rather than eliminate it.
Header.Parse decided what a line *is* using the one-argument
string.StartsWith(string), which is StringComparison.CurrentCulture. Under ICU
collation the C0 control characters and DEL are completely ignorable, and
HeaderReader.ValidateByte rejects only CR and bytes above 0x7F — so raw bytes
2D 01 01 3E 20 ("-", SOH, SOH, ">", space) satisfied StartsWith("-> ") and were
framed as a stanza. line[3..] then sliced off "-\x01\x01", leaving ">" as the
stanza type, which passes the printable-ASCII check. main accepted headers both
reference implementations reject, and disagreed with its own AOT build, where
invariant globalization makes the same comparison ordinal.
Every framing comparison is now explicitly Ordinal, across Header, Stanza,
PluginConnection, AgeKeygen and both plugin types.
Note on the test: an end-to-end Decrypt test is worthless here, because a bogus
header fails the MAC either way and AgeHmacException is also an AgeException — it
passes with the defect present. The assertion is therefore at the framing level,
on how the line is classified. Three of the five cases fail without the fix.
…s (I2, I3, H2a, H6) I2 — MlKem768X25519Recipient.Parse checked HRP, total length and case, and never decoded the 1184-byte ML-KEM encapsulation key. A recipients-file validator would pass a file that then failed partway through an encryption. Go's ParseHybridRecipient runs the ByteEncode/ByteDecode round trip at parse time; XWing.ValidatePublicKey now does the same. I3 — the scrypt work factor was capped at 20 on both construct and decrypt. The spec only says an identity implementation SHOULD apply an upper limit, so 20 was legal, but Go's ScryptIdentity defaults to 22 and cmd/age never lowers it — so the reference CLI happily produces files at 21 and 22 that main then refused, and main could not produce them either. Raised to 22 to match. That is a deliberate trade rather than an oversight: the cap bounds how much work an attacker-supplied header can demand, and 22 is ~15s on a modern machine. We now accept exactly what the reference accepts, which is the right default for a library whose job is to read age files. Verified: 20, 21 and 22 all round-trip. Two existing tests pinned the old boundary — one was literally named Unwrap_Rejects_WorkFactor_Over_20 — and now assert the 22/23 boundary. H2(a) — ScryptRecipient cleared its wrap key and the UTF-8 passphrase copy after the operation rather than in a finally, so a throw skipped both. Only the passphrase string itself remains unclearable, which would need a new public type. H6 — MlKem768X25519Identity.Recipient re-ran a full ML-KEM-768 key generation on every access, so decrypting an N-stanza header cost N keygens. Cached. This is a bounded constant factor, not a denial-of-service vector.
…H9, H11) H3 — three secrets crossed the plugin wire with nothing cleaned up. WriteStanza base64-encoded the body into an immutable string, so the wrapped file key (and the recovered one coming back) sat on the heap unclearable; SendWrapRequest passed fileKey.ToArray() inline with no reference left to clear. Both now use a pooled char[] and a named copy, cleared in a finally. Only the internal half is taken, as the survey advises — IPluginCallbacks is untouched, so the PIN a callback returns as a string is still beyond reach. H11 — the leading-whitespace skip had no counter, so a file consisting entirely of newlines was read to its end before the header was looked for. Bounded at 1 KiB, matching go-age. The limit is deliberately internal: a patch release should not grow the public surface, and the analyzer caught the first attempt to add it publicly, which is the guardrail working. H9 — an explicit rejection of a bare CR in an armor line, matching rage's LineContainsCr. Being straight about this one: it does NOT close a reachable acceptance. StreamReader.ReadLine splits on a lone CR and the fragments already fail the line-width rules, so a bare CR was rejected before this change too, just incidentally and with a confusing message. The test says so rather than implying it proves a fix. H8 — StreamEncryption's whole-stream Encrypt/Decrypt have no production callers (only 12 test sites) and buffer the entire input, contradicting the library's memory-bounded guarantee. Documented as test-only rather than deleted; removing them would churn the chunk-sequencing tests for no user benefit. Armor interop re-verified: 221 sizes through the reference CLI, 0 failures.
…essage, H5) I1 — armor is auto-detected only on a seekable stream, so armored input from a pipe reaches the binary header parser intact and its BEGIN marker was reported as "unsupported version: -----BEGIN AGE ENCRYPTED FILE-----". That sent people looking for a version problem that does not exist. The message now names the actual cause and the fix. Only the message. Full non-seekable armor support needs a lookahead wrapper and widens what the library accepts, against its own documented behaviour — a behaviour change that belongs in a minor release, not a patch. The survey lists the full fix under "do not backport" and suggests exactly this cheaper half. H5 — AgeRandomAccess.DecryptChunkAt returned without zeroing on its one throw path. Being straight: the guard is plaintext.Length == 0, so the abandoned array is zero-length and there was nothing to leak. Included so "every decrypted chunk is zeroed on every path" holds without a caveat, not because it fixed anything. That accounts for 39 of the survey's 40. H10 is deliberately excluded: the survey classes it as a documented divergence rather than a defect, consistent with IsArmored's own whitespace handling.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #91 +/- ##
==========================================
+ Coverage 91.41% 93.16% +1.75%
==========================================
Files 42 46 +4
Lines 2469 2679 +210
Branches 329 351 +22
==========================================
+ Hits 2257 2496 +239
+ Misses 153 124 -29
Partials 59 59
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Codecov flagged the two files the S1 fix introduced as the least covered in the
change — PluginLocator at 61% and PluginConnection at 52% — which is a fair hit:
the resolution logic *is* the security fix, and it had no direct tests beyond the
working-directory refusal.
Adds the resolution cases that were missing: absent PATH, search past the first
entry, earlier entry wins, a PATH entry the platform cannot express is skipped
rather than thrown out of, the environment-reading overload does not fall back to
the working directory, and PATHEXT handling on Windows.
Also adds the positive half of S1, which nothing covered: a plugin genuinely on
PATH is launched and its stanza used. That is the only test driving
PluginConnection's real process path — spawning, stanza framing over stdio, the
C6 stderr drain (the fake plugin writes ~4.5 KiB to stderr) and Dispose. Without
it the suite proved only that plugins are refused, never that they still work.
Three things that made the fake plugin harder than expected, recorded because
each cost a debugging round:
- POSIX sh redirects a background job's stdin from /dev/null, so draining with
`cat &` takes EOF immediately and the script exits under the client's writes.
The drain has to be in the foreground.
- the child inherits PATH, so replacing it outright leaves the plugin runnable
but unable to find touch or cat. Prepend instead.
- a stanza body ends at the first line under 64 characters, so a 6-char body
needs no terminator line while an empty one does.
Some of PluginLocator stays uncovered by design: the PATHEXT and Windows
branches cannot execute on the Linux job that uploads coverage.
The workflow uploaded from ubuntu-latest alone, on the stated assumption that "coverage is identical across platforms". That assumption held until this branch added PluginLocator, which contains genuinely platform-specific code: PATHEXT expansion and the Windows branch of the executable-bit check cannot execute on Linux by construction. The result was a coverage report claiming PluginLocator — the file implementing the S1 fix — sat at 61%, when the uncovered lines are Windows-only and are in fact exercised by the Windows job. Adding tests could never have moved it. Uploads now happen from all three matrix legs, flagged by OS, and Codecov merges them per commit.
The S1 tests were skipped on Windows because planting an executable was written with POSIX file modes. That left the platform where the vulnerability is most classic — CreateProcess searches the application directory and the working directory by documented design — with no end-to-end coverage at all. The fake plugin is now planted as a .CMD on Windows and a shell script elsewhere, so both the "never runs from the working directory" and the "does run from PATH" cases execute on all three legs of the matrix. This also repairs what the previous commit exposed: uploading coverage from every platform lifted PluginLocator from 61% to 79.5% (its PATHEXT and Windows branches are genuinely exercised, just not on Linux), but dropped PluginConnection from 79% to 67%, because the Windows report saw its process path skipped entirely. With the tests running everywhere that gap closes for the right reason rather than by hiding the measurement.
The Windows leg failed on Directory.Delete, not on any assertion: a just-written .CMD is typically still held open by the virus scanner when the test finishes, so removing the temp directory throws IOException after the test has already passed. Notably PluginOnPath_IsLaunchedAndItsStanzaIsUsed itself passed on Windows, which confirms the CMD fake plugin speaks the protocol correctly — the only problem was teardown. Temp directories live under %TEMP% and the OS reclaims them, so cleanup failure is now swallowed rather than deciding a verdict.
With uploads now arriving from every matrix leg, whichever landed first was deciding the status: PluginLocator was reported at 79.5% on one run and 61.4% on the next from identical code, depending on whether the Windows upload had arrived. after_n_builds: 3 makes Codecov wait for the full set before computing a status, so the number reflects the merged report rather than a race.
Owner
Author
|
Addressed the coverage report. Patch coverage 81.05% → 87.90% (94 → 60 missing lines); Three things, two of which were real problems rather than missing tests: Genuine gaps, now covered. A stale CI assumption. Coverage uploaded from Linux alone, on the stated basis that A reporting race. With three uploads, whichever landed first decided the status — What remains, and why I stopped. The residual 60 lines are error paths and defensive All four test jobs pass on Linux, macOS and Windows. 🤖 Addressed by Claude Code |
Chasing the patch-coverage gap turned up two things that were not coverage artifacts at all. C6 was half-finished. The stderr pipe was drained — fixing the deadlock — and the tail was stored in a property nothing ever read. The survey's fix shape asked for it to be appended to the exception message, "which main currently cannot do at all", and that half was missing: when a plugin dies without answering, its stderr is the only account of why, and we were still discarding it. PluginConnection now builds failures through a helper that quotes the tail, used by both plugin types for an unexpected end of output and for plugin-reported errors. Writing the test for it exposed a race in the fix itself: BeginErrorReadLine delivers asynchronously, so at the moment a failure is detected the plugin's last words have usually not arrived, and the tail came back empty exactly when it was most useful. Failure() now waits briefly for exit and flushes the async readers first — WaitForExit(int) does not do that, only the parameterless overload does. PluginLocator's catch around Path.Combine is removed: .NET Core dropped the invalid-character check, so it cannot throw and the handler was unreachable by construction. Unreachable defensive code is worse than none — it reads as a case that has been handled when it never has. The test that covered it stays, with a comment saying it now passes for a duller reason.
The Linux leg caught what macOS timing had hidden: when a plugin exits immediately, the client is still writing its request, and the closed pipe surfaced as System.IO.IOException out of PluginRecipient.Wrap — a method documented to throw AgePluginException. Same class of defect as C7, just on the write side rather than the read side. Which end of the protocol notices the death first is a timing accident: the client sends the whole request before reading a byte, so a fast exit breaks the write and a slow one breaks the read. Previously those produced different exception types on different machines. Both now report as AgePluginException with the plugin's stderr attached. The regression test drives it through a writer that throws rather than racing a real process, so it is deterministic everywhere. The end-to-end dying-plugin test stays as well, since it is the one that exercises the real pipe.
Writing the I2 regression test I had skipped showed that both fixes were
ineffective. BouncyCastle's MLKemPublicKeyParameters.FromEncoding does not check
the coefficients — it defers that to Encapsulate — so:
- ValidatePublicKey accepted a 1216-byte key of 0xFF and Parse returned a
recipient, which is precisely the defect I2 was meant to close
- C9's catch sat around FromEncoding, while the raw
"ArgumentException: Modulus check failed for ml-kem encapsulation" comes out
of Encapsulate, several lines further down and outside the try
FIPS 203's ByteDecode_12 requires every coefficient below q = 3329, and Go runs
the equivalent ByteEncode(ByteDecode(x)) == x round trip when parsing a
recipient. That check is now done directly: the encapsulation key is 1152 bytes
of 12-bit coefficients, three bytes to every two, and each must be under the
modulus. Cheap enough for a parse, unlike a trial encapsulation.
Applied in both places — at parse time so a bad recipient never reaches an
encryption, and inside Encaps, which is internal and reachable without going
through Parse.
Genuine post-quantum recipients are unaffected: all 24 ML-KEM tests pass,
including both interop directions against age v1.3.1.
Patch coverage was short of the bar and I had been treating the remainder as
unreachable without checking each line. Most of it was reachable.
- AgeEncrypt's multi-stanza dispatch and PluginRecipient.WrapAll: C4's seam had
no end-to-end test at all. The scripted tests all call WrapWithConnection
directly, so encrypting to a plugin recipient through the public facade — the
path that actually chooses IMultiStanzaRecipient — was never exercised.
- The wrong-sized-file-key guard, via a custom IIdentity handing back 15 bytes.
Caller-supplied code can return anything and accepting it would derive
garbage rather than fail.
- AgeRandomAccess over a payload with no chunks and over one too short to hold
an authentication tag: the same rejections the forward-only path makes.
- H7's kill path, with a plugin that answers and then sleeps past the grace
period instead of exiting.
- Base64Unpadded's span overload: too-small buffer, exact fit, and empty input.
DearmorStream's carriage-return guard is removed rather than covered. It was
unreachable by construction — StreamReader.ReadLine splits on \r, \n and \r\n
alike, so a line it returns can never contain a CR. Same reasoning already
applied to PluginLocator's Path.Combine catch. A bare CR is still rejected, by
the line-width rules, and the test now pins that outcome instead of implying a
fix. Keeping dead code to satisfy a coverage tool would be the wrong trade twice
over.
Local patch coverage 89.7% -> 95.2%. Of the remainder, nine lines are
PluginLocator's PATHEXT and Windows executable checks, which the Windows job
covers but a Linux report cannot see.
Auditing for half-finished work — the failure mode that hid the unread stderr tail and the ineffective ML-KEM check — turned up one fix with no test at all. H6's recipient cache was unguarded. Added a test that it is derived once and reused, and a second for the interaction with S5 that I had not thought through: a populated cache must not become a way to reach a disposed identity's derived key. It does not, but nothing said so. Also removes docs/BACKPORT_BRIEF.md, which was the prompt written to commission the survey rather than a project document. docs/BACKPORT_0.2.md, the survey itself, stays — every commit here cites a section of it.
Six public entry points repeated the same four lines verbatim, which is exactly how one of them ends up worded differently or missing the check. ArgumentGuard .ThrowIfEmpty takes its place, shaped like the BCL's own ThrowIfNull / ThrowIfNullOrEmpty helpers — the BCL has no span equivalent, and recipients and identities arrive as params ReadOnlySpan<T>. CallerArgumentExpression captures the parameter name, so the argument being named stays correct if a parameter is ever renamed. The noun is passed explicitly rather than derived from it, because "identities" does not depluralise by dropping a letter. Behaviour is unchanged and the tests assert that rather than assume it: both the exact message and ParamName are checked at every entry point. Which matters, because the guard had no tests at all. Eight of the nine new ones cover an entry point apiece; the ninth pins that the check runs before the input stream is touched, so passing no recipients cannot consume a caller's non-rewindable stream first.
Classic extension methods extend instances, so a static member on a framework
type used to be impossible. C# 14 extension members lift that, and this project
already targets LangVersion 14 — so the call site can read exactly as intended:
ArgumentException.ThrowIfEmpty(recipients, "recipient");
sitting beside the BCL's own ThrowIfNull and ThrowIfNullOrEmpty rather than
beside a bespoke helper class nobody would think to look for. The BCL has no span
form of its own; its overload takes a string, while recipients and identities
arrive here as params ReadOnlySpan<T>.
Kept internal. Extending a framework type is a liberty worth taking for six call
sites inside one library and not for anything a consumer would see.
Behaviour is unchanged and the tests prove it rather than assume it: they drive
the public entry points and assert both the exact message and ParamName, so they
pass across the swap without an edit.
You could not tell, reading EncryptDetached, whether a throw inside BuildHeaderAndFileKey leaked the key it had just generated. The answer was no — that method had its own catch/clear — but the guarantee lived in another method, and a caller had to go and read it to know. That non-locality is how S9 happened in the first place. All five abandoned-key sites the survey found were "the clear is somewhere else" situations, and the fix at the time added catch blocks rather than removing the reason they were needed. So the factory no longer creates what it cannot clear. BuildHeaderAndFileKey becomes BuildHeader(recipients, fileKey): it wraps a key it is given and owns nothing, so it needs no catch. The two callers create the key themselves, one line above the try whose finally clears it. Creation and guarantee are now visible together, and a reader never has to leave the method to be sure. Behaviour is unchanged: previously the factory's catch cleared on a throw, now the caller's finally does. The new test pins the property rather than the shape — a recipient that throws mid-wrap (a missing plugin, a declined touch prompt) must not leave a live key, checked on both the streaming and detached paths.
Too many, and the worst of them narrated what had been broken — which is what the commit messages are for. Kept spec citations, platform traps (the pipe buffer, PATHEXT, POSIX backgrounding) and the genuinely non-obvious, such as BouncyCastle checking ML-KEM coefficients in Encapsulate rather than FromEncoding. Dropped the defect histories and anything restating the code. Inline comment density across the change drops from 36% to 22%.
You were right that the try/finally was the wrong shape. A byte[] cannot carry the guarantee, so every site had to remember one — and the sites that forgot were the S9 defects. FileKey owns its 16 bytes and zeroes on Dispose, so `using` states the guarantee where the key is created and the compiler enforces it. Six ownership sites lose their try/finally; grep for ZeroMemory(fileKey) now returns nothing. Two entry points, Fresh() and Adopt(). Adopt exists because IIdentity.Unwrap is shipped public API returning byte[], so a recovered key arrives as a bare array; it validates the length and takes ownership, and zeroes the array it rejects — that array came from caller-supplied identity code and still held key material. That also absorbs the wrong-size guard UnwrapHeaderFromReader used to carry. Internal on purpose. IRecipient.Wrap takes a ReadOnlySpan and IIdentity.Unwrap returns a byte[], both shipped; callers still see exactly what they saw, and inside the library everything passes fileKey.Bytes. Net -45 lines across the two facades.
Six copies of the same "loop until the buffer is full or the stream ends" pattern, in DecryptStream, EncryptStream, ArmorStream, AsciiArmor, HeaderReader and the detached-payload nonce read. Stream.ReadAtLeast with throwOnEndOfStream: false is exactly that contract, so each becomes one line and the partial-read handling stops being restated. EnsureMaterialized moves the empty-write nudge into an extension member. Both call sites had the same two-line comment explaining why an empty plaintext still has to touch the output; that explanation now lives with the operation. FileKey.Size becomes private and AgeEncrypt.FileKeySize goes — both were left over from before FileKey owned the size.
The dearmor bool was named for cleanup but its job is ownership, and its
load-bearing half is `false` — `input` belongs to the caller, and disposing it
would close a stream they still hold. Same meaning as DecryptStream's ownsStream
and the BCL's leaveOpen, so it now uses that name.
Only DecryptReader still carries it: ownership leaves that method, passing to the
returned DecryptStream on success, so the catch covers the failure path alone and
a `using` cannot express it.
AgeHeader.Parse never transfers ownership, so which variable holds the stream can
say it instead — `using` on a null is a no-op:
using var dearmored = isArmored ? AsciiArmor.Dearmor(input) : null;
var reader = new HeaderReader(dearmored ?? input);
AgeRandomAccess was guarding nothing. DeArmorInput already disposed the
DearmorStream and returned a MemoryStream, and MemoryStream.Dispose does not free
its buffer, so the finally freed nothing while the flag doubled as the only proof
the (MemoryStream) downcast was safe. It now assigns the field and lets the
existing Dispose handle it: if construction throws, nothing is handed out and the
stream is garbage, which is all disposing would have achieved.
No behaviour change. StreamOwnershipTests already covers all six entry points in
both armored and binary shapes.
DeArmorIfNeeded had one caller left, and a private helper whose whole body is a two-line conditional is indirection without abstraction. Reading DecryptReader meant jumping away to learn the one fact that matters: ownsStream is true exactly when binaryInput is not the caller's stream. Inlined, those are adjacent lines. The bool goes with it. `dearmored` is now the ownership token — non-null means we made it — so cleanup is `dearmored?.Dispose()` and the flag DecryptStream's constructor needs is derived at the call, `ownsStream: dearmored is not null`, rather than tracked separately where it could drift. That also removes the one real merit of the wrapper-struct idea: a tuple could be destructured and the stream carried onward without its bit. There is no tuple now. All three dearmor sites read the same way, `dearmored ?? input`, so the only thing that stands out is the genuine difference — this one cannot use `using`, because on success ownership passes to the stream it returns.
pscheid92
force-pushed
the
backport/v0.2-fixes
branch
from
July 27, 2026 23:37
ce00325 to
a547c09
Compare
"cannot mix recipients with different security labels" is accurate and tells the user nothing about what they lost. Go special-cases the post-quantum mismatch (references/go-age/age.go:215) because that is the one where the consequence is worth spelling out: the file would be readable by a quantum computer, so the PQ recipient bought nothing. Other mismatches now name both labels, with the empty set printed as "none" rather than as an empty string. Pure message change — same exception type, same condition, no API impact. The comment above the loop now cites age-plugin.md:227 and says why comparing each recipient against the first is a complete check rather than a shortcut. Also records in BACKPORT_0.2.md, under S6, what the spec actually requires: a label is an unordered set per recipient (age-plugin.md:307), sets must match exactly with no partial overlap (:227), and duplicates are forbidden (:305) so Go's sorted-list equality is set equality. Notes which of the three documented idioms our string? Label can still express, that a set of size >= 2 cannot be, and the structural reason a property cannot carry plugin labels at all — it is read before wrapping, while a plugin only declares labels mid-conversation. Adds the scrypt divergence as deliberate: the spec gives scrypt an implicit random singleton label (:232), we enforce "must be alone" structurally instead, which also catches custom recipients that emit scrypt stanzas. Includes an unrelated tidy of DecryptReader that was already in the working tree: binaryInput moved inside the try, trailing newline dropped.
Two kinds of // comment were carrying their weight badly. Step narration: "Generate ephemeral X25519 key pair" above a line that generates an ephemeral X25519 key pair, "Check tag matches" above a tag comparison, "ML-KEM-768 encapsulate" above an Encapsulate call. 35 lines of that, gone. Bug history: several blocks explained what the code used to do wrong rather than what it now guarantees — "the old byte-level parser", "main refused genuine age-produced files", "the practical exposure of the old window was nil". That belongs in BACKPORT_0.2.md, which already has all of it at length with reproductions. In the source it reads as an apology and dates badly: once v0.2 is the only line anyone runs, "on main" means nothing. Twelve blocks collapsed to the constraint a reader actually needs, the longest from nine lines to three. What stayed: spec formulas next to the code implementing them (HPKE's labeled_ikm, the HKDF salt/info layouts, X-Wing's SHA3 combiner), because a reviewer checks those against the spec line by line; the BIP-173 citations in Bech32; and every comment explaining a constraint the code cannot state — why a buffer is zeroed, why stderr is drained off-thread, what FILE_INDEX means, which stream the dispose chain stops at. 64 of 322 lines removed, 9.7% -> 7.9% of code lines. Comments only: `git diff` filtered to non-comment lines is empty.
24 -> 15 and 19 -> 13 // lines, both files 16%/15% -> 10%. Five blocks were near-verbatim in both files: the stderr-quoting note, the no-UI-means-fail rule, the request-secret/request-public distinction, the executable-path validation, and the HRP shape. They are duplicated because the code is — one of the comments said so out loud, "this pair of methods is duplicated verbatim across the two plugin types". Narrating that instead of fixing it helps nobody, so the meta-commentary is gone and the surviving pairs are short enough that duplication costs little. The duplication itself is real and worth collapsing, but that is a refactor, not a comment change. Two HRP comments per file explained the same slice twice, once by example and once by bounds; merged into one that does both. Trimmed the remaining history: "silently overwriting left the discarded key material unzeroed on the heap", "left it on the heap with no reference to clear it by", "Overwriting here discarded every stanza but the last". What each guard protects is worth stating; what it used to fail to protect is in BACKPORT_0.2.md. What stayed is protocol semantics that genuinely are not inferable from the code: FILE_INDEX identifying the file rather than the stanza, why stanzas accumulate, why extension-labels is deliberately not advertised, and the mandatory yes label in a confirm command. Comments only. Both files kept their no-trailing-newline ending.
The published table had post-quantum keygen at 251 ns against X25519's 1,982 ns — eight times faster, which is backwards for ML-KEM-768 against a curve operation. The number was real and reproducible; it just measured the wrong thing. The two types defer work in opposite directions. X25519Identity.Generate does the keygen and Recipient wraps an already-computed public key. MlKem768X25519Identity .Generate fills a 32-byte seed and the ML-KEM keygen runs on first access to Recipient, cached there since H6. Timing Generate alone therefore compares a real keygen against an RNG fill. Adds X25519ToRecipient and MlKem768X25519ToRecipient — secret key to usable public key, which is the comparable figure: X25519 28.6 us 2.1 KB ML-KEM-768-X25519 91.3 us 28.5 KB Post-quantum costs ~3.2x the time and ~14x the memory. The Generate-only pair stays, labelled as what it is: the cost of minting a secret you will store rather than immediately turn into a recipient. Docs updated with both tables and why there are two.
The spec's ABNF (age.md:132) is
body = *full-line final-line
full-line = 64base64char LF
final-line = *63base64char LF
so the final line is not optional. Both write sites reconstructed that with a
loop over Math.Min(64, remaining) plus a trailing `if (length % 64 == 0)`, which
is the same rule derived backwards and needed a comment to explain that an empty
body and an exact multiple of 64 are the same case.
Writing full lines while at least 64 remain, then the final line unconditionally,
says it directly:
while (encoded.Length >= ColumnsPerLine) { write 64; write '\n'; advance }
write(encoded); write('\n');
The empty body and the exact-multiple case fall out — no modulo, no comment.
Both copies now call Stanza.WriteBody. PluginConnection keeps its pooled clearable
buffer and passes a span, so the file key still never reaches an immutable string.
Adds a boundary sweep at body lengths 0, 1, 47, 48, 49, 95, 96, 97 — the encoded
lengths that straddle 0, 64 and 128 chars — asserting every line but the last is
exactly 64, the last is shorter, and the stanza round-trips through Stanza.Parse.
Verified non-vacuous: making the final line conditional fails 3 of the 8.
Wire format unchanged; the 45 interop tests against the real age binary still pass.
There were three copies, not two: Stanza had the pair you spotted, and PluginConnection has a third with AgePluginException. All three carried the same `!`..`~` range and the same empty check. The messages differ on purpose — they blame different parties. Wire data is a malformed header, constructor input is a caller mistake, and a plugin sending a space is the plugin's fault. So only the rule moves: Stanza.IndexOfNonVChar, which PluginConnection now calls. Each site keeps its own exception and wording. Within Stanza the two remaining methods differ only in which exception carries the reason, so StanzaStringError returns the message and the two throwers are one line each. The range was untested at its edges. Existing cases use "a b" and "tüpe", which a one-off boundary would still reject, so a drift in the merged predicate would have gone unnoticed. Added a theory pinning 0x20/0x21/0x7E/0x7F — the argument separator, the first and last legal characters, and DEL, which passes the byte validator and has to fail here. Plus the parse path for DEL, where the failure is AgeHeaderException instead; space is excluded there because on the wire it is the separator, so "-> a b" is a legal stanza with an argument. Verified non-vacuous: widening the range to ' '..'~' fails the boundary theory.
ValidateStanzaString and EnsureValidStanzaString are synonyms in English, so
neither name said which one you were looking at — you had to read the body to find
the only real difference, which is who gets blamed.
ThrowIfMalformed(s) wire data -> AgeHeaderException
ThrowIfInvalidArgument(s, paramName) caller -> ArgumentException
That follows the convention already in use here: ArgumentNullException.ThrowIfNull,
ObjectDisposedException.ThrowIf, and this repo's own ArgumentException.ThrowIfEmpty.
The call sites now read as the thing that can go wrong at each of them.
StanzaStringError -> InvalidReason, since it lives in Stanza and the "or null when
fine" contract reads better as `if (InvalidReason(s) is { } reason)`.
Names only; no behaviour change.
PluginConnection.ValidateStanzaString -> ThrowIfMalformed, matching Stanza. Both take a string that arrived from outside and reject it as malformed; they differ only in where it arrived from, and the containing class already says that. So the shared name is the honest one, and the exception type follows from context: inside Stanza it is AgeHeaderException, inside PluginConnection AgePluginException. The `what` parameter stays, since the plugin messages name which part was bad. Three validators now, and the pair of names says what each is for: Stanza.ThrowIfMalformed header wire data -> AgeHeaderException Stanza.ThrowIfInvalidArgument caller input -> ArgumentException PluginConnection.ThrowIfMalformed plugin output -> AgePluginException Names only; no behaviour change.
The switch stated termination twice: `case > 0` decoded, then a separate `if (bodyLine.Length < 64) break` decided the loop was over. Three ordered guards say it once each — too wide is an error, non-empty decodes, short ends the body — and returning from the short-line branch drops the `while (true)` bookkeeping. Also uses ColumnsPerLine. WriteBody had the named constant and ReadBody still had the literal 64, in three places, for the same rule. AssembleBody -> Concat, and kept as a manual exactly-sized copy. List<byte> with AddRange or SelectMany().ToArray() would both be shorter, and both grow by reallocating — every abandoned backing array keeps a copy of the wrapped file key that nothing can reach to clear. Bech32 already rejected List<byte> for exactly this reason; the remark now says so where the shortcut is tempting. The over-wide line guard was tested on the plugin side but never on the header side, so a header line of 65 characters now proves it there too.
Three ifs all compared line.Length to ColumnsPerLine, in three different
directions, and nothing said how they related. Reading it meant reconstructing
`*full-line final-line` from >, >0 and < to work out which branch was the error,
which the continue and which the stop.
Splitting reading from interpreting fixes that. ReadBodyLine returns a line
guaranteed no wider than a full-line, so inside the loop "not full width" means
"final line" with nothing else to rule out. What is left is one decision in the
body — decode anything non-empty — and the grammar as the loop condition:
do { ... } while (line.Length == ColumnsPerLine);
which reads as: keep going while the lines are full-lines.
Same behaviour, same messages; an over-wide line still throws before it is
decoded, and an empty final line still contributes nothing.
Your framing, and it exposes a wart in the do-while I had just written: the
`if (line.Length > 0)` guard sat inside the loop but could only ever be false on
the last iteration. It is a statement about the final-line, run against every
full-line on the way there.
while ((line = ReadBodyLine(reader)).Length == ColumnsPerLine)
chunks.Add(Base64Unpadded.Decode(line));
chunks.Add(Base64Unpadded.Decode(line));
Now the loop decodes full-lines only, and the final-line is decoded once, outside,
where "MAY be empty" (age.md:87) is the whole reason that line exists. The loop
condition already read it, so "one more" is one more decode rather than one more
read.
The guard is gone rather than moved: Base64Unpadded.Decode returns [] for empty
input, so an empty final-line contributes an empty chunk and Concat sums the same.
Covered by the existing sweep — body lengths 0, 48 and 96 are exactly the cases
whose encodings end with an empty final-line.
The 65-character case proved less than it looked. 65 % 4 == 1 is not a decodable base64 length, so the decoder rejects it whether or not the width guard exists — removing the guard still failed the test, just with FormatException instead of AgeHeaderException. 68 is the case that carries the weight: it decodes cleanly to 51 bytes, so nothing but the width guard stands between it and a silently accepted over-long line. Confirmed by removing the guard, which parses it and returns a 51-byte body. Both widths are now checked, so the failure mode is pinned either way.
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.
Backports 39 of the 40 defects found by a differential audit of
mainagainst thereference
ageCLI, the Go implementation, and the Rustagecrate. The surveyitself is committed as
docs/BACKPORT_0.2.md— every fix below cites a section of it.Why now.
0.2.0-preview.3is what NuGet serves, and the v0.3 line is beingabandoned, so the v0.2 branch is the only living version for the foreseeable future.
Every bug fixed elsewhere but still present here is a bug in the only thing anyone
can install.
Scope
documented divergence rather than a defect, consistent with
IsArmored's ownwhitespace handling.
PublicAPI.Shipped.txtandPublicAPI.Unshipped.txtare untouched; a v0.2 consumer updates the package without editing a line. Where a
fix on the v0.3 branch was entangled with API redesign, the API-neutral form was
written fresh against this code shape instead.
warnings under
-p:TreatWarningsAsErrors=true.The five that matter most
age-plugin.mdprohibits it verbatim: "Paths relative to the current working directory MUST NOT be searched, even on platforms or systems where this is the default." Confirmed by direct experiment — .NET resolves a bareFileNamefrom the CWD even withUseShellExecute = false.==. Measured: failures at plaintext sizes 38, 39, 86, 87, 134, 135, 182, 183 — two in every 48. Now 0 failures across a 221-size sweep.AgeRandomAccessnever authenticated the final chunkageand this library's own forward-only path reject. The spec requires verifying the last chunk is a valid final chunk.Disposereturned pooled buffers twiceArrayPoolrenters handed the same array, from ordinaryusingcode. Surfaces to users as a spuriouschunk 0 authentication failedon a perfectly valid file.Behaviour changes worth a close look
documented invariant, but code relying on the old disposal will now leak a handle.
PluginRecipient.Wrapnow throws when a plugin returns multiplestanzas rather than silently returning the last. Louder failure replacing silent
data loss, but it is a new exception from a public method.
ScryptIdentitydefault. A judgement call, not the survey's: it buys reading files the reference
CLI produces, at the cost of accepting the same attacker-demanded work Go already
accepts.
FILE_INDEXandconfirmargument counts are now validated, soa non-conforming plugin that previously worked will now fail.
InvalidOperationExceptiontoAgeHeaderException.How each fix was verified
Every fix was checked by reintroducing the defect and confirming a test fails.
That caught two tests that would otherwise have been worthless:
MAC anyway and
AgeHmacExceptionis also anAgeException. Rewritten to assert atthe framing level, where the difference is observable.
already rejected incidentally; the guard makes it explicit, not newly safe.
Seven existing test assertions encoded the buggy behaviour and were corrected,
including
ConfirmWithoutLabels_UsesDefaultsandUnwrap_Rejects_WorkFactor_Over_20— both named the defect in their own titles. Each change is annotated with why the
old expectation was wrong; that is exactly the move that can hide a regression, so it
deserves review.
Honest caveats
BouncyCastle retains its own copies of key material.
already rejects at all eight agreement sites, so no zero shared secret was ever
used. What was broken is the exception contract — the CLI reported a merely
malformed input file as
This is a bug.PluginLocator's PATH resolution andPATHEXThandling are the most platform-sensitive code here and have never run onLinux or Windows. This PR exists partly to get them through the matrix.