From 04123adaa321e923be111e143b2ad9f34ad62cbe Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 09:30:34 +0000 Subject: [PATCH] fix(sso): stop sending OIDC logins to localhost; return users to the UI Two things sent an SSO login to the wrong place. The redirect/callback URL registered with the identity provider came from ANGLES_BASE_URL alone, defaulting to http://localhost:3000. Deployments set ANGLES_API_BASE_URL (for the UI and Swagger) and reasonably expected that to be used, and docker-compose passed ANGLES_BASE_URL=http://localhost:3000 explicitly whatever was configured. The base URL now falls back to ANGLES_API_BASE_URL (taking the Swagger scheme when it has none) before localhost, and docker-compose leaves ANGLES_BASE_URL empty unless set. After a login the API redirected to `/` (or `/login?error=true`) on its own origin, but the UI is usually on another port or host. The UI now passes its origin as `returnTo` when it starts a login; the API keeps it in the session for that login only and redirects back to it. Only an http(s) origin on the API's own host is accepted (the same rule CORS applies, now shared in app/utils/ui-origin.js), so it cannot be used as an open redirect. Without one the redirect stays relative, as before. A SAML assertion is usually posted back cross-site without the session cookie, so SAML keeps the old behaviour. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KgQXSUjuLVXmLWxobnMfSf --- app/routes/auth.routes.js | 24 ++++++- app/utils/ui-origin.js | 37 ++++++++++ config/auth.config.js | 23 ++++++- docs/authentication.md | 10 ++- server.js | 7 +- setup/docker-compose.yml | 15 +++-- setup/kubernetes/02-angles-config.yaml | 11 ++- test/settings.tests.js | 81 +++++++++++++++++++++- test/sso-urls.tests.js | 93 ++++++++++++++++++++++++++ 9 files changed, 276 insertions(+), 25 deletions(-) create mode 100644 app/utils/ui-origin.js create mode 100644 test/sso-urls.tests.js diff --git a/app/routes/auth.routes.js b/app/routes/auth.routes.js index 148271d..21bc544 100644 --- a/app/routes/auth.routes.js +++ b/app/routes/auth.routes.js @@ -4,6 +4,7 @@ const debug = require('debug'); const authConfig = require('../../config/auth.config.js'); const featureConfig = require('../../config/feature.config.js'); const loginThrottle = require('../utils/login-throttle.js'); +const { trustedUiOrigin } = require('../utils/ui-origin.js'); const { isProviderReady, getReadyProvider, @@ -141,6 +142,16 @@ module.exports = (app, path) => { error: 'This provider uses a credential login; post to /login instead.', }); } + // Where to send the browser once the identity provider is done: the UI passes its + // own origin, because it is usually on a different port or host from the API. Only + // an origin on the API's own host is kept (see ui-origin.js), and only for this + // login; without one the redirect stays relative to the API, as before. + const returnTo = trustedUiOrigin(req.query.returnTo, req); + if (returnTo) { + req.session.ssoReturnTo = returnTo; + } else if (req.session) { + delete req.session.ssoReturnTo; + } return passport.authenticate(strategyName(req.params.providerId))(req, res, next); }, ); @@ -154,21 +165,28 @@ module.exports = (app, path) => { // error handler, which would surface a bare 500 with a stack trace to someone who just // failed to log in. Both are the same thing from the user's perspective, so both land // on the login page, and the reason is logged rather than returned. + // + // The UI origin recorded when the login started is read up front: logging in replaces + // the session (to prevent session fixation), which discards it. A SAML assertion is + // usually posted back cross-site, which a SameSite=Lax session cookie does not + // accompany, so a SAML login normally falls back to the API's own origin. const ssoCallback = (req, res, next) => { const { providerId } = req.params; + const uiOrigin = (req.session && req.session.ssoReturnTo) || ''; + if (req.session) delete req.session.ssoReturnTo; return passport.authenticate(strategyName(providerId), (err, user, info) => { if (err) { log('SSO callback for provider %s failed: %s', providerId, err.message); - return res.redirect('/login?error=true'); + return res.redirect(`${uiOrigin}/login?error=true`); } if (!user) { log('SSO callback for provider %s denied: %s', providerId, (info && info.message) || 'no user'); - return res.redirect('/login?error=true'); + return res.redirect(`${uiOrigin}/login?error=true`); } return req.logIn(user, (loginErr) => { if (loginErr) return next(loginErr); // Successful authentication, redirect home. - return res.redirect('/'); + return res.redirect(`${uiOrigin}/`); }); })(req, res, next); }; diff --git a/app/utils/ui-origin.js b/app/utils/ui-origin.js new file mode 100644 index 0000000..9966a6d --- /dev/null +++ b/app/utils/ui-origin.js @@ -0,0 +1,37 @@ +/* + * Which browser origins count as "the Angles UI". + * + * The UI and the API are usually served from the same host on different ports (the UI on + * 3001, the API on 3000) or behind one proxy. The rule is the one CORS already applies: + * an origin belongs to the UI when its host is the API's own host, with `localhost` and + * `127.0.0.1` treated as the same host. Ports and schemes may differ. + * + * The SSO login uses the same rule to decide where to send the browser back to after + * the identity provider (`returnTo`). Accepting any origin there would make the API an + * open redirect, so anything else is ignored. + */ +const isLocal = (host) => host === 'localhost' || host === '127.0.0.1'; + +const isSameHost = (originHostname, requestHostname) => originHostname === requestHostname + || (isLocal(originHostname) && isLocal(requestHostname)); + +/** + * The origin (`scheme://host[:port]`) of `value` when it is an http(s) URL on the same + * host as the request, otherwise undefined. Only the origin is kept: the UI decides its + * own landing page. + */ +const trustedUiOrigin = (value, req) => { + if (typeof value !== 'string' || value.length === 0 || value.length > 2048) return undefined; + let url; + try { + url = new URL(value); + } catch (e) { + return undefined; + } + if (url.protocol !== 'http:' && url.protocol !== 'https:') return undefined; + if (url.username || url.password) return undefined; + if (!isSameHost(url.hostname, req.hostname)) return undefined; + return url.origin; +}; + +module.exports = { isSameHost, trustedUiOrigin }; diff --git a/config/auth.config.js b/config/auth.config.js index e2cc60f..bd1e695 100644 --- a/config/auth.config.js +++ b/config/auth.config.js @@ -28,6 +28,23 @@ const resolveSessionSecret = () => { return crypto.randomBytes(32).toString('hex'); }; +// The public origin the API is reached on, without a trailing slash: where identity +// providers send users back to. ANGLES_BASE_URL sets it explicitly. Without it, the API's +// own public URL (ANGLES_API_BASE_URL, which deployments already set for the UI and the +// Swagger docs) is used, so a deployment that sets that one does not silently register +// http://localhost:3000 with its identity provider. Like Swagger, a value without a +// scheme takes the first of SWAGGER_SCHEMES. +const resolveBaseUrl = () => { + const explicit = (process.env.ANGLES_BASE_URL || '').trim(); + const apiUrl = (process.env.ANGLES_API_BASE_URL || '').trim(); + let baseUrl = explicit || apiUrl || 'http://localhost:3000'; + if (!/^https?:\/\//i.test(baseUrl)) { + const scheme = (process.env.SWAGGER_SCHEMES || 'http').split(',')[0].trim() || 'http'; + baseUrl = `${scheme}://${baseUrl}`; + } + return baseUrl.replace(/\/+$/, ''); +}; + // These are only the in-memory defaults used before the persisted settings load at // startup. All authentication configuration (which providers exist, whether they are // enabled, issuers, client ids, secrets, group mappings) is managed exclusively through @@ -38,12 +55,14 @@ const resolveSessionSecret = () => { // The two exceptions below are genuine deployment/infrastructure values, not user config: // - sessionSecret: the session signing key (see resolveSessionSecret above). // - baseUrl: the public origin, used to derive each provider's callback URL. It depends -// on where the app is hosted and must match what is registered with the IdP. +// on where the app is hosted and must match what is registered with the IdP (see +// resolveBaseUrl above). module.exports = { + resolveBaseUrl, localAuthEnabled: true, sessionSecret: resolveSessionSecret(), // Public origin of this Angles instance, without a trailing slash. - baseUrl: (process.env.ANGLES_BASE_URL || 'http://localhost:3000').replace(/\/+$/, ''), + baseUrl: resolveBaseUrl(), // Populated from the database at startup by auth-settings-service. providers: [], }; diff --git a/docs/authentication.md b/docs/authentication.md index 3db9da3..d23b47f 100644 --- a/docs/authentication.md +++ b/docs/authentication.md @@ -9,12 +9,20 @@ changing a provider needs no restart and no redeploy. | Variable | Required | Purpose | | --- | --- | --- | -| `ANGLES_BASE_URL` | For SSO | Public origin of this instance, e.g. `https://angles.example.com`. Every provider's callback URL is derived from it and must match what is registered with the identity provider. Defaults to `http://localhost:3000`. | +| `ANGLES_BASE_URL` | For SSO | Public URL of the API, e.g. `https://angles.example.com`. Every provider's callback URL is derived from it and must match what is registered with the identity provider. Defaults to `ANGLES_API_BASE_URL` (the API URL the UI and the Swagger docs already use), and to `http://localhost:3000` when neither is set. | | `SESSION_SECRET` | In production | Signs session cookies. Startup fails without it when `NODE_ENV=production`. Anyone who knows it can forge a session for any account, including an admin, so generate a random value per deployment. | | `ANGLES_ADMIN_PASSWORD` | First run | Seeds the initial local admin account. | | `SECURE_COOKIES` | Over HTTPS | Set to `true` to mark the session cookie `Secure`. | | `TRUST_PROXY` | Behind a proxy | Set to `true` so `SECURE_COOKIES` works behind a TLS-terminating reverse proxy. | +After the identity provider, users return to the UI they started from: the UI sends +its own origin with the login, and the API redirects back to it. The API only accepts an +origin on its own host (the same rule it applies to cross-origin requests, with +`localhost` and `127.0.0.1` treated as one host), so it cannot be used to redirect +elsewhere. A SAML assertion is usually posted back cross-site without the session +cookie, so a SAML login lands on the API's own origin instead; serve the UI and the API +from one origin (through a reverse proxy) if you use SAML. + Provider secrets - OIDC client secrets, SAML private keys, LDAP bind credentials - are stored in the database and are **write-only** through the API. They are never returned; the settings response reports only a `…Set` boolean. Saving a form with a blank secret diff --git a/server.js b/server.js index 7121717..c17accf 100644 --- a/server.js +++ b/server.js @@ -19,6 +19,7 @@ const authSettingsService = require('./app/utils/auth-settings-service.js'); const featureSettingsService = require('./app/utils/feature-settings-service.js'); const adminSeedService = require('./app/utils/admin-seed-service.js'); const { securityHeaders } = require('./app/utils/security-headers.js'); +const { isSameHost } = require('./app/utils/ui-origin.js'); // mongo db config const dbConfig = require('./config/database.config.js'); @@ -41,11 +42,7 @@ const corsOptionsDelegate = (req, callback) => { if (origin) { try { const originUrl = new URL(origin); - const isLocal = (host) => host === 'localhost' || host === '127.0.0.1'; - const isSameHost = originUrl.hostname === req.hostname - || (isLocal(originUrl.hostname) && isLocal(req.hostname)); - - if (isSameHost) { + if (isSameHost(originUrl.hostname, req.hostname)) { corsOptions.origin = true; } else { corsOptions.origin = false; diff --git a/setup/docker-compose.yml b/setup/docker-compose.yml index 6196996..68c44bd 100644 --- a/setup/docker-compose.yml +++ b/setup/docker-compose.yml @@ -26,7 +26,10 @@ services: ports: - 3000:3000 environment: - - ANGLES_API_BASE_URL=http://localhost:3000 + # Public URL of the API: used by the Swagger docs and, unless ANGLES_BASE_URL is + # set, for each SSO provider's redirect/callback URL. Set it (here and on the + # frontend) to the URL users actually reach the API on. + - ANGLES_API_BASE_URL=${ANGLES_API_BASE_URL:-http://localhost:3000} - ANGLES_API_BASE_PATH=/rest/api/v1.0 - SWAGGER_SCHEMES=http - PORT=3000 @@ -40,10 +43,10 @@ services: # videos, traces and HTML snapshots. Stored on the angles_attachments volume. - ANGLES_ATTACHMENT_MAX_SIZE_MB=${ANGLES_ATTACHMENT_MAX_SIZE_MB:-100} - MONGO_URL=mongodb://angleshq:%40nglesPassword@mongodb:27017/angles - # Public origin of this instance. Each SSO provider's redirect/callback URL is - # derived from it, and must match what is registered with the identity provider, - # so set this to the URL users actually reach Angles on. - - ANGLES_BASE_URL=${ANGLES_BASE_URL:-http://localhost:3000} + # Overrides the public URL each SSO provider's redirect/callback URL is derived + # from, which must match what is registered with the identity provider. Leave it + # empty to use ANGLES_API_BASE_URL. + - ANGLES_BASE_URL=${ANGLES_BASE_URL:-} # Signs session cookies. Required when NODE_ENV=production; anyone who knows it can # forge a session for any account, so generate a random value per deployment. - SESSION_SECRET=${SESSION_SECRET:-} @@ -64,7 +67,7 @@ services: ports: - 3001:3001 environment: - - ANGLES_API_BASE_URL=http://localhost:3000 + - ANGLES_API_BASE_URL=${ANGLES_API_BASE_URL:-http://localhost:3000} - ANGLES_API_BASE_PATH=/rest/api/v1.0 - PORT=3001 depends_on: diff --git a/setup/kubernetes/02-angles-config.yaml b/setup/kubernetes/02-angles-config.yaml index b673be1..31df8f3 100644 --- a/setup/kubernetes/02-angles-config.yaml +++ b/setup/kubernetes/02-angles-config.yaml @@ -22,13 +22,12 @@ data: # Initial admin account is seeded on first startup from this password. Change it before # deploying (ideally move it to a Secret referenced via envFrom). ANGLES_ADMIN_PASSWORD: "ChangeMe!1" - # Public origin users actually reach Angles on, without a trailing slash. + # Public URL users actually reach the API on, without a trailing slash. # - # Only needed for SSO, but required as soon as any provider is configured: each - # provider's redirect/callback URL is derived from it and must match what is registered - # with the identity provider. The port below is the backend's hostPort; behind an - # Ingress or LoadBalancer set this to that public URL instead - # (e.g. "https://angles.example.com"). + # Each SSO provider's redirect/callback URL is derived from it and must match what is + # registered with the identity provider. Without it, ANGLES_API_BASE_URL (set on the + # pod) is used. The port below is the backend's hostPort; behind an Ingress or + # LoadBalancer set this to that public URL instead (e.g. "https://angles.example.com"). ANGLES_BASE_URL: "http://127.0.0.1:3002" # Signs session cookies. Startup fails without it when NODE_ENV=production. # diff --git a/test/settings.tests.js b/test/settings.tests.js index 1e33c8a..afb35d9 100644 --- a/test/settings.tests.js +++ b/test/settings.tests.js @@ -367,13 +367,17 @@ describe('Auth Settings API Tests', () => { // redirect to the IdP, and follow its redirect back to the callback. The ID token is // genuinely signed by the test IdP, so signature, issuer, audience and nonce // validation all run for real. - const completeLogin = async (providerId, claims) => { + // + // `returnTo` is the UI origin the login is started with, as the UI passes it. + const completeLogin = async (providerId, claims, returnTo) => { idp.nextClaims = claims; // A fresh agent per login, so the PKCE verifier and state stored in the session are // the ones this flow created. const agent = request.agent(app); - const start = await agent.get(`${baseUrl}auth/sso/${providerId}`).expect(302); + const start = await agent.get(`${baseUrl}auth/sso/${providerId}`) + .query(returnTo === undefined ? {} : { returnTo }) + .expect(302); // Follow the redirect to the IdP itself - a different server, so it needs its own // request rather than the app agent. @@ -593,6 +597,79 @@ describe('Auth Settings API Tests', () => { }); }); + // The UI and the API are usually on different ports (or hosts behind one proxy), so + // after the identity provider the API sends the browser back to the UI's origin. The + // supertest requests reach the API as 127.0.0.1, so localhost counts as the same host. + describe('OIDC login flow: returning to the UI', () => { + beforeEach((done) => { + adminAgent + .put(`${baseUrl}settings/auth`) + // defaultRole is explicit: a PUT that omits it keeps the stored value, and an + // earlier test sets one, which would let the "denied" login in. + .send({ providers: [oidcProvider({ defaultRole: '' })] }) + .expect(200) + .end(done); + }); + + it('returns to the UI origin the login was started from', async () => { + const { agent, res } = await completeLogin('okta', { + sub: 'return-1', email: 'return.ui@example.com', groups: ['angles-users'], + }, 'http://localhost:3001'); + res.status.should.equal(302); + res.headers.location.should.equal('http://localhost:3001/'); + await agent.get(`${baseUrl}auth/me`).expect(200); + }); + + it('returns a denied login to the UI login page', async () => { + const { res } = await completeLogin('okta', { + sub: 'return-2', email: 'return.denied@example.com', groups: ['some-other-group'], + }, 'http://localhost:3001'); + res.headers.location.should.equal('http://localhost:3001/login?error=true'); + }); + + it('keeps only the origin of the URL it is given', async () => { + const { res } = await completeLogin('okta', { + sub: 'return-3', email: 'return.path@example.com', groups: ['angles-users'], + }, 'http://127.0.0.1:3001/some/page?x=1'); + res.headers.location.should.equal('http://127.0.0.1:3001/'); + }); + + it('ignores an origin on another host, so it cannot be used as an open redirect', async () => { + const { agent, res } = await completeLogin('okta', { + sub: 'return-4', email: 'return.foreign@example.com', groups: ['angles-users'], + }, 'https://evil.example.org'); + res.headers.location.should.equal('/'); + await agent.get(`${baseUrl}auth/me`).expect(200); + }); + + it('ignores values that are not http(s) origins', async () => { + // eslint-disable-next-line no-script-url + const values = ['javascript:alert(1)', '//evil.example.org', 'not a url', 'http://user:pw@localhost:3001']; + // eslint-disable-next-line no-restricted-syntax + for (const [index, value] of values.entries()) { + // eslint-disable-next-line no-await-in-loop + const { res } = await completeLogin('okta', { + sub: `return-5-${index}`, email: `return.bad${index}@example.com`, groups: ['angles-users'], + }, value); + res.headers.location.should.equal('/'); + } + }); + + it('forgets the UI origin once the login is over', async () => { + const agent = request.agent(app); + await agent.get(`${baseUrl}auth/sso/okta`).query({ returnTo: 'http://localhost:3001' }).expect(302); + // A second login from the same browser without one goes back to the API origin. + idp.nextClaims = { sub: 'return-6', email: 'return.forget@example.com', groups: ['angles-users'] }; + const start = await agent.get(`${baseUrl}auth/sso/okta`).expect(302); + const authorize = await request(idp.issuer) + .get(start.headers.location.replace(idp.issuer, '')) + .redirects(0) + .expect(302); + const res = await agent.get(authorize.headers.location.replace(/^https?:\/\/[^/]+/, '')).redirects(0); + res.headers.location.should.equal('/'); + }); + }); + describe('GET /auth/sso/:providerId (runtime enablement)', () => { it('returns 404 for an unknown provider', (done) => { request(app).get(`${baseUrl}auth/sso/does-not-exist`).expect(404, done); diff --git a/test/sso-urls.tests.js b/test/sso-urls.tests.js new file mode 100644 index 0000000..b6a6b7a --- /dev/null +++ b/test/sso-urls.tests.js @@ -0,0 +1,93 @@ +/** + * The URLs an SSO login depends on: the public URL identity providers send users back to + * (each provider's callback is derived from it), and which UI origins a login may return + * to afterwards. + */ +const should = require('should'); +const { resolveBaseUrl } = require('../config/auth.config.js'); +const { trustedUiOrigin, isSameHost } = require('../app/utils/ui-origin.js'); + +const ENV_KEYS = ['ANGLES_BASE_URL', 'ANGLES_API_BASE_URL', 'SWAGGER_SCHEMES']; + +describe('SSO URL Tests', () => { + describe('resolveBaseUrl', () => { + let saved; + + beforeEach(() => { + saved = Object.fromEntries(ENV_KEYS.map((key) => [key, process.env[key]])); + ENV_KEYS.forEach((key) => { delete process.env[key]; }); + }); + + afterEach(() => { + ENV_KEYS.forEach((key) => { + if (saved[key] === undefined) delete process.env[key]; + else process.env[key] = saved[key]; + }); + }); + + it('uses ANGLES_BASE_URL when it is set', () => { + process.env.ANGLES_BASE_URL = 'https://angles.example.com/'; + process.env.ANGLES_API_BASE_URL = 'https://api.example.com'; + resolveBaseUrl().should.equal('https://angles.example.com'); + }); + + it('falls back to the API URL rather than localhost', () => { + process.env.ANGLES_API_BASE_URL = 'https://angles-api.example.com'; + resolveBaseUrl().should.equal('https://angles-api.example.com'); + }); + + it('treats an empty ANGLES_BASE_URL as unset, as docker-compose passes it', () => { + process.env.ANGLES_BASE_URL = ''; + process.env.ANGLES_API_BASE_URL = 'https://angles-api.example.com'; + resolveBaseUrl().should.equal('https://angles-api.example.com'); + }); + + it('adds the Swagger scheme to an API URL without one', () => { + process.env.ANGLES_API_BASE_URL = 'angles.internal:3000'; + process.env.SWAGGER_SCHEMES = 'https, http'; + resolveBaseUrl().should.equal('https://angles.internal:3000'); + process.env.SWAGGER_SCHEMES = ''; + resolveBaseUrl().should.equal('http://angles.internal:3000'); + }); + + it('defaults to http://localhost:3000 when nothing is configured', () => { + resolveBaseUrl().should.equal('http://localhost:3000'); + }); + }); + + describe('trustedUiOrigin', () => { + const req = (hostname) => ({ hostname }); + + it('accepts an origin on the API host on any port and scheme', () => { + trustedUiOrigin('https://angles.example.com:8443', req('angles.example.com')) + .should.equal('https://angles.example.com:8443'); + trustedUiOrigin('http://angles.example.com', req('angles.example.com')) + .should.equal('http://angles.example.com'); + }); + + it('treats localhost and 127.0.0.1 as the same host', () => { + trustedUiOrigin('http://localhost:3001', req('127.0.0.1')).should.equal('http://localhost:3001'); + isSameHost('127.0.0.1', 'localhost').should.equal(true); + }); + + it('keeps only the origin', () => { + trustedUiOrigin('http://localhost:3001/a/b?c=d#e', req('localhost')).should.equal('http://localhost:3001'); + }); + + it('rejects other hosts, look-alikes and non-http schemes', () => { + should.not.exist(trustedUiOrigin('https://evil.example.org', req('angles.example.com'))); + should.not.exist(trustedUiOrigin('https://angles.example.com.evil.org', req('angles.example.com'))); + // eslint-disable-next-line no-script-url + should.not.exist(trustedUiOrigin('javascript:alert(1)', req('localhost'))); + should.not.exist(trustedUiOrigin('//angles.example.com', req('angles.example.com'))); + should.not.exist(trustedUiOrigin('http://user:pw@localhost:3001', req('localhost'))); + }); + + it('rejects anything that is not a single string', () => { + should.not.exist(trustedUiOrigin(undefined, req('localhost'))); + should.not.exist(trustedUiOrigin('', req('localhost'))); + should.not.exist(trustedUiOrigin(['http://localhost:3001'], req('localhost'))); + should.not.exist(trustedUiOrigin(`http://localhost/${'a'.repeat(3000)}`, req('localhost'))); + }); + }); +});