Conversation
Decode reCAPTCHA enforcement state entries individually so one malformed entry or non-string field no longer discards the whole array (which silently treated enforcement as OFF). Reject recaptchaKey values with an empty site key segment. Decode backend underlying error entries individually so keyInvalid/ipRefererBlocked are still mapped when sibling fields are not strings. (#16728)
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of the Firebase Auth backend when parsing malformed reCAPTCHA configurations and backend error details. Specifically, it updates the error and configuration parsing logic to handle unexpected types and malformed entries gracefully without discarding valid data or crashing. It also adds comprehensive unit tests to verify these edge cases. The feedback suggests using the more idiomatic Swift method hasPrefix(_:) instead of starts(with:) when checking the error reason prefix.
Address review feedback: String.hasPrefix(_:) is the idiomatic way to check a string prefix, rather than the generic Sequence.starts(with:).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of the Firebase Auth SDK when parsing malformed reCAPTCHA configurations and backend error details from the server. Key changes include safer parsing of the errors array and recaptchaEnforcementState to prevent discarding valid entries, stricter validation of the recaptchaKey structure, and comprehensive unit tests covering these edge cases. Feedback on the changes suggests using optional chaining with compactMap for a more concise parsing implementation in GetRecaptchaConfigResponse.swift, and simplifying let _ = to _ = in the unit tests to align with Swift style guidelines.
Addresses review feedback: use optional chaining instead of a nested if-let when parsing recaptchaEnforcementState (no behavior change), and use `_ =` instead of `let _ =` in the new backend test.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves robustness when parsing malformed reCAPTCHA configurations and backend error details from the server. It updates the error and config parsing logic to safely handle unexpected types and malformed entries, and adds comprehensive unit tests. The review feedback suggests further tightening the validation of the recaptchaKey path structure in AuthRecaptchaVerifier to strictly match the expected projects/{project-id}/keys/{site-key} format.
…cha-config # Conflicts: # FirebaseAuth/CHANGELOG.md
This branch has not been deployed
2 participants
Context: #16728 — an Analytics server payload that was malformed crashed apps in production even though nothing changed on the client. This PR checks how FirebaseAuth parses config from the server, mostly the reCAPTCHA config (
getRecaptchaConfig), and makes a few of those paths more tolerant.Audit result
The Auth decoding paths I reviewed already use
as?casts, guard their array indexing, and have no force-unwraps on server data. I found no crash in these paths. What I did find: in some cases, one malformed field or entry from the server silently threw away all of the config. The changes are small fixes for that.Hardened paths
GetRecaptchaConfigResponse.recaptchaEnforcementState: this was decoded with oneas? [[String: String]]cast. If any entry was not a dictionary, or any field in any entry was not a string (for example a new boolean field, ornull), the cast failed and the whole array came backnil. reCAPTCHA enforcement was then treated asOFFfor every provider. Each entry is now decoded separately: non-dictionary entries are dropped and only string values are kept. Unknown providers/states are still skipped later on, as before.AuthRecaptchaVerifiersite key extraction: therecaptchaKeywas already required to split into 4/segments. But keys likeprojects/123/keys/or///passed that check and stored an empty site key. These are now rejected with the existing "Invalid siteKey" (recaptchaNotEnabled) error, and no config is cached. Behavior for valid keys and for the reCAPTCHA-off case is unchanged.AuthBackenderrorerrors[].reasonmapping: this used the same all-or-nothingas? [[String: String]]cast. A non-string sibling field (for example a numericcode) or a malformed entry stoppedkeyInvalid/ipRefererBlockedfrom mapping toinvalidAPIKey/appNotAuthorized. Each entry is now handled separately and onlyreasonhas to be a string.Reviewed with no change needed:
GetProjectConfigResponse(authorizedDomains),GetAccountInfoResponse(checksusers.count == 1before indexing, which makes theUserProfileUpdateforce-unwrap safe), the other*Response.swiftdecoders,splitStringAtFirstColon, and JWT parsing inAuthTokenResult.Tests
FakeBackendRPCIssuer.fakeRecaptchaConfigJSONlets tests send anygetRecaptchaConfigpayload.GetRecaptchaConfigTests: malformed entries mixed with valid ones, wrong-typed fields,NSNull/empty/missing fields.AuthTests(iOS): verifier keeps valid enforcement states when other entries are malformed or unknown; wrong types fall back toOFF; malformed site keys ("",456,projects/123/keys/,.../456/extra,///) throw.AuthBackendTests:keyInvalid/ipRefererBlockedstill map correctly when sibling fields and entries are malformed.These fail without the source changes (checked by reverting the sources and rerunning):
testGetRecaptchaConfigResponseKeepsValidEntriesWhenSomeAreMalformed,testRecaptchaConfigIgnoresMalformedEnforcementStateEntries,testRecaptchaConfigMalformedSiteKeyThrows,testUnderlyingErrorReasonWithMalformedSiblingFields. The others are regression coverage.Testing
xcodebuild -scheme AuthUnit teston the iOS Simulator (iPhone 16 Pro): 361 tests, 0 failures.