Skip to content

Commit c865327

Browse files
committed
fix(diagnostics): a passing check should not report an HTTP status
The self-test showed "PASS · TSP+DIDComm mediator accepts this wallet's origin — answered (HTTP 405)". The first person to read it asked what was broken. Nothing was. The status was named on purpose, reasoning that seeing the number would make it obvious a 4xx is expected and not the thing being tested. It did the opposite. A green PASS next to a 405 reads as a contradiction, and the reader has to already understand CORS probing to resolve it — which is precisely the knowledge this panel exists to not require. `checkCorsReachable` sends a plain GET against the same endpoint that would fail, so a POST-only auth route answers 405 and a REST base with no handler at its bare path answers 404. Both are complete passes: ANY status proves the origin was allowed, because a CORS refusal has no status to read at all. The number answers a question nobody asked, in a place where an unexplained number reads as a fault. So `checkCorsReachable` no longer returns it — a field whose only consumer was that misleading line is not worth keeping for a future caller who would be wrong to use it. Failures are untouched: they carry their detail and a stable `code`, which is where the diagnosis belongs. The REST row also gains the "with this extension's origin" clause it was missing, so both rows now say what was actually proven rather than what was received. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
1 parent 2fd8b16 commit c865327

1 file changed

Lines changed: 24 additions & 9 deletions

File tree

‎packages/extension/src/offscreen.ts‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -724,16 +724,19 @@ function extensionOrigin(): string | undefined {
724724
* browser checks `Access-Control-Allow-Origin` on the actual response and a
725725
* refusal surfaces exactly as it does on the real path. The **status does not
726726
* matter** — a `405` from a POST-only auth route is a complete pass, because
727-
* reading any status at all proves the origin was allowed. That is also why
728-
* this cannot be replaced by hitting a health endpoint: it must be governed
727+
* reading any status at all proves the origin was allowed. It is therefore
728+
* not returned: nothing can do anything useful with it, and the one place it
729+
* was used put a `405` next to a green PASS in a security self-test, which
730+
* reads as a contradiction. That the status is irrelevant is also why this
731+
* cannot be replaced by hitting a health endpoint: the probe must be governed
729732
* by the same policy as the request that fails. */
730733
async function checkCorsReachable(
731734
url: string,
732735
fetchImpl: typeof fetch,
733-
): Promise<{ ok: boolean; status?: number; error?: unknown }> {
736+
): Promise<{ ok: boolean; error?: unknown }> {
734737
try {
735-
const res = await fetchImpl(url, { method: "GET", cache: "no-store" });
736-
return { ok: true, status: res.status };
738+
await fetchImpl(url, { method: "GET", cache: "no-store" });
739+
return { ok: true };
737740
} catch (err: unknown) {
738741
return { ok: false, error: err };
739742
}
@@ -777,9 +780,18 @@ async function diagnoseMediator(
777780
id: `${idBase}.origin`,
778781
label: `${label} mediator accepts this wallet's origin`,
779782
status: "pass",
780-
// Naming the status makes it obvious to a reader that a 4xx is expected
781-
// and is not the thing being tested.
782-
detail: `${host} answered (HTTP ${cors.status}) with this extension's origin on the request.`,
783+
// No status code on a pass. It used to name one, reasoning that seeing
784+
// the number would make it obvious a 4xx is expected and not the thing
785+
// being tested. It did the opposite: a security self-test reporting
786+
// "PASS … HTTP 405" reads as a contradiction, and the first person to
787+
// look at it asked what was broken. Nothing is — a `GET` against the
788+
// auth endpoint is answered 405 because it wants a `POST`, and ANY
789+
// status is a pass here, since reading a status at all is what proves
790+
// the origin was allowed (a CORS refusal has no status to read). The
791+
// number answers a question nobody asked, in a place where an
792+
// unexplained number reads as a fault. Failures still carry their
793+
// detail and a `code`, which is where it is diagnostic.
794+
detail: `${host} answered a request carrying this extension's origin.`,
783795
});
784796
return checks;
785797
}
@@ -910,7 +922,10 @@ async function runDiagnostics(vtaDid: string): Promise<DiagnosticsReport> {
910922
id: "vta.rest",
911923
label: "Trust agent REST accepts this wallet's origin",
912924
status: "pass",
913-
detail: `${originOf(restUrl) ?? restUrl} answered (HTTP ${cors.status}).`,
925+
// Same wording as the mediator check above, and for the same reason —
926+
// this one also dropped the "with this extension's origin" clause, so
927+
// it read as a bare status with nothing saying what had been proven.
928+
detail: `${originOf(restUrl) ?? restUrl} answered a request carrying this extension's origin.`,
914929
});
915930
} else {
916931
const reachable = await probeReachable(restUrl, fetchImpl);

0 commit comments

Comments
 (0)