Handle missing Web Crypto API in non-HTTPS contexts when generating the already-submitted `sessionStorage` key #1911

ShyamGadde · 2025-03-10T11:14:56Z

Summary

Fixes #1904

Relevant technical choices

Returns null instead of blocking detection when the Web Crypto API is unavailable, allowing the script to continue running while still tracking session data when possible.
Logs a warning when the Web Crypto API is unavailable, advising users to use HTTPS or localhost for testing.
Catches errors during hashing and logs them instead of failing outright.
Ensures sessionStorage is only accessed if a valid key is generated.

Signed-off-by: Shyamsundar Gadde <[email protected]>

westonruter · 2025-03-10T18:36:28Z

plugins/optimization-detective/detect.js

+		// eslint-disable-next-line no-console
+		console.warn(
+			'[Optimization Detective] Web Crypto API is not available. This API is only available in secure contexts (HTTPS). Detection cannot proceed. If you are testing locally, ensure you use HTTPS or run on localhost.'
+		);
+		throw new Error(
+			'Web Crypto API is unavailable in this context. Try using HTTPS or localhost.'
+		);


Shouldn't this reuse the logger?

Suggested change

// eslint-disable-next-line no-console

console.warn(

'[Optimization Detective] Web Crypto API is not available. This API is only available in secure contexts (HTTPS). Detection cannot proceed. If you are testing locally, ensure you use HTTPS or run on localhost.'

);

throw new Error(

'Web Crypto API is unavailable in this context. Try using HTTPS or localhost.'

);

error(

'[Optimization Detective] Web Crypto API is not available. This API is only available in secure contexts (HTTPS). Detection cannot proceed. If you are testing locally, ensure you use HTTPS or run on localhost.'

);

return;

westonruter · 2025-03-10T19:09:19Z

plugins/optimization-detective/detect.js

