diff --git a/frontend/js/api.js b/frontend/js/api.js index 574398b..0413f17 100644 --- a/frontend/js/api.js +++ b/frontend/js/api.js @@ -213,6 +213,9 @@ export const api = { // Unlink an instance-wide SSO provider. The new password is required in the same call: // the account must never sit between credentials. ssoUnlink: (password) => request('/auth/oidc/unlink', { method: 'POST', body: JSON.stringify({ password }) }), + // Returns { url } to navigate to. Fetched rather than navigated to, because the session is + // a bearer token and a top-level navigation cannot carry one. + ssoLinkStart: (slug) => request(`/auth/oidc/${encodeURIComponent(slug)}/link/start`), totpSetup: () => request('/auth/totp/setup', { method: 'POST' }), totpEnable: (code) => request('/auth/totp/enable', { method: 'POST', body: JSON.stringify({ code }) }), totpDisable: (code) => request('/auth/totp/disable', { method: 'POST', body: JSON.stringify({ code }) }), diff --git a/frontend/js/views/settings.js b/frontend/js/views/settings.js index 5819cb0..156dc07 100644 --- a/frontend/js/views/settings.js +++ b/frontend/js/views/settings.js @@ -607,12 +607,20 @@ export async function render(container) { `); block.querySelectorAll('[data-link-slug]').forEach((btn) => { - btn.onclick = () => { + btn.onclick = async () => { const slug = btn.dataset.linkSlug; const name = providers.find((p) => p.slug === slug)?.name || slug; // Deliberately blunt: the password is destroyed, and that is the part people miss. if (!window.confirm(t('settings.signin_link_warning', { provider: name }))) return; - window.location.href = `/api/auth/oidc/${encodeURIComponent(slug)}/link/start`; + /* + * Fetch the authorize URL, then navigate to it. NOT location.href straight at the start + * route: the session is a bearer token in localStorage, so a top-level navigation arrives + * with no Authorization header and is refused as anonymous. + */ + try { + const { url } = await api.ssoLinkStart(slug); + window.location.href = url; + } catch (e) { showToast(e.message, 'error'); } }; }); } diff --git a/server/routes/auth.js b/server/routes/auth.js index 1155179..080b77c 100644 --- a/server/routes/auth.js +++ b/server/routes/auth.js @@ -1248,7 +1248,7 @@ router.post('/sso/start', express.urlencoded({ extended: false }), (req, res) => * account a link is for. Login and link therefore share one flow — the same PKCE, state, nonce and * verification — instead of a second copy that drifts. */ -async function beginOidc(req, res, provider, extra = {}, onError = backToApp) { +async function beginOidc(req, res, provider, extra = {}, onError = backToApp, asJson = false) { try { const doc = await oidc.discover(provider.issuer); const pkce = oidc.createPkce(); @@ -1279,9 +1279,20 @@ async function beginOidc(req, res, provider, extra = {}, onError = backToApp) { url.searchParams.set('nonce', nonce); url.searchParams.set('code_challenge', pkce.challenge); url.searchParams.set('code_challenge_method', pkce.method); + /* + * A LINK start is fetched, not navigated to. + * + * The session lives in localStorage and travels as an Authorization header, so a top-level + * `location.href` to an authenticated route arrives anonymous — which is exactly how this first + * shipped, and it 401'd every time. The caller therefore fetches this with its token and gets + * the authorize URL back to navigate to itself. The transaction cookie is still set by this + * response, because a same-origin fetch stores Set-Cookie normally. + */ + if (asJson) return res.json({ url: url.toString() }); res.redirect(url.toString()); } catch (err) { console.error(`[oidc] ${provider.slug} start failed:`, err.message); + if (asJson) return res.status(502).json({ error: 'The provider could not be reached' }); onError(res, { sso_error: 'provider_unavailable' }); } } @@ -1333,9 +1344,12 @@ router.post('/oidc/unlink', requireAuth, (req, res) => { router.get('/oidc/:slug/link/start', requireAuth, asyncRoute(async (req, res) => { const provider = oidcProviders.get(req.params.slug); - if (!provider) return backToSettings(res, { sso_error: 'unknown_provider' }); - if (provider.organizationId) return backToSettings(res, { sso_error: 'not_linkable' }); - await beginOidc(req, res, provider, { link: req.user.id }, backToSettings); + if (!provider) return res.status(404).json({ error: 'Unknown provider' }); + if (provider.organizationId) { + return res.status(400).json({ error: 'Only this server\'s own providers can be linked' }); + } + // JSON, not a redirect — see beginOidc. The browser cannot send a bearer token on a navigation. + await beginOidc(req, res, provider, { link: req.user.id }, backToSettings, true); })); router.get('/oidc/:slug/callback', asyncRoute(async (req, res) => { diff --git a/server/test/oidc-account-linking.test.js b/server/test/oidc-account-linking.test.js index a128e64..8b3d6fd 100644 --- a/server/test/oidc-account-linking.test.js +++ b/server/test/oidc-account-linking.test.js @@ -40,7 +40,7 @@ test('link start requires authentication and refuses org providers', () => { const body = handler('get', '/oidc/:slug/link/start'); assert.match(AUTH, /router\.get\('\/oidc\/:slug\/link\/start', requireAuth/, 'the link must be startable only by someone already signed in — that is the proof of ownership'); - assert.match(body, /provider\.organizationId.*not_linkable/s, + assert.match(body, /provider\.organizationId[\s\S]{0,160}status\(400\)/, "an organization's provider must never attach itself to a platform account"); assert.match(body, /link: req\.user\.id/, 'the account must come from the session, not from anything the browser can set'); @@ -89,6 +89,27 @@ test('link failures return to Settings, not the login page', () => { assert.match(AUTH, /function backToSettings\(res, params\)[\s\S]{0,200}#\/settings/); }); +test('link start answers with JSON, because a navigation cannot carry a bearer token', () => { + /* + * Shipped broken once: the Settings button did `location.href = .../link/start`, which is a + * top-level navigation. The session lives in localStorage and travels as an Authorization header, + * so the request arrived anonymous and requireAuth refused it — "Authentication required" on + * every click. The client must FETCH this with its token and navigate to the returned URL. + */ + const body = handler('get', '/oidc/:slug/link/start'); + assert.match(body, /beginOidc\([^)]*backToSettings, true\)/, + 'link start must run in JSON mode'); + assert.match(AUTH, /if \(asJson\) return res\.json\(\{ url: url\.toString\(\) \}\);/, + 'JSON mode must return the authorize URL rather than a 302'); + + const settings = require('fs').readFileSync( + require('path').join(__dirname, '..', '..', 'frontend', 'js', 'views', 'settings.js'), 'utf8'); + assert.match(settings, /await api\.ssoLinkStart\(slug\)/, + 'the client must fetch the start route so its Authorization header is sent'); + assert.doesNotMatch(settings, /location\.href = `\/api\/auth\/oidc/, + 'never navigate straight at the authenticated start route'); +}); + test('login and link share one flow, so verification cannot drift between them', () => { // beginOidc is the single place PKCE/state/nonce are minted; both entry points call it. assert.match(AUTH, /async function beginOidc\(req, res, provider, extra = \{\}/);