fix(extension): the relay's origin was body-supplied; signTrustTask could self-approve - #86
Merged
Merged
Conversation
…ould self-approve Two security regressions, both from earlier PRs in this series, both found by adversarially re-reading the code against the design rather than trusting it. **The relay read its origin from the message body.** #82 added an attested-origin gate that overwrites the body's claim with `sender.origin` for page-facing message types. #84 then added `requestTask` and never put it in that set. So the one method whose origin is stamped into `payload.ext` — and therefore rides *inside the digest an approver signs* — was reading it straight from the content script's `window.location.origin`, which any extension-internal context can assert. A hostile page cannot forge it; a hostile extension context, which is exactly the threat the gate exists for, can. `RUNTIME_REQUEST_TASK` is now in the set. **`signTrustTask` short-circuited on a remembered origin.** #82 routed it through `gatedConsent` for its ergonomics, without noticing that `gatedConsent` returns `true` with no prompt for a trusted origin. But this method signs an *arbitrary* envelope with the holder key, so a remember-grant that silenced it means "let this site sign anything, forever" — a tick made once on a login prompt authorizing a DID deactivation a year later. It now uses `requestConsent`, which always asks. Origin trust is not capability trust; that was the whole point, and I had inverted it on the most powerful method on the surface. To stop the first bug's *class* from recurring: the page-facing set now derives from `PAGE_FACING_RUNTIME_TYPES`, a single source in `bridge-protocol.ts`, and the content script's dispatch table is typed `Record<BridgeMethod, string>`. Adding a page-facing method without registering it is now a compile error naming the method — which is how `requestTask` should have been caught the first time. (The content script bundles as a classic script and cannot import values, so the two files cannot literally share the list; the type binding is what keeps them in step.) Also: the per-action prompts (`requestTask`, `signTrustTask`) no longer render a "Remember this site" checkbox. There is nothing to remember for a one-shot authorization, and a checkbox that silently discards its own tick lies to the user. A `noRemember` flag suppresses it. `npm run build`, `npm run lint`, `npm test` (101) clean. Verified the compile guard fails when a method is removed from the table. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
stormer78
added a commit
that referenced
this pull request
Jul 16, 2026
…4, D8-F6) (#88) F4 (offscreen.ts) — the inbound mediator listener re-armed with a single 2s setTimeout from onClose only, and startInbound swallowed errors without rescheduling. A mediator outage longer than one retry left the listener dead, silently missing every consent request until the MV3 worker happened to reboot. Replace with a per-VTA exponential-backoff scheduler (scheduleInboundReconnect): 2s doubling to 60s, re-arming on FAILED attempts too, resetting on a successful (re)connect. startInbound now returns success; reconcileInbound schedules a retry for any VTA that fails to come up and clears backoff for one that succeeds. onClose fires only on an unexpected drop, and the forgotten-VTA close path cancels any pending retry, so a deliberately-closed listener isn't resurrected (a cancellation guard also covers the in-flight-attempt race). (R1.5) F6 (background.ts) — the page-facing authenticated fetch proxy (handleApiGet/handleApiPost) called fetch() with no AbortSignal, so a blackholed VTA hung the page's await for minutes and stacked requests pinned the MV3 worker. Route both through a shared proxyFetch that applies AbortSignal.timeout (20s); a stall now surfaces as a clean { ok: false, error } via the dispatcher's existing catch. (R1.2, R4.1 one shared helper rather than two copies) Type-checks clean (tsc -b packages/extension). No version bump: the extension is an unpublished artifact and recent fix(extension) PRs (#82/#86) followed the same convention. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two security regressions, both introduced by earlier PRs in this series, both found by adversarially re-reading the code against the design rather than trusting my own PR descriptions.
1. The relay read its origin from the message body
#82 added an attested-origin gate: for page-facing message types, the background overwrites the body's
originclaim withsender.origin, which page content cannot forge.#84 then added
requestTask— and never added it to that set.So the one method whose origin is stamped into
payload.ext["openvtc.origin"]— and therefore rides inside the digest an approver signs — was reading it straight from the content script'swindow.location.origin. A hostile page can't forge that (the content script writes its own location), but a hostile extension context — the exact threat the gate was built for — can assert anything.The site shown to a human as the thing they're authorizing was body-supplied.
RUNTIME_REQUEST_TASKis now in the set.2.
signTrustTaskshort-circuited on a remembered origin#82 routed it through
gatedConsentfor the ergonomics, without noticing thatgatedConsentreturnstruewith no prompt for a trusted origin.This method signs an arbitrary envelope with the holder key. A remember-grant that silences it means "let this site sign anything, forever" — a tick made once on a login prompt authorizing a DID deactivation a year later.
It now uses
requestConsent, which always asks. Origin trust is not capability trust — the whole thesis of this work, and I had inverted it on the single most powerful method on the page-facing surface.Stopping the class, not just the instance
The page-facing set now derives from
PAGE_FACING_RUNTIME_TYPES, a single source inbridge-protocol.ts, and the content script's dispatch table is typedRecord<BridgeMethod, string>.Adding a page-facing method without registering it is now a compile error naming the method — which is how
requestTaskshould have been caught the first time. (The content script bundles as a classic script and cannot import values, so the two files can't literally share the list; the type binding keeps them in step.)Verified the guard has teeth: removing
requestTaskfrom the table failstscwithProperty 'requestTask' is missing.Also
The per-action prompts (
requestTask,signTrustTask) no longer render a "Remember this site" checkbox. A one-shot authorization has nothing to remember, and a checkbox that silently discards its own tick lies to the user. AnoRememberflag suppresses it.npm run build,npm run lint,npm test(101) clean.How these were found
Both were surfaced by an adversarial fact-check of the design note against the code. They passed every existing test — because the tests, like the note, encoded what the author believed. This is the second half of the process fix: the harness catches integration bugs, the adversarial read catches untested claims. I should have run both before pushing, not after merge.