Conversation
…ation
`AppCheckCoreAppAttestKeyIDStorage.getAppAttestKeyID()` reports "no key
stored" by throwing `appAttestKeyIDNotFound`, never by returning nil
(matching the rejected promise in the ObjC implementation). The rewritten
`attestationState()` used `try await`, so that error propagated out and
failed the entire token fetch, making the `guard let ... else { return
supportedInitialState }` fallback unreachable dead code.
Since a fresh install has no stored key ID, App Attest attestation failed
100% of the time on first launch.
In ObjC this worked because `FBLPromiseAwait` returned nil on rejection and
the out-error was deliberately ignored, letting the nil check fall through
to the initial state. Restore that behavior for both the key ID lookup and
the artifact lookup (a transient Keychain read failure now degrades to
re-attesting rather than failing the fetch).
The bug was hidden because `MockAppAttestKeyIDStorage` was scripted with
`.success(nil)`, a value the real storage never produces. The shared test
helper now throws `appAttestKeyIDNotFound` instead, so the whole suite
exercises the real storage contract; reverting the source fix now fails 5
tests.
Also fixes a merge artifact in MockRecaptchaSupport.swift that left a stray
`request:` argument and broke compilation of the Recaptcha test target.
…c level Three parity regressions in the logger: 1. `#if !NDEBUG` never compiled anything out. `NDEBUG` is a C preprocessor macro and is not a Swift compilation condition, so the expression was unconditionally true and all logging shipped in Release builds. The ObjC `GACAppCheckLog` compiled logging out via the same-named C macro, which did work there. This leaked App Check debug tokens into Release logs. Verified: the release build of AppCheckCoreLogger.swift.o now contains 0 occurrences of the log format string (debug contains 1). 2. `print` replaced `NSLog`, so output no longer reached the system log and was invisible in Console.app without a debugger attached. 3. `logLevel` was a plain static var. The ObjC class property was `atomic`, backed by a `volatile` static. Restored serialized access via a lock. Tests: restore the default log level in tearDown so the global state cannot leak between tests, and add a concurrent read/write test for `logLevel`.
…ests Two divergences in `getToken(limitedUse:)` request coalescing. 1. The chaining path used `_ = try? await ongoingTask.value`, swallowing the in-flight operation's failure so every queued caller went on to start a brand new attestation sequence. ObjC chained with `.thenOn`, which runs only on success, so a chaining caller was rejected with that same error (GACAppAttestProvider.m:213-216). During an outage this turned N queued callers into N sequential full attestation attempts instead of N fast failures, and handed callers a different error than the one that actually occurred. 2. The ongoing-operation slot was cleared in the originating caller's `defer`, not inside the operation. ObjC cleared it via `.thenOn`/ `.recoverOn` chained onto the operation itself (L228-236), guaranteeing the slot was nil before any chained waiter resumed. With the clear in the caller, a waiter could resume while the completed task was still parked in the slot and spin through repeated `.retry` recursions until the originator happened to get scheduled. Also removes the errSecDuplicateItem swallow in AppCheckCoreStorage (commit 1b743a7, added to quiet a test flake). It resumed the continuation with success on a failed keychain write, so the in-memory token silently diverged from what was actually persisted. ObjC propagated all keychain errors. The flake is fixed properly in the tests instead, by scoping each test's token key with a per-instance UUID and cleaning up in tearDown. Adds `GACAppCheckErrors.errorDomain` so Objective-C can still reach the error domain; Swift global constants are not bridged, so v11's `GACAppCheckErrorDomain` global had disappeared from the ObjC API entirely. Tests: adds the first coverage of the documented concurrent-request contract (chained-caller error propagation, and standard-request coalescing), plus an ObjC test pinning the error domain string. Verified both new coalescing tests fail against the pre-fix implementation.
In v11 every public completion handler was invoked from a bare FBLPromise
`.then` / `.catch`. Those dispatch onto `FBLPromise.defaultDispatchQueue`,
which `+[FBLPromise initialize]` sets to `dispatch_get_main_queue()` and
which nothing in this library or its known consumers ever reassigns. The
effective contract was therefore: completion handlers always arrive on the
main queue, and always asynchronously (FBLPromise uses an unconditional
`dispatch_group_async` with no same-queue fast path).
Nothing at the v11 call site mentions a queue, so the guarantee was invisible
and was dropped during the Swift port: `Task {}` at a nonisolated call site
resumes on the cooperative pool. UI-layer callers touching UIKit from the
handler would hit timing-dependent main-thread violations.
Restores main-queue delivery for:
- token(forcingRefresh:completion:)
- limitedUseToken(completion:)
- periodicTokenRefresh(completion:)
- tokenRefresher.updateWithRefreshResult + tokenDelegate.tokenDidUpdate,
which v11 ran inside the same bare `.then` in -[GACAppCheck refreshToken]
The delegate notification is awaited rather than fire-and-forget because the
v11 promise resolved only after that block ran, so callers were guaranteed the
delegate had already been notified before receiving the token.
The `async` variants intentionally keep resuming on the cooperative pool;
async callers are expected to hop themselves, and forcing a hop would be a new
divergence rather than parity.
Adds AppCheckCoreMainThreadDeliveryTests (6 cases) covering success, error,
off-main callers, and the delegate. All 6 fail without the source change.
…erage
Restores 8 cases that existed in the v11 Objective-C suite but had no Swift
equivalent after the rewrite. All are paths where the server returns HTTP 200
with an unusable body, or where a storage key is constructed — the kind of
coverage whose absence is invisible because the happy path and the HTTP-error
path still pass without it.
GACAppAttestAPIServiceTests (7 cases):
- getRandomChallenge with an empty response body
- getRandomChallenge with a non-JSON response body
- getRandomChallenge with valid JSON missing the `challenge` field
- getAppCheckToken network error / unexpected response
- attestKey network error / unexpected response
All 7 passed on first run, which is itself the parity evidence: the Swift
implementation already emits the same failure reasons as v11 ("Empty server
response body.", "JSON serialization error.", and naming the missing
`challenge` field) and propagates transport errors unwrapped rather than
translating them.
GACAppCheckDebugProviderTests (1 case):
- registeredUserDefaultsKey sanitization: `/` becomes `_`, empty components
fall back to "default"
This key namespaces the "debug token already registered" flag. A change to its
construction would silently orphan the flag for every existing install and
force a spurious re-registration, so it is worth pinning. The v11 test also
covered nil arguments; those are unrepresentable in Swift since both
parameters are non-optional String, so only the empty-string fallback is
reachable.
Full suite: 167 tests, 0 failures.
`pod lib lint` builds via xcodebuild against the podspec, which does not inherit `Package.swift`'s `swiftLanguageModes: [.v5]`. It therefore surfaced four `capture of non-Sendable type` warnings in the main-queue delivery added in 5007f69, and escalated them to fatal. `swift test` could not see them. Rebind the crossing values with `nonisolated(unsafe) let` rather than declaring the types `Sendable`: the refresher, the token delegate and the completion handler are all caller-supplied, so the library cannot promise thread-safety on the caller's behalf. The boundary itself is unchanged and matches the unsynchronized `dispatch_async` hand-off v11 performed. Also record in agents.md that the Ruby 2.7.5 override is now harmful, and that `swift build -Xswiftc -strict-concurrency=complete` reproduces the lint failure in seconds.
The prior note claimed SwiftPM suppresses concurrency diagnostics because of swiftLanguageModes: [.v5]. That is wrong. Verified empirically by reverting the fix and rebuilding: plain swift build emits all four #SendableClosureCaptures warnings at the same lines pod lib lint reported. The real difference is escalation, not visibility - SwiftPM exits 0 and the warnings scroll past, while pod lib lint treats them as fatal. Both build systems are in Swift 5 language mode, so the podspec and Package.swift are not actually divergent here.
SwiftPM emits Swift diagnostics but exits 0, so the four #SendableClosureCaptures warnings that later failed pod lib lint scrolled past every local and CI SPM build unnoticed. That left CocoaPods as the only gate able to reject them - a ~5 minute round-trip, and an accidental dependency rather than a designed one. Add a dedicated job running swift build --build-tests -Xswiftc -warnings-as-errors. Verified clean on a from-scratch build of both sources and tests (exit 0, zero warnings), so this does not red the tree on merge.
firebase-ios-sdk dropped Xcode 16.x, so this repo follows. Firebase's convention pairs macos-15 with Xcode_26.2, which both remaining legs now use. - spm.yml: matrix Xcode_16.4 -> Xcode_26.2, and the macos-14 / Xcode_16.2 include is removed entirely rather than repinned, since macos-14 does not carry Xcode 26.x. - app_check_core.yml: the macos-15 lint leg moves off Xcode_16.4; macos-26 was already on 26.2. - The new warnings-as-errors job is pinned to match. This also closes a verification gap rather than opening one: every gate run during this review (swift test, pod lib lint on four platforms, Catalyst, warnings-as-errors) was executed locally on Xcode 26.2, whereas CI had been on 16.4 - a toolchain nothing in this branch was ever checked against.
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
1 participant
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.
No description provided.