fix(requesthooks): cover reCAPTCHA and pin the NSBlock filter

[ncooke3]

Builds on 3153076, which tightened the block filter to an NSBlock ancestry check and added a hermetic stub-session test.

AppCheckRecaptchaProvider was missed. Both @objc initializers still declared [@convention(block) (NSMutableURLRequest) -> Void]?, and FIRRecaptchaProvider.m:126 calls them from Objective-C with a non-empty array, so that path still trapped. Switched to [Any]? like the core providers, and replaced the comment claiming @convention(block) is required to bridge arrays of closures, since that belief is what the fix disproves.

The test did not pin the filter it shipped with. Its only negative input was @"not a block", and an NSString is rejected by the class-name substring check too. Restoring contains("Block") left the test green. NSBlockOperation is the input that separates them: the substring check admits it, bit-casts an NSOperation into a callable, and the process dies on invocation. Added it, and moved the valid hook last so that rejecting an element cannot be mistaken for abandoning the array.

Split off a positive case shaped like FIRHeartbeatLogger's request hook, the only non-nil hook Firebase passes in production. It sets X-firebase-client and asserts the header survived, which shows the recovered block was invoked with the correct ABI and handed a usable request, rather than only that control reached it.

Rejected elements are now logged. A dropped hook is otherwise silent: the request still succeeds, just without the heartbeat payload.

[paulb777]

Review: #126 — "fix(requesthooks): cover reCAPTCHA and pin the NSBlock filter"

PR: google/app-check#126 · pb-swift@d224ae1e ← nc.appcheck.fix.objc.requesthook.bridging@c2ce51c8
Author: ncooke3 · 1 commit, 3 files, +100 / −15 · mergeable_state: clean
Relationship: stacked on #111; continues the requestHooks bridging thread that runs from fc4a7a0e → 2fa613b → 3153076f → d224ae1.

Note

Verified against the extracted PR head tree at c2ce51c8, the pb-swift base at d224ae1, and main@97f7d74d. Firebase call sites checked against firebase-ios-sdk@main.

---

Verdict

Approve. Both claims in the PR body are true, and the first one is a bug I missed in seven passes over #111.

Important

The reCAPTCHA provider is a real, live trap — and it predates the Swift rewrite. AppCheckRecaptchaProvider.swift:55, 67 on main@97f7d74d carries the identical [@convention(block) (NSMutableURLRequest) -> Void]? signature on both @objc initializers. This is not a regression introduced by #111 — it shipped in AppCheckCore 11.3.0 and is in released code today.

My miss

Across every pass on #111 I scoped the requestHooks audit to AppCheckCore/Sources and the four core providers. I never grepped AppCheckRecaptchaProvider/Sources, even though I read that file repeatedly for the M7 access-level analysis and quoted lines 34–96 of it. The signature was in plain sight at line 55. The reason the gap persisted is that my mental model was "the port introduced this," so I only looked at files the port touched — and this one the port left alone.

---

Claim-by-claim

1. reCAPTCHA was missed — ✅ confirmed, and the caller is real

Both @objc convenience inits declared the typed array. Firebase calls the first one from Objective-C:

// firebase-ios-sdk/FirebaseAppCheck/Sources/RecaptchaProvider/FIRRecaptchaProvider.m
id heartbeatHook = [app.heartbeatLogger requestHook];
GACRecaptchaProvider *recaptchaProvider =
    [[GACRecaptchaProvider alloc] initWithSiteKey:siteKey
                                     resourceName:app.resourceName
                                           APIKey:app.options.APIKey
                                     requestHooks:heartbeatHook ? @[ heartbeatHook ] : @[]];

The @[] fallback is why this has not been noticed: an empty NSArray has no elements to cast, so it bridges without trapping. The non-empty branch — the normal case — traps in the @objc thunk, before the RecaptchaEnterpriseSDKLoader guard ever runs.

