Skip to content

#6218 Use extension-specific oauth redirect url#6219

Open
martgil wants to merge 16 commits into
masterfrom
issue-6218-use-extension-specific-oauth-redirect-url
Open

#6218 Use extension-specific oauth redirect url#6219
martgil wants to merge 16 commits into
masterfrom
issue-6218-use-extension-specific-oauth-redirect-url

Conversation

@martgil
Copy link
Copy Markdown
Collaborator

@martgil martgil commented May 7, 2026

This PR replaces the Google robots.txt redirect URL to extension specific oauth redirect URL.

Closes #6218


Tests (delete all except exactly one):

  • Does not need tests (refactor only, docs or internal changes)

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@martgil martgil requested a review from sosnovsky as a code owner May 7, 2026 02:42
Copy link
Copy Markdown
Collaborator

@sosnovsky sosnovsky left a comment

Choose a reason for hiding this comment

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

Hi @martgil, a lot of tests are failing because local dev extension builds use dynamic extension id. Maybe there is some way to use fixed id for local builds, so we can add this id to the list of allowed redirect URIs for our OAuth app.

@martgil martgil self-assigned this May 7, 2026
@martgil martgil marked this pull request as draft May 7, 2026 08:49
@martgil
Copy link
Copy Markdown
Collaborator Author

martgil commented May 7, 2026

You're correct, Roma. I'll look for some other alternative.

@martgil
Copy link
Copy Markdown
Collaborator Author

martgil commented May 7, 2026

Hello @sosnovsky - Just a quick question. The Mozilla Firefox extension id has also this dynamically generated id, does this means, the extension id for both builds should matches exactly identical? Or does the Mozilla firefox build has to had the static extension id too?

@sosnovsky
Copy link
Copy Markdown
Collaborator

identity.getRedirectURL() in firefox uses id from browser_specific_settings property of manifest.json (https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/identity/getRedirectURL), so probably it shouldn't change between builds and remain static

@sosnovsky
Copy link
Copy Markdown
Collaborator

also firefox uses browser.identity.getRedirectURL() instead of chrome.identity.getRedirectURL()

@martgil
Copy link
Copy Markdown
Collaborator Author

martgil commented May 7, 2026

Got it - thanks!

@martgil
Copy link
Copy Markdown
Collaborator Author

martgil commented May 7, 2026

Hello @sosnovsky - I have managed to make a static extension id from which the current one we have on this PR is nnlimfkjgafpgmilbnaabplbokhpicbo. I have certificate where the key is originated from. It it technically a test file but should I keep it uploaded here somewhere in test/?

@sosnovsky
Copy link
Copy Markdown
Collaborator

is this certificate required for creating local builds? if no, then there is no need to put it in the repository
also your current solution adds key property directly to manifest.json which will probably affect our production builds. I think we should have a separate chrome-consumer-local build and inject key property only for this build type

@martgil
Copy link
Copy Markdown
Collaborator Author

martgil commented May 7, 2026

is this certificate required for creating local builds? if no, then there is no need to put it in the repository

Edit: No, it is not required for creating the local builds. Thanks for your answer.

I think we should have a separate chrome-consumer-local build and inject key property only for this build type

Got it - will do.

@martgil martgil marked this pull request as ready for review May 13, 2026 10:35
@martgil
Copy link
Copy Markdown
Collaborator Author

martgil commented May 13, 2026

Hi @sosnovsky - May I here your feedback on this one, please? I may have missed important ones.

Comment thread extension/manifest.json Outdated
},
{
"matches": ["https://www.google.com/robots.txt*"],
"matches": ["https://www.google.com/oauth2/callback*"],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it'll probably won't work, as we now use extension id specific redirect URIs, and https://www.google.com/oauth2/callback won't be called causing auth process stuck.

I also re-checked launchWebAuthFlow documentation, and looks like it should work with our current OAuth Client ID, so users won't need to re-authenticate and their current refresh tokens will stay valid. Also switching to launchWebAuthFlow will allow us to remove a lot of our custom OAuth code (like oauth2_inject.ts, oauth2_finish.ts etc).

So let's try to migrate auth to launchWebAuthFlow, hopefully it won't require a lot of code changes and we already have it implemented in ConfiguredIdpOAuth, so you can re-use some code from it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Roma. I have followed your suggestions. It doesn't seem to have required alot of code changes. Can you please give it another check?

@martgil martgil requested a review from sosnovsky May 14, 2026 08:48
Copy link
Copy Markdown
Collaborator

@sosnovsky sosnovsky left a comment

Choose a reason for hiding this comment

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

well done 👍

Comment thread extension/js/common/api/authentication/google/google-oauth.ts
Comment thread extension/js/common/api/authentication/google/google-oauth.ts
@martgil martgil requested a review from sosnovsky May 15, 2026 07:47
@martgil
Copy link
Copy Markdown
Collaborator Author

martgil commented May 15, 2026

Hi @sosnovsky - I have manually tested it on Firefox and it works well now. Then tests for the chrome build have passed too. Feel free to re-check at your convenience.

Copy link
Copy Markdown
Collaborator

@sosnovsky sosnovsky left a comment

Choose a reason for hiding this comment

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

works great now, well done 👍

}
return chrome.identity.getRedirectURL('oauth');
const redirectUri = chrome.identity.getRedirectURL('oauth');
if (navigator.userAgent.includes('Firefox')) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

let's use existing Catch.isFirefox() check

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.

Use extension-specific OAuth redirect URL instead of Google robots.txt

2 participants