diff --git a/public/js/nextcloud.js b/public/js/nextcloud.js index 6bdff768..7790fb44 100644 --- a/public/js/nextcloud.js +++ b/public/js/nextcloud.js @@ -64,25 +64,46 @@ }); // ── Sign in with Nextcloud ──────────────────────────────────────────── - // Nextcloud's own login flow. The tab is opened from the click itself, before - // any await, or a popup blocker eats it — the request that fetches the URL is - // allowed to finish afterwards and point the already-open tab at it. + // Nextcloud's own login flow: ask our server to start one, send the person to + // the URL Nextcloud hands back, and poll until they finish. + // + // The first version opened a blank tab during the click and pointed it at the + // URL once the request came back, which is the usual way around a popup + // blocker. It cannot work here: this app sends + // Cross-Origin-Opener-Policy: same-origin, so the handle to that tab is + // severed the moment it goes cross-origin, and assigning its location did + // nothing at all — a blank tab, and a button that looked broken. + // + // So no handle. window.open with 'noopener' needs none, and a click's user + // activation survives the fetch, so it is not treated as a popup. And the + // status line always offers the link itself, which works whatever the browser + // decides about opening windows. var pollTimer = null; function stopPolling(message, tone) { if (pollTimer) { clearInterval(pollTimer); pollTimer = null; } + setFlowStatus(message, tone); + } + + function setFlowStatus(message, tone, link) { var status = document.getElementById('nc-login-flow-status'); - if (status) { - status.textContent = message || ''; - status.style.color = tone === 'bad' ? 'var(--red)' : tone === 'good' ? 'var(--green)' : 'var(--g600)'; - } + if (!status) return; + status.textContent = message || ''; + status.style.color = tone === 'bad' ? 'var(--red)' : tone === 'good' ? 'var(--green)' : 'var(--g600)'; + if (!link) return; + status.appendChild(document.createTextNode(' ')); + var a = document.createElement('a'); + a.href = link; + a.target = '_blank'; + a.rel = 'noopener noreferrer'; + a.textContent = 'Open the sign-in page'; + status.appendChild(a); } document.getElementById('btn-nc-login-flow').addEventListener('click', function () { var url = (document.getElementById('nc-url').value || '').trim(); if (!url) { showToast('Enter your Nextcloud address first', 'error'); return; } - var tab = window.open('', '_blank'); // claimed while the click is still trusted stopPolling('Opening Nextcloud…'); fetch('/api/nextcloud/login-flow/start', { @@ -91,8 +112,10 @@ .then(function (r) { return r.json(); }) .then(function (data) { if (!data.success) throw new Error(data.error || 'Could not start sign-in'); - if (tab) tab.location.href = data.loginUrl; else window.open(data.loginUrl, '_blank'); - stopPolling('Waiting for you to finish signing in…'); + // Opened without a handle, so COOP has nothing to sever. If the browser + // blocks it anyway, the link in the status line is the way through. + window.open(data.loginUrl, '_blank', 'noopener'); + setFlowStatus('Waiting for you to finish signing in.', null, data.loginUrl); var until = Date.now() + 10 * 60 * 1000; pollTimer = setInterval(function () { @@ -114,7 +137,6 @@ }, 3000); }) .catch(function (err) { - if (tab) tab.close(); stopPolling(err.message, 'bad'); }); }); diff --git a/test/nextcloud-login-flow.test.js b/test/nextcloud-login-flow.test.js index 26e567f1..9d7cacce 100644 --- a/test/nextcloud-login-flow.test.js +++ b/test/nextcloud-login-flow.test.js @@ -66,13 +66,35 @@ test('a flow expires, and starting again replaces the old one', () => { assert.match(start, /flow\.owner === req\.user\.id && key !== handle\) loginFlows\.delete\(key\)/); }); -test('the tab is opened while the click is still trusted', () => { - // Opening after an await is what a popup blocker stops. +test('the sign-in tab is opened without a handle, which COOP would sever', () => { + // The original claimed a blank tab during the click and pointed it at the URL + // when the request came back — the usual way around a popup blocker, and + // broken here: this app sends Cross-Origin-Opener-Policy: same-origin, so the + // handle dies as soon as the tab goes cross-origin and assigning its location + // did nothing. A blank tab, and a button that looked dead. const handler = ui.slice(ui.indexOf("btn-nc-login-flow")); - const open = handler.indexOf("window.open('', '_blank')"); - const fetchAt = handler.indexOf("fetch('/api/nextcloud/login-flow/start'"); - assert.ok(open > -1 && open < fetchAt, 'the tab must be claimed before the request'); - assert.match(handler, /a popup blocker eats it|still trusted/); + assert.doesNotMatch(handler, /window\.open\(''/, 'no blank tab claimed up front'); + assert.doesNotMatch(handler, /tab\.location/, 'no handle to navigate'); + assert.match(handler, /window\.open\(data\.loginUrl, '_blank', 'noopener'\)/); +}); + +test('the sign-in URL is also offered as a real link', () => { + // Whatever the browser decides about opening windows, there is a way through. + const handler = ui.slice(ui.indexOf("btn-nc-login-flow")); + assert.match(handler, /setFlowStatus\('Waiting for you to finish signing in\.', null, data\.loginUrl\)/); + assert.match(ui, /a\.rel = 'noopener noreferrer'/); + assert.match(ui, /a\.target = '_blank'/); + // Built as DOM, not spliced into innerHTML. + assert.match(ui, /function setFlowStatus\(message, tone, link\)[\s\S]{0,400}createElement\('a'\)/); +}); + +test('helmet still sends the COOP header this works around', () => { + // If this ever stops being true the workaround is harmless, but the comment + // explaining it would be wrong, and the old pattern would look safe again. + const server = read('server.js'); + assert.match(server, /app\.use\(helmet\(\{/); + assert.doesNotMatch(server, /crossOriginOpenerPolicy:\s*false/, + 'COOP is on by default in helmet; turning it off would need its own reasoning'); }); test('polling stops: on success, on failure, and on a deadline', () => {