-
Notifications
You must be signed in to change notification settings - Fork 4
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
Unable to log in to Hypothesis from USA Today pages in Via #333
Comments
Interestingly when I visit that URL I get redirected to https://eu.usatoday.com/story/news/politics/2022/03/07/ukraine-russia-invasion-live-updates/9404740002/ and I can't reproduce there in Chrome 101. |
To make this more annoying, when I try to go to eu. I get redirected to www where I can reproduce it 🙃 The following is actually a WEBM file (my OS's screen recorder outputs WEBM, but GitHub doesn't support WEBM file uploads). It won't play in browsers without WEBM support. Chrome Version 99.0.4844.51 (Official Build) (64-bit) Screencast.from.03-07-2022.01.49.12.PM.webm.mp4 |
I was able to reproduce in Chrome using Sauce Labs to load the page from a US-based server, using the browser extension. The This might be something to do with the headers the top-level frame is returning:
|
The
I'm not clear how this interacts with iframes yet. This feature request in this issue is written with the assumption that the target audience is a web developer who can change the headers that web pages are served with. We can change these headers in Via, but not in the bookmarklet or browser extension. |
Based on @robertknight 's investigation. We really have 2 issues here:
|
Add a test case for pages served with the `Cross-Origin-Opener-Policy: same-origin` header, which currently breaks the client's login popup. This reproduces the issue from https://github.com/hypothesis/product-backlog/issues/1333.
Add a test case for pages served with the `Cross-Origin-Opener-Policy: same-origin` header, which currently breaks the client's login popup. This reproduces the issue from https://github.com/hypothesis/product-backlog/issues/1333.
This issue only affects new login attempts, existing logins can still work. A workaround is to first login when visiting another page, and then visit USA Today. When using the Chrome extension, this will work in browsers that allow Hypothesis to access its previous login state across different sites. |
For the case of the embedded client, an alternative same-origin messaging system may work. I got a proof of concept working in Chrome and Firefox using
A simple but ugly solution for this would be to add a manual "Close" button to the window that is displayed if the window is unable to automatically close itself for any reason. In the browser extension or custom clients we can't use BroadcastChannel proof of concept: Changes in h: diff --git a/h/static/scripts/post-auth.js b/h/static/scripts/post-auth.js
index fb5330700..c2d8677cd 100644
--- a/h/static/scripts/post-auth.js
+++ b/h/static/scripts/post-auth.js
@@ -11,50 +11,19 @@ import { settings } from './base/settings';
const appSettings = settings(document);
function sendAuthResponse() {
- if (!window.opener) {
- console.error('The client window was closed');
- return;
- }
-
const msg = {
type: 'authorization_response',
code: appSettings.code,
state: appSettings.state,
};
- // `document.documentMode` is a non-standard IE-only property.
- const isIE = 'documentMode' in document;
- if (isIE) {
- try {
- // IE 11 does not support `window.postMessage` between top-level windows
- // [1]. For the specific use case of the Hypothesis client, the sidebar
- // HTML page is on the same domain as the h service, so we can dispatch
- // the "message" event manually. Third-party clients will need to use
- // redirects to receive the auth code if they want to support IE 11.
- //
- // [1] https://blogs.msdn.microsoft.com/thebeebs/2011/12/21/postmessage-popups-and-ie/
-
- // Create an event in the target window.
- const clientWindow = window.opener;
- const event = clientWindow.document.createEvent('HTMLEvents');
- event.initEvent('message', true, true);
-
- // Clone the `msg` object into an object belonging to the target window.
- event.data = clientWindow.JSON.parse(JSON.stringify(msg));
-
- // Trigger "message" event listener in the target window.
- clientWindow.dispatchEvent(event);
- window.close();
- } catch (err) {
- console.error(
- 'The "web_message" response mode is not supported in IE',
- err
- );
- }
- return;
+ if (window.opener) {
+ window.opener.postMessage(msg, appSettings.origin);
+ } else {
+ const channel = new BroadcastChannel('client-login');
+ channel.postMessage(msg);
}
- window.opener.postMessage(msg, appSettings.origin);
window.close();
} Changes in the client: diff --git a/src/sidebar/services/auth.js b/src/sidebar/services/auth.js
index f00e7b1ec..77cbbadc9 100644
--- a/src/sidebar/services/auth.js
+++ b/src/sidebar/services/auth.js
@@ -270,9 +270,8 @@ export class AuthService extends TinyEmitter {
* then exchange for access and refresh tokens.
*/
async function login() {
- const authWindow = OAuthClient.openAuthPopupWindow($window);
const client = await oauthClient();
- const code = await client.authorize($window, authWindow);
+ const code = await client.authorize($window);
// Save the auth code. It will be exchanged for an access token when the
// next API request is made.
diff --git a/src/sidebar/util/oauth-client.js b/src/sidebar/util/oauth-client.js
index eeb821d32..a8b50f96d 100644
--- a/src/sidebar/util/oauth-client.js
+++ b/src/sidebar/util/oauth-client.js
@@ -116,11 +116,9 @@ export class OAuthClient {
* Returns an authorization code which can be passed to `exchangeAuthCode`.
*
* @param {Window} $window - Window which will receive the auth response.
- * @param {Window} authWindow - Popup window where the login prompt will be shown.
- * This should be created using `openAuthPopupWindow`.
* @return {Promise<string>}
*/
- authorize($window, authWindow) {
+ authorize($window) {
// Random state string used to check that auth messages came from the popup
// window that we opened.
//
@@ -149,7 +147,8 @@ export class OAuthClient {
}
$window.removeEventListener('message', authRespListener);
}
- $window.addEventListener('message', authRespListener);
+ const channel = new BroadcastChannel('client-login');
+ channel.addEventListener('message', authRespListener);
});
// Authorize user and retrieve grant token
@@ -160,10 +159,21 @@ export class OAuthClient {
authURL.searchParams.set('response_type', 'code');
authURL.searchParams.set('state', state);
- // @ts-ignore - TS doesn't support `location = <string>`. We have to
- // use this method to set the URL rather than `location.href = <string>`
- // because `authWindow` is cross-origin.
- authWindow.location = authURL.toString();
+ // In Chrome & Firefox the sizes passed to `window.open` are used for the
+ // viewport size. In Safari the size is used for the window size including
+ // title bar etc. There is enough vertical space at the bottom to allow for
+ // this.
+ //
+ // See https://bugs.webkit.org/show_bug.cgi?id=143678
+ const width = 475;
+ const height = 430;
+ const left = $window.screen.width / 2 - width / 2;
+ const top = $window.screen.height / 2 - height / 2;
+
+ // Generate settings for `window.open` in the required comma-separated
+ // key=value format.
+ const authWindowSettings = `left=${left},top=${top},width=${width},height=${height}`;
+ $window.open(authURL, 'Log in to Hypothesis', authWindowSettings);
return authResponse.then(rsp => rsp.code);
} |
The process of creating and navigating the login popup used to involve two steps, first creating a blank window and then navigating it to the final authorization URL. This was needed because, in Firefox, the popup window had to be created in the same turn of the event loop as the user's click on the "Log in" button (otherwise the popup blocker would trigger) but generating the authorization URL involved an async "fetch" of API links. The major browsers have now all settled on a more flexible model for allowing popups in response to user gestures, where the popup must be opened within a certain time window of the gesture. In practice the timeout seems to be ~1000ms in Safari and longer than that in other browsers. In this context we expect the async delay between the user clicking the "Log in" button and us creating the popup to be ~0ms, since the API links should already have been fetched at this point and so we're just fetching locally cached values. Based on this assumption, the flow for creating the login popup window has been simplified to create the popup window at the final URL immediately, removing the need to open a blank window as a first step. Simplifying the code here will make it easier to change how the popup window and sidebar communicate, eg. to resolve an issue with the new Cross-Origin-Opener-Policy header [1]. [1] https://github.com/hypothesis/product-backlog/issues/1333
The process of creating and navigating the login popup used to involve two steps, first creating a blank window and then navigating it to the final authorization URL. This was needed because, in Firefox, the popup window had to be created in the same turn of the event loop as the user's click on the "Log in" button (otherwise the popup blocker would trigger) but generating the authorization URL involved an async "fetch" of API links. The major browsers have now all settled on a more flexible model for allowing popups in response to user gestures, where the popup must be opened within a certain time window of the gesture. In practice the timeout seems to be ~1000ms in Safari and longer than that in other browsers. In this context we expect the async delay between the user clicking the "Log in" button and us creating the popup to be ~0ms, since the API links should already have been fetched at this point and so we're just fetching locally cached values. Based on this assumption, the flow for creating the login popup window has been simplified to create the popup window at the final URL immediately, removing the need to open a blank window as a first step. Simplifying the code here will make it easier to change how the popup window and sidebar communicate, eg. to resolve an issue with the new Cross-Origin-Opener-Policy header [1]. [1] https://github.com/hypothesis/product-backlog/issues/1333
Add a test case for pages served with the `Cross-Origin-Opener-Policy: same-origin` header, which currently breaks the client's login popup. This reproduces the issue from https://github.com/hypothesis/product-backlog/issues/1333.
Add a test case for pages served with the `Cross-Origin-Opener-Policy: same-origin` header, which currently breaks the client's login popup. This reproduces the issue from https://github.com/hypothesis/product-backlog/issues/1333.
If we are unable to rely on
A downside of this change is that it is going to be slightly slower due to the added redirect. An upside is it will remove the client's reliance on the non-standard Something I'm not sure about, and will need to check, is whether HTTP URLs are allowed to redirect to extension URLs. |
BroadcastChannel is supported in Safari 15.4 but I encountered an issue with it being unable to be used for popup => opener communication, whereas in Chrome and Firefox this works. This might relate to origin partitioning that is implemented in Safari and being planned for Chrome. See whatwg/html#5803. |
Some related discussion about the Cross-Origin-Opener-Policy specs and popups here: whatwg/html#7713. From the linked explainer:
That unfortunately might not help us since we can't control the COOP policy on the top-level document in all cases, though we can control it in Via. whatwg/html#6364 is also relevant. In particular the second bullet point in the issue description (under "possible alternatives") is what we are looking at implementing here with BroadcastChannel. |
The situation with standards is fluid, but whatwg/html#6364 has the most up-to-date discussion. My understanding of where this is likely to end up is:
So the status quo seems to be that there is no solution that works in all browsers, all methods of delivering the client and does not affect the page's ability to use web platform features that are depend on COOP. Some options solutions we can implement today:
|
@robertknight Focusing only on Via for this ticket (Extension covered in #353):
|
In general Via does not strip the With the original test URL (https://www.usatoday.com/story/news/politics/2022/03/07/ukraine-russia-invasion-live-updates/9404740002/) however I do see a difference. To avoid the redirect mentioned in #333 (comment) I ran these tests from one of our AWS servers:
Returns a response with both
Returns a response with a So there might be something about the way that ViaHTML makes the request which affects on USA Today's end whether a |
It turns out that one of the engineers working on these specifications has created a test page which can be used for testing this with Via. See https://via.hypothes.is/https://arturjanc.com/cgi-bin/coop/index-with-coop.py?coop=same-origin Inspecting the requests made when accessing this page through Via, I can see that Via has preserved the The lack of COOP header on the top-level frame may eventually cause problems for some sites if they rely on using features which are only available when these headers are set. Today however this is not an issue. If instead of using Via you visit https://arturjanc.com/cgi-bin/coop/index-with-coop.py?coop=same-origin directly and activate the extension, you'll find that login fails. What you can do for the extension is visit some other website, login there and then reload the page where COOP is set. This will continue to work in Chrome until third-party cookies are phased out. In Safari however third-party cookies are blocked and so this doesn't work. So to summarize: contrary to what I wrote earlier, it looks like the Cross-Origin-Opener-Policy header is not a problem for public Via today. This is because the header must be set on the top-level frame to take effect. In Via however, the proxied headers from the upstream site apply to the inner iframe where ViaHTML document is being loaded. In the LMS's Via we might eventually run into the problem again if the LMS itself enables the COOP header. I don't expect that will happen soon. |
We've agreed to close this issue for now since the problem doesn't currently affect Via. For the Chrome extension there is a workaround which is to log in using a different site. I will keep an eye on the relevant standards discussions and create / update issues as needed. |
Update 2022-04-19: The problem with usatoday.com and the Cross-Origin-Opener-Policy header does not affect Via at present. It only affects the extension and bookmarklet. See #333 (comment) for explanation.
Bug report form
Steps to reproduce
Expected behaviour
The Hypothesis login pop-up window should appear with appropriate username and password fields available.
Actual behaviour
An entirely blank pop-up window displays.
Browser/system information
Tested on:
Note: The issue does not seem to affect Safari 15.3
Additional details
If you are already logged in to Hypothesis all functionality works as expected.
The text was updated successfully, but these errors were encountered: