feat(auth): harden device authorization and native admission - #5021
Merged
Conversation
Restore consumed device requests after issuance failures, delete orphan sessions, and cover the repair paths.
Apply the repository formatter to the remaining changed test file.
Preserve refresh serialization and narrow device-auth poll terminal states under strict TypeScript.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of commit Files Reviewed (16 files)
Previous Review Summaries (4 snapshots, latest commit 2892037)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 2892037)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of commit Files Reviewed (5 files)
Previous review (commit bc1e291)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of commit Files Reviewed (8 files)
Previous review (commit da44fa5)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of the follow-up commit Files Reviewed (11 files)
Skipped per review rules
Previous review (commit 74bbfac)Status: 6 Issues Found | Recommendation: Address before merge Executive SummaryA critical challenge-id regression in Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (132 files)
Reviewed by kimi-k3 · Input: 101.5K · Output: 22.8K · Cached: 1.1M Review guidance: REVIEW.md from base branch |
…stall state Close the C15 Category B gap: KiloClaw read api_token_pepper but never blocked_reason, so a blocked account kept access until its pepper rotated. Three paths minted or accepted credentials for blocked users: - authMiddleware: every user-facing route - access-gateway: the cookie JWT minted after access-code redemption - kiloclaw-instance/config: the fresh Kilo API key minted for a sandbox Also relax mintInstallState to accept any organization member. The pre-C1 callback called ensureOrganizationAccess with no role filter, so restricting the mint to owner/billing_manager would have removed an existing capability from members. Drop verifyAndConsumeSignInCode, which lost its last production caller when the token route moved to reserve/settle/commit. Its coverage moves into the test file as a local helper.
…d decoders
The verifier carried its own CBOR decoder, DER TLV reader, and SPKI writer.
Hand-rolled parsers on untrusted attestation bytes are the wrong thing to own,
so delegate all three and keep only the App Attest rules.
- CBOR decoding moves to cbor2. It throws on truncated input and on trailing
bytes, so malformed payloads still fail closed.
- The nonce extension is now located by parsing the certificate as X.509 and
matching the extension OID, instead of scanning the raw DER for the OID
bytes. A nonce-shaped byte run elsewhere in the certificate can no longer be
mistaken for the extension.
- The inner SEQUENCE { [1] EXPLICIT OCTET STRING } grammar moves to asn1js and
is exported as parseAppleAttestNonceExtension, so the grammar stays unit
testable without re-signing a certificate.
- COSE coordinates go through a JWK and Node builds the SPKI encoding. The
output is byte-identical to the previous writer, and Node now rejects a
coordinate pair that is not a point on P-256.
Net 263 lines deleted, 98 added. Jest needs cbor2 and its ESM-only wtf8
dependency transformed, so both join the existing allowlist.
The client half of native admission was a stub: hasAttestationCapability returned a hardcoded false and getAdmission returned empty payloads, so no device ever sent an admission field and the server always took its counted legacy path. Wire it to @expo/app-integrity, which covers App Attest on iOS and Play Integrity on Android and ships with the Expo 57 SDK line. iOS: attest a fresh Secure Enclave key on first use, assert with the stored key id afterwards. An ERR_APP_INTEGRITY_INVALID_KEY rejection clears the stored id and re-attests once, so a reinstall or a device restore recovers instead of asserting against a key that no longer resolves. The key id is stored only after the attestation succeeds. Android: standard Play Integrity requests, chosen over classic for the warmed token provider and its better quota behaviour. The provider is prepared once per app launch behind a shared promise, and a rejected preparation is dropped so the next sign-in retries. Two server-side conventions had to match the library, and neither did: - Expo hashes the UTF-8 bytes of the challenge string into clientDataHash (Data(challenge.utf8) through SHA256). The server was hashing the base64url-decoded challenge, so every attestation and every assertion would have failed with a nonce or signature mismatch. Both paths now share one exported appAttestClientDataHash so they cannot drift. - Standard Play Integrity requests bind through requestDetails.requestHash and carry no nonce field. The server read only nonce, so every Android verdict would have been refused as NONCE_MISMATCH while the log said "missing nonce". It now reads requestHash and falls back to nonce so a classic-request build is not refused mid-rollout. A provider failure returns undefined rather than aborting sign-in. Admission is server-side policy and the server holds the mode switch, so failing closed in the client would lock out rooted devices, emulators, and devices without Play Services even while NATIVE_ADMISSION_MODE is off. Adds the App Attest entitlement, so iOS needs a new native build. The Play Integrity cloud project number reaches the client through the existing optional-config map; absent, Android reports no capability and sends nothing.
Contributor
Author
|
Note to self: |
…-8c25 Resolve conflicts: - auth-context.tsx: keep the telemetry teardown from main before the branch credential-write lock in signOut. - auth-context.test.ts / .test.tsx / trpc.test.ts: mock react-native, Sentry and the telemetry modules that auth-context now imports. - vitest.pure.config.ts: include both new test globs. - pnpm-lock.yaml: regenerate.
…-8c25 Resolve the drizzle migration conflict: main added 0204_drop_cost_insight_tables, so replace the branch migrations 0204 and 0205 with one regenerated 0205 that chains off main's snapshot. Re-apply the hand-written parts: the transaction splits around the CONCURRENTLY indexes and the GitHub installation dedup backfill.
jeanduplessis
approved these changes
Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Device-auth and native admission security foundation
Summary
What: Harden GitHub install callbacks with random, expiring, single-use state, GitHub admin proof, app-type-aware uniqueness, and scoped lifecycle mutations.
Why: Prevent forged callback state, unauthorized installation claims, cross-owner attachment, and sibling app mutation.
How: Store and atomically consume state, prove GitHub administration when OAuth code exists, resolve duplicate claims, and scope lookups and mutations by app type and installation id.
What: Split device authorization into a displayed user code and a separate high-entropy device secret.
Why: A displayed code must not act as a polling credential.
How: Store only a SHA-256 device-secret digest, poll through POST, atomically consume approved requests, and protect denial with a viewer token.
What: Add refreshable per-device sessions and preserve legacy token compatibility.
Why: Native credentials need shorter lifetimes and revocation without signing out old app builds.
How: Issue one-hour access tokens and rotating 30-day refresh tokens for opted-in clients, exchange stored old tokens on updated launch, and revoke at refresh.
What: Add native Apple nonce binding, Google authorization-code exchange, and native admission verification.
Why: Native identity assertions and new-device sign-in need replay and abuse protection.
How: Bind Apple nonce, exchange Google server auth codes, verify App Attest and Play Integrity server-side, and keep a counted legacy path until rollout enforcement.
What: Block credential minting and shared-verifier access for blocked users.
Why: Blocked accounts must not receive fresh device credentials.
How: Deny approved device requests, revoke sessions and refresh tokens, and check
blocked_reasonin the existing shared verifier.What: Preserve OTP codes through failed settlement and isolate attempt budgets.
Why: A server-side settlement failure must not consume a valid code or let another caller exhaust it.
How: Reserve, settle, and consume codes; add opaque challenge ids; equalize blocked-TLD responses with a 250ms floor.
What: Delay native sign-in analytics until authoritative gates pass.
Why: Refused attempts must not emit success events.
How: Defer the existing event payload in the native route and emit it only before a successful response.
What: Return GitHub app outcomes to the mobile app with a fallback.
Why: Users need a visible result when universal links do not open.
How: Use claimed
/cloud/sessionsrouting, preserve organization context, show four outcome states, and retry with a fresh install state.What: Prove compatibility and deterministic auth flows.
Why: Existing extension, web, and token consumers must remain usable.
How: Run compatibility suites and verify device auth, OTP, GitHub returns, refresh, revocation, and admission paths on iOS and Android.
Acceptance evidence
Rollout requirements
reportmode first.enforceonly after native App Attest and Play Integrity builds are available and the E2E evidence passes.enforce, clients that declare admission support fail closed on invalid, expired, replayed, or mismatched verdicts.deviceSessionId; updated clients exchange them on first launch.Deployment precondition
coderequirement hard-fail when absent.Known remaining work
blocked_reason; it needs its own section.NATIVE_ADMISSION_MODE=enforcewas not enabled in this environment; unit tests cover the enforce contract.Non-goals
Visual Changes