Skip to content

feat(node): scope CORS per route+method, hold the DIG loopback rule, keep the service stoppable - #291

Merged
MichaelTaylor3d merged 2 commits into
mainfrom
loop/mvp-batch-767-702-2880-2870
Aug 21, 2026
Merged

feat(node): scope CORS per route+method, hold the DIG loopback rule, keep the service stoppable#291
MichaelTaylor3d merged 2 commits into
mainfrom
loop/mvp-batch-767-702-2880-2870

Conversation

@MichaelTaylor3d

@MichaelTaylor3dMichaelTaylor3d commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

MVP batch for dig-node. One branch and one PR because all four tickets touch dig-node-service, and #767 reaches into dig-wallet — four branches would conflict on the same files.

Closes #767
Closes #702
Closes #2880
Closes #2870

Version: 0.134.0 → 0.135.0 (minor — new capability, no removed or renamed public API).


Blast radius checked

gitnexus is not indexed in this worktree and a fresh analyze was not run, so blast radius was established by call-graph grep + direct read, and this PR says so rather than implying a tool it did not use (§2.0 bound 2: falling back is permitted, concealing it is not).

Symbol changedDirect callers foundRadius
is_allowed_originreflects_originone, the CorsLayer predicate in server.rs::router + one unit testcontained; the function was private
RealLocalServer::serve_until_fetchedopen.rs only (trait LocalContentServer); the URL is generated and handed to the browsercontained — no external dialer names this address
win_service.rs::run_servicerun-service subcommand onlycontained, Windows-only
serve_with_shutdownsignature untouchednone

