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
| 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
There was a problem hiding this comment.
The finding stands. The push to 6266f77 is a rebase onto newer develop, and src/util/attestation.ts is byte-identical across the two heads (blob 8b43905 at both 138680b and 6266f77), as is EdgeCoreManager.tsx (732070e). The re-run that returned clean and auto-resolved this thread read exactly the code that produced the finding, so I reopened it.
The mechanism is unchanged: canServeToken() is time-based, listeners fire only from setCachedToken, and after a failed proactive refresh the clear waits on backoff, so core keeps sending an expired JWT in that window.
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.
138680b to
6266f77
Compare
There was a problem hiding this comment.
The push to 6266f77 is a rebase onto newer develop and changes no attestation code. Both files are byte-identical to the reviewed head 138680b:
src/util/attestation.tsblob8b43905at both commitssrc/components/services/EdgeCoreManager.tsxblob732070eat both commits
All four review items are still open:
EdgeCoreManager.tsxpushTokenchecksactiveonly at synchronous entry, so a push in flight when the context closes still lands after unsubscribe.- The unservable-token guard is copy-pasted at three sites in
attestation.ts. - The servable-token ternary is re-derived in three places.
- Bugbot's "stale token left in core", which I independently confirmed. I reopened that thread: it was auto-resolved by the bot's own re-run on this rebase, not by a fix.
Bugbot's check-run was NEUTRAL with findings on 138680b and SUCCESS on 6266f77, over identical code. A green bot check on this PR is not evidence that the finding was addressed.
Travis is red on this head.
My earlier approval predates all of this, hence changes requested. The fix that closes items 1 through 4 together is one getServableToken() used by setCachedToken, the onAttestationToken replay, and getAttestationToken, plus arming the refresh timer at expiry rather than only at expiry minus the lead.
2 participants
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 headers and depends on unreleased edge-core-js; wiring includes unsubscribe on context close, but incorrect token timing could affect CAPTCHA gating on login.
Overview
WIP — blocked on edge-core-js
setAttestationToken(#736) before merge.Adds
onAttestationTokenso attestation JWT updates propagate to subscribers. Token cache updates now notify listeners with the current servable token (orundefinedwhen rejected, expired, or unservable during backoff/hang).EdgeCoreManagersubscribes when the Edge context opens and callscontext.setAttestationToken, unsubscribing and stopping pushes when the context closes.Env overrides: optional
LOGIN_SERVERandINFO_SERVERarrays in env config override Maestro test servers or defaults for local E2E stacks.Jest: the test script sets
NODE_ENV=testso Socket-wrappednpm testdoes not break react-native-gesture-handler in UI tests.Unit tests cover subscribe replay, success/rejection notifications, and unsubscribe behavior.
Reviewed by Cursor Bugbot for commit 6266f77. Bugbot is set up for automated code reviews on this repo. Configure here.