SSL Support: Hide all HTTPS help messages when the certificate is trusted by youknowriad · Pull Request #1117 · Automattic/studio

youknowriad · 2025-03-24T11:29:19Z

Related issues

Fixes Feature Request: Handle already trusted Studio certificate. #1109
Fixes STU-255-linear-issue

Proposed Changes

Implements a function to check whether the CA certificate is trusted.
Hide all the SSL help texts when the CA certificate is trusted.
Auto enable https when the custom domain is enabled and the root certificate is trusted

Testing Instructions

Create an SSL site
If your CA certificate is not trusted, you should see help text on the creation form and on the settings tab

If your CA certificate is trusted, you should not see either of these.

youknowriad · 2025-03-24T11:29:38Z

I didn't not a add a dialog because the existing help text in the form is enough IMO.

bgrgicak

It works, but the component didn't update until I switched to another tab and back.
It's not a big deal, but it would be nice if we would periodically check in the background, or add a button so the user can check manually.

sejas

I tested it on macOS, and it works almost as expected. Great work!
It would be great, as Bero mentioned, to "refresh" the state when going back to Studio. You can use useWindowListener('focus', yourFunction); to trigger a call when the user focuses back to Studio.
As Wojtek said, "trusted/enabled" with the current implementation means "installed" in the keychain but not necessarily trusted by the user.

I was able to install and trust a certificate and after changing tabs I confirm the message was gone. 👍 .

New site

No CA	CA Installed

Edit site

No CA	CA Installed

While testing this PR I noticed some edge cases that are worth mentioning in case they are not already tracked in issues:

When editing a site > unchecking the HTTPS and saving does nothing.
While saving the edit site modal, every checkbox and input is "disabled" except "Enable HTTPS". Allowing the user to change the checkbox state when the saving is in process.
The documentation (p2gHKz-wsF-p2) mentions I should click on Open the Help → Trust Certificate menu option in WordPress Studio in order to trust the certificate. But actually that button was in Site settings.
When moving from custom domain to default domain, the links in my site point to the old URL, breaking the styles.

Trust certificate

studio-accept-certificate.mp4

Uncheck Enable HTTPS

enable-https-checkbox.mp4

sejas · 2025-03-25T12:24:55Z

+														{ __( 'Learn how' ) }
+														<span aria-label={ __( '(opens in a web browser)' ) }>&#8599;</span>


There should be a space between Learn how and the arrow. Same would apply to the Learn how in edit site overview. I've observed that we are not using the arrow to express external link in other "learn more" links, in my opinion the arrow make sense though.

Figma design ( RToz6tIuQ7nlZrikBte4GU-fi-6924_4203 ):

fredrikekelund · 2025-03-25T13:13:55Z

 		setIsChangeWpError( '' );
-		setEnableHttps( selectedSite.enableHttps ?? false );
-	}, [ currentWpVersion, selectedSite ] );
+		setEnableHttps( selectedSite.enableHttps ?? isCertificateTrusted );


This change makes it so the HTTPS checkbox is enabled by default when I open the Edit site form for a site with a custom domain without HTTPS.

Not sure if that's intentional, but it seems like unexpected behavior to me.

Unresolving this conversation because I still don't understand this change.

fredrikekelund · 2025-03-25T13:42:15Z

