Skip to content

helium/core: use webstore when extension proxy is off#1594

Draft
Umbranoxio wants to merge 1 commit intoimputnet:mainfrom
Umbranoxio:webstore-direct
Draft

helium/core: use webstore when extension proxy is off#1594
Umbranoxio wants to merge 1 commit intoimputnet:mainfrom
Umbranoxio:webstore-direct

Conversation

@Umbranoxio
Copy link
Copy Markdown

For your pull request to not get closed without review, please confirm that:

  • An issue exists where the maintainers agreed that this should be implemented
    (an approved feature request, or confirmed bug).
  • I tested that my contribution works locally, and does not break anything,
    otherwise I have marked my PR as draft.
  • If my contribution is non-trivial, I did not use AI to write most of it.
  • I understand that I will be permanently banned from interacting with this
    organization if I lied by checking any of these checkboxes.

Tested on (check one or more):

  • Windows
  • macOS
  • Linux

closes #1031 (probably #451 too but i didn't test that case)

changes:

  • extensions now download from the chrome web store directly when the proxy setting is turned off
  • installs, updates, snippets and pending installs now follow that setting more consistently
  • made url matching stricter

testing:

  • built on macos
  • installed and updated an extension with proxy on and off
  • did a quick test with chrome://net-export -- off used google, on used proxy
  • didnt test the external extension warning path but should be fine

@dumbmoron
Copy link
Copy Markdown
Member

this should be a flag and not default behavior

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 5, 2026

Comments Outside Diff (1)

  1. patches/helium/core/proxy-extension-downloads.patch, line 419-422 (link)

    P2 if ((true)) return false; is an unusual pattern

    CanUseUpdateService now unconditionally returns false via if ((true)) { return false; }. The double-paren is presumably there to suppress a "condition is always true" warning, but this reads as leftover debug code. If the intent is to permanently disable UpdateService in Helium, an unconditional return false; at the top of the function (with a comment explaining why) would be clearer.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "helium/core: use webstore when extension..." | Re-trigger Greptile

Comment on lines +348 to 361
+ Profile* profile = Profile::FromBrowserContext(context_.get());
+ PrefService* prefs = profile->GetPrefs();
+ GURL services_url = helium::GetServicesBaseURL(*prefs).Resolve("/ext");
+ const GURL& update_url = it.second.update_url();
+ bool is_webstore_update_url =
+ (update_url.host() == baseUrl.host() &&
+ update_url.path() == baseUrl.path()) ||
+ extension_urls::CanPatchUpdateUrl(update_url) ||
+ (update_url.host() == services_url.host() &&
+ update_url.path() == services_url.path());
+
+ if (is_webstore_update_url) {
+ it.second.OverrideURL(helium::GetExtensionUpdateURL(*prefs));
+ }
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Unchecked Profile::FromBrowserContext return value in update loop

Profile::FromBrowserContext(context_.get()) can return nullptr when context_ is not backed by a Profile (e.g., certain off-the-record or system contexts). The previous code guarded this behind if (prefs && ...) and also restricted the block to extensions whose host matched baseUrl. The new code executes the profile/prefs lookup unconditionally for every pending extension, so a null profile will cause a crash on profile->GetPrefs().

Comment on lines +94 to +101
+ downloader->set_manifest_query_params(GetManifestQueryParams(
+ helium::ShouldAccessExtensionService(*profile->GetPrefs())
+ ? UpdateQueryParams::CRX_DUMMY
+ : UpdateQueryParams::CRX));
+ downloader->set_ping_enabled_domain(
+ helium::ShouldAccessExtensionService(*profile->GetPrefs())
+ ? "helium.computer"
+ : "google.com");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Stale downloader config after proxy-setting toggle

manifest_query_params and ping_enabled_domain are baked into the ExtensionDownloader at creation time in EnsureDownloaderCreated(). When the user toggles kHeliumExtProxyEnabled, the pref-change hook triggers CheckSoon(), which calls EnsureDownloaderCreated() — but since downloader_ is already non-null, it is not recreated. The old query params (e.g., CRX_DUMMY vs CRX) and ping domain stay in effect until the downloader is destroyed (typically on browser restart). On the OFF→ON transition the proxy receives real platform params instead of the spoofed ones, defeating privacy.

Comment on lines +94 to +101
+ downloader->set_manifest_query_params(GetManifestQueryParams(
+ helium::ShouldAccessExtensionService(*profile->GetPrefs())
+ ? UpdateQueryParams::CRX_DUMMY
+ : UpdateQueryParams::CRX));
+ downloader->set_ping_enabled_domain(
+ helium::ShouldAccessExtensionService(*profile->GetPrefs())
+ ? "helium.computer"
+ : "google.com");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 ShouldAccessExtensionService called twice for the same pref state

helium::ShouldAccessExtensionService(*profile->GetPrefs()) is evaluated separately for set_manifest_query_params and set_ping_enabled_domain. Computing the result once into a local variable would be cleaner and avoids the redundant prefs read.

@Umbranoxio
Copy link
Copy Markdown
Author

this should be a flag and not default behavior

ah, alrighty
saw what was said in the linked issue but guess i misunderstood

@Umbranoxio Umbranoxio marked this pull request as draft May 5, 2026 14:31
@Umbranoxio
Copy link
Copy Markdown
Author

thought about it, guess that changes things a bit then.. there's a few ways this can be handled, which would you prefer @dumbmoron

  1. flag can be enabled but only "works" when the proxy setting is disabled
  2. flag acts as an override for the extension proxy setting silently (state not visible in the settings ui)
  3. flag disables extension proxy setting and greys it out appending a notice to its description thats its disabled cause the flag is enabled (basically a nicer override)
  4. ??

@dumbmoron
Copy link
Copy Markdown
Member

flag acts as an override for the extension proxy setting silently (state not visible in the settings ui)

i think this would be fine since it's kind of a fringe configuration, flags usually take precedence over everything else

@TotoCodeFR
Copy link
Copy Markdown
Contributor

TotoCodeFR commented May 6, 2026

i could test this out on windows if you want (once @Umbranoxio confirms it works for them), especially since that issue affects my work computers (services.helium.imput.net is blocked there)

actually maybe not, can't get the original code to build because of my pc specs, would have loved to though

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.

[FR]: Add an option to connect to Web Store directly, without Helium Services

3 participants