Skip to content
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

Add crossorigin=use-credentials to link[rel=manifest] for sake of Basic Auth #371

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

westonruter
Copy link
Collaborator

In a support topic, it was revealed that the Web App Manifest fails to load when Basic Auth is being used.

I was able reproduce this with a test plugin:

<?php
/**
 * Plugin Name: Basic Auth
 */


if ( is_admin() || 'cli' === php_sapi_name() ) {
	return;
}

if (
	! isset( $_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW'] )
	||
	'admin' !== $_SERVER['PHP_AUTH_USER']
	||
	'password' !== $_SERVER['PHP_AUTH_PW']
) {
	status_header( 401 );
	header( 'WWW-Authenticate: Basic realm="My Realm"' );
	echo "Authorization required";
	exit;
}

When loading a page after providing authentication, I got:

image

When crossorigin="use-credentials" was added, however, there was no issue loading the Web App Manifest.

This appears to be the the best practice per w3c/manifest#535 (comment). For more context and explanation for why this is needed (but isn't for link[rel=stylesheet], see w3c/manifest#535 (comment).

Solution also affirmed in koajs/basic-auth#19 (comment) and https://stackoverflow.com/a/51157352/93579.

If a site doesn't use HTTP Basic Auth, then sending credentials won't make any difference either.

@westonruter westonruter added the web-app-manifest Web App Manifests label Dec 19, 2020
@westonruter westonruter added this to the 0.6 milestone Dec 19, 2020
@codecov-io
Copy link

Codecov Report

Merging #371 (8b8e0ad) into develop (6c3500f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #371   +/-   ##
==========================================
  Coverage      24.28%   24.28%           
  Complexity       364      364           
==========================================
  Files             55       55           
  Lines           1865     1865           
==========================================
  Hits             453      453           
  Misses          1412     1412           
Flag Coverage Δ Complexity Δ
php 24.28% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
wp-includes/class-wp-web-app-manifest.php 53.27% <ø> (ø) 32.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c3500f...8b8e0ad. Read the comment docs.

@westonruter westonruter removed this from the 0.6 milestone Dec 19, 2020
@westonruter westonruter removed the request for review from felixarntz December 19, 2020 07:31
@westonruter
Copy link
Collaborator Author

Actually, using a service worker on a site with Basic Auth is currently broken. So fixing this one issue with the Web App Manifest doesn't have much value at all, since as soon as the service worker is installed and the auth expires, then users will get a 401 error responses without any auth prompt. For more details, see GoogleChrome/workbox#2515. This is not an issue with Workbox but rather a platform issue in Chrome and other browsers.

@westonruter westonruter changed the title Add crossorigin=use-credentials to link[rek=manifest] for sake of Basic Auth Add crossorigin=use-credentials to link[rel=manifest] for sake of Basic Auth Dec 21, 2020
Copy link
Collaborator

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Great!

@westonruter westonruter force-pushed the add/use-credentials-in-manifest-link branch from 8b8e0ad to 8fc13e5 Compare December 22, 2020 06:33
@westonruter
Copy link
Collaborator Author

Build for testing: pwa.zip

@westonruter westonruter force-pushed the add/use-credentials-in-manifest-link branch from 8fc13e5 to 7170ba4 Compare July 13, 2021 16:59
@google-cla google-cla bot added the cla: yes label Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants