Skip to content

fix(auth): compare App Attest key ids as bytes - #5052

Merged
iscekic merged 2 commits into
mainfrom
fix/app-attest-key-id-encoding
Aug 5, 2026
Merged

fix(auth): compare App Attest key ids as bytes#5052
iscekic merged 2 commits into
mainfrom
fix/app-attest-key-id-encoding

Conversation

@iscekic

@iscekic iscekic commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Problem

Every iOS sign-in fails with "Your device can't be verified" on the latest TestFlight build.

DCAppAttestService.generateKey returns the key id in standard base64, and @expo/app-integrity forwards it verbatim (ios/IntegrityModule.swift:17). The verifier re-encoded the credential id from authData as base64url and compared the two strings. For a 32-byte key id that is 44 chars with a = pad versus 43 chars with none, so the comparison can never succeed.

Result: every first-time iOS attestation returns KEY_ID_MISMATCHADMISSION_REQUIRED. Under enforce mode that is a hard sign-in block. Android is unaffected — Play Integrity needs no persistent key row.

The existing test built its fixture key id with .toString('base64url'), so it agreed with the bug instead of catching it.

Evidence

Axiom vercel dataset. From 16:52:57 UTC every admission-challengetoken pair is a 403, roughly 200 ms apart:

16:52:57  admission-challenge 200   →  16:53:00  token 403
16:53:27  admission-challenge 200   →  16:53:27  token 403
16:54:21  admission-challenge 200   →  16:54:21  token 403
16:55:22  admission-challenge 200   →  16:55:22  token 403
16:55:54  admission-challenge 200   →  16:55:54  token 403

Challenge traffic begins at 13:00 UTC with no 403s until ~16:52, which matches the mode moving from report to enforce.

Changes

  • native-admission-apple.ts — compare the decoded bytes. Node's base64 decoder accepts both alphabets, so either wire form verifies. The credential-id check now runs before the nonce hash: it is cheaper, and it is reachable in tests.
  • auth-fetch.ts — clear the stored key id when the server refuses admission. The client stored the id as soon as attestKeyAsync resolved, which says nothing about the server persisting it, so a refused device asserted against an unknown key forever. The guard sits in postAuth, where every native auth POST already routes through, so no future sign-in path can forget it.

Tests

5 new tests. 3 of them fail without the encoding fix.

  • web: 270 pass, typecheck clean
  • mobile: 3564 pass, lint clean

The 4 mobile typecheck errors (watermarkEventId, kilo-chat arity) are pre-existing on clean main from a stale @kilocode/trpc dist, not from this change.

Rollout

Devices that already attempted a sign-in hold a key id the server never persisted, so the server fix alone does not recover them — they need this mobile change in a new build, or an app reinstall to clear the keychain item.

Setting NATIVE_ADMISSION_MODE=report unblocks every iOS user immediately, independent of this PR.

Apple's `DCAppAttestService.generateKey` returns the key id in standard
base64. The verifier re-encoded the credential id from authData as
base64url and compared the strings, so the padding alone made every
first-time iOS attestation fail with KEY_ID_MISMATCH. Under enforce mode
that surfaced as "Your device can't be verified" on every iOS sign-in.

Compare the decoded bytes instead. Node's base64 decoder accepts both
alphabets, so either wire form verifies. The credential-id check now runs
before the nonce hash, which is both cheaper and reachable in tests.

The client stored the key id as soon as `attestKeyAsync` resolved, which
says nothing about the server persisting it, so a refused device then
asserted against an unknown key forever. Clear the stored id when the
server refuses admission, in `postAuth`, where every native auth POST
already routes through.
@iscekic iscekic self-assigned this Aug 5, 2026
Comment thread apps/web/src/lib/auth/native-admission-apple.ts
@kilo-code-bot

kilo-code-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Incremental review of commit b880c0e6: the previous WARNING (uncaught RangeError on 37–54 byte authData surfacing as a 500) is verified fixed — the length floor is raised to 55, the exact minimum for readUInt16BE(53) to parse safely, and a regression test covers the 37/48/54-byte window; no new issues found.

Files Reviewed (2 files)
  • apps/web/src/lib/auth/native-admission-apple.ts
  • apps/web/src/lib/auth/native-admission-apple.test.ts
Previous Review Summary (commit 177ad68)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 177ad68)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/auth/native-admission-apple.ts 192 Reordering the credential-ID parse before the nonce check makes authData.readUInt16BE(pos) throw an uncaught RangeError on attacker-controlled truncated authData (37–54 bytes), surfacing as INTERNAL_ERROR 500 instead of a clean 4xx refusal. Add a length guard so malformed input fails closed.

The core fix is correct: comparing decoded bytes instead of re-encoded strings resolves the base64/base64url padding mismatch that blocked every iOS attestation, Node's decoder accepting both alphabets keeps either wire form working, and the mobile key-id cleanup in postAuth closes the stuck-device loop with no circular-import or memory-leak concerns.

Files Reviewed (5 files)
  • apps/web/src/lib/auth/native-admission-apple.ts - 1 issue
  • apps/web/src/lib/auth/native-admission-apple.test.ts
  • apps/mobile/src/lib/auth/admission.ts
  • apps/mobile/src/lib/auth/admission.test.ts
  • apps/mobile/src/lib/auth/auth-fetch.ts

Fix these issues in Kilo Cloud


Reviewed by kimi-k3 · Input: 50.1K · Output: 7.1K · Cached: 271.4K

Review guidance: REVIEW.md from base branch main

The credential-ID parse reads a uint16 at offset 53, so it needs 55 bytes,
but the only length guard allowed 37. Moving that parse ahead of the nonce
check made offsets 37 to 54 reachable with crafted authData, where
readUInt16BE throws a RangeError and the route reports a 500 instead of
refusing admission.

Raise the floor to 55, the shortest authData that can hold the credential-ID
length prefix. A real attestation always exceeds it.
@iscekic
iscekic enabled auto-merge (squash) August 5, 2026 17:46
@iscekic
iscekic merged commit 1602dd0 into main Aug 5, 2026
21 checks passed
@iscekic
iscekic deleted the fix/app-attest-key-id-encoding branch August 5, 2026 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants