fix: close the actionable findings from a full security audit - #41
Merged
Conversation
…ng a login Three denial-of-service paths on the loopback login listener, each reachable by any local process during the 300-second login window. The `error` parameter was read before the constant-time `state` comparison, so a request that could not know `state` could still abort the login and choose the message the operator saw. `state` is now compared first, which is also what RFC 6749 practice asks for on error responses. Any `GET /callback` consumed the single-shot listener, so one stray request killed a login and the genuine redirect arrived at a dead port. A callback that carries no authorization response, or whose `state` does not match, now gets a 400 that leaks nothing and the listener keeps waiting. The pasted channel is deliberately not filtered, so pasting the wrong redirect still fails fast rather than timing out. The deadline was only checked in the accept loop's `WouldBlock` branch, so a client dribbling a byte every nine seconds held the listener far past the advertised timeout. The deadline is now recomputed each accept and bounds the per-stream read timeout. Also removes `Registration.client_secret`. It was never sent to the token endpoint and never persisted -- registration asks for `token_endpoint_auth_method: none` -- and it was the one secret in the crate sitting in a plain `String` inside a `Debug`-printable struct. Deleting it beats wrapping it: a secret a server volunteers is now dropped at parse. Discovered endpoints must now be `https`, or `http` on a loopback host. They are server-controlled through the discovery document, and the authorization endpoint is handed to the OS URI opener, so a compromised server could name a scheme that launches a local program. One behaviour change worth knowing: a wrong-`state` redirect over loopback now ends in `LOGIN_TIMEOUT` rather than `STATE_MISMATCH`, because it is ignored rather than acted on. `STATE_MISMATCH` is still reached through a paste. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e compare `fingerprint` is `sha256(value)` truncated to 72 bits, unkeyed and unsalted. The module documentation called that "far too little to attack a real secret through", which is only true of high-entropy values: recomputability is what makes the tag a correlation handle, and it is the same property that makes it a guess-confirmation oracle. A fingerprint of a human-chosen password in a readable log confirms a dictionary guess offline. Keying it would close that and is deliberately not done. The blocker is not the crate's zero-dependency rule -- HMAC-SHA256 is hand-rollable over the `sha2` already present -- nor the call sites, of which there are none yet. It is semantics: the function exists so two parties can agree their copies of a value differ, and the client and the server are different installs. A per-install key makes fingerprints incomparable and destroys the only use case; a fleet-wide key trades a log oracle for distributing a new long-lived secret everywhere. So the property is upheld by a documented calling rule instead, stated in terms of the entropy of the value rather than the width of the tag, and the overclaim is gone. A test pins the construction the documentation reasons about, so changing it forces a revisit of the paragraph. Also puts a `core::hint::black_box` in `constant_time_eq`. The XOR-accumulate loop is correct as written, but nothing stops an optimiser noticing that a non-zero accumulator can never return to zero and leaving early. The barrier is applied to the accumulator on every iteration rather than once at the end, which is what denies it that reasoning. miri still proves the crate pure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The token endpoint returns an access token and a refresh token as plaintext JSON, and the transport dropped that buffer without wiping it. The credential store already zeroizes the buffers it reads and writes, so this was the one place in the crate family holding token plaintext to the loose standard. Fixed on `Received` rather than at the call site. `Received` holds its body privately and exposes only a borrow, so nothing in `prick-auth` could reach it, and copying it out to wipe a copy would have left the original. A `Drop` impl covers every caller instead of the ones that remember -- a revealed secret value takes the same path as a token, and the transport cannot tell them apart. `Drop` forbids moving fields out, which is why `decode` now borrows and clones the response facts. Cloning is cheap and confined to error paths, and the facts are a status code and proxy-set headers: the type's own documentation already records that none of them can carry a secret. This is hygiene rather than a boundary. It shortens how long plaintext sits in a page that could be swapped or dumped; it cannot reach copies serde made while deserialising. Cargo.lock additionally carries `chacha20` 0.10.1 -> 0.10.2. The old version is yanked upstream, with no advisory attached, and reaches the tree through `rand`'s CSPRNG rather than as a bulk cipher. A lockfile cannot be split across commits, so it rides along here; `cargo audit` is clean again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The API authenticates on the ambient `CF_Authorization` cookie and had no
Origin check. There is deliberately no CORS, which blocks a cross-origin read
but does nothing about a write, and Hono's `json` validator does not reject a
mismatched media type -- it hands `{}` to the schema, so a cross-site form
post reaches the route with an empty body.
The reachable route was not `secrets:batch`. Its refine rejects the empty body
a form produces, answering 422. It was `POST /api/v1/admin/rekey`, whose
schema is fully defaulted: `{}` parses, `limit` becomes the maximum page, and
a global admin's browser re-encrypts a page of rows and writes an audit row
attributed to them. Verified at 200 before this change and 403 after.
`crossSiteGuard` is mounted on the root app ahead of every route mount, so a
route added later inherits it and it precedes both routing and authentication
-- a POST to a path that does not exist now answers 403 rather than 404,
which is what proves the ordering. It sits below `keyring` on purpose, so a
broken master key still reports a misconfiguration rather than an access
decision.
An `Origin` that disagrees with the request's own origin is refused, and
`Origin: null` is treated as somebody else's. A declared media type other than
`application/json` is refused with a 415; parameters are stripped, so
`charset=utf-8` passes.
Body presence is deliberately never consulted. An earlier version of this
guard tested `c.req.raw.body !== null` and 415'd every bodiless DELETE on the
wire, because `vitest-pool-workers` reports `null` in-process where workerd
over real HTTP presents a stream -- so the unit suite passed while every
revoke, group delete and member removal broke, in the CLI as well as the UI.
Enforcing only a declared media type is runtime-independent and keeps the
security property: a browser sets `Origin` on every request whose method is
not GET or HEAD, with no way for the initiating page to suppress it, so the
Origin check is what actually stops CSRF and the media type is defence in
depth. What is given up is one belt-and-braces case for credentialed
non-browser callers, and a test pins that residual rather than leaving it
implicit.
The wire shape is reproducible in-process with a `Blob` whose `type` is empty,
which is what the regression tests use, and a source sentinel fails the build
if `raw.body` reappears in the http tree.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Worker's new cross-site guard answers 415, and `ErrorKind::from_status` covered 400, 401, 403, 404, 409, 412, 413, 422, 429 and 5xx -- so a 415 fell through to `Unknown`, exit 1, with no hint. It gets its own `UnsupportedMediaType` kind rather than folding into `Validation`. `Validation` returns `None` from `hint()` by design, because a payload rejection's own message names the offending field and a fixed string cannot improve on it; mapping 415 there would have produced a classified error with no next step, and giving `Validation` a hint would have attached a `Content-Type` sentence to every field rejection. Sharing an exit code while splitting kinds whose fixes differ is the convention the crate already follows, and `PayloadTooLarge` is the direct precedent: its own kind, its own hint, the same exit 11. The machine code mirrors the Worker's `UNSUPPORTED_MEDIA_TYPE`, so the CLI table and the API reference agree. A correct `prk` cannot provoke this -- every API body goes through reqwest's `.json()` -- so in practice it means something rewrote or dropped `Content-Type` in transit, and the hint points at a proxy rather than at the caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`secrets_diff` took any absolute or cwd-relative `env_file` and read it. Tool arguments come from a language model that may have been prompt-injected, so a crafted call could point at a file outside the project. No value could leak -- the scanner builds no value string -- but it returned the key names present, and slightly more than that: `invalid_in_file` echoes the raw text before the first `=`, so a non-dotenv file gave back line prefixes. Containment is a shape rather than a policy. There is no denylist: `..`, an absolute path elsewhere and a symlink out are one case, decided by comparing a resolved path against a resolved root. Case is folded on win32 only -- folding on darwin would make `/Root/x` appear to live under `/root`, which on a case-sensitive volume is the hole being closed -- and the prefix test appends a separator so `/srv/app-backup` is not inside `/srv/app`. The check runs twice on purpose. Lexically in `secretsDiff` before anything is opened, because a `stat` on an attacker-chosen path is itself an answer about whether that file exists, and it covers the injected test reader so a double cannot be handed a path the real reader would refuse. Then canonically after `realpath`, because no string handling can see that a file inside the root is a link to `~/.aws/credentials`; the canonical path is the one opened, so the bytes read are the bytes checked. The root resolves from `--workspace`, then `PRICK_MCP_WORKSPACE`, then `process.cwd()`. The cwd default is the point: an MCP client starts its servers in the project it has open, so the ordinary case is bounded with no configuration, and a bound an operator has to switch on is a bound that is off. It is `realpath`'d once at startup, since a root that itself contains a symlink would match nothing beneath it, and a missing or non-directory value is fatal there rather than at the first tool call. Refusals quote back only the caller's own argument, never the root or the resolved path, which would answer "where does this operator keep their projects, under what user name". Reported paths are root-relative now; the absolute path, username and layout included, used to come back on the success path and on every error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`jdx/mise-action` was SHA-pinned but received no `version:`, so every run downloaded whatever mise release was newest and executed the entire pipeline through it. In `publish-npm` and `publish-crates`, which hold `id-token: write`, a compromised mise release or a hijacked release asset would have been enough to publish malicious packages with valid provenance attestations. The workflows already disable mise's cache in those jobs; the installer itself floated. Each workflow now declares `PINNED_MISE_VERSION` once and passes it to all seventeen call sites. The pinned release is deliberately not the newest: this repo sets a 72-hour `minimumReleaseAge` on its dependencies and writes down why, and pinning the release-pipeline interpreter to a binary published hours earlier would contradict that. The comment records the rule rather than just the number. `npm install --global npm@^11.5.1` becomes an exact pin, for the same reason: that client performs the OIDC publish, and the cooldown protecting workspace dependencies never applied to it. Both job-level credential blocks are gone. `CARGO_REGISTRY_TOKEN` and `WINGET_TOKEN` now reach only the step that uses them, so checkout, mise-action, the version stamp and -- most of all -- `cargo publish`'s dependency build scripts and proc-macros no longer run with a registry token in their environment. The emptiness probe is additionally gated to `push`, so a `workflow_dispatch` rehearsal carries the token in no step at all. This is what the existing comment about a credential not belonging in a job that has no use for it was already asking for. The composite action now verifies provenance before installing. This needed measuring rather than assuming: `npm audit signatures` refuses a global install outright, and aiming it at the global prefix reports only packages that are dependencies of something, leaving the CLI itself -- the one package a moved `latest` tag replaces -- unverified. So the resolved spec is staged into a temp directory where the audit does cover it, verified with `--include-attestations` (without which a package carrying no attestation appears in neither the invalid nor the missing list, which is the silent pass), and then installed globally by the exact version that verified, so a dist-tag moving between verify and install cannot slip a tarball through. It fails closed on npm's error document: a check that did not run is not a check that passed. The staging path is passed as a cwd rather than through an argv, since a runner temp path can contain characters `cmd` would read. Also commits `mise.lock`, which `mise.toml` has asked for with `lockfile = true` and `.gitattributes` already had a rule for. It carries checksums and URLs for all four platforms CI runs on, because `mise-action` adds `--locked` once a lockfile exists and a single-platform lockfile would have failed every other runner. `rust`, `npm:vite-plus` and `cargo:cargo-auditable` have no platform entries, uniformly, because those backends resolve no single artifact; a locked install of them was verified to succeed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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
2 participants
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.
A read-only security review of the whole repository, followed by fixes for
everything actionable. No critical or high-severity issues existed. Twelve
findings are closed here; three are deliberately left alone, with the
reasoning recorded below.
The two mediums
Cross-site writes on the
/apitransport. The API authenticates on theambient
CF_Authorizationcookie with no Origin check, and Hono's validatorhands
{}to the schema when the media type is not JSON — so a cross-siteform post reached the route with an empty body. The reachable route was not
secrets:batch, whose refine rejects that body; it wasPOST /api/v1/admin/rekey, whose schema is fully defaulted, so a globaladmin's browser would re-encrypt a page of rows and write an audit row
attributed to them. Verified at 200 before and 403 after.
The mise binary was unpinned in every workflow, including the publish jobs
that hold
id-token: write. A compromised mise release would have been enoughto publish malicious packages with valid provenance attestations. Now pinned
once per workflow across all seventeen call sites, to a release deliberately
three days old to match the repo's own 72-hour dependency cooldown.
A regression this caught before it shipped
The first version of the cross-site guard used
c.req.raw.body !== nulltodetect a body. That is
nullfor a bodiless request invitest-pool-workersbut a live stream in workerd over real HTTP, so 1170 unit tests passed while
every revoke, group delete and member removal returned 415 — in the shipped
CLI as well as the UI. The end-to-end suite caught it.
The cause went deeper than the symptom: the missing-media-type branch had
never been exercised in-process at all, because a
Requestbuilt with astring body is automatically given
text/plain;charset=UTF-8. The final guardnever consults body presence, the wire shape is now reproducible in-process
with an empty-
typeBlob, and a source sentinel fails the build ifraw.bodyreappears in the http tree.Everything else
stateis compared beforeerroris read; astray redirect no longer consumes the single-shot listener; the deadline now
bounds the per-stream read timeout, so a slowloris client cannot outlive it.
Registration.client_secretdeleted — never sent, never persisted, andthe one secret in the crate sitting in a
Debug-printableString.prick-api'sReceivedso it covers every caller rather than the ones that remember.
compromised server cannot name a scheme that launches a local program.
secrets_diffis confined to a workspace root, checked both lexicallybefore anything is opened and canonically after
realpath.staging the fetch where
npm audit signaturescan actually see it and theninstalling the exact version that verified.
cargo publish'sthird-party build scripts no longer see a registry token.
chacha20moved off the yanked 0.10.1;cargo auditis clean.mise.lockis committed, with all four CI platforms covered.Unknown, as its ownkind sharing exit 11 —
Validation.hint()is deliberatelyNone, sofolding it there would have produced a classified error with no next step.
considered and rejected: the function exists so a client and a server can
agree their copies of a value differ, and they are different installs, so a
per-install key destroys the only use case.
Deliberately not changed
Durable Object or KV and a fail-open/fail-closed decision, to duplicate
Cloudflare Access, which the intended deployment already requires in path.
X-Request-Idis an intentional correlation feature; thepattern excludes CR/LF, a non-match is replaced rather than rejected, and the
value is bound into SQL rather than concatenated.
and a multi-megabyte vendored asset. The contained alternative — the
noncethe renderer already accepts — is left as a follow-up.
Follow-ups worth filing
Two were discovered while fixing rather than by the audit.
DescriptionandDisplayNameare forwarded verbatim by the MCP server intotools/listresults, so anyone who can set a project description can put text directly
into a language model's context; the fix belongs in
packages/shared, whichthe Rust half also validates against. And with
mise.lockcommitted,mise install --lockedalso reads a developer's global mise config, so aglobally pinned tool absent from the repo lockfile makes that command fail
locally — CI is unaffected and
mise rundoes not pass--locked.Verification
One full
mise run ciover the integrated tree: 682 Rust tests, 131 undermiri, 1174 Worker tests, 101 end-to-end, 97 MCP, 129 action, clippy and every
linter clean, typecheck 0 errors,
auditanddenyclean,openapi:checkcurrent.
🤖 Generated with Claude Code