Exposure is narrower than it looks: FIRRecaptchaProvider.m was only added on 2026-06-10 (#16155) and the provider requires the caller to explicitly link RecaptchaEnterprise. But it is live on firebase-ios-sdk@main.

The sweep is now complete. @convention(block) survives in exactly one place in the PR head tree — the AppCheckCoreAPIRequestHook typealias itself, where it belongs. All nine requestHooks parameter declarations across both targets are [Any]?.

The replaced comment deserves its own note. The old text —

@convention(block) is required because the Swift compiler cannot automatically bridge collections of closures (like an Array) to Objective-C blocks. This attribute changes the closure's representation to match the Objective-C block heap layout.

— is half true, which is worse than false. The second sentence is correct: the attribute does change the closure's representation. The first sentence draws the wrong conclusion from it. Making the element ObjC-representable is not the same as making NSArray → Array bridging work, and it is precisely the belief that kept this signature in place for four months. Replacing it rather than amending it is the right call.

2. The test did not pin the filter it shipped with — ✅ confirmed, and this is the sharper finding

The 3153076f test passed @[ hook, @"not a block" ]. An NSString is rejected by both the shipped NSBlock ancestry check and the substring check it replaced, so the test could not tell them apart — restoring the weaker filter left it green. A test that cannot fail for the regression it was written against is not coverage.

NSBlockOperation is the discriminating input, and it is a good choice for two independent reasons: its class name contains Block, so the substring check admits it; and it is an NSOperation, so bit-casting it to a block and invoking it jumps through a garbage function pointer. It is also a plausible thing for a confused caller to actually pass.

Moving the valid hook to last is a small thing that matters: with the hook first, a compactMap that bailed on the first rejection instead of skipping it would still satisfy the expectation.

3. Positive case shaped like the heartbeat hook — ✅ good, and the right assertion

Splitting testRequestHooksBridging into a positive and a negative case is correct, and asserting that X-firebase-client survives onto the request is materially stronger than asserting the hook ran: it proves the recovered block was invoked with the right ABI and handed a usable NSMutableURLRequest, which is the property that was actually broken. The doc comment explaining why requestHooks:nil and Swift-closure tests both miss the bridge is the kind of thing that stops this from regressing a fourth time.

---

Findings

🟡 F1 — The fixed code path has no test

Every new and modified test targets _GACAppCheckAPIService. There is no Objective-C test that constructs GACRecaptchaProvider with a non-empty hooks array — the exact call shape being fixed. As of this PR the reCAPTCHA signature could be reverted to the typed form and the suite would stay green, which is the same failure mode the PR criticizes in 3153076f.

This is cheap to close, because the trap fires in the @objc thunk before the initializer body runs, so the test does not need RecaptchaEnterprise linked — a nil return is a pass:

#if TARGET_OS_IOS
- (void)testRecaptchaProviderRequestHooksBridging {
  if (@available(iOS 15.0, *)) {
    void (^hook)(NSMutableURLRequest *) = ^(NSMutableURLRequest *r) {};
    // Trapping happens during argument bridging; a nil result (SDK not linked)
    // is a pass, a crash is the regression.
    (void)[[GACRecaptchaProvider alloc] initWithSiteKey:@"key"
                                           resourceName:@"projects/p/apps/a"
                                                 APIKey:@"key"
                                           requestHooks:@[ hook ]];
  }
}
#endif

The obstacle is target layout, not difficulty:

Target	Language	Depends on
AppCheckCoreUnitObjC	ObjC	AppCheckCore only
AppCheckRecaptchaProviderUnit	Swift only	AppCheckRecaptchaProvider

SwiftPM targets cannot mix languages, so the Swift reCAPTCHA test target cannot host it. Adding "AppCheckRecaptchaProvider" to AppCheckCoreUnitObjC's dependencies is the one-line fix; the test body is #if TARGET_OS_IOS-gated, so the non-iOS platforms compile an empty file. Under CocoaPods no manifest change is needed at all — s.ios.source_files already builds both targets into a single module, so @import AppCheckCore; sees GACRecaptchaProvider on iOS.

🟡 F2 — code: .unknown is the only log site in the codebase without a specific message code

Every other AppCheckCoreLogger.log call in both targets names a themed code — .attestationRejected, .unexpectedHTTPCode, .stagingModeEnabled, .localDebugToken, and so on. This is the first and only use of .unknown (1001) in Sources. These codes render as I-GAC001001 and are what support uses to triage logs; a dedicated code in the existing scheme (the 3xxx request/HTTP band has only unexpectedHTTPCode = 3001, so invalidRequestHook = 3002 fits) makes a dropped heartbeat hook greppable instead of indistinguishable from any other unclassified error.

🟡 F3 — "Rejected elements are now logged" is true only in DEBUG

AppCheckCoreLogger.log wraps its entire body in #if DEBUG — deliberately, since that is the blocker fix that stopped debug tokens leaking into Release. So in every shipping build a dropped hook remains exactly as silent as before, and the PR body's framing ("Rejected elements are now logged. A dropped hook is otherwise silent") overstates what changed in production.

This is not a reason to change the code — the logger's design is right and special-casing it here would be worse. But it does mean the log line only helps a developer who is already debugging, not a user reporting missing heartbeat data in the field. Worth adjusting the PR description so the next reader does not assume field diagnosability that is not there.

🟢 F4 — No changelog entry

Given that this fixes a trap present in released 11.3.x and not just in the port, a [fixed] line under # 12.0.0 is warranted — and it is the natural place to note that GACRecaptchaProvider's requestHooks: is now NSArray<id> *, the same disclosure d224ae1 added for the core providers.

🟢 F5 — Minor

• Doc-comment style drift. The first reCAPTCHA init documents all four parameters under - Parameters:; the new comment on the second init is a bare - Parameter requestHooks: with no summary and no entry for siteKey, resourceName, APIKey, or actionName. Legal, but inconsistent within the same file.
• XCTFail inside the NSBlockOperation block is decorative. If the filter regresses, the process takes SIGSEGV rather than invoking the block, so the assertion never runs. Harmless, and the doc comment already says as much — but it reads as though "block invoked" were the expected failure signal.
• __block NSMutableURLRequest *capturedRequest is written on the session's delegate queue and read on the test thread. The expectation's internal synchronization orders this in practice, so it is fine; noting it only because it is the kind of thing that turns into a flake if the wait is ever restructured.

---

Summary

Correctness of the fix	✅ Complete — @convention(block) now appears only in the typealias
Correctness of the diagnosis	✅ Both claims verified against source and against the Firebase caller
Test quality	🟡 The new negative test genuinely discriminates the filter; the reCAPTCHA path it fixes is untested
Docs / observability	🟡 .unknown code, DEBUG-only logging, no changelog

Before merge: F1 (test the path being fixed) and F4 (changelog). F2 is a two-line change and worth taking while the file is open.

Separately: this bug is on main as well as pb-swift. If another 11.x patch ships before #111 merges, the reCAPTCHA signature fix should be cherry-picked, and the Firebase-side exposure tracked against #16155.

[ncooke3]

Merging, will address feedback in follow-up.