Harden Google ID token validation#97
Open
fabianmarian8 wants to merge 1 commit into
Open
Conversation
68 tasks
📝 WalkthroughWalkthroughThis PR centralizes Google ID token verification across five application modules by introducing a reusable ChangesGoogle ID Token Verification Centralization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Related Knowledge 3 documents with suggested updates are ready for review. BAWES Universe Backend Codebase Tour (studenthub)View Suggested Changes@@ -70,6 +70,7 @@
- **S3ResourceManager** (`common/components/S3ResourceManager.php`), **CloudinaryManager** (`common/components/CloudinaryManager.php`): Abstract storage for uploads (photos, IDs, etc.), used across apps.
- **Algolia** (`common/components/Algolia.php`): Handles search index synchronization for candidates, companies, etc.
- **JWT** (`common/components/JWT.php`): Handles JWT authentication.
+- **GoogleIdTokenVerifier** (`common/components/GoogleIdTokenVerifier.php`): Provides centralized, hardened validation of Google ID tokens for authentication flows across all portals (admin, company, staff, manager, candidate). Validates the `aud`, `iss`, `exp`, `email`, and `email_verified` claims before allowing account lookup/signup, replacing the previous pattern where each AuthController directly called Google's tokeninfo endpoint. Requires the `GOOGLE_OAUTH_CLIENT_IDS` environment variable to be configured with comma-separated OAuth client IDs. Configured as `googleIdTokenVerifier` in `common/config/main.php`.
- **SMSComponent** (`common/components/SMSComponent.php`): Sends SMS notifications.
- **ReCaptcha** (`common/components/ReCaptcha.php`): Integrates Google reCAPTCHA.
- **SlackLogger** (`common/components/SlackLogger.php`): Sends logs to a Slack webhook.DB, API endpoint, S3, Slack, and payment keysView Suggested Changes@@ -53,6 +53,7 @@
# IPSTACK_ACCESS_KEY=your_ipstack_access_key
# RECAPTCHA_SECRET_KEY=your_recaptcha_secret_key
# GOOGLE_MAP_TOKEN=your_google_map_token
+# GOOGLE_OAUTH_CLIENT_IDS=client_id_1.apps.googleusercontent.com,client_id_2.apps.googleusercontent.com
# NETLIFY_TOKEN=your_netlify_token
# GITHUB_TOKEN=your_github_token
# GITHUB_BRANCH=master
@@ -70,4 +71,5 @@
- Replace all `your_*` values with your actual credentials or secrets.
- The S3, Slack, and payment keys are directly mapped from the structure in [`common/config/main.php`](https://github.com/BAWES-Universe/plugn/blob/bc485b0a1da61d516955c5dc4fc29e95afccea92/common/config/main.php#L3-L172).
- Database and API endpoint keys are placeholders, as they are not defined in `main.php` and may be set elsewhere in your project.
+- `GOOGLE_OAUTH_CLIENT_IDS` is a comma-separated list of Google OAuth client IDs used to validate Google ID tokens during login flows across all portals (admin, company, staff, manager, candidate). The `GoogleIdTokenVerifier` component validates the `aud` (audience) claim in incoming tokens, ensuring tokens were issued for the correct OAuth client(s). This variable is required for Google login functionality to work properly.
- All other configuration keys in `main.php` are optional and can be set if your local environment requires them.Security, Auth & Permissions (RBAC + Tenant Scoping)View Suggested Changes@@ -48,9 +48,12 @@
ReCaptcha enforcement is present but commented out in login and related actions, indicating it may be disabled or optional. See [candidate/modules/v1/controllers/AuthController.php](https://github.com/BAWES-Universe/studenthub/blob/ca90a5502040ba8191fe9f80d29d0b6a4d2a6152/candidate/modules/v1/controllers/AuthController.php#L10-L1151) and [admin/modules/v1/controllers/AuthController.php](https://github.com/BAWES-Universe/studenthub/blob/ca90a5502040ba8191fe9f80d29d0b6a4d2a6152/admin/modules/v1/controllers/AuthController.php#L19-L276).
+Google ID tokens are validated via the shared `googleIdTokenVerifier` component (`common/components/GoogleIdTokenVerifier`), which provides centralized, hardened validation. The verifier checks the `aud` (audience) claim against `GOOGLE_OAUTH_CLIENT_IDS`, validates the `iss` (issuer) is `accounts.google.com`, ensures the token has not expired (`exp`), and requires `email` and `email_verified` claims before allowing account lookup or signup. This replaces the previous approach of calling Google's tokeninfo endpoint directly in each portal's AuthController. See [common/config/main.php](https://github.com/BAWES-Universe/studenthub/blob/ca90a5502040ba8191fe9f80d29d0b6a4d2a6152/common/config/main.php#L25-L130).
+
Rate limiting, throttling, and webhook secrets are not documented in the available sources. Unknown / Verify.
Auth/security-related environment variables include:
+- `GOOGLE_OAUTH_CLIENT_IDS` (comma-separated list of Google OAuth client IDs used to validate the `aud` claim in Google ID tokens during login flows across all portals – admin, company, staff, manager, and candidate; used in `common/components/GoogleIdTokenVerifier` in [common/config/main.php](https://github.com/BAWES-Universe/studenthub/blob/ca90a5502040ba8191fe9f80d29d0b6a4d2a6152/common/config/main.php#L25-L130))
- `GOOGLE_MAPS_API_KEY` (used in `common/components/GoogleMap` in [common/config/main.php](https://github.com/BAWES-Universe/studenthub/blob/ca90a5502040ba8191fe9f80d29d0b6a4d2a6152/common/config/main.php#L25-L130))
- ReCaptcha secret key (used in `common/components/ReCaptcha` in [common/config/main.php](https://github.com/BAWES-Universe/studenthub/blob/ca90a5502040ba8191fe9f80d29d0b6a4d2a6152/common/config/main.php#L25-L130))
How did I do? Any feedback? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
company/modules/v1/controllers/AuthController.php (1)
300-302: 💤 Low valueInconsistent email_verified check may skip email verification update.
The verifier's
isEmailVerified()acceptstrue(boolean),"true", or"1". However, the check here only matches when$response->email_verified == "true"(string comparison). If the tokeninfo response returns a booleantrue, this condition will pass due to loose comparison, but the intent seems unclear.Since the verifier already guarantees
email_verifiedis truthy before returning a response, consider simplifying to just check for truthiness:♻️ Suggested simplification
- if($response->email_verified && $response->email_verified == "true") { + if($response->email_verified) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@company/modules/v1/controllers/AuthController.php` around lines 300 - 302, The email verification check in AuthController (currently using if($response->email_verified && $response->email_verified == "true")) is too narrow; change it to a simple truthy check so any truthy tokeninfo value accepted by the verifier marks the contact verified. Replace that condition with a single truthiness test (e.g., if ($response->email_verified) or use the verifier's isEmailVerified(...) helper) and then set Contact::$contact_email_verification to Contact::EMAIL_VERIFIED and call $contact->save(false) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@company/modules/v1/controllers/AuthController.php`:
- Around line 300-302: The email verification check in AuthController (currently
using if($response->email_verified && $response->email_verified == "true")) is
too narrow; change it to a simple truthy check so any truthy tokeninfo value
accepted by the verifier marks the contact verified. Replace that condition with
a single truthiness test (e.g., if ($response->email_verified) or use the
verifier's isEmailVerified(...) helper) and then set
Contact::$contact_email_verification to Contact::EMAIL_VERIFIED and call
$contact->save(false) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21b006c0-0b77-4d51-a17e-4dde92f9e531
📒 Files selected for processing (8)
admin/modules/v1/controllers/AuthController.phpcandidate/modules/v1/controllers/AuthController.phpcommon/components/GoogleIdTokenVerifier.phpcommon/config/main.phpcompany/modules/v1/controllers/AuthController.phpmanager/modules/v1/controllers/AuthController.phpstaff/modules/v1/controllers/AuthController.phptests/check-google-id-token-hardening.py
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.
Summary
GOOGLE_OAUTH_CLIENT_IDSand validateaud,iss,exp,email, andemail_verifiedbefore account lookup/signup/claim #55
Validation
python3 tests/check-google-id-token-hardening.pygit diff --checkPHP/Codeception could not be run in this local environment because
phpis not installed. No live Google account, AWS/IAM/S3 access, candidate data, or real secret values were used.Demo evidence
The static guard verifies that all five
login-by-googlehandlers call the shared verifier, no handler keeps the directtokeninfo?id_token=path, and the verifier contains audience, issuer, expiry, email, and verified-email checks.Summary by CodeRabbit
Bug Fixes
Tests