dign chia-peers and control.chiaPeers.* were read, not modified (see #2870 below).

No HIGH/CRITICAL-risk edit in this diff. Nothing here touches signing, key handling, or a spend path. The one security-relevant change narrows an existing surface (#702).


1. #767 — DIG loopback rule

New:crates/dig-node-service/src/loopback.rs — one place that answers which loopback address a DIG service binds. 127.0.0.1 is reserved for the rest of the machine; 127.0.0.2 is dig-node (dig.local), 127.0.0.5 is dig-dns.

Fixed — and this one was a live regression. The ticket asks to confirm the P0 #745 ephemeral content server is on 127.0.0.2. It was not: open.rs bound ("127.0.0.1", 0) and advertised http://127.0.0.1:<port>. Now it walks loopback::ephemeral_bind_candidates() — DIG address first, 127.0.0.1 only where the DIG address cannot be bound at all (macOS without an lo0 alias), and a fall-back is logged, because a silent one is indistinguishable from the rule not being applied.

Made mechanical:tests/loopback_bind_guard.rs fails the build on a new literal-loopback bind in product source. It strips #[cfg(test)] modules by brace matching (so an ephemeral test fixture on 127.0.0.1:0 is correctly ignored) and flags bind calls only — dials are untouched, because localhost is a canonical §5.3 dialling tier and a guard that flagged it would assert something the ecosystem does not believe.

The guard immediately earned itself: it found two binds a manual sweep had missed, at crates/dig-wallet/src/sage/transport.rs:276-277.

What is NOT moved, and why — stated plainly rather than scoped away silently

Three bind sites keep a literal loopback address. All three are recorded in the guard's own DECLARED_EXCEPTIONS with their reason, so the coverage gap lives next to the guard instead of in someone's memory.

  1. dig-node-service/src/wallet_mtls.rs:106127.0.0.1:9776, the Sage-parity wallet mTLS listener. The address is a dial contract; moving it without the consumers is the half-migration §1.3b forbids. It is already off Sage's own 9257 (v0.128.0), so the collision that caused the incident is closed.
  2. dig-wallet/src/sage/transport.rs:276-277serve_dual's mTLS + HTTP-mirror binds. Not fixed for a structural reason: dig-wallet sits belowdig-node-service, so it cannot depend on the SSOT, and duplicating the constant would create exactly the rival implementation the allocation exists to prevent. Mitigating fact: serve_dual has no production call site (dig-node serves that surface via wallet_mtls.rs), so no shipped listener is on those lines.
  3. The control/content listener (config.rs:299server.rs:2044, 127.0.0.1:9778) — a cross-repo dial contract for dig-app, the extension and dign, and canonical docs(spec): correct the dig-node-core P2P dependency list to match Cargo.toml #132. Its address is computed, so the text-scanning guard cannot see it either; that reach floor is documented in both the module and the guard.

The follow-up this implies: the allocation's correct home is a foundation-level crate (dig-constants, L00) that every consumer can reference downward. That is release-first and cross-repo, so it is not this PR's, and #767 is status:needs-user on the ratified table regardless.


2. #702 — route/method-scoped CORS

CORS is now decided per request, not once for the router.

  • Local web/extension origins — the whole surface, unchanged.
  • Desktop-app originscontent reads only: GET/HEAD, and not /ws or /ws/status.

Why the discriminator is the method. A route-only split cannot express this policy, which is exactly why #693 deferred it: POST / multiplexes content reads and the open wallet-read methods onto one JSON-RPC endpoint, and /{method} serves the Sage-parity wallet RPC on POST and content on GET. Every open wallet-read method is reached by POST; every cross-origin content read a browser client makes (dig-urn-resolver's node-first tier, the reason #669 widened the set) is a GET carrying the X-Dig-* headers. So the split removes the wallet-read reach and leaves #669 intact.

Preflights are judged against Access-Control-Request-Method, not against OPTIONS, so the preflight answer matches the answer the real request gets. A preflight declaring no method fails closed for the app family.

SPEC.md §4.3 rewritten — the paragraph documenting this exposure as accepted-and-unenforced would otherwise have described behaviour that no longer exists.


3. #2880 — the SCM 1061 wedge

Which of the two causes it was: NEITHER, and the ticket asks, so here it is explicitly. 1061 normally means the service never reached RUNNING or never reported its accepted-controls mask. Measured against this code, win_service.rs reports Running with ServiceControlAccept::STOP before it serves, and the SCM caches that.

The defect is one layer in — in how the accepted control was acted on. The stop was bridged into the serve future by spawn_blocking(move || shutdown_rx.recv()), putting the stop path on tokio's blocking pool, which the wallet replica's synchronous database work also draws from. With the pool saturated the receiving task never ran, so the accepted stop was recorded and never observed: the service kept reporting Running, kept serving HTTP, and never stopped.

That also answers the ticket's fourth scope item. The correlation with the frozen replica (watched_addresses: null, static peak_height) is not a coincidence — a saturated blocking pool is the shared blocked resource, and it produces both symptoms. Fixing the stop path does not unfreeze the replica, but it does mean a frozen replica can no longer make the node unstoppable.

Two changes in service_control.rs:

  1. The stop is a tokio::sync::watch signal — delivered by the async runtime, needing no blocking thread. Raising it cannot block and cannot fail, so the control handler always answers the SCM promptly.
  2. Graceful shutdown is bounded (20s, inside the SCM's own 30s timeout). A body that will not wind down no longer holds the service Running; the service reports Stopped anyway and reports that run as failed, because a forced stop reported as clean is the same class of lie as the updater's Deferred behind exit code 0.

SPEC.md §9.0 records the guarantee normatively.

Not in this PR: scope items 2 and 3 (the updater must not report a repeated Deferred as a clean pass; whether it should escalate to a kill after N deferrals) are dig-updater changes, a different repo. Flagged for a sibling ticket rather than silently dropped.


4. #2870 — trusted Chia peer: ALREADY SHIPPED, no code in this PR

The already-shipped check (§2.0) says build nothing. The whole deliverable the corrected ticket asks for — "wire the surface to the writer that already exists" — is on main:

  • control.chiaPeers.add / .list / .removecrates/dig-node-service/src/control.rs:911-913, dispatched; declared at :208-210, master-token gated at :263-265. Shipped by PR fix(control)!: gate chiaPeers on the master-token tier and adopt dnci 0.18 #248 (2e73fd0).
  • dign chia-peers add|list|remove — the clap subcommand at crates/dig-node-service/src/entrypoint.rs:181-184, sub-actions at :466-485, mapped at :775-779; rendering at control_cli.rs:490-518. CLI plane shipped by PR feat(cli): control-parity subcommands + peer management (#426 #559) #45 (ed0e10a).
  • It writes the existing user_managed row via dig-wallet's add_peer — no second writer, exactly as the ticket requires.
  • The custody-grant framing the ticket demands is present in the user-facing help, not just in a comment: "its answers alone can advance, roll back, or complete this node's wallet replica, so a wrong or hostile one can give this node a false view of the chain — and of your money. Add only a node you run yourself."

One deliberate non-action. §5.3's idiom is CLI flag + $ENV + stored config, and there is no $DIG_CHIA_TRUSTED_PEER env var or config key. That is not an oversight: this row is a custody grant, the ticket says "Do NOT build a second writer", and an env var is a second configuration path into write authority — one that a stray value in a service environment could exercise with no user act. Recording the reasoning rather than the omission.


Evidence

Toolchain rustc 1.98.0 — the CI version, so -D warnings was evaluated against the same compiler.

GateResult
cargo clippy --workspace --all-targets --all-features -- -D warningsclean
cargo check -p dig-node-service --all-targetsclean
--lib service_control:: loopback::7 passed
--test loopback_bind_guard3 passed
--test server -- cors6 passed (incl. the pre-existing #669 tests)
--lib open::tests::real_local_server1 passed

cargo test --workspace is not locally runnable (a dig-node-core test exceeds 60s); CI covers the workspace.

Revert-proofs — four mutations, and which assertion fired

Each fix was reverted at the site the fix lives, on a committed tree, backed up by file copy (never git checkout). The assertion that fired was read in every case, not just the pass/fail.

1. #2880 — stop wait back onto the blocking pool (StopWaiter::wait)

test service_control::tests::a_stop_is_observed_while_the_blocking_pool_is_saturated ... FAILED
panicked at src/service_control.rs:221:
the stop signal must be delivered by the async runtime, never by a task queued on the
blocking pool
test result: FAILED. 4 passed; 1 failed

Exactly the intended assertion, and the other four stayed green — so the fixture isolates the delivery mechanism rather than breaking the module.

2. #702 — predicate back to router-wide

panicked at tests/server.rs:433:
a desktop-app origin must NOT be reflected on `POST /`
left: Some("tauri://localhost") right: None
test result: FAILED. 5 passed; 1 failed

3. #702 variant B — a ROUTE-only split (the nearest wrong implementation, and the trap "a read-only reflection on a route that also accepts a mutating method is not scoped")

panicked at tests/server.rs:440:
a desktop-app origin must NOT be reflected on the Sage-parity wallet RPC `POST /{method}`
left: Some("tauri://localhost") right: None

A different assertion fired (:440, not :433). That is the point of running both: a fixture that failed identically under either mutation would be pinning "the app origin is denied somewhere" instead of the placement. The GET/POST pair on one path is what separates them.

4a. #767 — ephemeral bind back to 127.0.0.1

panicked at src/open.rs:1232:
the ephemeral content server must serve from 127.0.0.2 (DIG loopback available: true),
got http://127.0.0.1:49203/photo.jpg

DIG loopback available: true is load-bearing: it proves the host-derivation took the strict branch, so the derived expectation was not quietly satisfied by the macOS fall-back allowance.

4b. #767 — the guard itself. A new literal-loopback bind was injected into peer_ping.rs (a file with no prior violation):

panicked at tests/loopback_bind_guard.rs:129:
a DIG service must never BIND a literal loopback host (#767) — 1 violation(s):
crates/dig-node-service/src/peer_ping.rs:243: std::net::TcpListener::bind(("127.0.0.1", 0))

The guard also carries its own two vacuity tests: every banned pattern is proven capable of matching (a ban entry that matches nothing bans nothing), the dial/classifier controls are proven not flagged, and the #[cfg(test)] stripping is proven to drop the fixture bind while keeping the product one.

All four reverts restored by copy; git status and git diff --stat both clean afterwards.


Not finished / follow-ups

  • #767 is not fully closable here. It is status:needs-user on the ratified allocation table, and its checklist is one child per repo. dig-node's row is done except the three declared exceptions above; the dig-constants (L00) rehoming that would let dig-wallet and every other consumer share the SSOT is a release-first cross-repo change.
  • #2880 scope items 2 and 3 are dig-updater work (repeated Deferred must not exit 0; escalation after N 1061s). Not attempted here.
  • The 1061 fix is reasoned from the code and the reported symptoms, not reproduced on a wedged host. The mechanism fits every observation and the guard test fails against the old code, but an SCM wedge cannot be reproduced in a unit test — so the attribution is strong, not proven.
  • Pre-existing latent flake cache_lock_is_exclusive_then_released untouched (eviction tests take the cross-process cache lock outside ENV_GUARD).

One more limitation in the guard, found while writing it

The scanner matches per line, so a bind call that cargo fmt splits across lines would evade it — the TcpListener::bind token and the "127.0.0.1" literal would land on different lines and neither alone trips the check. Recording it rather than leaving it to be discovered: when you find that a thing you meant to guard is invisible to your guard, that is a coverage finding, not a footnote.

Why it is not fixed here: in practice bind(("127.0.0.1", port)) is far too short for fmt to split, so the evasion needs a contrived call site, and hardening the scanner into a multi-line matcher is a change that needs its own revert-proof and another CI cycle. Flagged for the gate to redirect if it disagrees.

Related and already handled: fmt was applied to this branch and then all four fix tests plus the guard were re-run, precisely because formatting can move a bind onto a new line and silently blind the check. No bind in the workspace is currently split.

CI — GREEN at a0783868c0984ea7ef59cd70f22d610a8b0fb3cf

Rebased onto origin/main (8f2f416, the v0.134.0 release commit) so the branch is not stale under strict=true; the 0.134.0 -> 0.135.0 bump was re-verified as surviving the rebase in both Cargo.toml and Cargo.lock. One commit, 11 files.

check-merge-preconditions.sh asserts the required set BY NAME (not from the rollup):

draft=true mergeStateStatus=CLEAN unresolvedReviewThreads=0
Lint commit messages SUCCESS
Check version increment SUCCESS
Rustfmt SUCCESS
Clippy SUCCESS
Test + coverage SUCCESS

The script reports BLOCKEDsolely because draft=true, which is deliberate — the gate round has not returned, and a ready green PR is a merge invitation to a sibling lane.

The one CI red, and how it was cleared

Test + coverage failed on the previous head with dig-node-core::banned_address_patterns :: no_source_file_builds_a_socket_address_from_concatenated_text, TRY 1/2/3 — deterministic, not flaky. The finding was genuine and mine.

Two things were checked before touching anything, because a sweep that cannot fail for the right reason is worse than no sweep:

  1. Is the haystack real? Yes — it iterates rust_sources(crates_root()) and std::fs::read_to_strings real files, and it carries its own anti-vacuity floors: a MINIMUM_FILES_SCANNED count and a dig-node-service/-sibling-file floor, so a crates_root() that collapsed to one crate fails loudly rather than passing clean. It also ships the_scanner_flags_the_banned_construct_in_every_spelling, proving its matcher can fire.
  2. Fix the call site, not the sweep. Done. No widening, no exemption, no KNOWN_VIOLATIONS entry. The ban is right: format!("http://{host}:{port}/...") loses the brackets every IPv6 literal needs. The URL is now built from the SocketAddr — whose Display brackets v6 and leaves v4 alone — and the bind helper returns only the listener, so the advertised address is read back off what was actually bound and cannot disagree with it.

Revert-proof for that fix too. Restoring format!("http://{host}:{port}/{filename}") fires the same assertion, naming the exact line:

panicked at crates/dig-node-core/tests/banned_address_patterns.rs:390:
these lines build an address from text, which is invalid for every IPv6 literal (#1593).
Use `SocketAddr::new(ip, port)` ...:
dig-node-service\src\open.rs line 304: format!("http://{host}:{port}/{filename}")

Nothing was hidden behind the fail-fast bail

CI's run stopped at 882/2154 tests, so the whole workspace was re-run locally with CI's exact runner and --no-fail-fast:

cargo nextest run --workspace --locked --retries 2 --no-fail-fast
Summary [496.560s] 2146 tests run: 2146 passed (22 slow, 3 flaky), 3 skipped

Clean. The 3 flaky ones are pre-existing and unrelated to this diff — the_chain_reads_are_open_reachable_and_degrade_honestly, the_push_and_the_arrival_cursor_are_gated_while_the_chain_reads_are_open, wallet_rpc_answers_core_reads_on_loopback, all in dig-node-service::server, all from the process-global env serialization that file documents. Each passed on retry, and CI runs --retries 2 as well.

Note for whoever re-runs commitlint: the first failure was the PR title at 109 chars (the commit subject is 95), and re-running the failed run replays the frozen event payload with the old title. The green run is the one the title edit created, which the re-run had cancelled.

@MichaelTaylor3d
MichaelTaylor3dforce-pushed the loop/mvp-batch-767-702-2880-2870 branch 2 times, most recently from e35f977 to 21df7c3CompareAugust 21, 2026 04:28
@MichaelTaylor3dMichaelTaylor3d changed the title feat(node): MVP batch — DIG loopback rule, scoped CORS, SCM control responsiveness, trusted Chia peer surfacefeat(node): scope CORS per route+method, hold the DIG loopback rule, keep the service stoppableAug 21, 2026
@MichaelTaylor3d
MichaelTaylor3dforce-pushed the loop/mvp-batch-767-702-2880-2870 branch from 21df7c3 to 64d354cCompareAugust 21, 2026 04:38
@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

Resume-ready progress — lane pickup after the 600s watchdog

  • Branch:loop/mvp-batch-767-702-2880-2870
  • Head:0ec6b2af7fe744671f44ade4df09fdba40dd9a88
  • Status: DRAFT, staying draft until the gate round returns.

Done

  1. Salvaged the previous lane's uncommitted crates/dig-node-service/src/open.rs fix and committed it (0ec6b2a). The served-content URL is now built from the listener's SocketAddr rather than from host-and-port text, and bind_ephemeral_on_dig_loopback no longer hands its caller a host that must be kept in agreement with the port. This clears the deterministic red in dig-node-core::banned_address_patterns::no_source_file_builds_a_socket_address_from_concatenated_text (run 32447663982 / job 96670197329, three retries all failing).
  2. Confirmed the sweep's haystack is REAL, not a test-local literal.rust_sources(crates_root()) walks the actual crates/ tree from CARGO_MANIFEST_DIR, reads each .rs file off disk, and asserts three independent reach floors (MINIMUM_FILES_SCANNED = 100, MINIMUM_FORMAT_CALLS_SEEN = 600, MINIMUM_SIBLING_CRATE_FILES = 5) so a collapsed scope cannot pass as clean. The finding was genuine and the fix belongs at the callsite.
  3. Swept the whole tree for other violations of the same shape, using a faithful replica of the test's own matcher (format_callsfirst_string_literalinterpolated_colon_pairnames_an_address, with comment-only lines blanked). Result: 166 files, 1116 format! calls, 50 in the sibling crate, 0 offenders. Control-checked the replica against the pre-fix line, which it flags — so the zero is a real absence, not a broken matcher. open.rs was the only violation.

Remaining

  • Full suite. Only 882 of 2154 tests ran before nextest bailed on the failure above, so red may sit behind it. Next command, in /c/tmp/worktrees/dn-mvpbatch:
    cargo nextest run --workspace --all-features
    
  • Version: workspace Cargo.toml is already at 0.135.0 against main at v0.134.0 (8f2f416), and there is no package.json. The bump is present — do not add a second one.

@MichaelTaylor3d
MichaelTaylor3dforce-pushed the loop/mvp-batch-767-702-2880-2870 branch from 0ec6b2a to a095039CompareAugust 21, 2026 05:09
…keep the service stoppable
Three fixes that share `dig-node-service`, plus a measurement that says the fourth ticket needs no
code.
CORS (#702) is now decided per request rather than once for the router. Local web/extension origins
keep the whole surface; desktop-app origins are reflected for content reads only. The discriminator
has to be the method, not the route: `POST /` multiplexes content reads and the open wallet-read
methods onto one JSON-RPC endpoint, and `/{method}` serves the Sage-parity wallet RPC on POST and
content on GET, so a route-keyed decision must answer both traffic classes the same way. That is
what #693 deferred. Every open wallet-read method is reached by POST and every cross-origin content
read is a GET, so the split closes the wallet-read reach with #669 left intact. Preflights are
judged against `Access-Control-Request-Method` so the preflight answer matches the real one, and a
preflight declaring no method fails closed.
The DIG loopback rule (#767) gains a single source of truth in `loopback.rs` and a build-time guard.
The ephemeral content server was still on `127.0.0.1` despite the P0 #745 fix the ticket asks to
confirm; it now takes the DIG address, falling back only where that address cannot be bound at all
and logging when it does. The guard fails the build on a new literal-loopback bind, ignoring test
fixtures and dials, and it immediately found two binds a manual sweep had missed. Three sites keep a
literal address for cross-repo dial-contract or crate-layering reasons and are enumerated in the
guard with their reasons rather than left to memory.
The SCM 1061 wedge (#2880) was neither of its two usual causes: the service does reach RUNNING and
does report a STOP-accepting mask. The stop was bridged into the serve future by
`spawn_blocking(recv)`, putting it on tokio's blocking pool — the same pool the wallet replica's
synchronous database work draws from. With the pool saturated the receiving task never ran, so an
accepted stop was never observed and the service kept serving HTTP while the SCM could not stop it.
That is also why the wedge correlated with the frozen replica: one blocked resource, two symptoms.
The stop is now a watch signal delivered by the runtime itself, and graceful shutdown is bounded, so
a body that will not wind down no longer holds the service RUNNING and a forced stop is reported as
a failed run rather than a clean one.
One defect found by CI and fixed at the call site, not by widening the check that caught it. The new
ephemeral bind first built its URL as `format!("http://{host}:{port}/...")`, which dig-node-core's
`banned_address_patterns` sweep correctly rejects: text concatenation loses the brackets every IPv6
literal needs. The URL is now formatted from the `SocketAddr` itself, whose Display brackets v6 and
leaves v4 alone, and the bind helper returns only the listener so the advertised address can no
longer disagree with the bound one.
The trusted-Chia-peer surface (#2870) is already shipped and no code was added: `control.chiaPeers.*`
(PR #248) and `dign chia-peers` (PR #45) already wire the user-facing surface to the existing
`user_managed` writer, and the help text already names the custody grant. No env var or config key
was added on purpose — that would be a second configuration path into write authority.
Closes #767
Closes #702
Closes #2880
Closes #2870
Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d
MichaelTaylor3dforce-pushed the loop/mvp-batch-767-702-2880-2870 branch from a095039 to a078386CompareAugust 21, 2026 05:11
@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

Gate-ready — head a0783868c0984ea7ef59cd70f22d610a8b0fb3cf, all required checks green, staying DRAFT

The red is cleared.dig-node-core::banned_address_patterns::no_source_file_builds_a_socket_address_from_concatenated_text now passes; Test + coverage is green in 19m28s having run the whole suite rather than bailing at 882/2154.

Concurrent-writer note. The branch was rewritten while this lane worked: my commit 0ec6b2a and the prior 64d354c were both replaced by a078386, which squashed the same fix into the feature commit. git diff 0ec6b2a a078386 is empty and both trees hash to 3c158c4, so nothing was lost and there is no content divergence. No second fix was applied.

Required checks, asserted BY NAME from branch protection (check-merge-preconditions.sh): Lint commit messages SUCCESS · Check version increment SUCCESS · Rustfmt SUCCESS · Clippy SUCCESS · Test + coverage SUCCESS. unresolvedReviewThreads=0, mergeStateStatus=CLEAN. The script reports BLOCKED for one reason only — draft=true — which is deliberate: the gate round has not run.

Also green, though not required: Analyze (rust/actions/javascript-typescript), CodeQL, Release-script tests, and all four package builds (.deb amd64 + arm64, .msi, .pkg).

Version. Workspace Cargo.toml is at 0.135.0 against main at v0.134.0 (8f2f416); there is no package.json, so there is nothing to disagree with. No second bump was added.


Off-path finding, logged not fixed (§2.6): a pre-existing test-isolation flake in dig-node-core

dig-node-core tests::an_eviction_advertises_after_the_victim_is_gone fails under full-suite concurrency on Windows:

panicked at crates/dig-node-core/src/lib.rs:6062:9:
the tier-0 capsule was the sacrifice

It passed 3/3 in isolation (cargo nextest run -p dig-node-core -E 'test(an_eviction_...)') and it passed on Linux CI in the green Test + coverage run above, so this is a concurrency artefact rather than a behavioural defect.

Not caused by this PR, and not in a crate this PR touches. The test arrived on main in 1db4d6e ("feat(cache): retract evicted capsules through dig_sex::holdings (#280)"); this PR's only code change is in crates/dig-node-service/src/open.rs.

Likely mechanism, for whoever picks it up:two_capsule_cache sets the process-global DIG_NODE_CACHE and writes the cap through set_cache_cap_bytes, and takes ENV_GUARD — which only serialises it against tests that also take that guard. A sibling test in the same binary that touches DIG_NODE_CACHE without the guard moves the cap out from under the sweep, and the sweep then evicts nothing. The fix belongs in the fixture's isolation, not in the eviction path.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

loop-security: pre-merge audit IN PROGRESS

Head audited: a0783868c0984ea7ef59cd70f22d610a8b0fb3cf (resolved from gh pr view 291 --json headRefOid).
Merge-base: 8f2f4160e0313917924ac9e9f94f84b45bb686ef. Single commit, author michael@michaeltaylor.dev (correct identity).

Diff read in full (11 files, +1258/-53). Areas under audit:

  1. server.rs CORS predicate — allow-set, reflection + credentials, preflight vs real-request parity, Vary, Control-vs-PublicRead tier separation.
  2. loopback.rs / open.rs — whether the loopback allocation is an authorization boundary anywhere, and whether the bind-guard is vacuous.
  3. service_control.rs / win_service.rs — who can raise a stop, what the new forced-stop branch leaves behind.
  4. Scope check: no chia-peer / trusted-peer file appears in this diff — #2870 looks absent from the delta. Verifying, and will report it as a scope observation rather than a security finding if confirmed.

Findings posted as they are formed. Verdict posted before my final summary.

@MichaelTaylor3dMichaelTaylor3d left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness GATE: PASS

Head reviewed: a0783868c0984ea7ef59cd70f22d610a8b0fb3cf (resolved from gh pr view 291 --json headRefOid). Combined delta gated against main (8f2f416), not the last commit, because the branch had more than one author.

Required checks asserted BY NAME at this head — Lint commit messages, Check version increment, Rustfmt, Clippy, Test + coverage: all SUCCESS. unresolvedReviewThreads=0, mergeStateStatus=CLEAN, blocked solely by draft=true (correct — the gate had not returned).

1. CORS (#702) — attacked the ALLOW list, no widening found

reflects_origin is is_local_origin(o) || (is_app_origin(o) && is_content_read(m, p)) (crates/dig-node-service/src/server.rs:349-354). Against the prior is_local_origin(o) || is_app_origin(o), that is a strict subset for every (origin, method, path) triple. Specifically:

  • No state-changing method reachable from a browser origin that could not reach it before. App origins lost POST reach; the local/extension family is unchanged.
  • allow_methods, allow_headers, expose_headers, allow_private_network(true) are all untouched by the diff.
  • No allow_credentials anywhere in the crate — grepped; the layer never sets it, so the reflected-origin + credentials combination does not arise.
  • The origin is matched against fixed sets (is_local_origin / is_app_origin + the operator APP_ORIGINS_ENV allowlist, both unmodified) and only then echoed — never blind-reflected.
  • Preflight judged on Access-Control-Request-Method (server.rs:377-389) with an absent declared method falling through to OPTIONS, i.e. closed for the app family. Preflight answer and real answer agree.
  • is_websocket_path matches the exact registered paths, not a prefix (server.rs:367-369), so a future /ws-foo route is not swept into the carve-out.

Authorization derives from the request's own Parts (method, path) plus the request's Origin header — no process-global or ambient flag participates in the decision.

Revert-proofs discriminate placement, not outcome. The two nearest wrong implementations fire different assertions: router-wide fails tests/server.rs:433 (POST /), route-only fails tests/server.rs:440 (POST /{method}), and the GET/POST pair on the one path /get_sync_status is what separates them. The chrome-extension:// control at tests/server.rs:466 fails if a fix tightened the method for every origin. This is a property test, not an outcome test.

2. Loopback rule (#767)

loopback.rs is a bind-side allocation, not an authorization predicate, so the "recognises 127.0.0.1 but not ::1" bypass class does not apply here — nothing in this diff gates trust on loopback-ness. The §5.2 tension is addressed explicitly rather than skipped (loopback.rs:33-41, SPEC.md §4.0a): ::1 is a single address so a per-service v6 allocation is not expressible, and v6 is a second listener rather than the DIG-owned address. No hostname is treated as equivalent to a loopback socket; the guard flags binds only and deliberately leaves the §5.3 dialling tiers (localhost) alone, with that non-flagging proven by a control at tests/loopback_bind_guard.rs:170-181.

open.rs:293-303 reads the address back off the bound listener and formats from the SocketAddr, so the advertised URL cannot disagree with the bind and cannot drop IPv6 brackets. Spot-checked the sweep claim: git grep 'format!("http://{<ident>}:{' over crates/**/*.rs at this head returns zero hits, and crates/dig-node-core/tests/banned_address_patterns.rs is untouched by the diff.

3. Stoppability (#2880) — reachable, not harness-only, with the vacuity stated

The primitive is exercised under genuine saturation (max_blocking_threads(1) + one parked task, service_control.rs:172-200), which is the only fixture that can fail against the old spawn_blocking(recv) bridge — semantics did not change, only the executor, so a non-starving fixture would have passed both ways.

Reachability: win_service.rs:76-160 is the shipped run-service path and is wired to the same primitive; it is not a test double. Deadlock: raising a stop is watch::Sender::send (service_control.rs:74-76) — takes no lock a handler holds and no blocking thread. Stop-during-startup is handled, and handled for the right reason: StopWaiter::wait checks *rx.borrow() before changed() (service_control.rs:106-112), because changed() only reports post-subscription changes — a bare changed() would wedge on exactly the stop an update tool sends first.

The honest gap, already disclosed in the PR body and not charged against it: run_service itself is a cfg(windows) bin-adjacent path with no test, so the wiring is reasoned, not reproduced on a wedged host. Under §2.6 that is the right call for an MVP batch, and the primitive it wires is proven.

4. Trusted chia peers (#2870) — verified already-shipped, and correctly so

control.rs is not in this diff. On main: dispatch at control.rs:911-913, declared at :208-210. Trust comes only from an explicit operator act — no peer can assert its own trust, and there is no second writer. The master-token gate is delegated to ControlMethod::requires_master_token in dig-node-control-interface rather than restated as string literals (control.rs:294-313), with a lockstep test; an unrecognised control.* name fails closed to master-only. §908 holds — nothing here signs. Declining to add $DIG_CHIA_TRUSTED_PEER is the right call and the reasoning is recorded: an env var is a second configuration path into a custody grant.

5. Cross-ticket bleed — none found

Each ticket's change is present and independent: #702 touches only the CORS predicate, #767 only bind sites and a new module, #2880 only the stop path, #2870 nothing. No change relaxes a check another relies on. dig-node-core has zero files in the diff, which confirms the an_eviction_advertises_after_the_victim_is_gone flake is genuinely pre-existing — it arrived on main in 1db4d6e (#280) and is untouched here. Not charged.

6. §2.5 / rustdoc

New public items (loopback::*, service_control::*) all carry doc comments that state the WHY, not the WHAT, and both modules are declared with doc comments in lib.rs:60-63 and lib.rs:92-95. Reads cleanly.


Two non-gating observations are posted inline and resolved by me so they cannot block merge. Neither changes the verdict.

Verdict: PASS. Not undrafted, not merged, nothing edited.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

Orchestrator note for the gate round — four ticket premises were WRONG, and the implementer said so

Recording this on the PR so both gate legs read it and so it survives the lane that found it. Three
of the four tickets stated a cause or a blocker that measurement contradicted.
That matters for the
gate: verifying this diff against the tickets' stated reasoning would confirm the wrong thing.

#767 — the fix it asks you to "confirm" was already GONE, and a test was holding it that way

open.rs:253 bound ("127.0.0.1", 0) and its test asserted starts_with("http://127.0.0.1:"). So the
test pinned the exact address the loopback rule forbids — the #745 fix had been reverted at some
point and no test could ever go red. Worse, anyone re-applying the fix would have broken that test and
might have "fixed" the test instead.

This is why the new tests/loopback_bind_guard.rs is a source sweep rather than a unit test, and
the sweep immediately beat a careful hand-sweep: it found two binds the manual pass missed at
crates/dig-wallet/src/sage/transport.rs:276-277. Treat the per-repo enumerations in #767 as
probably incomplete everywhere for the same reason.

Gate should check: the guard strips #[cfg(test)] and distinguishes binds from dials (a dial to
loopback is legitimate; a bind is not). Also note the implementer's own stated limitation — the guard
is line-scoped, so a cargo fmt that splits a bind across lines evades it. Documented, low
practical risk, deliberately unfixed.

#702 — its stated blocker did not exist

The ticket claimed the router needed restructuring. It did not: AllowOrigin::predicate receives
&request::Parts, so a per-request predicate was enough (server.rs:194-207, policy fns
:324-374).

#2880 — NEITHER of the two causes the ticket names was the cause

The service does reach RUNNING with a STOP mask (win_service.rs:106). The real defect:
spawn_blocking(move || shutdown_rx.recv()) put the stop wait on tokio's blocking pool — the same
pool the wallet replica's SQLite work uses.
A saturated pool means the accepted stop is never
observed. Fixed with a watch signal plus a 20s bounded shutdown in a new service_control.rs.

That also answers scope 4: the replica freeze and the service wedge are the same blocked resource,
not two bugs. Scopes 2 and 3 are dig-updater and are NOT done here.

#2870 — no code was needed; it had already shipped

control.rs:208-210,263-265,911-913 (PR #248) and entrypoint.rs:181-184,466-485,775-779 (PR #45)
already write the existing user_managed row, and the custody wording is already in --help. Another
instance of the measured pattern where a ticket stays open long after the work lands.

The revert-proof set is stronger than usual — one entry is worth copying

Five mutations, each read for which assertion fired, not merely that something failed. The pair
worth noting:

MutationAssertion
CORS → router-widetests/server.rs:433 (POST /)
CORS → route-onlytests/server.rs:440 (POST /{method}) — a different assertion

Two mutations in opposite directions firing two different assertions is what proves the fixture
discriminates placement, not just presence. A single mutation firing a single assertion cannot.

Two follow-ups the gate should NOT charge against this PR

  1. The new guard is a rival of dig-node-core/tests/banned_address_patterns.rs — same family
    (source-sweep address hygiene), duplicated crates_root / rust_sources / scan-floor
    infrastructure. They could not be merged here because dig-node-core sits below
    dig-node-service in the crate hierarchy.
  2. The loopback address belongs in dig-constants (L00).dig-wallet structurally cannot
    reference an SSOT living in dig-node-service — which is precisely why the literal keeps getting
    re-derived. Cross-repo, release-first, NC-5.

Both are tracked together, since they are one problem: the loopback literal has no home low enough for
every consumer to reach.

Also confirmed, so the gate need not redo it

Nothing was hidden behind the banned_address_patterns fail-fast bail. CI's own runner re-run locally:
cargo nextest run --workspace --locked --retries 2 --no-fail-fast2146/2146 passed. The three
flakes are pre-existing dig-node-service::server env-serialisation ones, green on retry, and the
an_eviction_advertises_after_the_victim_is_gone Windows-concurrency flake arrived on main in
1db4d6e (#280) and is untouched here.

@MichaelTaylor3dMichaelTaylor3d left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two non-gating notes, resolved by me. Verdict is in the separate PASS review.

Comment threadcrates/dig-node-service/tests/loopback_bind_guard.rs
Comment threadSPEC.md
@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

SEC-1 (GATING, MEDIUM) — the forced-stop branch tells the SCM Stopped and then the process never exits

crates/dig-node-service/src/win_service.rs:156-163 (head a078386)

The claim the code makes

  • win_service.rs:152-153 logs "reporting Stopped anyway so the service manager can always stop this service".
  • win_service.rs:161-162 comments "the process itself exits cleanly rather than double-reporting".
  • SPEC.md §9.0 (added by this PR) states the service "MUST reach a stopped state, whatever the state of its internals".

What actually happens

rt (win_service.rs:127) is a local, so it is dropped when run_service returns at :163 — i.e. afterset_service_status(Stopped, ...) at :156. In tokio 1.53.0 (the pinned version), Runtime's drop closes the scheduler and then drops BlockingPool, whose Drop calls shutdown(None)shutdown_rx.wait(None) → an untimedblock_on:

  • tokio-1.53.0/src/runtime/blocking/pool.rs:282-286impl Drop for BlockingPool { fn drop(&mut self) { self.shutdown(None) } }
  • tokio-1.53.0/src/runtime/blocking/pool.rs:263if self.shutdown_rx.wait(timeout)
  • tokio-1.53.0/src/runtime/blocking/shutdown.rs:64-67Nonee.block_on(&mut self.rx), no timeout

A wedged spawn_blocking closure holds a shutdown_tx clone forever, so that oneshot never completes.

Measured, not reasoned

Standalone probe on tokio 1.53.0, reproducing this shape (multi-thread runtime, max_blocking_threads(1), one closure wedged, watch-based stop, then drop(rt)):

stop observed on saturated blocking pool: true <-- the fix works
reporting Stopped to the SCM (simulated), then dropping the runtime...
runtime drop STILL BLOCKED after 8.032948s -- run_service() never returns,
so the process never exits even though the SCM was told Stopped.
CONTROL: after releasing the wedged closure, drop returned in 50.1909ms
=> the block is the wedged blocking thread, nothing else.

The control matters: releasing the wedged closure makes the drop return in 50 ms, so the block is that thread and nothing else. Note the probe also confirms the watch signal half of the fix genuinely works — the stop IS observed on a saturated pool. The gap is strictly what happens after the deadline fires.

Exploitation / impact path

State: the exact wedge the module documents — a frozen wallet-replica synchronous DB call occupying the blocking pool (service_control.rs:16-24 describes this as the measured cause of the 1061 incident).

  1. SCM sends Stop. Handler raises the watch signal promptly. Correct.
  2. run_until_stopped returns (None, Forced) after GRACEFUL_STOP_DEADLINE (20s). Correct.
  3. set_service_status(Stopped, exit=1)the SCM and sc stop now report success.
  4. run_service returns → rt dropped → blocked forever. The process stays alive.

Consequences, in decreasing order of how well I can evidence them:

  • MEASURED-adjacent (mechanism proven above): a privileged action (sc stop) is reported as having taken effect when it has not. The process is still resident.
  • REASONED (Windows image-lock semantics, not executed here): a running .exe cannot be overwritten in place, so dig-updater replacing dig-node.exe still fails — which is the end-to-end failure #2880 exists to unblock (service_control.rs:8-12 cites dig-updater run looping on Deferred). The node therefore remains pinned on its current version, and this repo's update channel is the security-patch delivery path. Whether the update actually still fails depends on whether dig-updater overwrites or rename-then-writes; I did not verify dig-updater, so I am not claiming it as measured.
  • REASONED (SCM restart behaviour, not executed here): with the service marked Stopped, a subsequent StartService launches a second dig-node process. The first has already had its listeners cancelled by the scheduler shutdown, so no port conflict makes the collision visible. Two processes then share %PROGRAMDATA%\DigNode and the wallet replica DB. The cache/config path is protected (dig-node-core/src/lib.rs:619-639 flock + atomic temp/rename), so I am not claiming config corruption; the wallet replica is the unprotected one, and it is by hypothesis the component that was wedged.

Note this is not a regression against main — on mainblock_on never returns at all, so Stopped is never reported. But main cannot reach the new SCM-says-stopped-while-alive state, and the PR's own stated outcome is not achieved in the only scenario it targets.

Fix (one line, in the Forced branch only)

After set_service_status(Stopped, ...) at :156-160, do not fall through to an untimed drop. Either:

if outcome == StopOutcome::Forced{// The SCM has been told Stopped; the image lock is what blocks the updater, so leave.
rt.shutdown_background();// consumes the runtime, returns immediately
std::process::exit(exit asi32);}

or rt.shutdown_timeout(Duration::from_secs(1)) followed by std::process::exit. shutdown_timeout(Duration::ZERO) is also sufficient — shutdown.rs:40-42 early-returns on a zero timeout. Keep the graceful path exactly as it is; only the Forced path needs to stop waiting on a thread that by definition will not return.

A revert-proof for this is expressible without Windows: assert that a Forced outcome is followed by a bounded teardown, using the same saturated-pool fixture service_control.rs:178-218 already builds.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

SEC-2 (LOW, defense-in-depth — do NOT gate) — a GET-approved preflight still seeds the browser's CORS-preflight cache with POST, so an app origin can SEND a JSON-RPC POST it cannot read

crates/dig-node-service/src/server.rs:198-208 and the SPEC sentence at SPEC.md §4.6 ("so the preflight answer matches the answer the real request will receive").

The gap

effective_method (server.rs:374-384) correctly makes the Access-Control-Allow-Origin answer method-aware. But allow_methods is still the static router-wide list at server.rs:208:

.allow_methods([Method::GET,Method::POST,Method::OPTIONS])

and tower-http 0.6.11 emits it on every preflight, independent of the allow-origin verdict (tower-http-0.6.11/src/cors/mod.rs:678-681allow_methods.to_header runs inside the parts.method == Method::OPTIONS branch with no reference to the origin future). So an app-origin preflight that declares GET is approved and answers:

Access-Control-Allow-Origin: tauri://localhost
Access-Control-Allow-Methods: GET, POST, OPTIONS
Access-Control-Allow-Headers: content-type, x-dig-control-token

Per Fetch's CORS-preflight fetch, every method in Access-Control-Allow-Methods and every name in Access-Control-Allow-Headers becomes a cache entry for that (origin, url). A later request whose method has a cache entry and whose non-safelisted headers all have entries skips the preflight entirely.

Concrete chain

  1. Content in a desktop-app webview at tauri://localhost issues fetch(nodeUrl, {method:'GET', headers:{'content-type':'application/json'}}). Non-safelisted content-type ⇒ preflight with ACRM: GETapproved (correctly — it is a content read).
  2. The response seeds method entries GET/POST/OPTIONS and header entries content-type/x-dig-control-token for that URL.
  3. fetch(nodeUrl, {method:'POST', headers:{'content-type':'application/json'}, body:'{"jsonrpc":"2.0",...}'}) now hits the cache ⇒ no preflight ⇒ the POST is sent and the node executes it.

Why this is LOW and not gating

  • The response carries noACAO (reflects_origin denies app-origin POST), so the page cannot read it. #693's wallet-read exposure is genuinely closed — reads require reading.
  • The durable side effect on POST / dig.getContent/fetchRange is already gated independently: provenance_for (server.rs:964-968) folds a Sec-Fetch-Site: cross-site request's landing to Peer, and Sec-Fetch-Site is a forbidden header name a page cannot forge.
  • Every mutation and every control.* stays token-gated, and control.chiaPeers.*/pairing require the master token (control.rs:300-320).
  • No allow_credentials is set anywhere in the layer, so no cookie/credential is attached.
  • The app-origin family has no in-ecosystem consumer today: dig-app is a native Rust/egui app that reaches the node from Rust (dig-app-core/src/confirm/gui/window/pane/home.rs:550, agent.rs:197), so it sends no Origin at all and CORS never applies to it.

So the residual is: a compromised desktop-app webview gains a blind POST-firing primitive whose response it cannot read. Real, bounded, not a live vulnerability.

Cheap fix if you want it closed in this PR

AllowMethods has no predicate constructor (allow_methods.rs:27,36,47,66any/exact/list/mirror_request only), but mirror_request() is exactly right here and is a one-line change:

.allow_methods(AllowMethods::mirror_request())

A GET-declared preflight then answers Access-Control-Allow-Methods: GET and seeds only GET; a POST-declared preflight is already refused at the origin check, so nothing is cached for it. The local web/extension family is unaffected because its preflights declare the method they will actually use.

Separately: tighten the SPEC sentence

The added clause claims the preflight answer matches the real answer. That holds for ACAO only. Either narrow the sentence to Access-Control-Allow-Origin, or apply the mirror_request fix so the claim becomes true as written. A normative sentence that is true of one header and read as true of the response is the kind of drift the next reader acts on.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

loop-security VERDICT: CHANGES-REQUIRED

Head audited: a0783868c0984ea7ef59cd70f22d610a8b0fb3cf (re-resolved from gh pr view 291 --json headRefOid after the audit; unchanged throughout). Merge-base 8f2f4160e0313917924ac9e9f94f84b45bb686ef. Single commit, author michael@michaeltaylor.dev.

Exactly one finding gates: SEC-1. Everything else is clean or explicitly non-gating.

#SeverityGates?What
SEC-1MEDIUMYESForced-stop tells the SCM Stopped, then Runtime::drop blocks forever on the wedged blocking thread, so the process never exits. Measured, with a control. One-line fix.
SEC-2LOWnoA GET-approved preflight still seeds the browser preflight cache with POST, so an app origin can SEND a JSON-RPC POST it cannot read. Bounded by the Sec-Fetch-Site landing-fold, the token gates, and no allow_credentials. mirror_request() closes it.
SEC-3INFOnoSee below - the #767 origin move and the cookie jar.

I did not soften anything on the assumption the correctness gate covers it, and I did not coordinate with it.


SEC-3 (informational, no action required)

open.rs:293-296 moves the untrusted-blob ephemeral server from 127.0.0.1:<eph> to 127.0.0.2:<eph>. The doc at open.rs:373-378 claims the served page "shares no origin with anything else the user happens to be running". That is true of the origin (port is part of it). It is not true of the cookie jar - cookies ignore port, so the blob now shares cookie host 127.0.0.2 with the node's own bare-IP content surface (config.dig_local_addr(), bound at server.rs:2151-2156), where before it shared 127.0.0.1 with third-party local dev servers.

This is not a finding and needs no follow-up, because it grants strictly LESS than what already exists: every capsule served from dig.local / 127.0.0.2 already shares ONE FULL ORIGIN with every other capsule on that surface, which is total DOM and localStorage access, not merely cookies. The move also removes the shared cookie host with arbitrary third-party local servers, which is a net improvement. Recorded only so the doc sentence is not later read as an isolation guarantee it does not make.


Per ticket: does it introduce a security regression?

#767 - hold the DIG loopback rule: NO

loopback.rs is a bind-address SSOT (constants plus an ordered candidate list); it introduces NO authorization predicate, so no loopback-as-authz boundary was created or moved.

I separately confirmed the pre-existing loopback authz boundary is sound and family-complete, and that this diff does not touch it: the decision is taken from the socket peer address via ConnectInfo<SocketAddr> (server.rs:983), never from a header, through the shared is_loopback_addr (config.rs:430-437) which covers all of 127.0.0.0/8, ::1, and the v4-mapped ::ffff:127.0.0.1 form (read_origin_for, server.rs:893-904; requestor_for, :911-917). 0.0.0.0 and [::] do NOT satisfy it, and a non-loopback DIG_NODE_HOST is refused at startup unless DIG_NODE_ALLOW_REMOTE=1 (config.rs:453-461, fail-closed). A hostname that merely RESOLVES to loopback gets no privilege: host_is_allowed (config.rs:475-518) is EXACT matching after a port strip - no suffix or substring match - so the DNS-rebinding vector stays closed.

Reachability of 127.0.0.2 is identical to 127.0.0.1 (all of 127/8 is loopback on both target OSes). The fall-back log line carries fixed constants only, so no log injection. The new bind path is an ephemeral, non-blocking, deadline-bounded one-shot (open.rs:309-329) whose response headers are already CRLF-sanitized (open.rs:349-366) and whose URL is handed to an ARGV launcher with no shell (open.rs:162-199 uses rundll32 url.dll,FileProtocolHandler, explicitly not cmd /c start).

Guard vacuity: NOT vacuous. crate_src_dirs() resolves to crates/*/src and all five workspace members live there (root Cargo.toml:19-25), so the scan covers the whole workspace; files_scanned > 20 is a real haystack assertion; and every_banned_host_is_detectable_and_the_controls_are_not_flagged proves the pattern list can fire, with two honest negative controls. The two DECLARED_EXCEPTIONS are named with reasons and use narrow path-suffix matching.

#702 - scope CORS per route+method: NO

The change is STRICTLY NARROWING on every input: reflects_origin is is_local_origin(o) OR (is_app_origin(o) AND is_content_read(m, p)) at server.rs:334-338, versus the previous is_local_origin(o) OR is_app_origin(o). No origin is reflected that was not reflected before, and no state-changing method became reachable from any browser origin that could not reach it before. Specifics checked:

  • No reflection-without-allowlist. Both families are fixed sets: BUILTIN_APP_ORIGINS plus an exact-match operator env list (server.rs:398-407), and is_local_origin (:413-426) which delegates to the exact-match host_is_allowed. Nothing echoes an arbitrary origin.
  • No credentials.allow_credentials is never set on this layer, so the reflected-origin-plus-credentials same-origin-bypass class does not apply.
  • The preflight is not more permissive than the handler for Access-Control-Allow-Origin: effective_method (:374-384) judges Access-Control-Request-Method, and every failure mode fails CLOSED - absent header, non-UTF-8, unparseable method, and even a lowercase get all fall to OPTIONS, which is not a content read. The one place the preflight IS looser than the handler is Access-Control-Allow-Methods, which is SEC-2 and does not leak the response.
  • Vary is correct. Not overridden, so the tower-http default applies and is emitted on EVERY response, preflight or not (tower-http-0.6.11/src/cors/mod.rs:673; vary.rs:41-43 gives origin, access-control-request-method, access-control-request-headers). The response now also depends on method and path, both already part of any HTTP cache key. No shared-cache cross-origin serve.
  • Tier separation holds.control.* is reachable only over POST / and /ws; app origins are now denied on POST outright, so the control plane browser reachability NARROWED. /ws and /ws/status are additionally excluded from the app family by exact-path match (:359-361), and the socket own-Origin check is untouched - the four server.rs hunks in this diff are the layer, the predicates, the test imports and one test line; nothing near ws_wallet, host_guard or control_ingress_admits was modified.
  • No secret is GET-reachable. I checked health, version, openrpc, well_known and verify_ledger for token, seed and key material: none.
  • Compat. The narrowing revokes the POST reach that #693 documented for the app family. I could find no in-ecosystem consumer of it - dig-app is a native Rust/egui app that reaches the node from Rust (dig-app-core/src/confirm/gui/window/pane/home.rs:550, agent.rs:197) and therefore sends no Origin at all - so I do not believe an operator control is being silently broken. Flagging it explicitly since revoking a documented capability is reportable on its own; whether the #669 contract is fully preserved is the correctness gate call, not mine.

#2880 - keep the service stoppable: YES, see SEC-1

SEC-1 is the only defect. Everything else here is clean:

  • Who can trigger a stop: only the Windows SCM control handler (win_service.rs:88-101), which requires SERVICE_STOP access. StopSignal has NO other call site anywhere in the workspace, and there is no control.* method that stops, shuts down, restarts or exits. No peer-facing or PublicRead path reaches it. No new stop trigger, and no unauthenticated remote shutdown.
  • No lock held across an await, no self-deadlock.request() is let _ = watch::Sender::send - cannot block, cannot fail, and is correctly idempotent for a Fn handler the SCM may invoke twice. wait() checks *borrow() BEFORE changed() (service_control.rs:699-706), which is the right fix for a stop arriving during start-up; a bare changed() there would itself have been a hang. The removal of spawn_blocking(recv) is complete - no such pattern remains in dig-node-service outside the test fixture. My probe independently confirms the watch signal IS observed on a fully saturated blocking pool, so the core of the fix works.
  • Half-written state on a forced drop: largely mitigated, and I am not raising it. Config and cache writes go through flock plus atomic temp/rename (dig-node-core/src/lib.rs:619-639, :653-660), and a wedged blocking write is never interrupted - the runtime waits for it, which is precisely the cause of SEC-1.
  • Linux/systemd is not covered by run_until_stopped (serve() at server.rs:2019 still uses the bare shutdown_signal()), which I am NOT raising as a security finding: systemd escalates SIGTERM to SIGKILL after TimeoutStopSec, so the OS guarantees stoppability there. The Windows SCM has no equivalent, which is why Windows needed this.
  • No product-line unwrap, expect or panic! is introduced anywhere in the diff, and there is no partial_cmp(..).unwrap_or(Equal) and no a - b > c boundary form.

#2870 - trusted chia peers: NO, because there is no code for it in this delta

Verified: zero chia or trusted-peer files in the diff, and the only occurrences of trusted in the whole delta are the word "untrusted" in two unrelated doc comments. Nothing in this diff moves a key, a signing capability, a spend decision or a trust set, so the section-908 boundary and NC-12 are untouched.

But this PR carries Closes #2870, so merging closes a custody-adjacent ticket that no security gate examined. I therefore did a BOUNDED read of the already-shipped implementation so the close is not blind, and the answer is good:

  • Trust originates only from control.chiaPeers.add, and that method requires the MASTER control token, never a paired one (control.rs:263-265 plus the tier predicate at :296-320). A peer cannot assert, imply or negotiate its own trust, and a paired app cannot grant it.
  • The tier predicate DELEGATES to the contract crate (ControlMethod::requires_master_token) rather than restating a string list locally, with a lockstep test. The doc at control.rs:302-315 records that the earlier duplicated string list had already drifted and failed OPEN once, letting a paired token install an unrevocable peer. That rival implementation is gone, which is the right resolution rather than a corrected duplicate.
  • An unrecognised control.* name FAILS CLOSED to master-only (control.rs:317-320), so the next method added is not paired-reachable by default.
  • Source-kind trust is explicit and asymmetric: LocalNode gives trusted, Custom gives untrusted (dig-node-core/src/seams/chia_peer/light_client.rs:120-122).

This is a bounded read of the trust ORIGIN, not a full audit of that subsystem - I did not examine what a trusted peer claims are then permitted to do. If you want the "what does trust buy" half audited, that is a separate ticket against already-shipped code and should not block this PR.


What must change to flip this to PASS

SEC-1 only. In the Forced branch of win_service.rs, after set_service_status(Stopped, ...), do not fall through to an untimed Runtime drop - use rt.shutdown_background() then std::process::exit(exit as i32), or rt.shutdown_timeout(Duration::ZERO) then exit. Leave the graceful path exactly as it is. A revert-proof is expressible on any host using the same saturated-pool fixture service_control.rs:178-218 already builds, so this does not need a Windows runner.

SEC-2 and SEC-3 do NOT need to change for my PASS. SEC-2 is worth the one-line mirror_request() while the file is open (cheaper to fix than to file), plus narrowing the new SPEC sentence to name Access-Control-Allow-Origin specifically; if you would rather ticket it, that is a legitimate outcome and I will not re-gate on it.

I ran no mutating git command in any shared checkout. The only build I performed was a standalone cargo project under the session scratchpad (scratchpad/rtprobe), which touches no repo.

…e drop
SEC-1 (gating): the forced-stop branch reported `Stopped` to the SCM and then
returned, dropping the tokio runtime. `Runtime::drop` joins the blocking pool
with no timeout, and a forced stop is by definition the case where a blocking
task never finished, so the drop blocked forever AFTER the SCM had been told
the service was stopped. Measured on the pinned tokio 1.53.0: still blocked
after 8.03s, then 50.19ms once the wedged closure was released.
That is a privileged action reporting success without taking effect: `sc stop`
succeeds, the service is marked stopped, and the process stays alive holding
its binary image locked - the exact symptom dig_ecosystem#2880 exists to
remove. It also leaves an SCM-stopped-but-alive state from which a
`StartService` yields a second process against the same wallet replica.
A forced stop now abandons the pool (`shutdown_background`) and exits. The
graceful branch, the watch-based stop signal and the 20s deadline are
unchanged.
SEC-2 (low): `Access-Control-Allow-Methods` now mirrors the requested method.
tower-http emits it on every answered preflight independent of the origin
verdict, so a static [GET, POST, OPTIONS] answered an approved app-origin GET
preflight by also advertising POST, seeding the browser preflight cache with a
POST entry a later `POST /` could use to skip its preflight.
SEC-3: narrow two overclaiming sentences. The sandboxed-blob comment claimed
origin isolation from "anything else"; cookies ignore port, so the blob shares
the 127.0.0.2 cookie jar with the bare-IP content surface. The SPEC's
app-origin scoping sentence is true of Access-Control-Allow-Origin, not of the
whole response.
Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

Security findings addressed — 2f08d9b (was a078386)

SEC-1 (gating) — fixed.service_control::release_runtime now chooses the teardown the
outcome permits: a graceful stop drops the runtime normally, a Forced stop calls
shutdown_background() and win_service.rs then leaves via std::process::exit(exit). The
graceful branch, the watch-based stop signal and the 20s deadline are untouched.

The helper lives in the platform-neutral service_control.rs, not in win_service.rs, because
that module is cfg(windows) and this repo's ci.yml runs ubuntu-latest only — a test placed
there would have been compiled out of every gate.

Revert-proof (no Windows runner needed). The pre-existing saturated-pool fixture cannot see
this defect: it asserts a returned status, and both the correct and incorrect versions return
the same one. The distinguishing property is whether the teardown call ever comes back, so the new
test runs it on its own thread with the wedge still in force and asserts it completed.

Mutating the Forced arm back to drop(rt):

test service_control::tests::a_forced_stop_releases_the_runtime_without_joining_the_wedged_pool ... FAILED
panicked at crates\dig-node-service\src\service_control.rs:425:9:
releasing the runtime after a forced stop must not join the blocking pool: the SCM has already
been told `Stopped`, so a teardown that blocks leaves a service reported as stopped while the
process is still alive holding its binary image locked

Restored: 6/6 in service_control, 394/394 lib. cargo fmt --check and
clippy --workspace --all-targets -D warnings both clean.

SEC-2 (low) — fixed, one line..allow_methods(AllowMethods::mirror_request()) replaces the
static [GET, POST, OPTIONS], so the advertised method equals the one the origin predicate
actually judged and an approved app-origin GET preflight no longer seeds a POST entry in the
browser's preflight cache.

Corrected sentence 1 (SPEC, SEC-2). The app-origin scoping bullet now says what it governs:

This scoping governs Access-Control-Allow-Origin specifically, not the whole response: the
remaining Access-Control-* headers are emitted by the CORS layer on any preflight it answers,
independent of the origin verdict.

The "Allowed methods" line became a normative mirror requirement stating why a static set is wrong.

Corrected sentence 2 (SEC-3, open.rs), wording only. The old claim that the blob "shares no
origin with anything else the user happens to be running" is now:

This is ORIGIN isolation and nothing wider — in particular NOT cookie isolation, because cookies
ignore port and are keyed by host: the blob shares the 127.0.0.2 cookie jar with the bare-IP
content surface. Strictly less reach than the shared origin it replaces, so read the sentence
above as the separation it names, never as a general sandbox guarantee.

Not done, deliberately: no change to control.rs (not in this delta), no bind-guard widening,
no KNOWN_VIOLATIONS entry, no version change (workspace already 0.135.0 vs main's v0.134.0).
Closes #2870 stands. Staying DRAFT for a security-only re-gate.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

Orchestrator verification of the SEC-1/SEC-2 delta — a078386 -> 2f08d9b

Scoped re-gate per §1.10 (re-gate only the leg the fix touches), verified against the diff rather than by
re-running the full audit. The delta is 5 files / +128 / -8 and is exactly the remedy the security gate
prescribed.

SEC-1 — correct, and the test is where it can actually run.
release_runtime (service_control.rs:183-187) splits by outcome: Graceful => drop(rt) unchanged,
Forced => rt.shutdown_background(). win_service.rs then leaves via std::process::exit(exit as i32)
on Forced only. The watch signal, the graceful path and the 20s deadline are untouched.

The revert-proof lives in service_control.rs under #[cfg(test)], not in win_service.rs. That
matters: win_service.rs is cfg(windows) and this repo's ci.yml is ubuntu-only, so a test placed
there would have been compiled out of every gate — green, having never existed. Same class as the
target-gated-dependency blind spot that let a custody backend ship across five releases uncompiled.

The fixture also earns its keep for a reason worth recording: the pre-existing saturated-pool test
asserts a returned status, which the buggy and fixed versions return identically. The distinguishing
property is whether the teardown call ever comes back, so the new test runs it on its own thread with
the wedge still in force and asserts completion. A fix whose observable difference is "does this call
return" cannot be pinned by asserting a return value.

SEC-2 — correct, and I checked the one thing that could have made it a widening.
.allow_methods(AllowMethods::mirror_request()) echoes the requested method instead of declaring a
static [GET, POST, OPTIONS]. Mirroring is what stops a legitimately-approved GET preflight from also
advertising POST and seeding the browser's preflight cache with an entry that lets a later POST /
skip its preflight.

But mirroring answers with whatever is asked, so for an approved origin it could in principle
advertise DELETE or PUT. I enumerated the router: every route is get or post
/, /health, /version, /openrpc.json, /.well-known/dig-node.json, /ws, /ws/status,
/:method, /s/*path, /verify/*path (server.rs:242-273). No PUT/DELETE/PATCH route exists, so
the reachable method set for an approved origin is unchanged and axum answers anything else with 405.
Strictly narrowing in practice.

Latent coupling this creates, stated because it is invisible at the point it would bite:mirror_request()
is safe here because the router has no other methods. Adding a put/delete/patch route later
would expose it to every approved origin with no separate decision
— the CORS layer would advertise it
automatically. A static list would have failed closed on that; mirroring fails open. Anyone adding a
state-changing method to this router must revisit allow_methods in the same change.

Preconditions at 2f08d9b: five required contexts present and SUCCESS by name
(Lint commit messages, Check version increment, Rustfmt, Clippy, Test + coverage),
unresolvedReviewThreads=0, mergeStateStatus=CLEAN. Version stays 0.135.0 vs main's v0.134.0 — no
double bump. control.rs, the bind guard and KNOWN_VIOLATIONS untouched, as instructed.

One follow-up fact, not a defect: the Forced exit code now reaches the OS as well as the SCM, so a
forced stop exits non-zero. Nothing consumes that today, but dig-updater reading a process exit code
would see 1 on a forced stop — worth knowing before anyone wires update logic to it.

@MichaelTaylor3d
MichaelTaylor3d marked this pull request as ready for review August 21, 2026 06:59
@MichaelTaylor3d
MichaelTaylor3d merged commit 8d82aca into mainAug 21, 2026
15 checks passed
@MichaelTaylor3d
MichaelTaylor3d deleted the loop/mvp-batch-767-702-2880-2870 branch August 21, 2026 06:59
@MichaelTaylor3d
MichaelTaylor3d restored the loop/mvp-batch-767-702-2880-2870 branch August 22, 2026 17:11
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@MichaelTaylor3d@Frt682