-		) {
-			log(
-				'The current client session already submitted a fresh URL Metric for this URL so a new one will not be collected now.'
+	let alreadySubmittedSessionStorageKey;


Is this change needed? The above check for crypto.subtle should mean this change won't be needed, right?

For example, if I go to http://httpforever.com/ then I see that crypto.subtle is undefined in the console.

Yes, although we check for crypto.subtle, I think the try/catch is still necessary.

If we were to just return early from getAlreadySubmittedSessionStorageKey when crypto.subtle is unavailable (instead of throwing an error), the function would return undefined and the key would be undefined for all URLs the user visits. This could lead to misleading messages like "The current client session already submitted a fresh URL Metric..." before the freshnessTTL expires.

Another option would be to just throw a simple error when Web Crypto API is not available in the getAlreadySubmittedSessionStorageKey function (without logging):

throw new Error( 'Web Crypto API is unavailable' );

And then handle the detailed messaging in the catch block inside detect:

catch (err) { if ( err.message === 'Web Crypto API is unavailable' ) { error( 'Unable to create session storage key: Web Crypto API is not available. This API is only available in secure contexts (HTTPS). Detection cannot proceed. If you are testing locally, ensure you use HTTPS or run on localhost.' ); } else { error('Unable to create session storage key: ' + err.message); } return; }

This approach also avoids duplicate error messages.

Ok, but why would crypto.subtle need the try/catch?

Ok, but why would crypto.subtle need the try/catch?

I'm sorry for not being clear. I wasn't suggesting adding a try/catch in the getAlreadySubmittedSessionStorageKey function itself. Instead, I was referring to the try/catch block I added in the detect function where we call await getAlreadySubmittedSessionStorageKey().

The check for crypto.subtle is in the getAlreadySubmittedSessionStorageKey function, which throws an error when it's not available. Then, that error is caught in the detect function's try/catch block.

I've implemented this approach in a66325a to illustrate what I was describing.

Thanks for that. Here's another iteration on top of your suggestion: what if we have it so that the function just returns a constant string in the case of an error? This would essentially be bringing the functionality back to how it behaved before the URL hashing was introduced. Oh wait, actually that functionality is still in place:

performance/plugins/optimization-detective/detect.js

Lines 475 to 478 in a66325a

if ( isStorageLocked( getCurrentTime(), storageLockTTL ) ) {

warn( 'Aborted detection due to storage being locked.' );

return;

}

So how about it just return null to indicate that this isn't available? There is already storage locking going on with a session storage key, so this is basically a secondary (redundant?) lock.

Here's what I have in mind: d5679ec

This will ensure that URL Metrics can continue to be collected on HTTP sites. The already-submitted "lock" is just intended to increase the diversity of the URL Metrics being collected, so if it's not available it's not a big deal. Better to skip it altogether in favor of collecting URL Metrics as opposed to doing nothing.

Thanks for iterating on this! That approach makes a lot of sense—returning null instead of blocking detection entirely keeps things functional while still maintaining some level of session tracking. I hadn't fully considered that before.

Signed-off-by: Shyamsundar Gadde <[email protected]>

plugins/optimization-detective/detect.js

westonruter · 2025-03-11T16:57:26Z

plugins/optimization-detective/detect.js

-		) {
-			log(
-				'The current client session already submitted a fresh URL Metric for this URL so a new one will not be collected now.'
+	let alreadySubmittedSessionStorageKey;


Thanks for that. Here's another iteration on top of your suggestion: what if we have it so that the function just returns a constant string in the case of an error? This would essentially be bringing the functionality back to how it behaved before the URL hashing was introduced. Oh wait, actually that functionality is still in place:

performance/plugins/optimization-detective/detect.js

Lines 475 to 478 in a66325a

if ( isStorageLocked( getCurrentTime(), storageLockTTL ) ) {

warn( 'Aborted detection due to storage being locked.' );

return;

}

So how about it just return null to indicate that this isn't available? There is already storage locking going on with a session storage key, so this is basically a secondary (redundant?) lock.

…lable

github-actions · 2025-03-12T02:27:09Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ShyamGadde <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: swissspidy <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

westonruter

Thank you for iterating on this with me!

swissspidy · 2025-03-12T08:13:37Z

plugins/optimization-detective/detect.js

-		urlMetricGroupStatus.minimumViewportWidth,
-		urlMetricGroupStatus.maximumViewportWidth || '',
-	].join( '-' );
+	if ( ! window.crypto || ! window.crypto.subtle ) {


Nit: only checking window.crypto should suffice. There's no combination where the former is available but the latter isn't.

So like so?

Suggested change

if ( ! window.crypto || ! window.crypto.subtle ) {

if ( ! window.crypto ) {

Nevertheless someone could try to polyfill crypto.getRandomValues() but not define crypto.subtle.

So like so?

Yes that's what I meant.

Nevertheless someone could try to polyfill crypto.getRandomValues() but not define crypto.subtle.

Sounds unlikely to me but if we are concerned about that then we should keep the check.

plugins/optimization-detective/detect.js

Handle missing Web Crypto API in non-HTTPS contexts

ecf0fbd

Signed-off-by: Shyamsundar Gadde <[email protected]>

ShyamGadde added [Type] Bug An existing feature is broken [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Mar 10, 2025

ShyamGadde added this to the optimization-detective 1.0.0-beta3 milestone Mar 10, 2025

westonruter reviewed Mar 10, 2025

View reviewed changes

Eliminate duplicate error messages

a66325a

Signed-off-by: Shyamsundar Gadde <[email protected]>

westonruter reviewed Mar 11, 2025

View reviewed changes

westonruter and others added 2 commits March 11, 2025 15:22

Make already-submitted sessionStorage key optional if crypto not avai…

d5679ec

…lable

Merge branch 'trunk' into fix/od-on-non-https-site

2e48f6f

ShyamGadde marked this pull request as ready for review March 12, 2025 02:26

ShyamGadde requested a review from felixarntz as a code owner March 12, 2025 02:26

westonruter approved these changes Mar 12, 2025

View reviewed changes

westonruter requested a review from swissspidy March 12, 2025 05:35

swissspidy reviewed Mar 12, 2025

View reviewed changes

plugins/optimization-detective/detect.js Show resolved Hide resolved

swissspidy approved these changes Mar 12, 2025

View reviewed changes

westonruter merged commit 8efc280 into WordPress:trunk Mar 12, 2025
18 of 19 checks passed

westonruter changed the title ~~Handle missing Web Crypto API in non-HTTPS contexts~~ Handle missing Web Crypto API in non-HTTPS contexts when generating the already-submitted sessionStorage key Mar 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle missing Web Crypto API in non-HTTPS contexts when generating the already-submitted `sessionStorage` key #1911

Handle missing Web Crypto API in non-HTTPS contexts when generating the already-submitted `sessionStorage` key #1911

ShyamGadde commented Mar 10, 2025 •

edited

Loading

westonruter Mar 10, 2025

westonruter Mar 10, 2025

westonruter Mar 10, 2025

ShyamGadde Mar 11, 2025

ShyamGadde Mar 11, 2025 •

edited

Loading

westonruter Mar 11, 2025

ShyamGadde Mar 11, 2025

ShyamGadde Mar 11, 2025

westonruter Mar 11, 2025

westonruter Mar 11, 2025

ShyamGadde Mar 12, 2025

westonruter Mar 11, 2025

github-actions bot commented Mar 12, 2025

westonruter left a comment

swissspidy Mar 12, 2025

westonruter Mar 12, 2025

westonruter Mar 12, 2025

swissspidy Mar 12, 2025

	if ( isStorageLocked( getCurrentTime(), storageLockTTL ) ) {
	warn( 'Aborted detection due to storage being locked.' );
	return;
	}

	if ( ! window.crypto \|\| ! window.crypto.subtle ) {
	if ( ! window.crypto ) {

Handle missing Web Crypto API in non-HTTPS contexts when generating the already-submitted sessionStorage key #1911

Handle missing Web Crypto API in non-HTTPS contexts when generating the already-submitted sessionStorage key #1911

Conversation

ShyamGadde commented Mar 10, 2025 • edited Loading

Summary

Relevant technical choices

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

ShyamGadde Mar 11, 2025 • edited Loading

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

Choose a reason for hiding this comment

github-actions bot commented Mar 12, 2025

westonruter left a 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

Handle missing Web Crypto API in non-HTTPS contexts when generating the already-submitted `sessionStorage` key #1911

Handle missing Web Crypto API in non-HTTPS contexts when generating the already-submitted `sessionStorage` key #1911

ShyamGadde commented Mar 10, 2025 •

edited

Loading

ShyamGadde Mar 11, 2025 •

edited

Loading