Skip to content

fix: export per-site signing key instead of legacy slot#2

Merged
sanity merged 2 commits intomainfrom
fix-export-signing-key-prefix
Apr 12, 2026
Merged

fix: export per-site signing key instead of legacy slot#2
sanity merged 2 commits intomainfrom
fix-export-signing-key-prefix

Conversation

@sanity
Copy link
Copy Markdown
Contributor

@sanity sanity commented Apr 12, 2026

Problem

Users reported that edits to imported sites don't persist after refresh — including on the same node after export → remove → re-import. The imported site was readable but every edit silently reverted on refresh.

Root cause

DelegateRequest::GetSigningKey carried no prefix, so the delegate handler called load_signing_key(ctx, None). With None, load_signing_key skips per-prefix storage and reads only the legacy delta:signing_key slot. When any legacy key was present (leftover from V1–V4 or from another site migration), export silently returned that unrelated key paired with the current site's owner_pubkey. The resulting token's signing_key and owner_pubkey were mismatched, so every signed page/config/deletion on the importing side failed the contract signature check on the network and UPDATEs were dropped. Optimistic local updates hid the failure until the next refresh fetched the unchanged network state.

This explains all three Matrix bug reports:

  • Export from node A → import on node B → B's edits don't stick (wrong key in token)
  • Export → remove → re-import → viewable but not updatable (same mechanism)
  • Same-node re-import wipes the correct per-prefix key with the wrong legacy one

Approach

  1. Add prefix: Option<String> to DelegateRequest::GetSigningKey and pass it through to load_signing_key, which already prefers the per-prefix slot.
  2. request_export passes the currently-selected site's prefix.
  3. Defence in depth: handle_signing_key_response verifies the returned key's public key matches the site's owner_pubkey before emitting a token. If they don't match, we show an explicit error instead of silently producing a broken token. Turns any future regression of this bug into a loud failure.
  4. Gate the export response handler on SHOW_EXPORT, so the legacy-migration code path (which still issues GetSigningKey { prefix: None } to probe pre-V7 delegates) can't spuriously populate export state.
  5. Legacy migration keeps prefix: None — that's the correct probe against pre-V7 delegates where the legacy slot is the only place a key can live.

Testing

  • New CBOR round-trip regression test (get_signing_key_request_roundtrips_with_prefix) guarding the wire format so a future refactor can't silently drop the prefix again.
  • cargo test -p delta-core — 18/18 passing.
  • cargo fmt + cargo clippy --workspace --all-targets clean.
  • ./scripts/check-migration.sh confirms the V7 migration entry was captured before the delegate code change.

Migration

Adds legacy_delegates.toml entry V7 captured before the delegate code change via ./scripts/add-migration.sh, so users whose signing keys live in the pre-V7 delegate still migrate to the new delegate on next load. Per AGENTS.md delegate upgrade workflow.

Why CI didn't catch this

There are no integration tests exercising the real delegate through the UI's export/import round trip — the existing tests cover state signing and CBOR round-trips in delta-core, but the delegate's storage behavior under prefix: None vs Some(_) was untested. The new CBOR round-trip test narrows this gap for GetSigningKey specifically; a broader follow-up would exercise the delegate binary end-to-end.

Closes the edit-after-import bug class reported on Matrix.

[AI-assisted - Claude]

sanity added 2 commits April 12, 2026 12:12
## Problem

Users reported that edits to imported sites don't persist after
refresh. Reproducible on the same node by: create site → export →
remove → re-import → edit → refresh. Only the original (pre-export)
copy accepted edits.

## Root cause

`DelegateRequest::GetSigningKey` had no `prefix` field, so the delegate
handler called `load_signing_key(ctx, None)`. With `None`,
`load_signing_key` skips the per-prefix slot entirely and only reads
the legacy `delta:signing_key` single-key slot. When a legacy key
happened to be present (left over from V1–V4 delegates or another
site), export silently returned that unrelated key paired with the
current site's `owner_pubkey`. The resulting token contained a
mismatched signing_key / owner_pubkey pair, so after import every
signed page failed contract validation on the network and UPDATEs were
silently rejected. Optimistic in-memory updates made the bug look
intermittent until the next refresh fetched the unchanged network
state.

## Fix

- Add `prefix: Option<String>` to `DelegateRequest::GetSigningKey`.
- Delegate handler passes it through to `load_signing_key`, which
  already prefers the per-prefix slot over the legacy fallback.
- `request_export` passes the currently-selected site's prefix.
- Defence in depth: `handle_signing_key_response` verifies that the
  returned key's public key matches the site's `owner_pubkey` before
  producing a token, and refuses with an error if it doesn't. This
  turns the silent corruption into a loud failure if the bug ever
  regresses.
- Only treat the `SigningKey` response as an export result when the
  export modal is open, so the legacy-migration code path (which also
  issues `GetSigningKey { prefix: None }` against old delegates) can't
  spuriously populate the export error.
- Legacy migration still uses `prefix: None` intentionally — it's
  probing pre-V7 delegates where the legacy slot is the only place a
  key can live.

## Testing

- Added `get_signing_key_request_roundtrips_with_prefix` CBOR
  round-trip test guarding the wire format so a future refactor
  can't silently drop the prefix again.
- `cargo test -p delta-core` — 18/18 passing.
- `cargo clippy --workspace --all-targets` clean.

## Migration

Adds legacy_delegates.toml entry V7 (captured before the delegate
code change via `./scripts/add-migration.sh`) so users whose signing
keys live in the pre-V7 delegate can still migrate them to the new
delegate on next load.

