WIP: Wire attestation tokens into edge-core-js context #6154
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -142,9 +142,42 @@ let unsupported = false | |
| // warning per day of app uptime so the refresh cadence cannot re-prompt. | ||
| let lastClockWarnAtMono: number | undefined | ||
|
|
||
| const tokenListeners = new Set<(token: string | undefined) => void>() | ||
|
|
||
| const setCachedToken = (next: CachedToken | undefined): void => { | ||
| cachedToken = next | ||
| const token = canServeToken() ? cachedToken?.token : undefined | ||
| for (const listener of tokenListeners) { | ||
| try { | ||
| listener(token) | ||
| } catch (error) { | ||
| console.warn('[attestation] token listener threw', error) | ||
| } | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Stale token left in coreMedium Severity
Additional Locations (2)  Reviewed by Cursor Bugbot for commit 138680b. Configure here.
Contributor
There was a problem hiding this comment. Confirmed against 138680b, not a false positive. The window opens when a proactive refresh fails, because the next clear then waits on the failure backoff (and RN throttles background timers): sequenceDiagram
participant tmr as refresh timer
participant att as attestation.ts
participant core as edge-core-js
participant srv as login server
att->>core: setAttestationToken(jwt)
Note over att: scheduleRefresh at expiry minus 5 min
tmr->>att: handshake attempt
att--xtmr: handshake fails, arm backoff
Note over att: token expires, canServeToken false,<br/>no listener fires
core->>srv: request with expired jwt
srv->>srv: verify fails on every key, force-refresh, fails again
srv-->>core: served as unattested
tmr->>att: backoff fires, setCachedToken(undefined)
att->>core: setAttestationToken(undefined)
Impact is fail-open rather than a security hole (the server re-checks expiry), but it costs the user a CAPTCHA they earned the right to skip, and each such request takes the login server's slow verify path: the double key-loop always, plus a real JWKS fetch up to once per 60s per worker (see my Cheapest fix that also closes my two nit threads: a single |
||
|
|
||
| /** | ||
| * Subscribe to attestation token changes. Returns an unsubscribe function. | ||
| * Immediately invokes the listener with the current servable token (if any). | ||
| */ | ||
| export const onAttestationToken = ( | ||
| listener: (token: string | undefined) => void | ||
| ): (() => void) => { | ||
| tokenListeners.add(listener) | ||
| try { | ||
| listener(canServeToken() ? cachedToken?.token : undefined) | ||
|
Contributor
There was a problem hiding this comment. Nit: the servable-token ternary now lives in three places (here,
Contributor
There was a problem hiding this comment. Still open on 138680b. A shared |
||
| } catch (error) { | ||
| console.warn('[attestation] token listener threw', error) | ||
| } | ||
| return () => { | ||
| tokenListeners.delete(listener) | ||
| } | ||
| } | ||
|
|
||
| /** Test-only: clear module state between Jest cases. */ | ||
| export const resetAttestationForTests = (): void => { | ||
| cachedToken = undefined | ||
| tokenListeners.clear() | ||
| inFlight = undefined | ||
| if (refreshTimer != null) clearTimeout(refreshTimer) | ||
| refreshTimer = undefined | ||
|
|
@@ -355,7 +388,7 @@ const refreshWithEnrolledKey = async ( | |
| // this attempt - the key and token belong to a live handshake now, and clearing | ||
| // them would force it into a needless re-attestation. | ||
| assertCurrent(attempt) | ||
| setCachedToken(undefined) | ||
| console.warn( | ||
| `[attestation] assertion rejected (${response.status}); re-attesting` | ||
| ) | ||
|
|
@@ -457,6 +490,12 @@ const delay = async (ms: number): Promise<void> => { | |
| const armTimer = (delayMs: number): void => { | ||
| if (refreshTimer != null) clearTimeout(refreshTimer) | ||
| refreshTimer = setTimeout(() => { | ||
| // If the cached token can no longer be served (expiry), clear it so | ||
| // onAttestationToken listeners (e.g. EdgeCoreManager → setAttestationToken) | ||
| // drop the stale JWT before the handshake runs. | ||
| if (cachedToken != null && !canServeToken()) { | ||
|
Contributor
There was a problem hiding this comment. Nit: this guard is copy-pasted at three sites (armTimer, the handshake catch, the watchdog); a |
||
| setCachedToken(undefined) | ||
| } | ||
| runHandshake() | ||
| }, delayMs) | ||
| } | ||
|
|
@@ -605,7 +644,7 @@ const runHandshake = (): void => { | |
| } | ||
| lastFailureAt = undefined | ||
| consecutiveFailures = 0 | ||
| setCachedToken(freshToken) | ||
| console.log('[attestation] handshake ok') | ||
| scheduleRefresh(freshToken.expiresMono) | ||
| }) | ||
|
|
@@ -628,6 +667,11 @@ const runHandshake = (): void => { | |
| attempt.countedFailure = true | ||
| } | ||
| console.warn('[attestation] handshake failed:', String(error)) | ||
| // Drop an already-unservable JWT so listeners stop feeding edge-core a | ||
| // stale token for the full backoff window. | ||
| if (cachedToken != null && !canServeToken()) { | ||
| setCachedToken(undefined) | ||
| } | ||
| scheduleRetryAfterFailure() | ||
| }) | ||
| .finally(() => { | ||
|
|
@@ -657,6 +701,11 @@ const runHandshake = (): void => { | |
| consecutiveFailures += 1 | ||
| attempt.countedFailure = true | ||
| } | ||
| // Drop an already-unservable JWT so listeners stop feeding edge-core a | ||
| // stale token while we wait out the hang backoff. | ||
| if (cachedToken != null && !canServeToken()) { | ||
| setCachedToken(undefined) | ||
| } | ||
| // An attempt that never settles leaves nothing else to re-arm the loop. | ||
| scheduleRetryAfterFailure() | ||
| }, HANDSHAKE_WATCHDOG_MS) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushTokenchecksactiveonly at its synchronous entry, so a push already in flight when the context closes still lands after unsubscribe. Worst case is a warn or a set on the discarded context, so minor, but re-checkingactivewhen the call settles would tighten it.sequenceDiagram participant att as attestation.ts participant mgr as EdgeCoreManager participant ctx as EdgeContext att->>mgr: listener(token) mgr->>mgr: active is true, proceed mgr-)ctx: setAttestationToken(token) async ctx-->>mgr: close event mgr->>mgr: active = false, unsubscribe ctx-->>mgr: earlier push settles on closed contextThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still open on 138680b (head unchanged since the review). Related to the Bugbot "stale token left in core" thread on this PR, which I independently confirmed: both are the push bridge lacking a guard the pull path (
getAttestationToken) gets for free.