From c68e3a62197b750fc8696d71b09ab9b5ec5287a3 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 12 Sep 2026 21:36:51 +0200 Subject: [PATCH] fix: "Sign in with Nextcloud" opens the sign-in page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The button did nothing. It claimed a blank tab during the click and pointed it at the login URL once the request returned — the standard way around a popup blocker, and it cannot work in this app: helmet sends Cross-Origin-Opener-Policy: same-origin, which severs the handle to that tab the moment it goes cross-origin. Assigning its location was a no-op. A blank tab opened, nothing else happened. The handle was never needed. window.open with 'noopener' asks for none, and a click's user activation outlives the fetch, so the browser does not treat it as a popup. The status line now also carries the sign-in URL as an ordinary link, so there is a way through whatever any particular browser decides about opening windows. Verified the server side against the real Nextcloud first: the flow starts, both returned URLs pass the SSRF guard and the same-host check. The fault was entirely in the browser. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU --- public/js/nextcloud.js | 44 +++++++++++++++++++++++-------- test/nextcloud-login-flow.test.js | 34 +++++++++++++++++++----- 2 files changed, 61 insertions(+), 17 deletions(-) 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', () => {