Repository navigation
fix(node): require Content-Digest on a signed request - #306
Merged
Merged
Conversation
require_signature rebuilds the RFC 9421 signing string from the request
itself, and it read the Content-Digest header with a .unwrap_or("")
fallback. When the header was absent, the covered content-digest component
came out as the empty string on both the signing side and the verifying
side, so the Ed25519 check passed over a string that committed to nothing
about the body. The comparison that would have caught it was guarded on the
presence of that same absent header, so it was skipped. The two defects
compose: a signed request could carry any body at all, and the handler
behind the middleware saw it as authenticated.
The header is now required. An absent Content-Digest is a 400 with
missing_content_digest, and the digest-versus-body comparison is
unconditional, since presence is established before it runs. There is no
longer a path through the middleware on which the body goes uncompared.
Refusing is safe for every client that already works. sign_request computes
the digest with no branch and always returns it in SignedHeaders, and all
eight production call sites attach it as a Content-Digest header: the three
in gl's HTTP client, the fetch and push requests in git-remote-gitlawb, the
node's peer announce, the sync push in sync.rs, and the sync notify in
api/repos.rs. So no conforming client regresses. What the refusal removes is
the case where a caller signed an empty digest and sent a body no signature
covered.
HttpSignature::parse now also refuses a covered component that appears more
than once, per RFC 9421 section 2.1. A repeated identifier says nothing the
single one did not, and it lengthens the signing string a verifier has to
build from a list the caller chooses.
Contributor
|
No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the primary security fix: requiring Content-Digest for signed node requests. It does not mention duplicate component rejection, but a title need not cover every change. |
| Description check | ✅ Passed | The description is complete and follows the repository template. It explains the security issue, lists both changes, identifies issue #305, provides verification commands, records completed checks, an… |
| Linked Issues check | ✅ Passed | The changes satisfy issue #305. Signed requests without Content-Digest now receive HTTP 400 with missing_content_digest, body verification is unconditional, and duplicate covered components are reject… |
| Out of Scope Changes check | ✅ Passed | The changes remain focused on HTTP signature validation and the linked security objectives. The additional rejection of empty or parameterized components is related parser validation and does not intr… |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 1 files. |
Full details: Description check
-
Explanation
The description is complete and follows the repository template. It explains the security issue, lists both changes, identifies issue
#305, provides verification commands, records completed checks, and documents protocol compatibility.
Full details: Linked Issues check
-
Explanation
The changes satisfy issue
#305. Signed requests without Content-Digest now receive HTTP 400 with missing_content_digest, body verification is unconditional, and duplicate covered components are rejected. The separate panic in issue#348is explicitly outside this PR.
Full details: Out of Scope Changes check
-
Explanation
The changes remain focused on HTTP signature validation and the linked security objectives. The additional rejection of empty or parameterized components is related parser validation and does not introduce unrelated functionality.
Finishing Touches
-
Generate docstrings
-
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
fix/require-content-digest-on-signed-requests
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
ℹ️ Recent review info
-
⚙️ Run configuration
-
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5497ab30-e8ed-4897-a400-0248e0d828ab
📒 Files selected for processing (1)
-
crates/gitlawb-core/src/http_sig.rs
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.
ℹ️ Recent review info
-
⚙️ Run configuration
-
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
5497ab30-e8ed-4897-a400-0248e0d828ab
📒 Files selected for processing (1)
-
crates/gitlawb-core/src/http_sig.rs
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.
-
11 tasks done
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-core/src/http_sig.rs`:
- Around line 76-86: The HttpSignature::parse component validation must handle
parameterized component identifiers consistently instead of comparing raw text
while later methods only support bare names. Either reject any component
parameters during parsing, or normalize and preserve them through
missing_components() and build_signing_string(); also add coverage proving
reordered equivalent parameters cannot bypass duplicate detection.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 617ccfde-796b-48d3-be02-77351de052b9
📒 Files selected for processing (2)
crates/gitlawb-core/src/http_sig.rscrates/gitlawb-node/src/auth/mod.rs
beardthelion
added
crate:core
jatmn
requested changes
Aug 9, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Make duplicate-component validation linear
crates/gitlawb-core/src/http_sig.rs:79
This scans the remaining component list for every component. An unauthenticated caller can send aSignature-Inputwith many distinct quoted components, so the new check performs quadratic string comparisons before timestamp, key, required-component, or signature validation rejects it. This server uses Axum/Hyper's default HTTP/1 configuration and does not set a tighter header buffer limit (Hyper permits roughly 417 KiB by default), enough for tens of thousands of short component tokens and billions of comparisons.optional_signatureroutes invoke this path whenever either signature header is present, including public read routes without an IP limiter, allowing repeated header-only requests to monopolize worker CPU. Track seen components in aHashSet(or enforce a strict component count) and add coverage for a long distinct-component list.
Collaborator
Author
|
Heads-up while this is open, since it touches
This PR's diff does not touch that indexing (it adds |
This was referenced Aug 15, 2026
3 tasks done
Use a HashSet for covered-component duplicate detection so hostile Signature-Input headers cannot force quadratic work before rejection. Reject RFC 9421 component parameters during parse since only bare names are supported end to end.
Collaborator
Author
|
Addressed both review items on 2572169. Duplicate-component validation now tracks seen bare names in a HashSet instead of scanning the tail slice per entry. Revert-check: restoring the old loop made Parameterized component identifiers are rejected at parse (
|
Gravirei
added a commit
to Gravirei/node
that referenced
this pull request
Aug 28, 2026
…e#26 split 1/4) This is the handler-level half of Split PR 1. The previous commit added the migration and the DB methods; this one threads them through crates/gitlawb-node/src/api/repos.rs:2007 (git_receive_pack), the cert issuer, and the startup drain. CHANGES IN THE HANDLER ====================== In git_receive_pack, AT THE LAST POSSIBLE MOMENT before the smart_http::receive_pack call, the handler now: 1. Generates a per-handler request_id (UUID). 2. Captures the raw Signature, Signature-Input, and Content-Digest headers from the request. 3. Calls db.insert_pending_ref_transitions(request_id, ...) which writes one row per ref update in state 'prepared'. The receive_pack call runs as before. After it returns: 4. On Ok: db.mark_pending_ref_transitions_applied(request_id) — the row is the ONLY thing that promotes a 'prepared' row to 'applied', and the drain reads only 'applied' rows. A process crash before this call leaves the row in 'prepared', which the drain never promotes. 5. On Err: db.mark_pending_ref_transitions_cancelled(request_id) — a failed receive_pack leaves the row in 'cancelled', which the drain never promotes. This is what closes the reviewer's two proofs: Proof 1 (crash window): if the process dies after mark_pending_ref_transitions_applied but before the bookkeeping writes, the row is in 'applied' and the next startup drain re-derives the push event, the per-ref certificate (carrying the ORIGINAL pusher DID, not a placeholder), and the anchor handoff. The drain uses the persisted authentic pusher DID and signature header, not a recovered placeholder. Proof 2 (failed receive-pack): the row is only ever flipped to 'applied' in the explicit Ok branch above. A 'prepared' or 'cancelled' row is invisible to the drain, so a failed or dropped receive_pack cannot turn a prepared intent into completed accounting or anchoring. BOOKKEEPING IS NOW DETERMINISTIC-ID =================================== The post-Ok bookkeeping at api/repos.rs:2448 now uses: - record_push_with_id with push_event_id_for(request_id, first_ref) — ON CONFLICT (id) DO NOTHING, so a recovery re-pass is a no-op. - issue_ref_certificate_idempotent with ref_cert_id_for(request_id, ref_name) — ON CONFLICT (repo_id, ref_name) DO NOTHING, returns None if a live-path cert already exists. - insert_anchor_job_idempotent with anchor_job_id_for(repo_id, ref_name, old_sha, new_sha) — the per-transition tuple key, so two pushes to the same ref produce one anchor upload per landed state. The legacy entry points (record_push, issue_ref_certificate, insert_ref_certificate) remain for callers that prefer a fresh UUID per cert; they are #[allow(dead_code)] for the PR 3 cert/CLI compat pass to decide whether to keep or remove. STARTUP DRAIN ============= crates/gitlawb-node/src/main.rs calls durable_outbox::drain_pending_ref_transitions(state, 1000) ONCE before serving, after migrations and after the existing peer / quarantine prunes. Non-fatal: a transient drain failure logs and leaves the rows for the next startup. durable_outbox::drain_pending_ref_transitions reads every 'applied' row, calls derive_one (which re-derives the three artifacts using the persisted authentic pusher DID and signature header), then deletes the row. A second drain pass is a no-op for both the artifacts (idempotent inserts) and the row (gone after the first pass). NEW END-TO-END TESTS ==================== crates/gitlawb-node/src/durable_outbox.rs adds three end-to-end tests in drain_tests, complementing the eight DB-layer tests in db::pending_ref_transition_tests: - drain_re_derives_all_three_artifacts_for_an_applied_row: the reviewer's first proof. Inserts a row in 'applied' state (the crash window), drains, asserts exactly one push event row, exactly one cert row carrying the original pusher DID (not a placeholder), and exactly one anchor job row. Asserts the deterministic cert id matches. Asserts a second drain pass is a no-op. - cancelled_row_produces_no_artifacts: the reviewer's second proof for the cancelled state. A row in 'cancelled' (receive_pack returned Err) is invisible to the drain. - prepared_row_produces_no_artifacts: the reviewer's second proof for the prepared state. A row in 'prepared' (handler crashed between insert_prepared and the post-Ok branch) is invisible to the drain. Each test names the invariant it pins and the production line it covers. Reverting that line turns the named assertion red. Compiles clean, 1099 tests pass with 0 regressions, clippy clean under -D warnings, fmt clean. Cross-PR overlap (declared in the PR description): - Twigpine#134 (anchors auth): composes. The /arweave/anchors route already requires auth; this PR does not change the route. - Twigpine#285 (advisory-lock session affinity): composes. The durable intent is written inside the same handler that holds the lock from Twigpine#285; no changes to the lock layer. - Twigpine#306 (Content-Digest on signed requests): composes. PR 1 persists the Content-Digest header that Twigpine#306 makes mandatory. - Twigpine#314 (small-order Ed25519): independent. PR 1's tests use strong keys. - Twigpine#324 (libp2p keypair persistence): independent. PR 1 does not touch p2p identity. - Twigpine#325 (gossip ref-update auth): independent. PR 1's signed envelope is the HTTP-side equivalent, not the gossip-side. - Twigpine#382 (replication withheld-subtree trees): independent. PR 1 does not touch replication or pin selection.
kevincodex1
added a commit
that referenced
this pull request
Aug 31, 2026
A claim is provenance, so the signature stored beside it has to reach the bytes stored beside it. It did not. The signing string covers the body only through a content-digest line, and require_signature rebuilds the covered values from the request: with no Content-Digest header the covered digest is the empty string on the signing side and the verifying side alike, the Ed25519 check passes over method, path and nothing else, and the digest-versus-body comparison was skipped for want of a header. An owner could sign that once and post any body under it. The row that produced carried a valid signature, a signing string that verifies under the owner's key, and a request_body the signing string says nothing about — history nobody can re-verify, which is the one thing the row is for. The write path now recomputes the digest of the bytes it is about to persist and requires the signing string to carry exactly that line, which is the write-time form of the check re_verify already ran against a stored row. Line-exact rather than a substring search: build_signing_string emits one component per line, and a contains would also be satisfied by the digest appearing inside another component's value. This is deliberately not the only line of defence, and deliberately not the middleware's. PR #306 requires the header in require_signature and closes the same hole one layer earlier for every signed route; that belongs there, because no other route's correctness should depend on this handler. Neither substitutes for the other. Without #306 the empty digest is still accepted everywhere else; without this, a later relaxation of the middleware resumes writing unverifiable history in silence. Nothing here touches require_signature, so the two do not collide. The route's advertised 8 KiB body bound was also a bound on what gets stored rather than on what gets buffered: require_signature collects and hashes the whole body before any handler runs, so by the time the handler measured it the allocation had happened, and concurrent requests multiplied exactly the cost the number was written to prevent. A RequestBodyLimitLayer now carries the same figure at the transport, outside the auth layers and inside the rate limiters, matching the shape the git routes already use. The handler keeps its own check: it is the one that holds for a handler reached by any other route table. The hand-injected test material had to become coherent with the request it rides on, because a stand-in built over other bytes now takes every bare-router test down the refusal branch. It is built from the body it accompanies and carries a real content-digest line.
Collaborator
Author
|
@kevincodex1 LGTM |
12 of 17 tasks
euxarist
reviewed
Sep 27, 2026
euxarist
left a comment
euxarist
left a comment
Contributor
There was a problem hiding this comment.
Closes a real hole: at HEAD an absent Content-Digest signs the empty digest and the comparison is skipped entirely, so a signed request could carry an uncovered body. Requiring the header, making the comparison unconditional, and rejecting parameterized/duplicate covered components (necessary for duplicate detection to be sound at all) with a 20k-component linearity probe is thorough, and all eight production sign_request sites attach the header so no conforming client regresses.
beardthelion
deleted the
fix/require-content-digest-on-signed-requests
branch
November 4, 2026 22:58
10 of 17 tasks
euxarist
pushed a commit
to euxarist/node
that referenced
this pull request
Nov 6, 2026
… 2/4) (Twigpine#524) * fix(node): ANS-104 transport, three-outcome probe, verify endpoint (Twigpine#26 split 2/4) Reviewer 2 closed PR Twigpine#224 on 2026-08-28 with a directive: split into four narrow PRs. This is Split PR 2 (Arweave transport and verification). The P1 finding the reviewer assigned to this split: the legacy `anchor_item_present` mapped `BAD_REQUEST` and `GONE` to `Ok(false)`, and the recovery code interpreted false as permission to reclaim the row and pay for another upload. Neither 400 nor 410 proves a previously paid item was never accepted: 410 can describe an artifact that existed but is no longer served, and 400 can be produced by gateway, proxy, or routing failure. The concrete failure sequence the reviewer named: bundler accepts the item, the response or terminal DB write is lost, recovery probes the persisted item_id, the gateway returns 400 or 410, and the node pays for a second immutable item for the same transition. Fix: model the probe result as three outcomes, and authorize a re-upload only on a trustworthy, protocol-defined absence. NEW MODULE crates/gitlawb-node/src/ans104.rs - DataItem: the on-wire ANS-104 shape (signature, owner, target, anchor, tags, data — all base64url-encoded except target/anchor). - deep_hash: the canonical signing input per the spec. The signature is over a SHA-256 mix of the dataitem/list/map discriminators, the signature type, owner, target, anchor, the recursively-hashed tag list, and the data payload. A hand-rolled sha256 of the JSON body would produce a hash no Arweave gateway would recognize; the deep-hash is the only correct form. - sign_data_item: Ed25519 over deep_hash. - verify_data_item: parses the owner, checks the signature against an expected pubkey, and reports a specific failure reason on each branch. - 7 unit tests covering round-trip, flipped signature, wrong expected key, mutated data, wire-shape round-trip, deep-hash stability, and the empty-tags edge case. NEW MODULE crates/gitlawb-node/src/arweave_v2.rs - ProbeOutcome: Present (2xx, body parses as ANS-104, sig verifies), DefinitivelyAbsent (404 with a known JSON body shape), Indeterminate (everything else). The reviewer's three-outcome model. - ProbeOutcome::permits_reupload: the recovery policy. ONLY DefinitivelyAbsent authorizes a paid re-upload. Indeterminate keeps the outbox non-terminal; Present skips re-payment. - probe_anchor_item: the gateway probe. 2xx bound to a different item id is Indeterminate, not Present. 2xx with a body that does not parse as ANS-104 is Indeterminate. Oversized bodies are Indeterminate (defense against body-stuffing). - read_capped_body: enforces PROBE_MAX_BODY_BYTES (1 MiB) on the full body, with a Content-Length fast-path that rejects before reading. - verify_anchor: the full path the public verify endpoint takes. Fetches the data item, parses it, verifies the Ed25519 signature against the persisted node_did, decodes the data payload as JSON, and reports the result. - 15 unit tests pinning each classification boundary: 400/410/5xx/transport/2xx-non-json/2xx-bad-sig/2xx-different- owner are all Indeterminate; 404 with empty or known-JSON body is DefinitivelyAbsent; 2xx with a valid signed item is Present; the re-upload policy is exhaustive. NEW DB METHOD crates/gitlawb-node/src/db/mod.rs - get_arweave_anchor_by_item_id: looks up an existing arweave_anchors row by its id (which is the Irys tx id, the same value the Arweave gateway uses to serve the data item). Used by the public verify endpoint to fetch the persisted node_did so the envelope signature can be verified against it. NEW CONFIG crates/gitlawb-node/src/config.rs - GITLAWB_ARWEAVE_GATEWAY_URL: the Arweave gateway the probe reads from. Defaults to https://arweave.net; operators can point it at a private mirror. NEW HTTP ENDPOINT crates/gitlawb-node/src/api/arweave.rs - GET /api/v1/arweave/anchors/verify/{item_id}: the public verify endpoint. Returns {item_id, status, verified, owner_did, data_payload, error} where status is one of "verified" / "definitively_absent" / "indeterminate". - 3 endpoint-level tests covering the verified, 404, and 400 shapes via the live router, asserting the public surface surfaces the three outcomes. WIRE-UP crates/gitlawb-node/src/server.rs - The verify route is registered alongside the existing /api/v1/arweave/anchors list route. NOT IN THIS SLICE (the bundler upload, the next commit): - The handler that builds a DataItem from a ref-cert and POSTs it to the bundler is not in this PR. The ANS-104 module is ready to be called by it; the cert/push-event flow in PR 1 persists the row that the bundler upload would consume. - PR 1's recovery drain produces an `anchor_jobs` row keyed on the (repo, ref, old, new) tuple; the next slice reads that row, builds the ANS-104 data item, POSTs to the bundler, and writes the resulting tx id to arweave_anchors. Compiles clean, 1113 tests pass with 0 regressions, clippy clean under -D warnings, fmt clean. Cross-PR overlap (declared in the PR description): - Twigpine#134 (anchors auth): composes. The verify endpoint reads from arweave_anchors; Twigpine#134's auth layer applies unchanged. The probe model is server-side and unauthenticated by design. - Twigpine#285 (advisory-lock session affinity): independent. The probe is read-only. - Twigpine#306 (Content-Digest on signed requests): independent. The probe does not sign requests. - Twigpine#314 (small-order Ed25519): composes. The verify path calls Did::to_verifying_key which already enforces the small-order check from Twigpine#314. - Twigpine#324 (libp2p keypair persistence): independent. - Twigpine#325 (gossip ref-update auth): independent. The verify path is for the public Arweave gateway, not gossip. - Twigpine#382 (replication withheld-subtree trees): independent. * fix(node): address reviewer findings on Twigpine#26 split 2/4 (transport) - P1 (DB lookup): get_arweave_anchor_by_item_id now filters on irys_tx_id (the externally-routable item id) rather than the internal UUID primary key. The production writer stores a fresh UUID in `id` and the Irys response / ANS-104 derived id in `irys_tx_id`; the old filter would always 404. New integration tests exercise the production writer path with distinct values for the two columns. - P1 (verify-gate): verify_anchor now gates on repo read via authorize_repo_read. The route is split in server.rs: the public list_anchors stays ungated (Twigpine#134 tracks that surface), the verify route gets optional_signature per the team memory `axum-layer-vs-merge-pitfall.md`. Denials on row-lookup, gate, and missing repo all collapse to the same opaque 404 so the public endpoint does not leak anchor-row existence. - P1 (ANS-104 deep-hash): rewritten to the verified SHA-384 recursive list/blob algorithm (matches `arbundles` JS reference). deep_hash returns [u8; 48]; signature is over the raw 48-byte digest. Three reference vectors from `Irys-xyz/arbundles/src/__tests__/deepHash.spec.ts` are bit-exact-asserted as the interop canary per the team memory `self-roundtrip-tests-do-not-prove-interop.md`. - P1 (artifact-identity): v2 verify now derives the protocol id via DataItem::id() (base64url(SHA256(signature))) and requires equality with the requested URL item_id. A node key signs many data items, so a valid signature only proves who signed the response — not that the served item is the one the caller asked to verify. The team memory `verify-against-artifact-id-not-signer.md` is the policy. - P1 (verifier can't read this head's writes): verify_anchor now accepts BOTH the v2 ANS-104 format and the v1 raw-JSON shape the live path on this branch actually writes. The v1 path does a field-equality check on repo, ref_name, old_sha, new_sha, node_did against the persisted row (no signature in v1; Irys storage plus the JSON parse are the integrity guarantee). - P2 (ProbeOutcome in result): AnchorVerifyResult now carries outcome: ProbeOutcome. The HTTP handler maps the structured field to the status string instead of parsing the human- readable error message — any future wording change in arweave_v2.rs can no longer silently reclassify a DefinitivelyAbsent as Indeterminate. - P2 (empty 404 body): classify_404 returns Indeterminate for bodyless 404s. The team memory `distinguish-unknown-from-empty.md` is the policy: a bodyless 404 from a proxy is not a proof of absence, and the recovery policy (permits_reupload -> true on DefinitivelyAbsent) authorizes a paid, irreversible re-upload. - P2 (streaming cap): read_capped_body no longer uses resp.bytes() (which buffers the full response before checking size). It now reads chunk-by-chunk and aborts the moment the cumulative byte count crosses the cap, so a chunked response without Content-Length cannot force unbounded allocation. * fix(node): address three reviewer findings on Twigpine#26 split 2/4 (round 2) * fix(node): address five reviewer findings on Twigpine#26 split 2/4 (round 3) * fix(node): address reviewers round-4 on Twigpine#26 split 2/4 (interop) * fix(node): address coderabbit on Twigpine#26 split 2/4 (raw tags, vint overflow, golden key) * fix(node): enforce artifact-id in probe_item on Twigpine#26 split 2/4 * fix(node): require expected owner for probe Present on Twigpine#26 split 2/4 * fix(node): narrow Twigpine#26 split 2/4 to codec and policy types * fix(node): silence dead-code for deferred slice API on Twigpine#26 split 2/4 * fix(node): doc and bounds-check cleanup in ans104 on Twigpine#26 split 2/4 * docs(node): point tests doc at Ed25519 golden script on Twigpine#26 split 2/4 * fix(node): strict owner lengths, sign-before-hash on Twigpine#26 split 2/4 * fix(node): keep ans104 tags and field widths consistent on hash and encode DataItem keeps both the decoded `tags` and the original Avro `raw_tags`, both public. After a parse, changing `tags` left deep_hash and verify_data_item using the stale raw bytes, and to_binary wrote a header tag count that no longer matched the payload, so the frame failed to parse back. Take the tag block for hashing and encoding from one helper that decodes `raw_tags` and requires it to equal `tags`, and fail closed otherwise. The header count comes from the same validated pairs. Parsed multi-block and size-prefixed items still re-encode byte-exact and keep their signatures. deep_hash now also rejects a non-empty target or anchor that is not 32 bytes, as to_binary already does, so sign_data_item fails instead of signing an item that cannot be encoded. The legacy non-base64url anchor fallback is kept. The module and signature-type docs now say binary parsing accepts sigtypes 1 to 4 while signing and verifying are Ed25519 only on this slice. * build(scripts): declare the golden script's direct npm dependencies ans104_golden_ed25519.mjs imports bs58, @noble/ed25519 and base64url, and ans104_golden.mjs imports base64url, but package.json listed only arbundles, so they resolved only through hoisting. Declare them at the versions already locked, update the lockfile root entry (no resolved version or hash changes), and note the regeneration command in the golden output file. --------- Co-authored-by: Gravirei <gravirei@users.noreply.github.com>
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refuses a signed request that carries no
Content-Digestheader, and refuses a duplicated covered component inSignature-Input.Motivation & context
Closes #305
The header was read with a fallback to the empty string. With the header absent, the covered
content-digestis empty on both the signing and verifying sides, so verification passes and the body comparison is then skipped for want of a header. A request could carry a body its signature never covered while presenting as validly signed. Every write to a node authenticates this way, so the guarantee a signature is meant to give did not hold for a caller who simply left a header off.The parser change is smaller: RFC 9421 section 2.1 says a component identifier appears at most once, and a repeat conveys nothing while repeating a line in the signing string a verifier has to build.
Kind of change
What changed
gitlawb-node:require_signaturereturns 400 with amissing_content_digestcode when the header is absent, and the body comparison is now unconditional.gitlawb-core:HttpSignature::parserejects a repeated covered component.How a reviewer can verify
The first test signs
content-digestas the empty string and then sends no header, which is the exact shape that slipped through, and asserts the request is refused before any handler runs. Both tests were checked against an injected revert of the production line they cover, so a regression turns them red rather than leaving them green.Before you request review
cargo test --workspacepasses locallycargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare clean.env.exampleupdated if behavior or config changed (or N/A)Protocol & signing impact
did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formatsNo conforming client regresses.
sign_requestcomputes the digest unconditionally and returns it inSignedHeaders, and all eight production call sites attach the header:gl/src/http.rs(three),git-remote-gitlawb/src/main.rs(two),gitlawb-node/src/main.rs,sync.rs, andapi/repos.rs. The only paths that omit it are git-remote's unsigned ones, which carry no signature headers at all and are refused earlier. Both changes refuse things a conforming client never sends, so previously signed history is unaffected.Notes for reviewers
This started life inside #304, the commit status surface, because I found it while making stored status claims re-verifiable. It does not belong there: that feature does not need it, this is a protocol change, and the two want different scrutiny. #304 has been rebased to drop it and now depends on nothing here.
Summary by CodeRabbit
Content-Digestheader.