Add app/device attestation for gated info-server requests - #6076
Add app/device attestation for gated info-server requests#6076paullinator wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Hung handshake blocks later attempts
- The shared handshake lock now expires after the caller timeout and stale attempts cannot clear a newer lock.
- ✅ Fixed: Unvalidated expires breaks token cache
- Attestation responses now require expires to be a finite number before caching the token.
Or push these changes by commenting:
@cursor push 836aed9d72
Preview (836aed9d72)
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -29,7 +29,7 @@
const REFRESH_LEAD_MS = 2 * 60 * 1000
// Small skew so a token that is about to expire is treated as unusable.
const CLOCK_SKEW_MS = 5 * 1000
-// Max time getAttestationToken() blocks waiting on the initial handshake.
+// Max time a caller waits and one handshake attempt holds the shared lock.
const GET_TOKEN_TIMEOUT_MS = 3 * 1000
let cachedToken: CachedToken | undefined
@@ -89,6 +89,9 @@
if (typeof token !== 'string') {
throw new Error('attest response missing token')
}
+ if (typeof expires !== 'number' || !Number.isFinite(expires)) {
+ throw new Error('attest response missing expires')
+ }
cachedToken = { token, expires }
}
@@ -115,7 +118,7 @@
*/
const runHandshake = (): void => {
if (inFlight != null) return
- inFlight = performHandshake()
+ const handshake: Promise<void> = performHandshake()
.then(() => {
if (cachedToken != null) scheduleRefresh(cachedToken.expires)
})
@@ -123,8 +126,13 @@
console.warn('[attestation] handshake failed:', String(error))
})
.finally(() => {
- inFlight = undefined
+ if (inFlight === handshake) inFlight = undefined
})
+ inFlight = handshake
+ // A stuck native call may continue, but it must not block later attempts.
+ setTimeout(() => {
+ if (inFlight === handshake) inFlight = undefined
+ }, GET_TOKEN_TIMEOUT_MS)
}
/**You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Watchdog allows overlapping handshakes
- Kept the handshake lock after the watchdog until the uncancellable native promise settles and added regression coverage proving no second native attestation starts.
Or push these changes by commenting:
@cursor push fdfed897e3
Preview (fdfed897e3)
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
@@ -140,7 +140,7 @@
await expect(tokenPromise).resolves.toBe('jwt-token')
})
- it('releases a hung handshake after the watchdog so a later attempt can succeed (Task 2.2)', async () => {
+ it('keeps a hung handshake locked after the watchdog', async () => {
mockGetAttestation.mockImplementation(
async () => await new Promise(() => {}) // never settles
)
@@ -156,25 +156,19 @@
await Promise.resolve()
await Promise.resolve()
- // Watchdog fires and clears the lock.
+ // The watchdog reports the hung handshake without clearing its lock.
await jest.advanceTimersByTimeAsync(
attestationTimingForTests.HANDSHAKE_WATCHDOG_MS
)
- // A subsequent attempt can start after the lock is released.
- const expires = Date.now() + 10 * 60 * 1000
- mockGetAttestation.mockResolvedValue({
- keyId: 'key2',
- attestation: 'att2',
- bundleId: 'co.edgesecure.app'
- })
- mockSuccessfulHandshake(expires)
-
+ // A subsequent request waits on the existing handshake rather than
+ // starting overlapping native attestation work.
const tokenPromise = getAttestationToken()
- await Promise.resolve()
- await Promise.resolve()
- await Promise.resolve()
- await expect(tokenPromise).resolves.toBe('jwt-token')
+ await jest.advanceTimersByTimeAsync(
+ attestationTimingForTests.GET_TOKEN_TIMEOUT_MS
+ )
+ await expect(tokenPromise).resolves.toBeUndefined()
+ expect(mockGetAttestation).toHaveBeenCalledTimes(1)
})
it('suppresses retries during the failure backoff window (Task 2.3)', async () => {
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -46,10 +46,8 @@
const CLOCK_SKEW_MS = 5 * 1000
// Max time getAttestationToken() blocks waiting on the initial handshake.
const GET_TOKEN_TIMEOUT_MS = 3 * 1000
-// Watchdog: a handshake that has not settled after this long is considered
-// hung; release the lock so a later attempt can start. Sized well above a
-// slow-but-legitimate handshake so concurrent handshakes never overlap in
-// normal operation (Apple rate-limits attestation).
+// Watchdog: log when a handshake has not settled after this long. The lock
+// remains held since native attestation cannot be cancelled safely.
const HANDSHAKE_WATCHDOG_MS = 90 * 1000
// After a failed handshake, don't retry (and don't make gated callers wait)
// for this long. Keeps a persistently-failing device from adding 3s of
@@ -258,12 +256,11 @@
if (inFlight === handshake) inFlight = undefined
})
inFlight = handshake
- // A hung native call must not block all future attempts. Only clear the
- // lock if this same handshake still holds it.
+ // Do not release the lock here: the native call may still be running, and
+ // overlapping attempts can corrupt shared attestation key state.
setTimeout(() => {
if (inFlight === handshake) {
- console.warn('[attestation] handshake watchdog fired; releasing lock')
- inFlight = undefined
+ console.warn('[attestation] handshake watchdog fired; still in progress')
}
}, HANDSHAKE_WATCHDOG_MS)
}You can send follow-ups to the cloud agent here.
03eb1ba to
1c7b261
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Unguarded attestation console logging
- Guarded both routine assertion fallback logs with ENV.DEBUG_VERBOSE_LOGGING so production builds remain quiet.
Or push these changes by commenting:
@cursor push 7b1cb6d483
Preview (7b1cb6d483)
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -1,5 +1,6 @@
import { NativeModules, Platform } from 'react-native'
+import { ENV } from '../env'
import { fetchInfo } from './network'
/**
@@ -156,7 +157,9 @@
await EdgeAttestation.clearKey().catch(() => {})
} catch (error) {
// noKey / invalidKey / native failure: fall through to full attestation.
- console.log('[attestation] assertion unavailable:', String(error))
+ if (ENV.DEBUG_VERBOSE_LOGGING) {
+ console.log('[attestation] assertion unavailable:', String(error))
+ }
}
// The challenge above was consumed (or expired); fetch a fresh one for
// the fallback attestation.
@@ -188,7 +191,9 @@
await EdgeAttestation.clearKey().catch(() => {})
} catch (error) {
// noKey / native failure: fall through to full attestation.
- console.log('[attestation] assertion unavailable:', String(error))
+ if (ENV.DEBUG_VERBOSE_LOGGING) {
+ console.log('[attestation] assertion unavailable:', String(error))
+ }
}
// The challenge above was consumed (or expired); fetch a fresh one for
// the fallback attestation.You can send follow-ups to the cloud agent here.
1c7b261 to
552fef1
Compare
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Stale JWT after assert rejection
- Cleared cachedToken immediately when either iOS or Android assertion refresh is rejected before full re-attestation begins.
Or push these changes by commenting:
@cursor push db21325fa5
Preview (db21325fa5)
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -150,6 +150,7 @@
return
}
// Server rejected the assertion: discard the key and re-attest.
+ cachedToken = undefined
console.warn(
`[attestation] assertion rejected (${assertResponse.status}); re-attesting`
)
@@ -182,6 +183,7 @@
cacheTokenFromResponse(await assertResponse.json())
return
}
+ cachedToken = undefined
console.warn(
`[attestation] assertion rejected (${assertResponse.status}); re-attesting`
)You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Stale handshake clears valid token
- Guarded assertion-rejection cache invalidation with the handshake generation so stale attempts cannot clear a newer token.
Or push these changes by commenting:
@cursor push c8daef9e90
Preview (c8daef9e90)
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -124,7 +124,9 @@
* there is nothing to do (no native module / unsupported platform). Never
* caches directly; the caller commits the result.
*/
-const performHandshake = async (): Promise<CachedToken | undefined> => {
+const performHandshake = async (
+ generation: number
+): Promise<CachedToken | undefined> => {
// No native module (e.g. unsupported platform / dev environment).
if (EdgeAttestation == null) return undefined
@@ -163,7 +165,7 @@
// so any previously-minted token is suspect too. Drop it now so gated
// callers do not keep sending a token the server already rejects while
// re-enrollment is in progress; discard the key and re-attest.
- cachedToken = undefined
+ if (generation === handshakeGeneration) cachedToken = undefined
console.warn(
`[attestation] assertion rejected (${assertResponse.status}); re-attesting`
)
@@ -197,7 +199,7 @@
}
// Server rejected the assertion: drop the now-suspect cached token (see
// the iOS branch above) before discarding the key and re-attesting.
- cachedToken = undefined
+ if (generation === handshakeGeneration) cachedToken = undefined
console.warn(
`[attestation] assertion rejected (${assertResponse.status}); re-attesting`
)
@@ -265,7 +267,7 @@
// lock released so a newer handshake can start. Tag each attempt so a stale
// one that finally resolves cannot clobber the newer handshake's token.
const generation = ++handshakeGeneration
- const handshake: Promise<void> = performHandshake()
+ const handshake: Promise<void> = performHandshake(generation)
.then(freshToken => {
if (generation !== handshakeGeneration) return
lastFailureAt = 0You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: iOS attestation lacks native serialization
- Serialized getAttestation, generateAssertion, and clearKey through a dedicated native queue held until each asynchronous App Attest operation completes.
Or push these changes by commenting:
@cursor push f334b72fdd
Preview (f334b72fdd)
diff --git a/ios/edge/EdgeAttestation.swift b/ios/edge/EdgeAttestation.swift
--- a/ios/edge/EdgeAttestation.swift
+++ b/ios/edge/EdgeAttestation.swift
@@ -23,7 +23,24 @@
// assertions (no re-attestation).
private static let keychainService = "co.edgesecure.app.appattest"
private static let keychainAccount = "keyId"
+ // The JS watchdog can start another handshake before an older native call
+ // returns, so keep App Attest and Keychain operations serialized end-to-end.
+ private static let appAttestQueue = DispatchQueue(
+ label: "co.edgesecure.app.appattest.operations"
+ )
+ private func performSerialized(
+ _ operation: @escaping (@escaping () -> Void) -> Void
+ ) {
+ EdgeAttestation.appAttestQueue.async {
+ let finished = DispatchSemaphore(value: 0)
+ operation {
+ finished.signal()
+ }
+ finished.wait()
+ }
+ }
+
private func storeKeyId(_ keyId: String) {
clearKeyId()
guard let data = keyId.data(using: .utf8) else { return }
@@ -87,39 +104,44 @@
return
}
- // A fresh key is generated per handshake: an App Attest key can only be
- // attested once, so reuse would require the assertion flow instead.
- service.generateKey { keyId, error in
- if let error = error {
- reject("generateKey", error.localizedDescription, error)
- return
- }
- guard let keyId = keyId else {
- reject("generateKey", "Failed to generate an App Attest key", nil)
- return
- }
-
- // The client data is the challenge's UTF-8 bytes; the server recomputes
- // SHA256(challenge) to validate the attestation nonce.
- let clientDataHash = Data(SHA256.hash(data: Data(challenge.utf8)))
-
- service.attestKey(keyId, clientDataHash: clientDataHash) { attestation, error in
+ performSerialized { finish in
+ // A fresh key is generated per handshake: an App Attest key can only be
+ // attested once, so reuse would require the assertion flow instead.
+ service.generateKey { keyId, error in
if let error = error {
- reject("attestKey", error.localizedDescription, error)
+ reject("generateKey", error.localizedDescription, error)
+ finish()
return
}
- guard let attestation = attestation else {
- reject("attestKey", "Failed to produce an attestation object", nil)
+ guard let keyId = keyId else {
+ reject("generateKey", "Failed to generate an App Attest key", nil)
+ finish()
return
}
- // Persist the key id so subsequent handshakes refresh via assertions
- // instead of a full (rate-limited) attestation.
- self.storeKeyId(keyId)
- resolve([
- "keyId": keyId,
- "attestation": attestation.base64EncodedString(),
- "bundleId": Bundle.main.bundleIdentifier ?? ""
- ])
+
+ // The client data is the challenge's UTF-8 bytes; the server recomputes
+ // SHA256(challenge) to validate the attestation nonce.
+ let clientDataHash = Data(SHA256.hash(data: Data(challenge.utf8)))
+
+ service.attestKey(keyId, clientDataHash: clientDataHash) { attestation, error in
+ defer { finish() }
+ if let error = error {
+ reject("attestKey", error.localizedDescription, error)
+ return
+ }
+ guard let attestation = attestation else {
+ reject("attestKey", "Failed to produce an attestation object", nil)
+ return
+ }
+ // Persist the key id so subsequent handshakes refresh via assertions
+ // instead of a full (rate-limited) attestation.
+ self.storeKeyId(keyId)
+ resolve([
+ "keyId": keyId,
+ "attestation": attestation.base64EncodedString(),
+ "bundleId": Bundle.main.bundleIdentifier ?? ""
+ ])
+ }
}
}
}
@@ -134,31 +156,36 @@
reject("unsupported", "App Attest is not supported on this device", nil)
return
}
- guard let keyId = loadKeyId() else {
- reject("noKey", "No attested App Attest key is stored", nil)
- return
- }
- let clientDataHash = Data(SHA256.hash(data: Data(challenge.utf8)))
- DCAppAttestService.shared.generateAssertion(keyId, clientDataHash: clientDataHash) { assertion, error in
- if let error = error as? DCError, error.code == .invalidKey {
- // The key no longer exists (reinstall/restore); force re-attestation.
- self.clearKeyId()
- reject("invalidKey", "Stored App Attest key is invalid", error)
+ performSerialized { finish in
+ guard let keyId = self.loadKeyId() else {
+ reject("noKey", "No attested App Attest key is stored", nil)
+ finish()
return
}
- if let error = error {
- reject("generateAssertion", error.localizedDescription, error)
- return
+ let clientDataHash = Data(SHA256.hash(data: Data(challenge.utf8)))
+ DCAppAttestService.shared.generateAssertion(keyId, clientDataHash: clientDataHash) {
+ assertion, error in
+ defer { finish() }
+ if let error = error as? DCError, error.code == .invalidKey {
+ // The key no longer exists (reinstall/restore); force re-attestation.
+ self.clearKeyId()
+ reject("invalidKey", "Stored App Attest key is invalid", error)
+ return
+ }
+ if let error = error {
+ reject("generateAssertion", error.localizedDescription, error)
+ return
+ }
+ guard let assertion = assertion else {
+ reject("generateAssertion", "Failed to produce an assertion", nil)
+ return
+ }
+ resolve([
+ "keyId": keyId,
+ "assertion": assertion.base64EncodedString(),
+ "bundleId": Bundle.main.bundleIdentifier ?? ""
+ ])
}
- guard let assertion = assertion else {
- reject("generateAssertion", "Failed to produce an assertion", nil)
- return
- }
- resolve([
- "keyId": keyId,
- "assertion": assertion.base64EncodedString(),
- "bundleId": Bundle.main.bundleIdentifier ?? ""
- ])
}
}
@@ -167,7 +194,10 @@
_ resolve: @escaping RCTPromiseResolveBlock,
rejecter reject: @escaping RCTPromiseRejectBlock
) {
- clearKeyId()
- resolve(nil)
+ performSerialized { finish in
+ self.clearKeyId()
+ resolve(nil)
+ finish()
+ }
}
}You can send follow-ups to the cloud agent here.
5091e83 to
40713eb
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 4 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 4 issues found in the latest run.
- ✅ Fixed: Assert errors wipe enrolled keys
- Assertion transport, server, parsing, and unexpected native failures now propagate without clearing the enrolled key or triggering full re-attestation.
- ✅ Fixed: Stale handshake clears live key
- Key invalidation and deletion now occur only while the rejecting handshake generation is still current.
- ✅ Fixed: Android clearKey blocks bridge thread
- Android key deletion and lock acquisition now run on a background thread before resolving the bridge promise.
- ✅ Fixed: Network responses skip cleaners
- Challenge and token JSON responses now pass through dedicated cleaners with the cached token type derived from its cleaner.
Or push these changes by commenting:
@cursor push bc1e67e9a9
Preview (bc1e67e9a9)
diff --git a/android/app/src/main/java/co/edgesecure/app/EdgeAttestationModule.kt b/android/app/src/main/java/co/edgesecure/app/EdgeAttestationModule.kt
--- a/android/app/src/main/java/co/edgesecure/app/EdgeAttestationModule.kt
+++ b/android/app/src/main/java/co/edgesecure/app/EdgeAttestationModule.kt
@@ -198,16 +198,19 @@
@ReactMethod
fun clearKey(promise: Promise) {
// Best-effort: force re-enrollment when the server rejects an assertion
- // (unknown key, revoked serial, disabled app). Resolve regardless.
- try {
- synchronized(keystoreLock) {
- val keyStore = KeyStore.getInstance("AndroidKeyStore")
- keyStore.load(null)
- keyStore.deleteEntry(KEY_ALIAS)
+ // (unknown key, revoked serial, disabled app). Keystore access can block
+ // behind key generation, so keep it off the React Native bridge thread.
+ Thread {
+ try {
+ synchronized(keystoreLock) {
+ val keyStore = KeyStore.getInstance("AndroidKeyStore")
+ keyStore.load(null)
+ keyStore.deleteEntry(KEY_ALIAS)
+ }
+ } catch (ignored: Exception) {
+ // Best effort.
}
- } catch (ignored: Exception) {
- // Best effort.
- }
- promise.resolve(null)
+ promise.resolve(null)
+ }.start()
}
}
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -1,3 +1,4 @@
+import { asNumber, asObject, asString } from 'cleaners'
import { NativeModules, Platform } from 'react-native'
import { fetchInfo } from './network'
@@ -34,10 +35,9 @@
const EdgeAttestation: NativeAttestation | undefined =
NativeModules.EdgeAttestation
-interface CachedToken {
- token: string
- expires: number // epoch milliseconds
-}
+const asChallengeResponse = asObject({ challenge: asString })
+const asCachedToken = asObject({ token: asString, expires: asNumber })
+type CachedToken = ReturnType<typeof asCachedToken>
// Relaunch the handshake this long before the current token expires, so a fresh
// token is (re)fetched by the background engine well ahead of expiry.
@@ -55,6 +55,7 @@
// for this long. Keeps a persistently-failing device from adding 3s of
// latency to every gated request.
const FAILURE_BACKOFF_MS = 60 * 1000
+const KEY_REJECTION_STATUSES = new Set([400, 401, 403, 404])
let cachedToken: CachedToken | undefined
let inFlight: Promise<void> | undefined
@@ -84,14 +85,26 @@
const hasLiveToken = (): boolean =>
cachedToken != null && Date.now() < cachedToken.expires - CLOCK_SKEW_MS
+const isMissingKeyError = (error: unknown): boolean => {
+ if (typeof error !== 'object' || error == null) return false
+ const code = 'code' in error ? error.code : undefined
+ const message = error instanceof Error ? error.message : undefined
+ return (
+ code === 'noKey' ||
+ code === 'invalidKey' ||
+ message === 'noKey' ||
+ message === 'invalidKey'
+ )
+}
+
/** Obtain a single-use challenge from the info server. */
const fetchChallenge = async (): Promise<string> => {
const challengeResponse = await fetchInfo('v1/attest/challenge')
if (!challengeResponse.ok) {
throw new Error(`challenge request failed: ${challengeResponse.status}`)
}
- const { challenge } = await challengeResponse.json()
- if (typeof challenge !== 'string' || challenge === '') {
+ const { challenge } = asChallengeResponse(await challengeResponse.json())
+ if (challenge === '') {
throw new Error('challenge response missing challenge')
}
return challenge
@@ -106,17 +119,12 @@
* result before it clobbers a fresher token.
*/
const parseTokenResponse = (json: unknown): CachedToken => {
- const { token, expires } = (json ?? {}) as {
- token?: unknown
- expires?: unknown
- }
- if (typeof token !== 'string') {
- throw new Error('attest response missing token')
- }
- if (typeof expires !== 'number' || !Number.isFinite(expires)) {
+ const tokenResponse = asCachedToken(json)
+ const { expires } = tokenResponse
+ if (!Number.isFinite(expires)) {
throw new Error('attest response missing expires')
}
- return { token, expires }
+ return tokenResponse
}
/**
@@ -146,11 +154,20 @@
let challenge = await fetchChallenge()
// iOS fast path: assert with the stored attested key (no Apple round
- // trip, no new key). Falls back to full attestation when there is no
- // stored key, the key is invalid, or the server rejects the assertion.
+ // trip, no new key). Falls back to full attestation only when there is no
+ // stored key, the key is invalid, or the server rejects the enrolled key.
if (isIos) {
+ let native:
+ | Awaited<ReturnType<NativeAttestation['generateAssertion']>>
+ | undefined
try {
- const native = await EdgeAttestation.generateAssertion(challenge)
+ native = await EdgeAttestation.generateAssertion(challenge)
+ } catch (error) {
+ if (!isMissingKeyError(error)) throw error
+ console.log('[attestation] assertion unavailable:', String(error))
+ challenge = await fetchChallenge()
+ }
+ if (native != null) {
const assertResponse = await fetchInfo('v1/attest/apple/assert', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
@@ -163,32 +180,34 @@
if (assertResponse.ok) {
return parseTokenResponse(await assertResponse.json())
}
- // Server rejected the assertion: the enrolled key is no longer trusted,
- // so any previously-minted token is suspect too. Drop it now so gated
- // callers do not keep sending a token the server already rejects while
- // re-enrollment is in progress; discard the key and re-attest. Only when
- // this is still the current handshake, so a stale (watchdog-released)
- // attempt cannot wipe a token a newer handshake already cached.
- if (generation === handshakeGeneration) cachedToken = undefined
+ if (!KEY_REJECTION_STATUSES.has(assertResponse.status)) {
+ throw new Error(`assert request failed: ${assertResponse.status}`)
+ }
+ if (generation !== handshakeGeneration) return undefined
+ cachedToken = undefined
console.warn(
`[attestation] assertion rejected (${assertResponse.status}); re-attesting`
)
- await EdgeAttestation.clearKey().catch(() => {})
- } catch (error) {
- // noKey / invalidKey / native failure: fall through to full attestation.
- console.log('[attestation] assertion unavailable:', String(error))
+ await EdgeAttestation.clearKey()
+ challenge = await fetchChallenge()
}
- // The challenge above was consumed (or expired); fetch a fresh one for
- // the fallback attestation.
- challenge = await fetchChallenge()
}
// Android fast path: sign the challenge with the enrolled Keystore key
- // (no new key, no RKP dependency). Falls back to full attestation when
- // there is no stored key or the server rejects the assertion.
+ // (no new key, no RKP dependency). Falls back to full attestation only when
+ // there is no stored key or the server rejects the enrolled key.
if (!isIos) {
+ let native:
+ | Awaited<ReturnType<NativeAttestation['signChallenge']>>
+ | undefined
try {
- const native = await EdgeAttestation.signChallenge(challenge)
+ native = await EdgeAttestation.signChallenge(challenge)
+ } catch (error) {
+ if (!isMissingKeyError(error)) throw error
+ console.log('[attestation] assertion unavailable:', String(error))
+ challenge = await fetchChallenge()
+ }
+ if (native != null) {
const assertResponse = await fetchInfo('v1/attest/android/assert', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
@@ -201,21 +220,17 @@
if (assertResponse.ok) {
return parseTokenResponse(await assertResponse.json())
}
- // Server rejected the assertion: drop the now-suspect cached token (see
- // the iOS branch above), guarded by the current generation, before
- // discarding the key and re-attesting.
- if (generation === handshakeGeneration) cachedToken = undefined
+ if (!KEY_REJECTION_STATUSES.has(assertResponse.status)) {
+ throw new Error(`assert request failed: ${assertResponse.status}`)
+ }
+ if (generation !== handshakeGeneration) return undefined
+ cachedToken = undefined
console.warn(
`[attestation] assertion rejected (${assertResponse.status}); re-attesting`
)
- await EdgeAttestation.clearKey().catch(() => {})
- } catch (error) {
- // noKey / native failure: fall through to full attestation.
- console.log('[attestation] assertion unavailable:', String(error))
+ await EdgeAttestation.clearKey()
+ challenge = await fetchChallenge()
}
- // The challenge above was consumed (or expired); fetch a fresh one for
- // the fallback attestation.
- challenge = await fetchChallenge()
}
// 2. Produce a platform attestation bound to the challenge.You can send follow-ups to the cloud agent here.
27e32bb to
4e5569c
Compare
271e330 to
1e8bbfe
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Watchdog drops late valid token
- Stale handshake completions now cache a late valid JWT when no fresher live token exists, so a post-watchdog failed retry no longer discards a successful older attestation during backoff.
Or push these changes by commenting:
@cursor push 98b1157448
Preview (98b1157448)
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -60,8 +60,9 @@
let inFlight: Promise<void> | undefined
let refreshTimer: ReturnType<typeof setTimeout> | undefined
let lastFailureAt = 0
-// Monotonic id of the latest handshake attempt; a resolved handshake only
-// commits its token when its generation is still current (see runHandshake).
+// Monotonic id of the latest handshake attempt; used so a stale (watchdog-
+// released) completion cannot clobber a token a newer handshake already
+// cached (see runHandshake).
let handshakeGeneration = 0
/** Test-only: clear module state between Jest cases. */
@@ -274,7 +275,13 @@
const generation = ++handshakeGeneration
const handshake: Promise<void> = performHandshake(generation)
.then(freshToken => {
- if (generation !== handshakeGeneration) return
+ if (generation !== handshakeGeneration) {
+ // Watchdog-superseded: a newer attempt already owns the generation.
+ // Still accept a late valid JWT when nothing fresher is cached (the
+ // newer attempt may have failed and entered backoff). Never clobber
+ // a live token a newer handshake already produced.
+ if (freshToken == null || hasLiveToken()) return
+ }
lastFailureAt = 0
if (freshToken != null) {
cachedToken = freshTokenYou can send follow-ups to the cloud agent here.
1e8bbfe to
8c97735
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Done
Or push these changes by commenting:
@cursor push 3d21ad7707
Preview (3d21ad7707)
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -175,7 +175,11 @@
)
await EdgeAttestation.clearKey().catch(() => {})
} catch (error) {
- // noKey / invalidKey / native failure: fall through to full attestation.
+ // noKey / invalidKey / native failure: the enrolled key is unusable
+ // (invalidKey also clears the stored key id natively), so drop any
+ // cached JWT before falling through to full attestation - same
+ // fail-closed semantics as a non-OK assert response above.
+ if (generation === handshakeGeneration) cachedToken = undefined
console.log('[attestation] assertion unavailable:', String(error))
}
// The challenge above was consumed (or expired); fetch a fresh one for
@@ -210,7 +214,9 @@
)
await EdgeAttestation.clearKey().catch(() => {})
} catch (error) {
- // noKey / native failure: fall through to full attestation.
+ // noKey / native failure: drop any cached JWT before falling through
+ // to full attestation (see the iOS catch above).
+ if (generation === handshakeGeneration) cachedToken = undefined
console.log('[attestation] assertion unavailable:', String(error))
}
// The challenge above was consumed (or expired); fetch a fresh one forYou can send follow-ups to the cloud agent here.
Introduce a background attestation engine (src/util/attestation.ts) that runs the platform attestation handshake (Apple App Attest / Android Keystore attestation) at app boot and caches a short-lived token, self-rescheduling to refresh it two minutes before expiry. getAttestationToken() returns the cached token immediately, or waits up to three seconds for the initial handshake before returning undefined. fetchInfo (util/network.ts) carries no attestation logic; the attestation-gated plugins (Simplex/Banxa jwtSign and createHmac) call getAttestationToken() and attach the x-attestation-token header themselves only when a token is available, otherwise letting the info server decide. Co-authored-by: Cursor <[email protected]>
e7359c8 to
d263e7f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Done
Or push these changes by commenting:
@cursor push f7173c7ecb
Preview (f7173c7ecb)
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -293,6 +293,14 @@
if (generation !== handshakeGeneration) return
lastFailureAt = Date.now()
console.warn('[attestation] handshake failed:', String(error))
+ // Keep the background engine alive across transient failures. The timer
+ // that fired this attempt is gone, and scheduleRefresh only runs on
+ // success — without a backoff retry the loop would stall until a gated
+ // caller or app restart kicks runHandshake again.
+ if (refreshTimer != null) clearTimeout(refreshTimer)
+ refreshTimer = setTimeout(() => {
+ runHandshake()
+ }, FAILURE_BACKOFF_MS)
})
.finally(() => {
if (inFlight === handshake) inFlight = undefinedYou can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Gated calls bypass exponential backoff
- runHandshake now uses the same exponential failureBackoffMs() as scheduleRetryAfterFailure, so gated plugin calls respect the growing backoff instead of a flat 60s gate.
Or push these changes by commenting:
@cursor push a2adf47c69
Preview (a2adf47c69)
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -273,16 +273,25 @@
}
/**
+ * Current failure backoff: base `FAILURE_BACKOFF_MS`, doubling per consecutive
+ * failure up to `MAX_BACKOFF_MS`. Shared by the background retry timer and the
+ * gated-call gate in `runHandshake` so plugin traffic cannot outpace the
+ * exponential policy.
+ */
+const failureBackoffMs = (): number =>
+ Math.min(
+ FAILURE_BACKOFF_MS * 2 ** Math.max(0, consecutiveFailures - 1),
+ MAX_BACKOFF_MS
+ )
+
+/**
* Schedule the next handshake after a failed or hung attempt. `scheduleRefresh`
* only runs on success, so without this the engine would sit idle until a gated
* call or an app restart. The wait doubles with each consecutive failure up to
* `MAX_BACKOFF_MS` to keep a hopeless device from re-attesting forever.
*/
const scheduleRetryAfterFailure = (): void => {
- const backoffMs = Math.min(
- FAILURE_BACKOFF_MS * 2 ** Math.max(0, consecutiveFailures - 1),
- MAX_BACKOFF_MS
- )
+ const backoffMs = failureBackoffMs()
// A cached token may still have most of its life left - a stale handshake can
// land one while a newer attempt is failing. Never retry sooner than
// `scheduleRefresh` would have, or a failing device would re-attest every
@@ -304,7 +313,7 @@
*/
const runHandshake = (): void => {
if (inFlight != null) return
- if (Date.now() - lastFailureAt < FAILURE_BACKOFF_MS) return
+ if (Date.now() - lastFailureAt < failureBackoffMs()) return
// A handshake whose native call hangs past the watchdog has its `inFlight`
// lock released so a newer handshake can start. Tag each attempt so a stale
// one that finally resolves cannot clobber a token a newer handshake alreadyYou can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Attest failure drops retryable key
- Persist generated key ids in a pending Keychain slot before attestKey and reuse them on retry so serverUnavailable no longer forces a new generateKey.
- ✅ Fixed: Past expiry triggers refresh loop
- parseTokenResponse now rejects already-past expires so a bad mint fails into backoff instead of scheduling a 0ms refresh loop.
Or push these changes by commenting:
@cursor push a009ba80ba
Preview (a009ba80ba)
diff --git a/ios/edge/EdgeAttestation.swift b/ios/edge/EdgeAttestation.swift
--- a/ios/edge/EdgeAttestation.swift
+++ b/ios/edge/EdgeAttestation.swift
@@ -6,11 +6,12 @@
/// Native bridge for iOS App Attest (app-level attestation).
///
-/// Exposes two methods to JS:
+/// Exposes methods to JS:
/// - isSupported(): resolves true only on real devices that support App Attest
-/// - getAttestation(challenge): generates a fresh App Attest key, attests it
-/// against SHA256(challenge), and resolves { keyId, attestation }, where
-/// attestation is the base64-encoded CBOR attestation object.
+/// - getAttestation(challenge): attests a pending (or freshly generated) App
+/// Attest key against SHA256(challenge), and resolves { keyId, attestation },
+/// where attestation is the base64-encoded CBOR attestation object
+/// - generateAssertion(challenge) / clearKey(): assertion refresh and reset
@objc(EdgeAttestation)
class EdgeAttestation: NSObject {
@objc static func requiresMainQueueSetup() -> Bool {
@@ -28,30 +29,35 @@
label: "co.edgesecure.app.appattest.serial"
)
- // Keychain persistence for the attested App Attest key id. App Attest private
- // keys live in the Secure Enclave keyed by this id; Apple recommends storing
- // the id in the Keychain so it survives across launches and is reused for
- // assertions (no re-attestation).
+ // Keychain persistence for App Attest key ids. App Attest private keys live
+ // in the Secure Enclave keyed by this id; Apple recommends storing the id in
+ // the Keychain so it survives across launches.
+ //
+ // `keyId` holds a successfully attested key (reused for assertions).
+ // `pendingKeyId` holds a key that was generated but not yet attested — kept
+ // across transient `attestKey` failures (e.g. serverUnavailable) so the next
+ // handshake retries with the same key instead of calling `generateKey` again.
private static let keychainService = "co.edgesecure.app.appattest"
private static let keychainAccount = "keyId"
+ private static let keychainPendingAccount = "pendingKeyId"
- private func storeKeyId(_ keyId: String) {
- clearKeyId()
- guard let data = keyId.data(using: .utf8) else { return }
+ private func storeAccount(_ account: String, value: String) {
+ clearAccount(account)
+ guard let data = value.data(using: .utf8) else { return }
let query: [String: Any] = [
kSecClass as String: kSecClassGenericPassword,
kSecAttrService as String: EdgeAttestation.keychainService,
- kSecAttrAccount as String: EdgeAttestation.keychainAccount,
+ kSecAttrAccount as String: account,
kSecValueData as String: data
]
SecItemAdd(query as CFDictionary, nil)
}
- private func loadKeyId() -> String? {
+ private func loadAccount(_ account: String) -> String? {
let query: [String: Any] = [
kSecClass as String: kSecClassGenericPassword,
kSecAttrService as String: EdgeAttestation.keychainService,
- kSecAttrAccount as String: EdgeAttestation.keychainAccount,
+ kSecAttrAccount as String: account,
kSecReturnData as String: true,
kSecMatchLimit as String: kSecMatchLimitOne
]
@@ -61,15 +67,39 @@
return String(data: data, encoding: .utf8)
}
- private func clearKeyId() {
+ private func clearAccount(_ account: String) {
let query: [String: Any] = [
kSecClass as String: kSecClassGenericPassword,
kSecAttrService as String: EdgeAttestation.keychainService,
- kSecAttrAccount as String: EdgeAttestation.keychainAccount
+ kSecAttrAccount as String: account
]
SecItemDelete(query as CFDictionary)
}
+ private func storeKeyId(_ keyId: String) {
+ storeAccount(EdgeAttestation.keychainAccount, value: keyId)
+ }
+
+ private func loadKeyId() -> String? {
+ loadAccount(EdgeAttestation.keychainAccount)
+ }
+
+ private func clearKeyId() {
+ clearAccount(EdgeAttestation.keychainAccount)
+ }
+
+ private func storePendingKeyId(_ keyId: String) {
+ storeAccount(EdgeAttestation.keychainPendingAccount, value: keyId)
+ }
+
+ private func loadPendingKeyId() -> String? {
+ loadAccount(EdgeAttestation.keychainPendingAccount)
+ }
+
+ private func clearPendingKeyId() {
+ clearAccount(EdgeAttestation.keychainPendingAccount)
+ }
+
@objc(isSupported:rejecter:)
func isSupported(
_ resolve: @escaping RCTPromiseResolveBlock,
@@ -103,26 +133,20 @@
EdgeAttestation.serialQueue.async {
let done = DispatchSemaphore(value: 0)
- // A fresh key is generated per handshake: an App Attest key can only be
- // attested once, so reuse would require the assertion flow instead.
- service.generateKey { keyId, error in
- if let error = error {
- reject("generateKey", error.localizedDescription, error)
- done.signal()
- return
- }
- guard let keyId = keyId else {
- reject("generateKey", "Failed to generate an App Attest key", nil)
- done.signal()
- return
- }
-
- // The client data is the challenge's UTF-8 bytes; the server recomputes
- // SHA256(challenge) to validate the attestation nonce.
+ // Attest `keyId` against SHA256(challenge). On success, promote the key
+ // from pending → attested so assertions can reuse it. On invalidKey,
+ // discard the pending id so the next call generates a fresh key. On
+ // other errors (notably serverUnavailable), leave the pending id so the
+ // next handshake retries attestKey with the same key — Apple's guidance.
+ let attestPending: (String) -> Void = { keyId in
let clientDataHash = Data(SHA256.hash(data: Data(challenge.utf8)))
-
service.attestKey(keyId, clientDataHash: clientDataHash) { attestation, error in
defer { done.signal() }
+ if let error = error as? DCError, error.code == .invalidKey {
+ self.clearPendingKeyId()
+ reject("invalidKey", "App Attest key is invalid", error)
+ return
+ }
if let error = error {
reject("attestKey", error.localizedDescription, error)
return
@@ -131,9 +155,8 @@
reject("attestKey", "Failed to produce an attestation object", nil)
return
}
- // Persist the key id so subsequent handshakes refresh via assertions
- // instead of a full (rate-limited) attestation.
self.storeKeyId(keyId)
+ self.clearPendingKeyId()
resolve([
"keyId": keyId,
"attestation": attestation.base64EncodedString(),
@@ -142,6 +165,28 @@
}
}
+ // Retry a previously generated but unattested key when present; otherwise
+ // generate a new one and persist it before attestKey so a transient
+ // failure does not orphan the Secure Enclave key.
+ if let pendingKeyId = self.loadPendingKeyId() {
+ attestPending(pendingKeyId)
+ } else {
+ service.generateKey { keyId, error in
+ if let error = error {
+ reject("generateKey", error.localizedDescription, error)
+ done.signal()
+ return
+ }
+ guard let keyId = keyId else {
+ reject("generateKey", "Failed to generate an App Attest key", nil)
+ done.signal()
+ return
+ }
+ self.storePendingKeyId(keyId)
+ attestPending(keyId)
+ }
+ }
+
done.wait()
}
}
@@ -197,6 +242,7 @@
) {
EdgeAttestation.serialQueue.async {
self.clearKeyId()
+ self.clearPendingKeyId()
resolve(nil)
}
}
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
@@ -132,6 +132,34 @@
await expect(tokenPromise).resolves.toBeUndefined()
})
+ it('rejects attest responses with an already-past expires', async () => {
+ mockFetchInfo.mockImplementation(async (path: string) => {
+ if (path === 'v1/attest/challenge') {
+ return jsonResponse({ challenge: 'chal-1' })
+ }
+ return jsonResponse({ token: 'jwt-token', expires: Date.now() - 1 })
+ })
+
+ initAttestation()
+ const tokenPromise = getAttestationToken()
+ await jest.advanceTimersByTimeAsync(
+ attestationTimingForTests.GET_TOKEN_TIMEOUT_MS
+ )
+ await expect(tokenPromise).resolves.toBeUndefined()
+
+ // Past expiry must fail the handshake (backoff), not schedule a 0ms refresh
+ // that would tight-loop challenge/attest as fast as the network allows.
+ const attestCallsBefore = mockFetchInfo.mock.calls.filter(
+ ([path]) => path === 'v1/attest/apple' || path === 'v1/attest/android'
+ ).length
+ await jest.advanceTimersByTimeAsync(1)
+ await flush()
+ const attestCallsAfter = mockFetchInfo.mock.calls.filter(
+ ([path]) => path === 'v1/attest/apple' || path === 'v1/attest/android'
+ ).length
+ expect(attestCallsAfter).toBe(attestCallsBefore)
+ })
+
it('caches a token when expires is a finite number (Task 2.1)', async () => {
const expires = Date.now() + 10 * 60 * 1000
mockSuccessfulHandshake(expires)
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -124,10 +124,10 @@
/**
* Validate an attest/assert token response. Both `token` and `expires` are
* validated; a malformed response throws and is treated as a failed handshake
- * (a non-finite `expires` would otherwise fire `setTimeout` immediately and
- * spin the handshake loop). The parsed token is returned to the caller rather
- * than cached directly, so `runHandshake` can drop a stale (watchdog-released)
- * result before it clobbers a fresher token.
+ * (a non-finite or already-past `expires` would otherwise fire `setTimeout`
+ * immediately and spin the handshake loop). The parsed token is returned to
+ * the caller rather than cached directly, so `runHandshake` can drop a stale
+ * (watchdog-released) result before it clobbers a fresher token.
*/
const parseTokenResponse = (json: unknown): CachedToken => {
const { token, expires } = (json ?? {}) as {
@@ -140,6 +140,10 @@
if (typeof expires !== 'number' || !Number.isFinite(expires)) {
throw new Error('attest response missing expires')
}
+ // A past expiry schedules a 0ms refresh and would tight-loop the engine.
+ if (expires <= Date.now()) {
+ throw new Error('attest response expires is in the past')
+ }
return { token, expires }
}You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Watchdog double-counts failures
- Watchdog now advances handshakeGeneration when releasing a hung attempt so a later rejection of the same attempt cannot increment consecutiveFailures again.
Or push these changes by commenting:
@cursor push 3766c35dbd
Preview (3766c35dbd)
diff --git a/src/util/attestation.ts b/src/util/attestation.ts
--- a/src/util/attestation.ts
+++ b/src/util/attestation.ts
@@ -400,10 +400,13 @@
// A hang leaves nothing to re-arm the loop: the hung attempt either never
// settles or rejects once a newer generation owns the state, and neither
// schedules a retry. Retry instead, or the engine sits idle until a gated
- // call. `lastFailureAt` stays untouched so a gated caller can still start a
- // fresh handshake right away. A hang inside the native attestation counts
- // against the backoff just like a rejection - the quota is spent either
- // way.
+ // call. Advance generation now so a late reject during the backoff window
+ // cannot double-count this attempt (or set `lastFailureAt`) after the
+ // watchdog already armed a retry. `lastFailureAt` stays untouched so a
+ // gated caller can still start a fresh handshake right away. A hang inside
+ // the native attestation counts against the backoff just like a rejection
+ // - the quota is spent either way.
+ handshakeGeneration += 1
if (attempt.usedAttestation) consecutiveFailures += 1
scheduleRetryAfterFailure()
}, HANDSHAKE_WATCHDOG_MS)You can send follow-ups to the cloud agent here.
bd63feb to
1e4a65a
Compare
Review fixes for the attestation engine. Keep the background loop alive. A proactive refresh that failed left the engine idle until the next gated call, and a handshake abandoned by the watchdog rescheduled nothing at all. Both now arm a retry. Back off by what a failure cost. Everything up to getAttestation is a plain info-server round trip, so an offline device keeps retrying at the floor, while attempts that burn a rate-limited platform attestation double the wait up to a cap. Gated callers share that policy: the Banxa order poll runs every 3s, so a flat gate let plugin traffic re-attest every minute no matter how far out the backoff had grown. Never spin. The refresh delay has a floor, since the token lifetime is remote config and a value below the refresh lead would put every client into a continuous handshake loop, and a token that is unusable on arrival now fails into backoff instead of being cached. Retire an abandoned handshake's generation when the watchdog releases the lock, so a late settle cannot count one failure twice or clobber state a newer attempt owns. A late valid JWT is still accepted when nothing live is cached. On iOS, keep a generated-but-unattested App Attest key for a serverUnavailable retry, per Apple's guidance, instead of burning a new key on every attempt.
1e4a65a to
88fbb5c
Compare
Make the backoff the previous fixup added actually take effect. Record the failure time when the watchdog gives up, not just the failure count. The gate reads a timestamp, so counting alone left it reading one no hang ever set: a device whose native call keeps hanging started a fresh handshake on the next gated call after every watchdog release, burning one platform attestation per window and stalling every gated request for the full token timeout - the latency the backoff exists to avoid. Stop a retired attempt instead of guarding a single line of it. The generation check protected the cached token and nothing else, so an attempt the watchdog had abandoned still cleared the key a live handshake had just enrolled, and still spent a rate-limited attestation nobody was waiting on. Keep an unusable token response out of the re-attestation path. The "expires must be in the future" check landed inside the assert fast path's try, which answers everything by falling through to a full attestation - so a bad mint or a device clock ahead of the token lifetime escalated to spending rate-limited quota every backoff window rather than failing into backoff. Only a missing/invalid key or an outright rejection escalates now. The two near-identical platform branches are one helper, since both had to change the same way. Enforce the "in flight or armed" invariant rather than asserting it. All arming goes through one place, and the path that declines to start a handshake re-arms instead of swallowing the tick that woke it, which a backwards wall-clock step could otherwise turn into a permanent idle. A rejection from isSupported is a bridge failure and retries; only an explicit false is terminal, and that now stops the engine outright. Floor the spacing between handshakes so no outcome can drive a tight loop, and clear a watchdog when its attempt settles. On iOS, bound how long one operation may hold the serial queue. An attestKey that never calls back used to wedge it for the life of the process, blocking every later key operation including the clearKey the JS engine recovers with. Promises settle once, since the timeout and a late callback can both reach for the same one. clearKey no longer discards the pending key: the server rejecting an assertion says nothing about a key it has never seen, and dropping it throws away the retry the previous fixup added it for. Drain microtasks generously in the tests. A short drain sampled a half-finished handshake and read as "nothing happened", which is part of why these races passed review. Co-authored-by: Cursor <[email protected]>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9fe9041. Configure here.
Reserve re-attestation for the two things it can actually fix. Do not read a failure to answer as a verdict on the key. Any non-OK assert response used to clear the enrolled key and spend a platform attestation, so a 5xx from the info server - or a 429 from it throttling us - would have every device in the fleet discard its key and re-attest at once. That is a fleet-wide run at the Apple and Android rate limits triggered by an outage that fixes itself. Only a 4xx other than 429 is the server judging the key; anything else fails into backoff and keeps the key. Same distinction on the native side: a signature that timed out says nothing about whether the key can sign, so it retries the cheap path rather than escalating. noKey, invalidKey and an outright signing failure still escalate, since re-enrolling is the only way forward and being stricter would strand a device with an unusable key. On iOS, do not enrol a key from an attestKey callback that lost the race with the operation timeout. The attestation object went out with the handshake that already failed, so the server never verified that key and would reject an assertion from it - storing it only costs the next handshake a round trip before it re-attests anyway. The key still leaves pendingKeyId, because attestKey succeeded and it can never be attested again. Co-authored-by: Cursor <[email protected]>
|
bugbot run |
|
@cursor review |
|
Current version of PR was reviewed by /review-bugbot with additional instructions on Jul 29, 10:28 PDT. It flagged 1 finding. Show 1 finding1. Late App Attest callbacks wipe newer keys
After the 120s Bugbot on commit |
Stop a stale App Attest callback from clearing a newer key. Giving up on the 120s operation timeout releases the serial queue while the DCAppAttest callback is still outstanding, so it can run much later alongside a newer operation. Its verdict is about the key it was handed, but every clear it performed applied to whatever was stored at that moment. A newer handshake that had generated or enrolled a different key would lose it: clearPendingKeyId threw away a key being held for retry, forcing another generateKey, and the invalidKey path deleted a live enrolled key, forcing a full rate-limited attestation to replace one that was working fine. Make each callback-initiated clear conditional on the stored id still being the one that operation was working on. Read-then-delete is not atomic, so a callback racing a mid-write operation can still clear the newer id, but that window is microseconds against the two-minute one it closes and it costs a retry rather than corrupting state. clearKey from JS stays unconditional: it runs on the queue and is about whatever is enrolled now. Also drop the claim that a late callback touching the Keychain was the lesser evil, which stopped being true once a late success no longer enrolled its key. Co-authored-by: Cursor <[email protected]>
Keep Android clearKey off the native-modules thread. getAttestation and signChallenge each do their Keystore work on a spawned Thread, but clearKey took keystoreLock on the calling thread - the shared native-modules thread. Attested EC key generation is slow, more so for StrongBox, so an in-progress getAttestation can hold that lock for seconds, and every other native module in the app would wait behind it. JS calls clearKey exactly when a handshake is already in flight, which is when the lock is most likely to be held. The docs claimed all three methods already ran off the JS thread, which was only true of the other two. Co-authored-by: Cursor <[email protected]>
|
bugbot run |
Scope clearKey to the key JS meant, and bound the Android lock. clearKey deleted whatever was enrolled at the moment it ran, on both platforms. It can wait a long time for the queue or the Keystore lock, and by then a newer handshake may have enrolled a replacement - so a 401 about one key could take out its successor, leaving a cached JWT with no matching key and forcing a needless re-enrollment. JS now names the key the server actually refused and native drops it only while it is still the stored one. The previous round asserted this call was safe because it runs under the lock and concerns whatever is enrolled now; that reasoning was wrong, and the note claiming it is gone. Bound how long Android waits for the Keystore lock. Keystore work is local and synchronous, so a wedged generateKeyPair or sign cannot be interrupted the way the iOS semaphore bounds a hung attestKey - but the operations behind it need not wait forever, which is what kept one wedge from disabling every later attestation, refresh and clear for the life of the process. keystoreLock becomes a ReentrantLock with a 60s tryLock, below the JS watchdog so JS gets a real answer, rejecting with the timeout code the engine already reads as transient. That also makes the transient path reachable on Android, which previously emitted no code the engine treated as retryable. Not changed: asserting the generation again after getAttestation returns. The platform attestation is already spent by that point, so aborting saves no quota, and it would defeat the deliberate salvage of a late valid JWT when nothing is cached - which has its own test. Co-authored-by: Cursor <[email protected]>
|
bugbot run |
Correct the clearKey comment about how it settles. It claimed the method resolves regardless, which stopped being true when failing to acquire the Keystore lock became a timeout rejection. Co-authored-by: Cursor <[email protected]>
Fix the Objective-C bridge to match the new clearKey selector. Adding the keyId argument changed the Swift selector to clearKey:resolver:rejecter:, but EdgeAttestation.m still declared the promise-only clearKey:rejecter:. Nothing catches that: swiftc never reads the bridge, and an RCT_EXTERN_METHOD mismatch is not a build error - React Native only finds out on the device, as a selector that fails to resolve. It would have disabled clearKey on iOS entirely, which is the only path that recovers from a key the server has rejected. Add a test comparing the selectors the two files declare, so a signature change cannot ship half-applied again. Confirmed it fails against the unfixed bridge. It also pins that clearKey still takes a keyId, since matching selectors alone would not notice the argument disappearing from both files at once. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Done
Or push these changes by commenting:
@cursor push b295f48a55
Preview (b295f48a55)
diff --git a/ios/edge/EdgeAttestation.m b/ios/edge/EdgeAttestation.m
--- a/ios/edge/EdgeAttestation.m
+++ b/ios/edge/EdgeAttestation.m
@@ -19,7 +19,8 @@ @interface RCT_EXTERN_MODULE (EdgeAttestation, NSObject)
rejecter:(RCTPromiseRejectBlock)reject)
RCT_EXTERN_METHOD(clearKey
- : (RCTPromiseResolveBlock)resolve
+ : (NSString *)keyId
+ resolver:(RCTPromiseResolveBlock)resolve
rejecter:(RCTPromiseRejectBlock)reject)
@end
@@ -19,7 +19,8 @@ @interface RCT_EXTERN_MODULE (EdgeAttestation, NSObject)
rejecter:(RCTPromiseRejectBlock)reject)
RCT_EXTERN_METHOD(clearKey
- : (RCTPromiseResolveBlock)resolve
+ : (NSString *)keyId
+ resolver:(RCTPromiseResolveBlock)resolve
rejecter:(RCTPromiseRejectBlock)reject)
@endYou can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit fda3866. Configure here.


Summary
x-attestation-tokento SimplexjwtSignand BanxacreateHmacrequests so gated signing can require hardware attestation.docs/APP_ATTESTATION.mdand adds an optionalINFO_SERVERenv override for local testing.Test plan
Made with Cursor