-
Notifications
You must be signed in to change notification settings - Fork 71
Linux: auto-trust Firefox profiles and surface in-app cert notice #3472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Changes from 6 commits
4cc38c1
5676950
9970a70
a197a6f
f40de8b
b03b5ab
1194d83
98cfd90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import fs from 'node:fs'; | ||
| import os from 'node:os'; | ||
| import path from 'node:path'; | ||
| import { findOnPath } from 'src/lib/find-on-path'; | ||
|
|
||
| export function isFirefoxInstalledOnLinux( homeDir: string = os.homedir() ): boolean { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might be missing something, but from what I see, if we decide to drop that new UI notice, we could get rid of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in |
||
| if ( findOnPath( 'firefox' ) !== null ) { | ||
| return true; | ||
| } | ||
| // Snap and Flatpak wrappers don't always land on $PATH for the current | ||
| // shell session, so fall back to the profile / data dirs they create. | ||
| const candidates = [ | ||
| path.join( homeDir, '.mozilla', 'firefox' ), | ||
| path.join( homeDir, 'snap', 'firefox' ), | ||
| path.join( homeDir, '.var', 'app', 'org.mozilla.firefox' ), | ||
| ]; | ||
| return candidates.some( ( dir ) => fs.existsSync( dir ) ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,9 @@ const DOCS_LINKS = { | |
| docsSslInStudio: { | ||
| en: 'https://developer.wordpress.com/docs/developer-tools/studio/ssl-in-studio/', | ||
| }, | ||
| docsSslLinuxFirefox: { | ||
| en: 'https://developer.wordpress.com/docs/developer-tools/studio/ssl-in-studio/#linux-firefox', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In relation to https://github.com/Automattic/studio/pull/3472/changes#r3240894626 we could do That would lead to a new
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved by |
||
| }, | ||
| docsMcp: { | ||
| en: 'https://developer.wordpress.com/docs/developer-tools/studio/mcp-on-studio/', | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| /** | ||
| * @vitest-environment node | ||
| */ | ||
| import fs from 'node:fs'; | ||
| import { vi } from 'vitest'; | ||
| import { findOnPath } from 'src/lib/find-on-path'; | ||
| import { isFirefoxInstalledOnLinux } from '../detect-linux-browsers'; | ||
|
|
||
| vi.mock( 'node:fs', () => ( { | ||
| default: { existsSync: vi.fn() }, | ||
| existsSync: vi.fn(), | ||
| } ) ); | ||
|
|
||
| // Match the specifier used by the implementation so vi.mock targets the | ||
| // same module identity regardless of how the resolver normalises paths. | ||
| vi.mock( 'src/lib/find-on-path', () => ( { | ||
| findOnPath: vi.fn(), | ||
| } ) ); | ||
|
|
||
| const HOME = '/home/tester'; | ||
|
|
||
| function mockExistingPaths( paths: string[] ) { | ||
| // Normalise separators so the test passes on Windows CI, where | ||
| // path.join in the implementation produces backslashes. | ||
| const normalize = ( p: string ) => p.replace( /\\/g, '/' ); | ||
| const expected = paths.map( normalize ); | ||
| vi.mocked( fs.existsSync ).mockImplementation( ( candidate: fs.PathLike ) => | ||
| expected.includes( normalize( String( candidate ) ) ) | ||
| ); | ||
| } | ||
|
|
||
| describe( 'isFirefoxInstalledOnLinux', () => { | ||
| beforeEach( () => { | ||
| vi.resetAllMocks(); | ||
| mockExistingPaths( [] ); | ||
| vi.mocked( findOnPath ).mockReturnValue( null ); | ||
| } ); | ||
|
|
||
| it( 'returns true when firefox is on PATH', () => { | ||
| vi.mocked( findOnPath ).mockImplementation( ( cmd ) => | ||
| cmd === 'firefox' ? '/usr/bin/firefox' : null | ||
| ); | ||
| expect( isFirefoxInstalledOnLinux( HOME ) ).toBe( true ); | ||
| } ); | ||
|
|
||
| it( 'returns true when ~/.mozilla/firefox exists', () => { | ||
| mockExistingPaths( [ `${ HOME }/.mozilla/firefox` ] ); | ||
| expect( isFirefoxInstalledOnLinux( HOME ) ).toBe( true ); | ||
| } ); | ||
|
|
||
| it( 'returns true when Snap Firefox data dir exists', () => { | ||
| mockExistingPaths( [ `${ HOME }/snap/firefox` ] ); | ||
| expect( isFirefoxInstalledOnLinux( HOME ) ).toBe( true ); | ||
| } ); | ||
|
|
||
| it( 'returns true when Flatpak Firefox dir exists', () => { | ||
| mockExistingPaths( [ `${ HOME }/.var/app/org.mozilla.firefox` ] ); | ||
| expect( isFirefoxInstalledOnLinux( HOME ) ).toBe( true ); | ||
| } ); | ||
|
|
||
| it( 'returns false when neither PATH nor profile/data dirs exist', () => { | ||
| expect( isFirefoxInstalledOnLinux( HOME ) ).toBe( false ); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we kept only the original notice we already have on trunk? Instead of adding this new one we could:
I think that could streamline the user experience and also make the code simpler.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ivan, I want to push back a bit here. My biggest concern with falling back to just the existing notice is what it would take to also cover the "Firefox installed but never launched" case you mentioned. Are you comfortable with the notice being persistently visible in the settings tab? That's the part I'm most hesitant about.
The existing notice is gated on
!isCertificateTrusted. On Linux that already factors in Firefox profile trust viaareAllFirefoxProfilesTrustedLinux, but it's vacuously true when no profile exists — so to surface the notice in the installed-but-never-launched case, we'd have to gate it on something likeisFirefoxInstalled, or makeisRootCATrustedreturn false in that scenario. Either way, that signal doesn't auto-clear: a user who has Firefox installed (common on Ubuntu, where it ships by default) but never launches it would see the notice sitting in settings indefinitely.To be upfront about the tradeoff: my current PR doesn't really solve that edge case either — the new Firefox notice is also gated on
!isCertificateTrusted, so it stays silent until a profile actually exists. The difference is that when a profile does show up, the user gets Firefox-specific guidance instead of the generic message, andisFirefoxInstalledOnLinuxis what lets us know to do that. I'd rather lean on docs for the never-launched case than make the notice permanent for everyone with Firefox preinstalled.What I was going for overall is the closest analog to the Chrome trust-status check we already have: only surface platform-specific guidance when it's actionable. Happy to trim the supporting code if parts feel heavy — but I'd like to keep the conditional surfacing rather than collapsing it into a docs-only path. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Rahul! 👋🏼 I created this draft PR to illustrate what I meant: #3488. Basically, there would be just one notice that would be there only when it makes sense. So if the user did not start Firefox, the notice would not be rendered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ivan — your draft made this much easier to evaluate, and it landed exactly where you said it would: tested the trust-then-launch flow end-to-end on the Linux VM and the existing notice reappears on focus return as you described, then the second Trust click silently imports into the new profile.
Folded your commit into this PR as
1194d83f, so the new Firefox notice + its detection/IPC/RTK Query plumbing are all gone. Updating the PR description next to reflect the new shape. Happy to leave #3488 to you to close once you've had a chance to look.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates, Rahul! I will now re-review and test this PR.