[AI-assisted - Claude]
- Add new `GetSigningKeyForPrefix { prefix: String }` variant instead of
  modifying the existing `GetSigningKey` unit variant. Modifying the unit
  variant would have broken CBOR wire compatibility with pre-V7 delegates,
  silently killing the legacy-migration path that rescues signing keys
  from older delegate versions. `GetSigningKey` stays as a unit variant
  for legacy probes; `GetSigningKeyForPrefix` is the V7+ request for the
  per-site signing key used during export.

- Correlate export responses to requests via a new `PENDING_EXPORT`
  signal holding a snapshot of the target site's prefix/name/owner_pubkey.
  Previously, a `SigningKey` response from a concurrent legacy-migration
  probe or an arrival after the user switched sites could be paired with
  the wrong site's `owner_pubkey`, producing a spurious "delegate returned
  a signing key that does not match this site's owner" error even though
  the user's real request hadn't returned yet.

- Reject `SiteRole::Visitor` up front in `request_export` so users get a
  clear "only owned sites can be exported" message instead of the
  defence-in-depth mismatch error.

- Eliminate the silent-hang path when `current_site()` returns None on
  response: the response handler is now entirely driven by
  `PENDING_EXPORT`, which is always set before the request fires.

- Refactor the delegate's `load_signing_key` to delegate its priority
  logic to a new pure function `select_key_bytes<F>(prefix, read)`, which
  can be unit-tested without a `DelegateCtx`. New tests in site-delegate
  cover: per-prefix wins when both slots populated (the exact pre-V7
  bug), legacy fallback when no prefix supplied, legacy fallback when
  per-prefix slot empty (pre-V5 users migrating to V7), and not-found.

- Extract `validate_signing_key_matches` from `handle_signing_key_response`
  into a pure helper and unit-test it in the UI crate: accept matching
  keys, reject keys whose pubkey doesn't match the site owner, reject
  malformed byte lengths. Locks in the defence-in-depth guard.

- Add a CBOR wire-format test asserting that `GetSigningKey` still
  serializes as a bare text string (externally-tagged unit variant), so
  a future refactor can't silently break wire compatibility with pre-V7
  delegates.

- Update the `GetSigningKeyForPrefix` round-trip test to match the new
  variant shape.

[AI-assisted - Claude]
@sanity
Copy link
Copy Markdown
Contributor Author

sanity commented Apr 12, 2026

Addressed review feedback in dae126d:

Code-first + Skeptical (critical): wire-format break. Reverted to keeping GetSigningKey as a unit variant (for CBOR wire compat with pre-V7 delegates, so legacy-migration probes keep working) and added a new GetSigningKeyForPrefix { prefix: String } variant that only V7+ delegates understand. Added a wire-format regression test asserting GetSigningKey still serializes as a bare text string.

Skeptical: response correlation race. Added a PENDING_EXPORT signal holding a snapshot of the target site at request time. The response handler is now keyed off the snapshot, not current_site(), so a concurrent legacy-migration SigningKey response or a user-initiated site switch can no longer produce spurious owner_pubkey mismatches.

Skeptical: visitor sites. request_export now rejects SiteRole::Visitor up front with a clear error.

Code-first: silent hang on current_site() is None. The whole response path is now driven by PENDING_EXPORT; if that's not set, the response is simply ignored (it was a legacy-migration probe). If the user hits Export with no site selected, they get an explicit "No site selected to export." error.

Testing (critical): extracted select_key_bytes from load_signing_key in the delegate and added 4 unit tests directly exercising the pre-V7 bug condition (per-prefix vs legacy priority with both populated, both empty, each singly populated). Also extracted validate_signing_key_matches in the UI and added 3 unit tests for the defence-in-depth guard (accept matching, reject mismatched, reject malformed length).

Big-picture: the site_contract.wasm hash does change (b92da83d → 53e3395f) because common/ rebuilt. The existing WASM upgrade path handles this automatically (stored contract_key vs computed → GET old, PUT new). Delegate migration V7 entry is unchanged.

All tests passing locally: delta-core 19/19, site-delegate 4/4, delta-ui 3/3, clippy clean.

[AI-assisted - Claude]

@sanity sanity merged commit 62fe35b into main Apr 12, 2026
2 of 3 checks passed
sanity added a commit that referenced this pull request Apr 12, 2026
Fresh bug report: export hangs on "Delta: routing signing request
through legacy delegate" for sites created under the current release.

Root cause: PR #2 added `DelegateRequest::GetSigningKeyForPrefix`
but `request_prefix` in `ui/src/freenet_api/delegate.rs` only
extracted prefixes from `SignPage`/`SignPageDeletion`/`SignConfig`.
The new variant fell into the `_ => None` arm, so
`send_signing_request` saw `prefix = None`, fell back to the
`HAS_CURRENT_LEGACY_KEY` heuristic, and for any user who had ever
migrated through a legacy delegate that flag was `false` — routing
the export request to the legacy delegate. Pre-V7 delegates cannot
deserialize `GetSigningKeyForPrefix` (unit-variant wire format ≠
struct-variant wire format) so the request hung forever, no
response, no error, spinner indefinitely.

Fix:

1. Add `GetSigningKeyForPrefix` to `request_prefix` so the router
   sees the correct prefix and checks `CURRENT_KEY_PREFIXES`
   directly against it.

2. Introduce `variant_is_v7_plus` and refuse to fall back to a
   legacy delegate for any such variant, even if somehow the
   current-delegate check fails. Send to the current delegate
   unconditionally so we get a clean error rather than silent hang.

Every future `DelegateRequest` variant introduced in a delegate-WASM
upgrade must be added to `variant_is_v7_plus` (or a similarly-named
"post-that-version" gate) or risk the same silent-hang class of bug.

[AI-assisted - Claude]
Sign up for free to 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.

1 participant