Conversation
Closes #227. The three tests named for the 4 KiB key-file cap never executed it: each seeded a partial key directory, and `checkKeyDirConsistency` runs before anything is loaded, so they asserted on the consistency error and passed without reaching `readCapped`. **Falsified, not inferred.** With both size guards short-circuited to `false`: | | Before this PR | After | |---|---|---| | `TestNew_oversizedKeyFileRejected` | PASS | FAIL | | `TestNew_oversizedPublicKeyFileRejected` | PASS | FAIL | | `TestNew_oversizedRefreshSecretRejected` | PASS | FAIL | The cap could have been deleted entirely and the suite would have stayed green. The fix is to seed a complete valid directory and then replace one file with an oversized one, and to assert on the size-cap error specifically. `assert err != nil` is what let this survive — it is satisfied by whichever failure arrives first. `internal/keymanager` coverage 87.4% -> 88.9%. `go vet`, `golangci-lint` (0 issues) and `go test -race ./...` pass. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
First pass of the sweep in #229, scoped to `auth/oauth`. I did not read the tests looking for weak ones — I deleted each security control in turn and checked whether anything went red. Five survived: | Control | Before | After | |---|---|---| | Redirect hop limit (`len(via) >= 5`) | survives deletion | detected | | Cross-origin redirect (the token POST replays the client secret) | survives deletion | detected | | RSA modulus bounds (2048–16384) | survives deletion | detected | | EC coordinates outside the field | survives deletion | detected | | ID token with no `sub` | survives deletion | detected | Every one had been added deliberately as hardening. None was being executed. ### Why the redirect ones could never have been caught end to end `safeRedirect` applies four guards in order — hop count, https, private host, same origin. Any test that redirects between local servers dies at the second or third, because an `httptest` server is plaintext on a loopback address. The last two were structurally unreachable through `Exchange`, so `TestExchange_refusesUnsafeRedirect` stayed green with either deleted, and more integration tests would not have helped. They are driven against the function directly now, each case asserting its own rejection reason rather than accepting any error. **I nearly shipped a worse version of this.** My first attempt replaced the redirect test with one pointing at a live second server, on the theory that the original passed only because `evil.example` does not resolve. It passed with the cross-origin check deleted too — the plaintext guard fired first. Same trap as #227: an assertion satisfied by whichever failure arrives first. That is what sent me to the unit-level test. ### The rest `parseJWK` gets direct tests for the modulus floor and ceiling, an EC coordinate wider than the field, an unknown curve, and — new — **P-384 and P-521, which the module advertises and had never once parsed**. The empty-subject case signs a token with `sub` removed; accepting one hands the consumer an empty subject, which their storage is liable to treat as a real account key. ### Measured `go vet`, `golangci-lint` (0 issues) and `go test -race ./...` pass. `auth/oauth` coverage 87.7% -> 90.2%, which clears the gate it was under in #218. Every new test was confirmed by deleting the control it names and watching it fail. That is the only reason to believe any of them. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
) Second pass of #229, in the module with the widest blast radius. Same method: delete each control, see whether anything goes red. | Control | Before | After | |---|---|---| | EdDSA algorithm check (`eddsaKeyFunc`) | survives deletion | detected | | `WithExpirationRequired` | survives deletion | detected | | `WithIssuer` | survives deletion | detected | | `WithAudience` | survives deletion | detected | | 8 KiB length cap, access path | survives deletion | detected | | 8 KiB length cap, refresh path | survives deletion | detected | | Refresh token accepted as access | already detected | — | | Unknown `kid` falling back to another key | already detected | — | Claim validation here is entirely parser options — there is no hand-written check for `exp`, `iss` or `aud` — so removing an option removes a control with nothing to notice, and every existing test builds a well-formed token. **Without `WithExpirationRequired` a token carrying no `exp` is accepted, and it never expires.** Without the issuer and audience options, a token minted by the same key for a different service verifies here: the cross-service reuse those lines say they defend against. ### The algorithm check needed a different kind of test `mapJWTError` collapses everything into `ErrTokenInvalid`, so from outside there is no way to distinguish a token refused for its algorithm from one refused for its signature. Deleting the check cannot change any observable behaviour of the public API, which is exactly why no test could catch it. It is asserted against `eddsaKeyFunc` directly now — HS256, RS256 and `none` — including that no key is handed back alongside the rejection. **And a correction I would rather state than bury.** I expected the end-to-end forgery to work with the check deleted: an HS256 token keyed on the Ed25519 public key, which is public by definition. It does not. golang-jwt's HMAC verifier refuses any key that is not literally `[]byte`, and `ed25519.PublicKey` is a named type that does not assert to it — measured, not assumed: ``` ed25519.PublicKey asserts to []byte: false HS256.Verify with ed25519.PublicKey: key is of invalid type: HMAC verify expects []byte ``` So the alg check is defence in depth rather than the only thing between the library and forgery, and the comment above it overstates slightly. That second layer is real but incidental — it holds only while the keyfunc returns `ed25519.PublicKey`, and a refactor to `[]byte(pub)` would remove it silently. There is now a test pinning the concrete return type, and it catches that refactor. ### The length cap took two attempts My first version fed the verifier a string of filler. The parser rejects that as malformed with or without the cap, so it stayed green under mutation — the same trap as #227 and as my first redirect test in #232, for the third time tonight. Padding a *properly signed* token past the limit makes the cap the only thing that can refuse it. ### Measured `go vet`, `golangci-lint` (0 issues), `go test -race ./...` all pass. `auth/jwt` 94.4% -> 95.0%, module total 92.3%. Every new test was confirmed by deleting the control it names and watching it fail. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Part of #219. Adds a caller for the new `schedule-freshness` reusable so that a cron which stops firing shows up as a red check on ordinary work instead of as nothing at all. **I expect this to fail right now, and that is the proof.** The newest successful *scheduled* run of both `audit.yml` and `fuzz.yml` is 2026-06-29 — 28 days ago, well past the 15-day limit. Tonight's runs were `workflow_dispatch`, which deliberately does not count: the point is to measure the schedule, not that somebody pressed a button. So a red check here is the check working, reporting a real four-week gap that nothing else reported. It goes green when the schedule actually fires. Both crons are Monday 06:00 and 07:00 UTC, and I toggled them off and on last night to re-register them, so tomorrow's run is the test of whether that worked. If it does not fire, this stops being a CI problem and becomes a support ticket — and this check is what will keep saying so instead of letting it slide for another month. Pinned to the reusable's commit rather than a tag on purpose: `Glyndor/.github` is only tagged once a consumer has proved a workflow green, and this is that consumer. I move the pin to the release tag afterwards. --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Third pass of #229, in `auth/password`. This one is different from the previous two: **the tests already existed, with exactly the right names.** `TestParsePHC_memoryAboveCeilingRejected`, `TestParsePHC_memoryBelowFloorRejected`, `TestParsePHC_iterationsAboveCeilingRejected`, `TestParsePHC_iterationsZeroRejected`, `TestParsePHC_parallelismZeroRejected` — all present, all passing, none of them reaching the bound it names. The fixture is why: ```go parsePHC("$argon2id$v=19$m=4000000000,t=3,p=2$c2FsdA$a2V5") ``` `c2FsdA` decodes to `salt`, four bytes. `parsePHC` checks the salt length before it looks at the parameters, so every one of these strings was refused there and the parameter bounds were never evaluated. Deleting any of them left the suite green: | Guard | Before | After | |---|---|---| | Algorithm label is `argon2id` | survives deletion | detected | | argon2 version matches | survives deletion | detected | | Memory within `minMemory`..`maxMemory` | survives deletion | detected | | Iterations within `1`..`maxIterations` | survives deletion | detected | | Parallelism at least 1 | survives deletion | detected | | Salt length | detected (this is what all of them were hitting) | detected | | Derived key length | detected | detected | ### Why these bounds are load-bearing `Verify` derives the key using the parameters **carried inside the stored hash**, not the module's config — deliberately, so old hashes keep working after the work factors are tuned up. The consequence is that a stored string decides how much memory and CPU a login costs. The existing test's own comment says it: a hash claiming `m=4000000000` would have `argon2.IDKey` attempt a multi-terabyte allocation. The bound is the only thing preventing that, and nothing was checking the bound. ### The fix The fixture now carries a salt and key of exactly the lengths `parsePHC` requires, so the parameter under test is the only thing wrong with it. There is a companion test asserting the in-range fixture parses cleanly — without that, the rejections would prove nothing, since a fixture that is rejected for any reason at all would satisfy them. They assert at `parsePHC` rather than through `Verify` on purpose: `parsePHC` refuses before `argon2.IDKey` is reached, so the test proves the rejection without asking the machine to attempt the allocation it is guarding against. ### Measured `go vet`, `golangci-lint` (0 issues), `go test -race ./...` pass. `auth/password` stays at 95.8% — this changes what the tests *mean*, not how many lines they touch, which is rather the point. Running total for the sweep: **sixteen controls found dead across three modules.** `auth/apikey`'s constant-time comparison and id-format check were mutated too and both were already covered. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Closes #219. The pin already pointed at the commit that became `v1.11.0` — it was pinned to the merge of Glyndor/.github#105 while the reusable was still waiting for a consumer to prove it green, which #226 then did. This adds the version comment beside it. The SHA is what makes the pin safe. The comment is what makes it visible: without a version to compare against, Dependabot never proposes a bump and the pin quietly rots. That is the difference between a reusable this repository happens to call and one the organisation can adopt. ### Where #219 stands | | | |---|---| | Crons re-registered | `Security audit` and `Fuzz` both fired on 2026-07-27 — 09:55 and 10:30 UTC, against crons set for 06:00 and 07:00. Nearly four hours late, which is normal for GitHub and worth remembering before calling one dead. | | First ever green | `audit.yml` had **never** completed a scheduled run successfully; its only previous cron-triggered run, on 2026-06-29, is the one that failed. | | Guard proven both ways | It reported the real 27-day gap while the schedules were dark, and `Within the 15-day limit` once they resumed. | Closing #219 on the authcore side. **What is not done is the rollout**: epistle, unitpm, glyndor.net, apt and podup all carry schedules and none of them has a freshness check. That is org work and needs its own issue rather than living in this one. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Jaro-c
added
type:chore
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.
Release PR for v1.11.6.
What changes for a consumer
Nothing. There is no code change in this release — every commit is a test or a CI caller. It is worth cutting anyway so
maindoes not drift behinddevelop, which is the state that left the MIT relicense undelivered for a day earlier this week.What it is
A sweep for tests that pass for the wrong reason (#229), which found sixteen security controls that could be deleted with the suite staying green, across four modules:
auth/jwt(test(jwt): exercise the verification guards that survived deletion #233) — the EdDSA algorithm check,WithExpirationRequired,WithIssuer,WithAudience, and the 8 KiB length cap on both the access and refresh paths. WithoutWithExpirationRequireda token carrying noexpis accepted, and it never expires.auth/oauth(test(oauth): exercise the guards that survived being deleted #232) — the redirect hop limit, the cross-origin redirect check, the RSA modulus bounds, the EC coordinate bounds, and the rejection of an ID token with nosub.P-384andP-521had never been parsed, though the module advertises both.auth/password(test(password): make the PHC bound tests reach the bounds #234) — the argon2id algorithm label, the version, and the memory, iteration and parallelism ranges. Tests for these already existed with the right names; their fixture had a four-byte salt, soparsePHCrefused it on the length check and never reached the bounds. Those bounds are what stand between a stored hash and a multi-gigabyte allocation on the next login, becauseVerifyderives the key with the parameters carried inside the hash.internal/keymanager(test(keymanager): make the size-cap tests reach the size cap #228) — the 4 KiB key-file cap, whose three dedicated tests seeded a partial directory so the consistency check failed first.Also here: the repository now asserts from ordinary CI that its scheduled
auditandfuzzare still firing (#226, #219), using the newschedule-freshnessreusable inGlyndor/.githubv1.11.0.Verified on go1.26.5
go build,go vet,golangci-lint(0 issues),go test -race ./...across all nine packages, andgovulncheckreporting no vulnerabilities. Coverage 92.3%, up from 91.0%:auth/jwt95.0%,auth/oauth90.2%,internal/keymanager88.9%.Every test added here was confirmed by deleting the control it names and watching it fail. Three of them needed a second attempt because the first version passed under mutation — the failure modes are recorded in the individual pull requests rather than smoothed over.