+	const platform = process.platform;
+	if ( platform === 'win32' ) {
+		try {
+			const { stdout } = await execFilePromise( 'certutil', [ '-store', 'ROOT', CA_NAME ] );


Similar to @wojtekn's comment about the security command on macOS, I would use something like certutil -verify ${ CERT_PATH } here and look for the following output (or parts of it):

Verified Issuance Policies: All Verified Application Policies: 1.3.6.1.5.5.7.3.1 Server Authentication 1.3.6.1.5.5.7.3.2 Client Authentication Cert is a CA certificate Cannot check leaf certificate revocation status CertUtil: -verify command completed successfully.

When the certificate is on the keychain but not fully trusted, I get the following output:

Verified Issuance Policies: All Verified Application Policies: None Cert is a CA certificate Cannot check leaf certificate revocation status CertUtil: -verify command completed successfully.

And when it's not on the keychain, I get the following output:

Verifies against UNTRUSTED root Cert is a CA certificate Cannot check leaf certificate revocation status CertUtil: -verify command completed successfully.

I'm going to need your help here. I did make changes for windows, but I'm not sure relying on the output string is good enough (apparently the output might differ), I'm currently relying on the exit code only but I'm also not sure whether that's sufficient, I'd appreciate some testing :)

apparently the output might differ

I understand if you have a general concern regarding the output structure, but I'm still curious if you verified this assumption in testing or in documentation?

exit code

The exit code for certutil -verify doesn't seem to change depending on the certificate trust status in my testing.

I looked closely at the PowerShell PKI module, which, in some ways, seems like a more scripting-friendly option. However, I was not able to get the correct output when the certificate is on the keychain, but not properly trusted (open the certmgr program > Trusted Root Certification Authorities > Certificates > WordPress Studio CA > Properties > Purposes > Disable all purposes). This might seem like an edge case, but I don't have full confidence in the PowerShell approach since it doesn't report this correctly.

I would love to suggest that we employ a different strategy: request a local HTTPS URL and check for HTTPS errors. This would work cross-platform and seems more bulletproof, but it also requires the proxy server to be started (we recently delayed proxy server upstart in #1089).

I'm not sure this is better (or that it would work, for that matter), but I wanted to raise it nonetheless. Have you considered this option, @youknowriad?

I didn't not consider it, I'm happy to try it but it would mean reverting the lazy start for the proxy.

I see. The Microsoft docs for certutil do say this:

Certutil isn't recommended to be used in any production code and doesn't provide any guarantees of live site support or application compatibilities. It's a tool utilized by developers and IT administrators to view certificate content information on devices.

Our use of certutil wouldn't be mission-critical, but there's clearly an inherent fragility.

Feel free to explore the PowerShell PKI module option to see if that works. Parsing the output should definitely be easier.

I would use an output to avoid additional complexity. If we checked for UNTRUSTED and got an unexpected output, in the worst case scenario users would not see a message asking them to trust the certificate.

If we stick with certutil, we should parse the Verified Application Policies: part of the output, too. I'm not sure if both the Server Authentication and Client Authentication policies are needed, but at least one of them is.

So I've updated the code here to check for the Verified Application Policies but my confidence with my updates is not 100%. I was not able to test despite spending a lot of time trying to setup a windows environment.

That said, even if it fails, the risks are small to the users. (hide/show a message)

mcsf · 2025-03-27T15:47:36Z

+				'-s',
+				CA_NAME,


Looking at the manual (man security), option -s is deprecated in favour of -n:

-s sslHost Specify SSL host name for the ssl policy. (This option is deprecated; use -n instead.) -n name Specify a name to be verified, e.g. the SSL host name for the ssl policy, or RFC822 email address for the smime policy. For backward compatibility, if the -n option is provided without an argument, it will be interpreted as equivalent to -N.

That said, CA_NAME isn't a host name, it's the string used as certicates' commonName, so I'm not sure what this option adds to the command.

The -n flag is apt for the domain-specific certificates:

security verify-cert -c domains/foo.wp.local.crt -p ssl -s foo.wp.local # ok security verify-cert -c domains/foo.wp.local.crt -p ssl -s example.org # nope

but it doesn't work with the CA:

security verify-cert -c studio-ca.crt -p ssl -s example.org # no errors # but this is misleading!

Let's remove the -s CA_NAME options. As @mcsf mentioned, it doesn't make a difference.

mcsf · 2025-03-28T15:30:54Z

+	const checkCertificateTrust = useCallback( () => {
+		getIpcApi()
+			.isCATrusted()
+			.then( ( trusted ) => {
+				if ( isMounted.current ) {
+					setIsTrusted( trusted );
+				}
+			} );
+	}, [ setIsTrusted ] );


Not super important, but there are a bunch of different identifiers coexisting:

use certificate trust

check certificate trust

is CA trusted

is Root CA trusted

With just a little refactoring we could probably just have useIsRootCATrusted and isRootCATrusted. Especially if in ipc-handlers.ts we imported the whole module to avoid collisions:

export async function isRootCATrusted( _event: IpcMainInvokeEvent ): Promise< boolean > { return certificateManager.isRootCATrusted(); }

mcsf

Can't vouch for the Windows side, but LGTM on macOS!

Copilot

Pull Request Overview

This PR adds support for hiding HTTPS help messages when the CA certificate is trusted. The changes include introducing a new IPC method and associated hook to check certificate trust, updating site settings logic to conditionally hide SSL help texts, and adding corresponding tests for the new functionality.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/preload.ts	Added new ipcRenderer API “isCATrusted” to check certificate trust status.
src/modules/site-settings/edit-site-details.tsx	Updated logic to conditionally enable HTTPS based on certificate trust.
src/lib/certificate-manager.ts	Implemented isRootCATrusted to verify if the system trusts the CA certificate.
src/ipc-handlers.ts	Exposed isCATrusted IPC handler wrapping the underlying certificate trust check.
src/hooks/use-certificate-trust.ts	Introduced a React hook to track certificate trust state.
Various test files	Added mocks for isCATrusted to simulate trusted certificate scenarios.

gavande1 · 2025-04-24T07:06:49Z

+			const { stdout } = await execFilePromise( 'certutil', [
+				'-verify',
+				'-urlfetch',
+				'-purpose',


I am getting the unknown option -purpose error on Windows 11 Pro. It needs some refactoring.

gavande1 · 2025-04-24T07:08:14Z

FYI, I am taking over this task. p1745233995626849-slack-C06DRMD6VPZ

Copilot

Pull Request Overview

This PR implements the SSL help text hiding functionality when the CA certificate is trusted. Key changes include:

Adding a new IPC API (isCATrusted) and a corresponding hook (useCertificateTrust) to check certificate trust.
Updating UI components (e.g. EditSiteDetails, SiteForm, ContentTabSettings) to conditionally hide HTTPS help messages based on the certificate trust.
Enhancing certificate-manager functions and updating tests to simulate trusted certificates.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/preload.ts	Added new isCATrusted IPC call.
src/modules/site-settings/edit-site-details.tsx	Updated certificate trust handling in form state and update logic.
src/lib/certificate-manager.ts	Introduced isRootCATrusted function and improved certificate trust logic.
src/ipc-handlers.ts	Mapped isCATrusted to certificate-manager functionality.
src/hooks/use-certificate-trust.ts	Created new hook to check certificate trust via IPC.
Test files	Added mocks for isCATrusted to support the new behavior in tests.
src/components/site-form.tsx & content-tab-settings.tsx	Updated conditional rendering to hide help texts based on trust status.

Comments suppressed due to low confidence (2)

src/modules/site-settings/edit-site-details.tsx:102

Using the nullish coalescing operator with isCertificateTrusted to set enableHttps may unintentionally force HTTPS to be enabled even when not explicitly desired; please verify that this behavior aligns with the intended UX.

setEnableHttps( selectedSite.enableHttps ?? isCertificateTrusted );

src/modules/site-settings/edit-site-details.tsx:159

Combining the state flag enableHttps with isCertificateTrusted in this condition may override the user’s HTTPS preference; review the logic to ensure it reflects the intended behavior when the certificate is trusted.

enableHttps: !! usedCustomDomain && ( isCertificateTrusted || enableHttps ),

gcsecsey

Thanks for driving this forward @gavande1! 🙌

This is looking good, and works well. When trusting the certificate, the change is also immediately reflected in the Studio app.

macOS:

Certificate	Add new site form	Settings tab
Not trusted
Trusted

On Windows, I had the certificate already installed as trusted, so I first removed it by running certutil -delstore ROOT "WordPress Studio CA" in an elevated Command Prompt:

Certificate	Add new site form	Settings tab
Not trusted
Trusted

gcsecsey · 2025-05-02T13:21:38Z

+export function useCertificateTrust(): boolean {
+	const isMounted = useRef( true );
+	const [ isTrusted, setIsTrusted ] = useState< boolean >( false );
+
+	const checkCertificateTrust = useCallback( () => {
+		// If the certificate is already trusted, don't check it again
+		if ( isTrusted ) {
+			return;
+		}
+
+		getIpcApi()
+			.isCATrusted()
+			.then( ( trusted ) => {
+				if ( isMounted.current ) {
+					setIsTrusted( trusted );
+				}
+			} )
+			.catch( ( error ) => {
+				console.error( 'Failed to check certificate trust:', error );
+			} );
+	}, [ setIsTrusted, isTrusted ] );
+
+	useWindowListener( 'focus', checkCertificateTrust );
+
+	useEffect( () => {
+		isMounted.current = true;
+		checkCertificateTrust();
+		return () => {
+			isMounted.current = false;
+		};
+	}, [ checkCertificateTrust ] );
+
+	return isTrusted;
+}


Suggested change

export function useCertificateTrust(): boolean {

const isMounted = useRef( true );

const [ isTrusted, setIsTrusted ] = useState< boolean >( false );

const checkCertificateTrust = useCallback( () => {

// If the certificate is already trusted, don't check it again

if ( isTrusted ) {

return;

}

getIpcApi()

.isCATrusted()

.then( ( trusted ) => {

if ( isMounted.current ) {

setIsTrusted( trusted );

}

} )

.catch( ( error ) => {

console.error( 'Failed to check certificate trust:', error );

} );

}, [ setIsTrusted, isTrusted ] );

useWindowListener( 'focus', checkCertificateTrust );

useEffect( () => {

isMounted.current = true;

checkCertificateTrust();

return () => {

isMounted.current = false;

};

}, [ checkCertificateTrust ] );

return isTrusted;

}

export function useCertificateTrust(): boolean {

const [ isTrusted, setIsTrusted ] = useState< boolean >( false );

const checkCertificateTrust = useCallback( () => {

// If the certificate is already trusted, don't check it again

if ( isTrusted ) {

return;

}

getIpcApi()

.isCATrusted()

.then( ( trusted ) => {

setIsTrusted( trusted );

} )

.catch( ( error ) => {

console.error( 'Failed to check certificate trust:', error );

} );

}, [ setIsTrusted, isTrusted ] );

useWindowListener( 'focus', checkCertificateTrust );

useEffect( () => {

checkCertificateTrust();

}, [ checkCertificateTrust ] );

return isTrusted;

}

Condsider this a suggestion, but after testing this a bit, I don't think using a ref here prevents any extra executions. Was it a different purpose this was added for?

Theoretically, the component could very well have unmounted between the checkCertificateTrust call and when it resolves, and updating the state of an unmounted component will generate a warning.

I am considering refactoring useCertificateTrust using Redux to avoid managing local states. I will create an additional PR to allow everyone to review it easily.

I created follow up PR: #1325

fredrikekelund · 2025-05-05T09:09:36Z

Taking a look at this now.

gavande1 · 2025-05-06T13:24:52Z

@fredrikekelund I added functionality of auto enabling https when the custom domain is enabled and the root certificate is trusted while creating a new site. The changes were added in 0169e08.

…sted

…s trusted

fredrikekelund · 2025-05-07T06:57:33Z

@gavande1, friendly tip, you don't need to rebase the PR branch on trunk to resolve conflicts – a simple merge PR works perfectly fine. The benefit of that approach is that the original commit history is preserved and it's easier for reviewers to pull the latest changes.

fredrikekelund

Latest changes work well 👍

fredrikekelund · 2025-05-07T07:01:14Z

+	// If the custom domain is enabled and the root certificate is trusted, enable HTTPS
+	useEffect( () => {
+		if ( useCustomDomain && isCertificateTrusted && setEnableHttps ) {
+			setEnableHttps( true );
+		}
+	}, [ useCustomDomain, isCertificateTrusted, setEnableHttps ] );


Definitely beyond the scope of this PR, but as we've identified before (#994 (comment)), SiteForm should really be refactored to handle state internally and then pass an entire object containing the form data back to the parent when submitted.

Created issue STU-457 to get back on refactoring.

youknowriad · 2025-05-07T08:32:35Z

Thanks for pushing this one to the end. Highly appreciated folks.

youknowriad added the Enhancement label Mar 24, 2025

youknowriad requested a review from a team March 24, 2025 11:29

youknowriad self-assigned this Mar 24, 2025

bgrgicak reviewed Mar 25, 2025

View reviewed changes

Comment thread src/components/site-form.tsx Outdated

bgrgicak reviewed Mar 25, 2025

View reviewed changes

wojtekn reviewed Mar 25, 2025

View reviewed changes

Comment thread src/lib/certificate-manager.ts Outdated

sejas reviewed Mar 25, 2025

View reviewed changes

fredrikekelund reviewed Mar 25, 2025

View reviewed changes

youknowriad force-pushed the hide/ssl-help-trusted-certificate branch from aea13b8 to 29a44b1 Compare March 25, 2025 16:56

mcsf reviewed Mar 28, 2025

View reviewed changes

mcsf approved these changes Mar 28, 2025

View reviewed changes

youknowriad requested a review from Copilot March 31, 2025 13:20

Copilot AI reviewed Mar 31, 2025

View reviewed changes

wojtekn removed the Enhancement label Apr 10, 2025

gavande1 force-pushed the hide/ssl-help-trusted-certificate branch from 3109041 to 7f33cd8 Compare April 24, 2025 07:00

gavande1 reviewed Apr 24, 2025

View reviewed changes

gavande1 requested review from a team, bgrgicak, Copilot, fredrikekelund, gavande1, mcsf, sejas and wojtekn April 28, 2025 12:43

Copilot AI reviewed Apr 28, 2025

View reviewed changes

gavande1 force-pushed the hide/ssl-help-trusted-certificate branch from 76acdc7 to cc37a3a Compare May 2, 2025 06:40

gcsecsey approved these changes May 2, 2025

View reviewed changes

gavande1 force-pushed the hide/ssl-help-trusted-certificate branch from f889895 to 0169e08 Compare May 6, 2025 13:22

gavande1 requested a review from fredrikekelund May 7, 2025 06:21

youknowriad and others added 21 commits May 7, 2025 12:19

SSL Support: Hide all HTTPS help messages when the certificate is tru…

7b5eff1

…sted

Cleanup

5e6b371

Remove extra variable

b35c940

More solid certificate checking

376fa94

Remove isWindows checks

6c462ac

Refresh the UI on window focus

227abdb

Fix unit tests

826b285

Check for valid policies

ccc8422

Rebase clean up

37130b4

Cleanup post rebase

6dc8059

Cleanup post rebase

17f69ff

Do not use -purpose flag for verifying certificate

e071076

Avoid running command if certificate is already trusted

e5da6f1

Fix linting errors

3b0295a

Refactor mac command to verify ssl

8bec37d

Refactor command to verify ssl on windows

698c807

Change import order

dd4524e

Remove useRef

c0850e2

Remove default IpcMainInvokeEvent param

d2f75b6

Respect enableHttps checkbox value

bf58a3a

Enable https when custom domain is enabled and the root certificate i…

977bc7d

…s trusted

gavande1 force-pushed the hide/ssl-help-trusted-certificate branch from 0169e08 to 977bc7d Compare May 7, 2025 06:49

fredrikekelund approved these changes May 7, 2025

View reviewed changes

gavande1 merged commit 705b8b8 into trunk May 7, 2025

gavande1 deleted the hide/ssl-help-trusted-certificate branch May 7, 2025 07:28

		{ __( 'Learn how' ) }
		<span aria-label={ __( '(opens in a web browser)' ) }>↗</span>

Uh oh!

Conversation

youknowriad commented Mar 24, 2025 • edited by gavande1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Proposed Changes

Testing Instructions

youknowriad commented Mar 24, 2025

Uh oh!

bgrgicak left a comment

Choose a reason for hiding this comment

Uh oh!

sejas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

sejas Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekelund Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

fredrikekelund Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf left a comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Choose a reason for hiding this comment

gavande1 commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

gcsecsey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavande1 May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekelund commented May 5, 2025

gavande1 commented May 6, 2025

fredrikekelund commented May 7, 2025

fredrikekelund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented May 7, 2025

Labels

9 participants

youknowriad commented Mar 24, 2025 •

edited by gavande1

Loading

sejas left a comment •

edited

Loading

sejas Mar 25, 2025 •

edited

Loading

fredrikekelund Mar 26, 2025 •

edited

Loading

fredrikekelund Mar 26, 2025 •

edited

Loading

gavande1 commented Apr 24, 2025 •

edited

Loading

gavande1 May 5, 2025 •

edited

Loading