Skip to content

feat: specify aud for apple oidc & support all clients - #221

Merged
paolodamico merged 7 commits into
mainfrom
apple-client-id
Aug 3, 2026
Merged

feat: specify aud for apple oidc & support all clients#221
paolodamico merged 7 commits into
mainfrom
apple-client-id

Conversation

@paolodamico

@paolodamico paolodamico commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Actually the cursor summary is pretty good. This PR is complimentary to worldcoin/bedrock#392 where all the audiences are introduced to support all clients for Sign in with World ID


Note

Medium Risk
Changes Apple OIDC audience selection in the authentication path; behavior is intentional but clients must send correct aud or rely on the default allowlist entry.

Overview
Sign in with Apple no longer picks the OIDC client ID from the client-name header. Clients may send an optional aud on OidcToken::Apple (bundle ID); the verifier checks it against Environment::allowed_apple_client_ids() for staging/production (World App, World ID, web/Android, sandbox). If aud is omitted, verification still uses the first allowlisted ID for backward compatibility.

AuthHandler::verify and validate_factor_registration drop the client_name argument; backup routes no longer read the removed client-name header. InvalidAud maps to oidc_token_invalid_aud. Tests and mocks cover multiple Apple audiences and the no-aud path.

Reviewed by Cursor Bugbot for commit 80e73e1. Bugbot is set up for automated code reviews on this repo. Configure here.

@paolodamico

Copy link
Copy Markdown
Contributor Author

@codex review

@paolodamico

Copy link
Copy Markdown
Contributor Author

cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 38c109c. Configure here.

Comment thread src/oidc_token_verifier.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 38c109ce89

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/oidc_token_verifier.rs
Comment thread src/types/environment.rs
Comment thread src/oidc_token_verifier.rs Outdated
@aurel-fr
aurel-fr requested a review from Copilot August 3, 2026 21:52
@paolodamico paolodamico changed the title feat: specify aud for apple oidc feat: specify aud for apple oidc & support all clients Aug 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Apple OIDC token verification to support multiple client audiences by allowing an optional aud on OidcToken::Apple, validating it against an environment-specific allowlist, and removing the legacy client-name header plumbing from auth and backup routes.

Changes:

  • Add optional aud to OidcToken::Apple and validate Apple audiences via per-environment allowlists.
  • Remove client-name header usage across routes/auth and replace Environment::apple_client_id(...) with allowed_apple_client_ids().
  • Update mock OIDC utilities/tests and introduce a dedicated error code for invalid Apple audiences.

Holistic assessment (risk & readiness):

  • Medium risk: this changes authentication behavior (audience selection) in a core auth flow and affects client compatibility.
  • Noted issues are low severity (mostly correctness of messages/comments and small maintainability tweaks), but given the auth-surface impact, this needs final human review.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test-utils/src/mock_oidc_server.rs Updates mock Apple audience/client ID and token generation helpers for new audience behavior.
src/types/oidc_token.rs Extends OidcToken::Apple with optional aud.
src/types/error.rs Maps InvalidAud into a dedicated API error response.
src/types/environment.rs Replaces single Apple client ID selection with per-env allowlists.
src/routes/verify_factor.rs Removes client-name extraction/plumbing; keeps client-version logging.
src/routes/sync_backup.rs Removes client-name header handling from sync flow.
src/routes/retrieve_metadata.rs Removes client-name header handling from metadata retrieval.
src/routes/retrieve_from_challenge.rs Removes client-name header handling; simplifies span fields.
src/routes/delete_factor.rs Removes client-name header handling from factor deletion.
src/routes/delete_backup.rs Removes client-name header handling from backup deletion.
src/routes/create_backup.rs Removes client-name header handling from backup creation.
src/routes/add_sync_factor.rs Removes client-name header handling from sync factor registration.
src/routes/add_factor.rs Updates Apple token match arm to accommodate new aud field; removes client-name plumbing.
src/oidc_token_verifier.rs Adds Apple audience allowlist validation and new InvalidAud error variant; removes client-name from verification.
src/headers.rs Removes the client-name header constant.
src/auth.rs Removes client-name parameter threading through AuthHandler verification/registration paths.
Suppressed comments (1)

src/types/environment.rs:279

  • Same grammar issue as above: "Imperative this is the first one" is likely unintended phrasing and is easy to misread during incident/debugging.
                // Imperative this is the first one for backwards compat (World App iOS was the first client supported)
                "org.worldcoin.insight", // World App iOS

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/oidc_token_verifier.rs Outdated
Comment thread src/oidc_token_verifier.rs
Comment thread src/types/oidc_token.rs
Comment thread src/types/environment.rs Outdated
@paolodamico
paolodamico merged commit 00f91bd into main Aug 3, 2026
12 checks passed
@paolodamico
paolodamico deleted the apple-client-id branch August 3, 2026 22:56
@github-actions github-actions Bot mentioned this pull request Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants