test(nc-12): measure the NC-12 doc claims in seams/dig_peer, guard 2 - #504
Conversation
Anchor commit for the NC-12 doc-claim audit (dig-node#356). Co-Authored-By: Claude <noreply@anthropic.com>
…ghest The NC-12 doc-claim audit (dig-node#356) measured the 16 NC-12 mentions in seams/dig_peer/. Two claims were ASSERTED-ONLY with a surviving one-line mutation; both are guarded here. 1. module_relay's "a refusal leaves RESOURCE_UNAVAILABLE unchanged". That file carries zero tests. A refusal that narrates WHICH gate refused is an oracle a stranger reads for free: it reveals whether the operator enabled relaying and when a requestor has exhausted its proxy allowance -- the rate limiter's own state, told to the party it bounds. The guard drives two arms that refuse at DIFFERENT gates and pins the frames equal. 2. holder_cache's first-hand-only contract. FirstHandHolderCache::remember delegates it to the caller in its own doc, so nothing observes it. Caching hearsay would let one lying hop plant a holder this node re-serves as its own knowledge for the whole TTL. The guard pins the peer's record INTO the answer and OUT of the cache, with a first-hand record present so the assertion is not satisfied by an empty cache. Both fixtures use two distinguishable values and a pinned inequality, so a later edit cannot collapse them into a single value that proves nothing. Refs #356 Co-Authored-By: Claude <noreply@anthropic.com>
|
IN PROGRESS — not the verdict. Independent correctness gate, fresh context, head Posting as I establish things so nothing is lost to a watchdog kill. 1. Denominator: CONFIRMEDIndependently counted with a case- and separator-tolerant pattern ( Of those, exactly 3 are inside
The near-miss I checked and cleared: 13 doc claims; 2. Guard 2 (
|
loop-security interim 1 — IN PROGRESS, not the verdictAuditing head Verdict 1 spot-check:
|
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
CHANGES-REQUIRED
Head reviewed: 86f06bfc377bda1ce343ea7048cc201f1789a3a1 (resolved from gh pr view 504 --json headRefOid, not taken from the dispatch brief). Base origin/main 476055fb, which is also the merge-base — the branch is not behind.
Fresh context, independent gate. I did not build: the branch worktree is held by another process, and the evidence comment on #356 already records a full-suite run and both mutation proofs, which I checked for internal consistency rather than re-running. Stated rather than implied.
Two gating findings, both cheap, both the same class: a coverage claim broader than its evidence. In an audit whose entire deliverable is the measurement, an over-stated verdict is the defect.
CONFIRMED — what I attacked and could not break
1. The denominator (12, not 15) — holds. Counted independently with a separator- and case-tolerant pattern (nc[- _]?12, case-insensitive) rather than the literal, so a spelling variant could not hide outside the count: 16 hits, all 16 the literal NC-12. Exactly 3 sit inside #[cfg(test)], verified by locating the enclosing attribute: module_reshare.rs:1789 (under the module-level gate at :1000) and module_transport.rs:1150/:1175 (under the nested gate at :866). The trap I checked and cleared: module_reshare.rs:670 sits 31 lines under a #[cfg(test)] at :639, but that attribute is item-level on registry(), so :670 is a genuine doc claim on the production staged_bytes. module_transport.rs:774/:821 are one property — both state the hop's-claim bound, both discharged by the same ceiling test. 13 doc claims, 12 distinct properties.
2. a_peers_hearsay_reaches_the_answer_but_never_the_first_hand_cache — not vacuous, not circular. The worry you named is the right one and it is handled: engine(vec![provider(1,&cid)], &[2], Some(ask)) populates the DHT leg, so the empty-slate escape at holder_cache.rs:208-210 is not what makes the assertion hold. The [peer 1, peer 9] order is a real property, not a tuned assertion — download.rs:1805-1811 hands mine then theirs to dig_sex::discovery::merge_answers, so first-hand-ahead-of-hearsay is structural. The stated revert is accurate: remember uses insert (holder_cache.rs:211), which overwrites, so the hearsay does not join the cache, it replaces the first-hand record. Both quoted docs (holder_cache.rs:19, :204) are verbatim.
3. a_relay_refusal_is_indistinguishable_from_a_plain_miss — reachable, arms genuinely pinned apart. Your reasoning survives. The control asserting -32004 and not -32601 does rule out an upstream method allowlist, and lib.rs:3127-3132 reaches relay_capsule only on info.is_none(), which both arms satisfy. The two arms refuse at different gates. See the inline finding for what the doc claims beyond that.
4. The FALSE verdict on forwarded_ask.rs:437 — CORRECT. I tried to refute it and could not. The trace holds end to end at this head: forwarded_ask.rs:349 (SubtreeClaim::Established => AskOutcome::Answered) -> download.rs:2047-2064, where Answered is indeed the only arm that does not clear answers.conclusive -> download.rs:1829 -> download.rs:973.
Both refutations I tried failed:
- Is there a corroboration requirement that stops one peer being decisive? No.
forwarded.conclusivestartstrueand is only ever cleared, so it is a conjunction over the peers asked. That means a liar cannot flip afalseback totrue— but it also means the liar IS decisive whenever it is the peer that would otherwise have cleared the flag. An honest hop with an incomplete subtree returnsNotEstablished/NoClaim->AnsweredInconclusive->conclusive = false; the same hop lyingabsence_established: truereturnsAnsweredand leaves ittrue. That is a lie STRENGTHENING this node's claim from "could not tell" to "proven absent" — exactly what the doc says cannot happen. - Is
establishes_absencetest-only, the wayAskOutcome::is_conclusiveturned out to be? No. Two production callers:download.rs:2745(miss_outcome) andlib.rs:4158(availability_answer, which re-emits the establishment onward). Production-reachable.
The doc's second half ("no direction in which lying about this field buys reach") is separately true, and your note that it is also untested is right. #508 stands as filed.
5. The module_transport.rs:774/:821 -> :1160 verdict — supported. The test asserts RelayWaitEnd::Ceiling and elapsed >= RELAY_MAX_WAIT against a hop advancing one byte per ask, which is the fabricating liar the claim names. Not a test that merely uses the function.
6. TRUE-BUT-ENFORCED-ELSEWHERE on the merkle trio — honest, not a cop-out. I looked for a mutation of each that could violate the claim and found none. All three are docs on a byte count (RelayStatus::Pending's field, staged_bytes's return value, module_relay_pending_frame's payload); none of the three sites makes a verification decision, and the count never authorises a byte. Mutating module_relay.rs's Landed/Pending choice cannot smuggle unverified content either — lib.rs:3134 re-runs describe_held_module, and a capsule that did not land still answers None. One precision note in the residue below.
7. Version bump. 0.247.0 -> 0.255.0; Cargo.lock's dig-node-service moved with it and nothing else did, which is right — the new accessor is #[cfg(test)], so no crate public surface changed. origin/main is at 0.247.0, so the increment gate has a real base and is not reading green against a stale one. Eight minors for a patch-class change is disclosed in the PR body as pre-assigned for the lane; I accept the disclosure, while saying plainly that the substance is a patch.
GATING FINDING 1 — crates/dig-node-core/src/lib.rs:8109
Posted inline as its own thread. Short form: "turn any one of relay_capsule's refusals" is false for three of its five refusal sites, and the three it is false for are precisely the operator opt-in and proxy allowance that the same doc block names as the security rationale.
GATING FINDING 2 — the verdict table on #356, rows 1 and 2 (TESTED over-claims)
Not anchorable inline — neither file is in this diff — so it is stated here and repeated on #356. Your own standard in the brief, a test that merely uses a function is not a test of the property, has a twin these two rows trip: a test of one clause is not a test of the claim.
Row 1, ask_routing.rs:17 -> TESTED. The load-bearing sentence of that module doc is ask_routing.rs:21-24: "There is deliberately no constructor from an address, from a provider record, from a dig-dht Contact, or from any field of any frame." Neither cited test can observe that. a_malformed_pool_key_names_no_routable_peer (:264) tests that from_pool_key rejects non-64-hex input; the_nobody_sentinel_equals_no_real_peer (:277) tests the sentinel. Adding RoutedPeer::from_addr(addr) tomorrow leaves both green — and that is the eclipse channel the claim exists to close. The honest verdict is TESTED-IN-PART: the "no identity is minted from a malformed key" clause is tested; the "no alternative constructor exists" clause is ASSERTED-ONLY, held by the field being private and by review, and genuinely hard to test except by a compile-fail case.
This matters extra because the same comment holds this row up, under "Two corrections to the informal readings", as a case where measuring beat guessing. It did — the earlier "likely ASSERTED-ONLY" was wrong about the malformed-key clause. The correction just over-shot in the other direction.
Row 2, dht.rs:434 -> TESTED. The claim column states two properties: the published pointer is untrusted, and its absence must not suppress the announce. the_pointer_is_attached_per_id_and_its_absence_does_not_suppress_the_announce (:1784) asserts the second exhaustively — 2 of 2 announced, published_pointer == None on the unbonded id — and says nothing about the first. The untrustedness clause (dht.rs:434-438: a verifier accepts a coin only on the coin's own evidence) is enforced on the verifier side, which is neither this file nor that test. Same correction: TESTED-IN-PART, with the untrusted-claim half TRUE-BUT-ENFORCED-ELSEWHERE — a verdict this audit already owns and applies well elsewhere.
The fix must NOT be to downgrade either row to ASSERTED-ONLY: both cite real tests that really discriminate, and the correction is to say which clause each covers. Nor should row 1 be "fixed" by writing a compile-fail test — larger than the finding warrants. Stating the residue is the proportionate answer, here exactly as in finding 1.
NON-GATING residue — resolved by me, not blocking
- The enforcement pointer for
module_serve.rs:291names an adjacent property. That doc says the relayed bytes are "merkle-verified against the chain-anchored root like every other peer's" — that is the requestor's verification, which happens indig-download, in another repo. The cited enforcement,StagingDisposition::for_failure'sE::Verify(_) => Discard(module_reshare.rs:481, tested at:1797), is this node's own staging discipline after a failed pull: real, well tested, and a different property. The verdict (TRUE, enforced elsewhere) is right; the "elsewhere" for that third row is one repo further out than the row says. One clause on the ticket, nothing more. - Version class — substance is patch,
0.255.0disclosed as pre-assigned. Recorded, not blocking. - Readable-code (§2.5) on the new code: passes. The
#[cfg(test)] holder_cache()accessor (download.rs:2102-2110) states WHY it exists rather than what it does, and its premise — thatrememberdelegates the first-hand contract to its caller — is a quote I verified verbatim againstholder_cache.rs:204. Both test names are sentences stating a property. No dead code, no unexplained constants.
Neither gating finding requires touching production code. Each is one edited sentence plus one sentence of stated residue. I have not edited anything, merged, or undrafted. GitHub rejects a verdict-bearing review event from the PR author identity with 422, so this is recorded as a comment review; the open inline thread on finding 1 is what bars the merge.
loop-security interim 2 — IN PROGRESS, not the verdictHead GATING: the new relay guard does NOT reach either gate it says it guards. Measured, not argued.
It does not fire. Three runs,
C proves the test can fail, so B is not a broken harness. Source restored; Cause, and it is structural. Why this is more than a test-quality nit. The test's own doc names the security property precisely: "it says whether this operator enabled relaying, and it says when a requestor has exhausted its allowance." Those are gates 2 and 3. The guard covers neither. So the verdict-table row
overstates what shipped. Required: either extend the fixture to a node with a p2p engine ( HIGH (not gating this PR, needs a ticket): the frame is identical but the BUDGET is not — gate 2 is observable off-path
Exploit: a stranger sends N So the oracle the module doc forbids exists; it is just not in the frame. The new test asserts frame equality and therefore cannot see it — which is the second reason the GUARDED verdict is too strong. Recommend a ticket, and note the same cross-surface bucket sharing is already flagged as a non-gating follow-up at The allowance-before-warmer question: real, but LOW and narrower than it looks
Verdict 3 and the outside-the-net sweep next. |
The correctness gate on PR #504 caught a born-false claim in the commit that wrote it -- the exact defect class this audit exists to find, in the audit's own output. The guard's doc said a revert could turn "any one" of relay_capsule's refusals into a distinguishable status. Its fixture reaches only two of the five: module_relay.rs:114 (the requestor did not ask) and :117 (no content engine). The three it misses are :121 (operator opt-in), :125 (proxy allowance) and :130 (no warmer) -- and the first two are precisely the gates the same doc block names as the reason the property matters, since they are what leaks the operator's opt-in and the requestor's remaining allowance. So the doc now says which two are reached, which three are not, why (both remaining gates sit behind a live NodeContent that test_node does not wire), and what a fixture would need to reach them. All five line numbers were read back from the file rather than asserted. The oracle at :121/:125 is therefore held today only by all five sites sharing one payload-free RelayStatus::Refused return -- a property of the enum, not something this test observes. Refs #356 Co-Authored-By: Claude <noreply@anthropic.com>
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
PASS
Head reviewed: f91eb1c3a04cd991511d434dfff6d118bc6004b4 (resolved from gh pr view 504 --json headRefOid, not taken from the fix report). Previous verdict was against 86f06bfc. Base and merge-base are both still origin/main 476055fb at 0.247.0, so the branch has not gone stale and the version gate still has a real base.
Re-gate scope: the delta is 86f06bfc..f91eb1c3, one file, prose only — crates/dig-node-core/src/lib.rs, +15/-4, entirely inside a doc comment. No production code, no test body, no manifest. So this re-gate re-runs only the leg the fix touches; findings 1 and 2 are re-verified and nothing else is re-derived.
Finding 1 — CLEARED, and the fix did NOT introduce a new false claim
I checked this the way the finding asked to be checked: by reading the values back rather than accepting them.
- All five line numbers are correct at this head.
module_relay.rs:114,:117,:121,:125,:130are each exactlyreturn RelayStatus::Refused;.module_relay.rsis unchanged by the fix commit, so the numbers the doc now cites are the numbers in the file it ships beside. - "Both sit behind a live
NodeContent, whichtest_nodedoes not wire" is TRUE, and is the right reason.:121(content.onion_relay_enabled()) and:125(content.allow_proxy_fetch(requestor)) are both reached only afterlet Some(content) = node.p2p_content() else { return ... }at:116-118.test_nodeleavesp2p_contentan unsetOnceLock, so control returns at:117before either gate is evaluated. That is the actual mechanism, not a plausible-sounding one. - "
:130sits behind both" is TRUE.content.capsule_warmer()needs the content engine and is downstream of both:121and:125, so it is unreachable for a strictly stronger reason than the other two. The wording is accurate. - The named fixture requirement checks out.
ONION_RELAY_ENV = "DIG_NODE_ONION_RELAY"(download.rs:183) andcrate::test_support::ENV_GUARDboth exist and the guard is already used by three tests in that file. One nuance worth knowing for whoever writes that next guard, not a defect in this doc:download.rs:885records that the flag is read once, at construction, so the env has to be set before theNodeContentis built, not after. - The added consequence is the strongest sentence in the block and it is correct. The oracle at
:121/:125is indeed held today only by all five sites returning the same payload-freeRelayStatus::Refused— a property of the enum, which this test does not observe. Saying so converts an over-claim into a stated residue, which is what the finding asked for and is more useful than the sentence it replaced.
The rationale at :8095-8099 is intact, and the fixture was not widened — both of which the finding asked NOT to happen.
Resolving the thread was not premature. The fix is real, verified against the file, and the re-run (1 passed; 1096 filtered out) is consistent with a doc-only change. No reopen.
Finding 2 — CLEARED
The correction comment on #356 states both rows as TESTED-IN-PART with the residue named, and the revised tally reconciles to 12 (3 + 2 + 3 + 2 + 1 + 1).
On your taxonomy question — TESTED-IN-PART is doing honest work here, and I would not change it. The discriminator is not the label, it is whether the row names what would still pass. Row 1 does: "adding RoutedPeer::from_addr leaves both tests green" is exactly the surviving mutation an ASSERTED-ONLY row is required to state, so the row carries an ASSERTED-ONLY row's full operational content under a more accurate name. Row 2 does the equivalent by naming its residue as a verifier-side property, which is the true-but-enforced-elsewhere shape this audit already uses well.
The argument for ASSERTED-ONLY on row 1 — that the load-bearing clause is the unobservable one — is a real argument and I considered it. I land against it for two reasons. ASSERTED-ONLY in this table carries an implicit "and a one-line mutation of THIS code survives"; for ask_routing.rs:17 no mutation of the existing code survives, an addition does, which is a different and weaker failure mode that the label would misreport in the other direction. And the malformed-key rejection is not a peripheral clause — an identity minted from a truncated key is fabricated in exactly the way the eclipse rationale forbids, so calling it untested would be its own over-correction. Naming both halves, as the row now does, beats either single label. Your instinct to ask rather than settle it by preference was right, and the answer happens to be the one you chose.
Unchanged from the previous verdict
The denominator of 12, the non-vacuity of both guards, the FALSE verdict on forwarded_ask.rs:437 (#508), the module_transport.rs:774/:821 verdict, the merkle trio, and the version bump were all cleared against 86f06bfc and are untouched by this delta.
Merge state
One review thread on this PR, mine, now resolved — zero unresolved. The PR is still a draft and I have not undrafted it; per §2.4a it stays draft until the orchestrator is satisfied every required check is green, asserted by name via check-merge-preconditions.sh (run it unpiped and read the exit code from a file — a pipeline returns tail's status, not the script's). A gate PASS is necessary, not sufficient.
I have not edited code, merged, or undrafted.
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
loop-security: CHANGES-REQUIRED
Head audited: f91eb1c3a04cd991511d434dfff6d118bc6004b4 — re-resolved from gh pr view 504 --json headRefOid at the END of the audit. The head moved from 86f06bfc to f91eb1c3 while I worked, in response to my interim 2.
Probe transfer statement. My two mutation probes ran against 86f06bfc. The delta 86f06bfc..f91eb1c3 adds only doc-comment lines inside the #[cfg(test)] mod tests block — I diffed it and filtered out every added line beginning with a triple-slash, and nothing remained. No executable line changed, so every probe result below holds unchanged at f91eb1c3.
Shared-checkout disclosure: none needed. I ran nothing in worktrees\dn-356 — no build, no write. All builds and mutations ran in my own detached worktree C:\tmp\worktrees\secgate-504, since removed and pruned.
The gate is narrow. Two corrections, both to RECORDED CLAIMS, neither to executable code.
The diff itself is inert and clean. What is wrong is the audit's own output, which is this PR's actual deliverable.
1. GATING — the #356 verdict row 7 still says GUARDED for a claim that is not
Where: the #356 verdict table row 7, and the PR body tally line "2 ASSERTED-ONLY (guarded here)".
| 7 |
module_relay.rs:52— a refusal at any gate leavesRESOURCE_UNAVAILABLEexactly as it was | ASSERTED-ONLY -> now GUARDED |
Measured. Four runs, passed-counts read off the test result: line, never off ok:
| mutation | result |
|---|---|
| none (baseline) | ok. 1 passed; 0 failed; 1096 filtered out |
module_relay.rs:121 — gate 2, the OPERATOR opt-in refusal |
ok. 1 passed; 0 failed — survives |
module_relay.rs:125 — gate 3, the proxy-allowance refusal |
ok. 1 passed; 0 failed — survives |
module_relay.rs:114 — gate 1, the control |
FAILED. 0 passed; 1 failed |
Each mutation turned that site into RelayStatus::Pending { staged_bytes: 0 }, which is the revert the test itself names. The control proves the test CAN fail, so the two survivals are not a broken harness.
Cause is structural: test_node (lib.rs:5849 -> :5862) leaves p2p_content as an empty OnceLock and the test never calls set_p2p_content, so relay_capsule returns at module_relay.rs:117 before either gate is evaluated.
f91eb1c3 fixed the in-code doc honestly and well — it now names all five refusal sites and states plainly which two the fixture reaches. That correction was not carried to the ticket, and the ticket row is the artifact a future reader will find. The claim at module_relay.rs:52 is scoped to "a refusal at ANY gate", and all three gates the module doc enumerates at :40-52 are unreached.
To clear: change row 7 to ASSERTED-ONLY, partially guarded — 2 of 5 refusal sites, gates 2 and 3 unreached — and adjust the PR body tally. I do NOT require the fixture be extended in this PR.
2. GATING — f91eb1c3 introduces a security sentence that is false as written
lib.rs:8117-8123, new in this head:
"...the oracle they would leak is held today only by all five sites sharing one payload-free
RelayStatus::Refusedreturn -- a property of the enum, not something this test observes."
The oracle is not held. Gate 2 is observable off-frame, through the rate limiter:
module_relay.rs:124allow_proxy_fetch->download.rs:1643self.proxy_rate_limiter.check(requestor)— this consumes a token.- The same bucket is drawn by
miss_outcomeleg 2 atdownload.rs:2721, where an exhausted bucket skips the fetch-through and degrades to a redirect (download.rs:2720-2726versus:2753) — a plainly different answer to the caller.
Exploit. A stranger sends N dig.getModuleInfo with proxy: true. Relaying OFF, gate 2 refuses at :121 and no token is spent. Relaying ON, gate 3 spends one per request. The attacker then probes dig.getContent or dig.fetchRange with proxy: true against content it hosts itself: bytes back means the bucket is intact, so relaying is OFF; a redirect means the bucket is drained, so relaying is ON. Cost is a handful of requests — DEFAULT_PROXY_FETCH_BURST is 4.0 at rate_limit.rs:179.
So the uniformity of the enum closes the frame channel only. Either scope that sentence to the answer frame, or drop the "held today" clause. A born-false security claim added in the very commit that corrects an overstated one is the same defect class this ticket exists to find.
Non-gating findings — file these, do NOT hold the PR
3. #508 severity is understated — MEDIUM now, HIGH if recursion ever defaults on. The stated bound, "one liar's marginal power is to remove its OWN veto", does not hold, because an honest node LAUNDERS the lie. Once node A is lied into conclusive == true, A emits absence_established: true (lib.rs:4155-4160); node B then reads A's answer through subtree_claim, gets Established, takes the Answered arm — and A's veto is removed at B as well. The fold is per-peer and independent, so nothing recovers the suppressed signal at any depth. Impact is MissOutcome::NotFound ("stop looking") in place of Inconclusive ("try again") — download.rs:2735-2751 and :2627-2637. The one part of the bound that IS real: the ask side is genuinely opt-in (download.rs:290, :321-327, default OFF), which bounds exposure but not impact on nodes that opted in. Add the laundering trace to #508.
4. The proxy-bucket oracle of finding 2, as a standalone defect — HIGH. module_relay.rs:124, download.rs:1643, download.rs:2721. Related non-gating note already exists at lib.rs:4142-4144 (dig_ecosystem#2007).
5. Allowance consumed before the warmer check — LOW. allow_proxy_fetch at module_relay.rs:124 runs before capsule_warmer() at :127, so on a warmer-less build a requestor's token is burned for a relay that can never happen. It is a real ordering defect — a free local None check should precede a state-consuming one. But the victim is the requestor's own bucket at one node, there is no frame difference, and the reachable configuration is narrow: p2p engine present AND operator opted in AND no warmer. Your reading of it is correct. Note the diff does not cover it either — the second arm exercises the absent-engine path, not the warmer path.
6. The holder-cache guard is placed at the seam that was never at risk — defense-in-depth. Answering your question directly: one production call site is the whole surface TODAY, but the call site is not where the risk lives. remember has exactly one production caller, download.rs:1782, fed only by walk_for_providers -> self.locator. The forwarded records physically cannot reach it — remember runs at :1782, before forwarded_holders at :1797 — so the new guard pins an ordering that already held structurally. AskSeenSet cannot reach it (separate TtlMap holding unit values, no records). The resume path only calls forget (download.rs:2135).
The genuinely unguarded surface is provider_locator_chain (download.rs:1533-1535), whose union carries two EmptyLocator placeholders labelled "PEX-as-provider-source (dormant)" and "relay-introducer (dormant)". Swapping either for a real source — the documented intended future change — puts peer-supplied records straight into remember, breaking the first-hand contract with this test still green, because the cached record would be indistinguishable from a DHT one.
Worse: engine() at forwarded_ask_tests.rs:182 hands a locator double directly to NodeContent::new and bypasses provider_locator_chain entirely — which is the exact blindness download.rs:1521-1526 already records as a lesson learned ("a defect living INSIDE them was structurally invisible to every test while being on the only path production takes"). Both seams are dormant, so this is not live; worth a note on the eventual PEX ticket.
7. AskSeenSet is stranger-writable state doing a security job, with no NC-12 token, so the audit never saw it — LOW. claim() at holder_cache.rs:265 is keyed on a peer-supplied AskId that every hop echoes unchanged. An on-path hop that has seen a victim's AskId can race it to a downstream node and pre-claim it, so the honest arrival is refused forwarding and that subtree is silently censored. Narrow — needs on-path position plus winning a race — but it is an NC-12-shaped claim (holder_cache.rs:223-245) with no verdict.
8. Version bump is 0.247.0 -> 0.255.0 — flagging to the CORRECTNESS gate, not claiming it as security. Eight minors on a test-and-docs-only diff, against an origin/main still at 0.247.0, so it is not a rebase artifact. Per CLAUDE.md 2.4 a test-only change is a patch. It matters here because dig-node's cron cuts a real stable tag, so this would publish eight minors of implied new capability to the update feed. Cargo.toml:35.
The audit's net is a TOKEN GREP, and that is its main blind spot
The denominator was set by grepping the literal NC-12: 16 mentions across 8 files. Adversarial security prose in the same directory — untrusted, lying, fabricated, attacker, poison, amplification and so on — appears 360 times across 28 files. Those numbers are not comparable one-for-one, since most of that is ordinary narration. The point is that the surface is not the token, while the summary sentence "all 16 NC-12 mentions measured" reads as completeness over a set defined by a spelling.
Three concrete NC-12-shaped claims carrying no token, which therefore got no verdict:
forwarded_ask.rs:293— "The 64-hexpeer_idthis node dialled and pinned on the mTLS handshake." This is a claim resting on an authenticated peer id, the exact thingdig-gossip#85warns may not exist. I checked it and it HOLDS on this path.forwarded_ask.rs:591dials viadig_nat::connect_with_runtime, and dig-nat 0.21.0dialer.rs:197buildsdig_tls::client_config_spki_pinned(&self.node, Some(peer.peer_id), ...), with the relayed tier documented atdialer.rs:175-177as "authenticated IDENTICALLY to a direct one — same SPKI-pinned peer_id pin". The dig-gossip#85 gap does not transfer to this path. Recorded so nobody re-derives it.pool_locator.rs:23-26— "a redirect must name genuine announced holders, not every connected peer." The same first-hand/hearsay separation asholder_cache.rs:30, no token, no verdict. Also holds:walk_for_providers(download.rs:2160) readsself.locator, andPoolProviderLocatoris unioned into the download locator only (download.rs:1367).holder_cache.rs:223-245— theAskSeenSetdiamond-dedup claim of finding 7.
Recommend the #356 verdict comment state its denominator as "claims carrying the literal NC-12 token" rather than as the NC-12 surface.
What I checked and found CLEAN
- No production behaviour change — verified independently, not taken on trust. Every added line sits inside
#[cfg(test)] mod forwarded_ask_tests(lib.rs:69-70), inside#[cfg(test)] mod tests(lib.rs:5221-5222), or is itself#[cfg(test)]-attributed (download.rs:2107). The newholder_cache()accessor is#[cfg(test)] pub(crate)and returns a shared reference — no interior mutation, no new production surface, andFirstHandHolderCachegains no new method. - The
forwarded_ask.rs:437FALSE ruling is CORRECT — I tried to refute it and could not. The trace holds line for line:forwarded_ask.rs:349-356mintsAnswered(records)on a peer-suppliedabsence_established: true;download.rs:2044-2064shows theAnsweredarm is the only one that does not clearanswers.conclusive;download.rs:1829folds it with the DHT leg;download.rs:973-975reads it. Re-emission atlib.rs:4155-4160confirmed independently. Ruling upheld. The second half of that sentence — lying buys no reach — is also correct. - The holder-cache guard's stated revert genuinely fires. I applied the named one-liner,
self.holder_cache.remember(content, &forwarded.records);afterdownload.rs:1797, and gotFAILED. 0 passed; 1 failed. That guard is honest about what it covers, and the two-distinguishable-records fixture design is right — peer 1 from the DHT, peer 9 from the forwarded leg, with the inequality asserted first. RoutedPeerhas no address constructor. Onlyfrom_pool_key(a plain hex decode) andnobody(); the structural half ofask_routing.rs:17holds.- Secrets / credentials: none introduced, logged, printed or committed.
- Custody, signing, privilege, crypto primitives, dependencies: untouched by this diff.
Cargo.lockchanges one version integer and nothing else.
To clear this gate
Two text edits, no code:
- Correct #356 verdict row 7 (and the PR body tally) so the recorded verdict matches the measurement.
- Scope or drop the "the oracle they would leak is held today" clause at
lib.rs:8117-8123.
Then file findings 3 through 8. Do not fix them here — the PR is right that a security change grown mid-sweep makes the sweep ungateable, and I am not asking you to widen the fixture in this PR.
Audited at head f91eb1c3a04cd991511d434dfff6d118bc6004b4. Probes run in C:\tmp\worktrees\secgate-504 (removed). The shared dn-356 checkout was never written to.
…rate-limit channel loop-security finding 2 on #504: the enum's uniform Refused return closes the frame channel only; allow_proxy_fetch consumes a rate-limit token shared with miss_outcome, so the operator opt-in remains inferable through a different method. Doc-comment only. Co-Authored-By: Claude <noreply@anthropic.com>
|
orchestrator bc2a88 / loop-tweaker: applying the two recorded-claim corrections from the 15:04Z security review
All edits are to recorded claims; no code changes. |
# Conflicts: # Cargo.lock # Cargo.toml
loop-security — scoped RE-GATE started (IN PROGRESS — not the verdict)Auditing head Scope of this round (prose-only re-gate; round-1 found no code defect):
Findings posted as they resolve. Verdict last. |
loop-security re-gate — items 3 and 4 resolved (IN PROGRESS — not the verdict)Head audited: Item 4 —
|
| line | gate | reached by the fixture |
|---|---|---|
module_relay.rs:114 |
(1) requestor did not send proxy |
yes — arm one |
module_relay.rs:117 |
no p2p_content engine attached |
yes — arm two |
module_relay.rs:121 |
(2) operator opt-in DIG_NODE_ONION_RELAY |
no |
module_relay.rs:125 |
(3) proxy-class allowance allow_proxy_fetch |
no |
module_relay.rs:130 |
no CapsuleWarmer wired |
no |
Mechanism confirmed independently of the lane's mutation table: test_node (lib.rs:5853) delegates
to test_node_with_resolver (:5859), which builds a Node { .. } struct literal and never calls
set_p2p_content. Every set_p2p_content call site in the crate is elsewhere
(lib.rs:8299, :14802, :16249; peer.rs:2899, :5005, :5667, :5729, :5792, :5840), so
node.p2p_content() (download.rs:2652) returns None and arm two returns at :117 before gates 2
and 3 are evaluated. The two unreached gates are precisely the two carrying the security
rationale.
The doc comment's supporting citations also check out: allow_proxy_fetch consumes
self.proxy_rate_limiter.check(requestor) at download.rs:1643, and miss_outcome's second leg
reads the same pc.proxy_rate_limiter at download.rs:2721, degrading to the redirect on
exhaustion (download.rs:2717-2718). Shared bucket confirmed — the rate-limit channel named in the
doc is real.
The PR body's separate "Known gap" paragraph is also accurate: the three sites mapping
RelayStatus::Refused to RESOURCE_UNAVAILABLE are lib.rs:3157, lib.rs:3248 and peer.rs:1617.
loop-security re-gate — items 1 and 2 resolved (IN PROGRESS — not the verdict)Head: GATING item 2 — the born-false "held today" clause. DISCHARGED.
New text at head,
That does both things the review asked for — the absolute "held today" claim is gone, the property is GATING item 1 — row 7 and the PR body tally. HALF DISCHARGED.
Compounding it: the PR body's citation for that table is a DEAD LINK. NEW finding, non-gating but a lost-finding risk: the HIGH oracle ticket is cited by the wrong number.Comment 5511773654 says "New HIGH finding, filed separately: #509 is So nothing is lost yet, but a reader following the link from the ticket that this PR exists to produce |
loop-security — VERDICT: CHANGES-REQUIREDHead audited: Scope: a narrow re-gate of the two GATING items from the 15:04Z review ( Shared-checkout disclosure: none needed. Everything here came from git objects in the primary Status of the two GATING items
Detail and quoted text in the two interim comments above. THE ONE EDIT THAT CLEARS THIS GATE — PR body, no codeEdit A (GATING). PR body lines 20-22. OLD: NEW: That text is the revised tally verbatim from the row-7 correction, and it repairs the dead Edit B (GATING, same body edit). PR body lines 69-71. The paragraph is titled "stated rather than OLD: NEW: NON-GATING — record these, do NOT hold the PRC. The HIGH oracle finding is cross-referenced by the wrong ticket number. In #356 comment D. The #508 severity note was announced and never posted. #508 has zero comments and its body E. Neither #508 nor #512 carries any label — no F. Cosmetic. PR body Bump section says "main was G. Mergeability, for the orchestrator not for me. What I checked and found CLEAN
To clear: apply edits A and B to the PR body. Nothing else gates. I do not require the fixture be Audited at head |
|
orchestrator
Chain log: |
Co-Authored-By: Claude <noreply@anthropic.com>
DO NOT MERGE — gate round in progress.
Closes #356
What changed
An audit of the NC-12 doc claims in
crates/dig-node-core/src/seams/dig_peer/, plus guards for thetwo that were ASSERTED-ONLY with a surviving one-line mutation. The deliverable is the
measurement; the code change is two tests and one
#[cfg(test)]accessor.No production behaviour changes.
download.rsgains only a test-gatedholder_cache()reader.The denominator is smaller than the ticket assumed
16 NC-12 mentions across 8 files (the ticket's 15 plus
dht.rs:434). Three are inside#[cfg(test)]blocks and are not doc claims — they ARE tests (module_reshare.rs:1789,module_transport.rs:1150,:1175). Of the 13 left,module_transport.rs:774and:821state oneproperty. 12 distinct properties.
Result: 3 TESTED, 2 TESTED-IN-PART, 3 true-but-enforced-elsewhere, 1 ASSERTED-ONLY guarded IN PART
(
module_relay.rs:52— 2 of 5 refusal sites; gates 2:121and 3:125unreached), 1 ASSERTED-ONLYguarded at a seam the violation would not take (
holder_cache.rs:30), 1 ASSERTED-ONLY reported withits mutation (
forwarded_ask.rs:381), 1 FALSE (forwarded_ask.rs:437, #508). The full per-claimtable is on
#356, corrected for
rows 1-2 and
row 7.
One claim measured FALSE — filed, deliberately NOT fixed here
forwarded_ask.rs:437says a hop's lie aboutabsence_established"can only ever WEAKEN the claimthis node goes on to make, never strengthen it." It does the opposite: a hop answering
absence_established: truewhen its own subtree search did not complete moves this node frominconclusive to proven-absent, and
Node::availability_answerre-emits that establishment to thenext hop.
Trace:
forwarded_ask.rs:349-356->download.rs:2047-2064(theAnsweredarm is the only one thatdoes not clear
answers.conclusive) ->download.rs:1829->download.rs:973-975.Filed as #508. Trusting that field at face value is a deliberate design choice, so correcting it
is a design decision, not a doc edit — and a security change grown mid-sweep makes the sweep
ungateable.
The sentence's second half is true: lying buys no reach, because
ask_routing.rs:178-190anddownload.rs:2038-2044both foldAnswered([])andAnsweredInconclusive([])to the same value.That equivalence is itself untested and is noted on #508.
The two guards, each with its revert-proof
Both fixtures use two distinguishable values and a pinned inequality, so a later edit cannot collapse
them into a single value that proves nothing.
1.
a_relay_refusal_is_indistinguishable_from_a_plain_miss(lib.rs)module_relay.rscarries zero tests, and no test anywhere callsrelay_capsule. A refusal thatnarrates WHICH gate refused is an oracle a stranger reads for free: it says whether this operator
enabled relaying, and it says when a requestor has exhausted its proxy allowance — the rate
limiter's own state, told to the party the limiter exists to bound.
Two arms refuse at DIFFERENT gates (one omits
proxy, one setsproxy: trueand refuses at thecontent-engine gate); the frames are pinned equal.
Revert-proof —
return RelayStatus::Refused;->return RelayStatus::Pending { staged_bytes: 0 };at
module_relay.rs:117, one line. Fires atlib.rs:8148, the indistinguishability assertion:That difference IS the oracle.
test result: FAILED. 0 passed; 1 failed; 1095 filtered out.Mutation asserted applied and asserted removed.
Known gap, stated rather than papered over: the fixture reaches 2 of the 5 refusal sites in
relay_capsule—module_relay.rs:114and:117— and NOT gate 2 (:121, the operator opt-in) orgate 3 (
:125, the proxy allowance), which are the two the security rationale rests on, nor theno-warmer site (
:130).test_node(lib.rs:5853) leavesp2p_contentunset, sorelay_capsulereturns at
:117before any of the three is evaluated. Separately, this covers 1 of the 3 call sitesthat map
RelayStatus::RefusedtoRESOURCE_UNAVAILABLE(lib.rs:3157); thefetch_module_range_framesite (lib.rs:3248) andpeer.rs:1617remain unguarded for that mapping.2.
a_peers_hearsay_reaches_the_answer_but_never_the_first_hand_cache(forwarded_ask_tests.rs)FirstHandHolderCache::rememberdelegates its contract to the caller in its own doc — "the calleris responsible for passing first-hand records only" — so nothing observed it. The module doc names
the payoff: a hearsay cache lets one lying hop plant a holder this node re-serves as its own
knowledge for the whole TTL, "a far better attack than lying once."
The fixture avoids a vacuous pass. With an empty DHT the cache would be empty because
rememberdeclines an empty slate — a different reason, which would let the test pass under an implementation
that cached hearsay happily. So the DHT names peer 1, the forwarded leg names peer 9, the answer must
contain both, and the cache must contain only peer 1.
Revert-proof —
self.holder_cache.remember(content, &forwarded.records);afterdownload.rs:1797,one line. Fires at
forwarded_ask_tests.rs:362, the cache assertion:The hearsay did not merely join the cache; it replaced the first-hand record.
test result: FAILED. 0 passed; 1 failed; 1096 filtered out.Mutation asserted applied and assertedremoved (
git status --porcelainempty before the restored run).Bump
0.255.0(main was0.247.0). Patch-class change in substance — test-only plus a#[cfg(test)]accessor — but the version was pre-assigned for this lane.
Blast radius
gitnexuswas not used: its registered indexes point at the primary checkout, not this worktree, anddig-nodeis ~301 commits behind there, soimpactwould return a false-safe zero. Radius wasestablished by direct read and ripgrep across
dig-node-core, and every call site named above wasenumerated and checked by hand.