WIP: Wire attestation tokens into edge-core-js context - #6154
WIP: Wire attestation tokens into edge-core-js context#6154paullinator wants to merge 2 commits into
Conversation
Socket sets NODE_ENV=development when it wraps npm, which makes react-native-gesture-handler treat Jest as a non-test environment, so every GestureDetector test fails to locate its view in the native tree.
Push info-server attestation tokens to core via setAttestationToken and allow LOGIN_SERVER env overrides for local E2E stacks.
| let active = true | ||
| const pushToken = (token: string | undefined): void => { | ||
| if (!active) return | ||
| context.setAttestationToken(token).catch((error: unknown) => { |
There was a problem hiding this comment.
pushToken checks active only 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-checking active when 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 context
There was a problem hiding this comment.
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.
| // 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()) { |
There was a problem hiding this comment.
Nit: this guard is copy-pasted at three sites (armTimer, the handshake catch, the watchdog); a dropUnservableToken() helper keeps a future servability change from missing one path.
| ): (() => void) => { | ||
| tokenListeners.add(listener) | ||
| try { | ||
| listener(canServeToken() ? cachedToken?.token : undefined) |
There was a problem hiding this comment.
Nit: the servable-token ternary now lives in three places (here, setCachedToken, and getAttestationToken's tail); a getServableToken() used by all three keeps subscribers and pollers in lockstep.
There was a problem hiding this comment.
Still open on 138680b. A shared getServableToken() would also give the stale-token issue Bugbot flagged a single place to fix.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Undrafting to let bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Stale token left in core
- Armed a serve-until timer in setCachedToken so onAttestationToken listeners (edge-core) are cleared at the clock-skew deadline instead of waiting for a later handshake tick or failure backoff.
Or push these changes by commenting:
@cursor push b713c71554
Preview (b713c71554)
diff --git a/src/__tests__/util/attestation.test.ts b/src/__tests__/util/attestation.test.ts
--- a/src/__tests__/util/attestation.test.ts
+++ b/src/__tests__/util/attestation.test.ts
@@ -2158,6 +2158,37 @@
expect(listener.mock.calls).toContainEqual([undefined])
})
+ it('fires with undefined when a cached token becomes unservable', async () => {
+ // Refresh is armed while the token is still servable, and a failed
+ // refresh then waits out FAILURE_BACKOFF_MS. Listeners must still drop
+ // the JWT at the skew window - not whenever that later tick happens.
+ const { CLOCK_SKEW_MS, FAILURE_BACKOFF_MS, MIN_REFRESH_MS } =
+ attestationTimingForTests
+ const lifetimeMs = MIN_REFRESH_MS + CLOCK_SKEW_MS + 10 * 1000
+ const listener = jest.fn<(token: string | undefined) => void>()
+ onAttestationToken(listener)
+ listener.mockClear()
+ mockSuccessfulHandshake(lifetimeMs / 1000)
+ initAttestation()
+ await flush()
+ expect(listener.mock.calls).toEqual([['jwt-token']])
+ listener.mockClear()
+
+ mockCheapFailingHandshake()
+ await jest.advanceTimersByTimeAsync(MIN_REFRESH_MS)
+ await flush()
+ // Still inside the servable window, so the failure path must not clear.
+ expect(listener.mock.calls).toEqual([])
+ await expect(getAttestationToken()).resolves.toBe('jwt-token')
+
+ // Cross the skew window, but stay well short of the failure backoff.
+ await jest.advanceTimersByTimeAsync(CLOCK_SKEW_MS + 10 * 1000)
+ await flush()
+ expect(CLOCK_SKEW_MS + 10 * 1000).toBeLessThan(FAILURE_BACKOFF_MS)
+ expect(listener.mock.calls).toEqual([[undefined]])
+ await expect(getAttestationToken()).resolves.toBeUndefined()
+ })
+
it('stops notifying after unsubscribe', async () => {
const { REFRESH_LEAD_MS } = attestationTimingForTests
const REFRESH_UNTIL_MS = 5 * 60 * 1000
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -119,6 +119,11 @@
let cachedToken: CachedToken | undefined
let inFlight: Promise<void> | undefined
let refreshTimer: ReturnType<typeof setTimeout> | undefined
+// Drop the JWT when it crosses the skew window, so push listeners (edge-core)
+// stop sending a token getAttestationToken would already withhold. The
+// handshake timer is not this clock: it is armed while the token is still
+// servable, and a failed refresh can leave the next tick behind a backoff.
+let serveUntilTimer: ReturnType<typeof setTimeout> | undefined
// `undefined` means no prior stamp. Initializing these to `0` worked with
// `Date.now()` (epoch is always far past) but a monotonic clock starts near
// zero, so `0` would look like "just now" and park every first handshake behind
@@ -146,6 +151,19 @@
const setCachedToken = (next: CachedToken | undefined): void => {
cachedToken = next
+ if (serveUntilTimer != null) clearTimeout(serveUntilTimer)
+ serveUntilTimer = undefined
+ if (cachedToken != null) {
+ const serveMs = cachedToken.expiresMono - CLOCK_SKEW_MS - monotonicNow()
+ if (serveMs > 0) {
+ serveUntilTimer = setTimeout(() => {
+ serveUntilTimer = undefined
+ if (cachedToken != null && !canServeToken()) {
+ setCachedToken(undefined)
+ }
+ }, serveMs)
+ }
+ }
const token = canServeToken() ? cachedToken?.token : undefined
for (const listener of tokenListeners) {
try {
@@ -181,6 +199,8 @@
inFlight = undefined
if (refreshTimer != null) clearTimeout(refreshTimer)
refreshTimer = undefined
+ if (serveUntilTimer != null) clearTimeout(serveUntilTimer)
+ serveUntilTimer = undefined
lastFailureAt = undefined
lastHandshakeAt = undefined
consecutiveFailures = 0You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 138680b. Configure here.
| console.warn('[attestation] token listener threw', error) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Stale token left in core
Medium Severity
onAttestationToken only fires from setCachedToken, but canServeToken can flip to false while cachedToken still holds the JWT. getAttestationToken already withholds that value; opportunistic clears in the refresh timer and failure paths often run later (or after backoff), so EdgeCoreManager can keep feeding edge-core an expired token on login requests until then—especially after a failed proactive refresh or when JS timers lag in the background.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 138680b. Configure here.
There was a problem hiding this comment.
Confirmed against 138680b, not a false positive. canServeToken() is purely time-based (monotonicNow() < expiresMono - CLOCK_SKEW_MS) but listeners only fire from setCachedToken, so the servable-to-expired transition emits nothing on its own. getAttestationToken re-evaluates on every read and is safe; the push bridge into edge-core is not.
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 with-api-key thread on EdgeApp/edge-login-server#194).
Cheapest fix that also closes my two nit threads: a single getServableToken() used by setCachedToken, the onAttestationToken replay, and getAttestationToken, with the refresh timer armed at expiry rather than only at expiry minus the lead, so the clear does not depend on a successful handshake.



Summary
setAttestationToken/x-attestation-token). Merge that first, publish a newedge-core-jsversion, then bumppackage.jsonhere before merging.context.setAttestationTokenwhen the Edge context opens.LOGIN_SERVER/INFO_SERVERenv overrides for local E2E stacks.NODE_ENV=testfix so Socket-wrappednpm testdoes not break GestureDetector tests.Dependencies
edge-core-jsinpackage.json/ lockfile to the published release that includes Eliran/shitcoins #736.Test plan
x-attestation-tokenwhen a token is availablenpm testNote
Medium Risk
Touches login-server attestation wiring and env server overrides; blocked on edge-core-js API but mis-timed tokens could affect CAPTCHA bypass behavior until core is bumped.
Overview
WIP — depends on edge-core-js
setAttestationToken(#736) before merge.Adds
onAttestationTokenso attestation JWT updates propagate to listeners.EdgeCoreManagersubscribes whenEdgeContextopens and callscontext.setAttestationToken, clearing the token on context close or when the cache becomes unservable (expiry, failed handshake, watchdog).LOGIN_SERVERandINFO_SERVERenv overrides (alongside existingINFO_SERVER) let debug/E2E builds point at local login and info servers without Maestro test-server mode.Sets
NODE_ENV=testin thenpm testscript so Socket-wrapped Jest runs keepreact-native-gesture-handlerin test mode.Reviewed by Cursor Bugbot for commit 138680b. Bugbot is set up for automated code reviews on this repo. Configure here.