commit e45ddafc1349169b09ebb28a4d049961f06d4773
parent f55d8d39bc2f992264e889655daec9833eae68d8
Author: finwo <finwo@pm.me>
Date: Sat, 26 Sep 2026 18:10:46 +0200
Say which origins are registered when a redirect uri is refused
Diffstat:
7 files changed, 129 insertions(+), 16 deletions(-)
diff --git a/conductor.example.yaml b/conductor.example.yaml
@@ -52,8 +52,9 @@ auth:
# setting it true uses the built-in one even with a provider configured.
local:
enabled: null
- # Where the app is allowed to be sent back to, when it is not served
- # from server.public_url. Comma separated.
+ # Where the app may be sent back to, when it is not served from
+ # server.public_url. Comma separated absolute URLs, compared by origin.
+ # "*" allows any, which is for local testing only.
redirect_uris: null
# Setting a discovery url switches authentication to the provider. Built-in
# accounts are used when it is unset. Everything else about the provider,
diff --git a/docs/auth.md b/docs/auth.md
@@ -126,11 +126,20 @@ id derived from the code's own id, so redeeming it twice collides on the
primary key and is refused. Codes are single use without a table of spent
codes.
-`auth.local.redirect_uris` is where the browser may be sent back to. It
-defaults to `server.public_url` and is compared by origin, so an
-application served from somewhere else needs its origin listed. A redirect
-uri that is not listed is refused outright rather than redirected to, or
-this would be an open redirect.
+`auth.local.redirect_uris` is where the browser may be sent back to. It is
+a comma separated list of absolute URLs, compared by origin, and defaults
+to `server.public_url`. A redirect uri that is not listed is refused
+outright rather than redirected to, or this would be an open redirect. The
+refusal names what is registered, because `localhost` and `127.0.0.1` are
+different origins and that is otherwise a long afternoon.
+
+```sh
+CONDUCTOR_LOCAL_REDIRECT_URIS=http://127.0.0.1:8080,http://localhost:8080
+```
+
+An entry without a scheme is refused at startup rather than silently
+matching nothing. `*` allows any http or https origin, which is meant for
+local testing and should not be anywhere near a deployment.
Using an external provider
--------------------------
diff --git a/src/conductor/auth/index.js b/src/conductor/auth/index.js
@@ -71,7 +71,23 @@ export default async function authRoutes(fastify, { cfg, auth, users, refreshTok
done(null, out);
});
- const allowed = (uri) => cfg.auth.local.redirect_uris.some((entry) => sameOrigin(uri, entry));
+ const registered = cfg.auth.local.redirect_uris;
+ const anywhere = registered.includes('*');
+
+ function allowed(uri) {
+ if (typeof uri !== 'string' || uri.length === 0) return false;
+ if (anywhere) return /^https?:/.test(uri);
+ return registered.some((entry) => sameOrigin(uri, entry));
+ }
+
+ function refuse(reply, what, given) {
+ return reply.code(400).type('text/plain; charset=utf-8').send(
+ `${what} ${JSON.stringify(given ?? null)} is not registered for this conductor.\n`
+ + `Registered: ${registered.join(', ')}\n`
+ + 'Set auth.local.redirect_uris, or CONDUCTOR_LOCAL_REDIRECT_URIS, to a comma separated\n'
+ + 'list of origins. Use * to allow any, which is for local testing only.\n'
+ );
+ }
function oauthError(reply, status, error, description) {
return reply.code(status).send({ error, error_description: description });
@@ -83,9 +99,8 @@ export default async function authRoutes(fastify, { cfg, auth, users, refreshTok
const { client_id: clientId, redirect_uri: redirectUri, response_type: responseType } = req.query;
const { code_challenge: challenge, code_challenge_method: method, state } = req.query;
- if (typeof redirectUri !== 'string' || !allowed(redirectUri)) {
- reply.code(400).type('text/plain; charset=utf-8')
- .send('redirect_uri is not registered for this conductor');
+ if (!allowed(redirectUri)) {
+ refuse(reply, 'redirect_uri', redirectUri);
return null;
}
@@ -233,10 +248,7 @@ export default async function authRoutes(fastify, { cfg, auth, users, refreshTok
fastify.get('/auth/logout', async (req, reply) => {
const target = req.query.post_logout_redirect_uri;
if (typeof target === 'string' && target.length > 0) {
- if (!allowed(target)) {
- return reply.code(400).type('text/plain; charset=utf-8')
- .send('post_logout_redirect_uri is not registered for this conductor');
- }
+ if (!allowed(target)) return refuse(reply, 'post_logout_redirect_uri', target);
return reply.redirect(target, 303);
}
return reply.redirect(issuer, 303);
diff --git a/src/lib/config.js b/src/lib/config.js
@@ -207,7 +207,8 @@ function deepFreeze(obj) {
function redirectUris(value, publicUrl) {
const given = Array.isArray(value) ? value : String(value ?? '').split(',');
const uris = given.map((u) => String(u).trim().replace(/\/+$/, '')).filter(Boolean);
- return uris.length > 0 ? uris : [publicUrl.replace(/\/+$/, '')];
+ if (uris.length === 0) return [publicUrl.replace(/\/+$/, '')];
+ return uris.includes('*') ? ['*'] : uris;
}
// Reads a 32 byte key given as hex or base64. Returns a Buffer, or null.
@@ -262,6 +263,22 @@ function validate(cfg) {
errors.push('auth.local.enabled is false and no auth.oidc.discovery_url is set, so nobody could sign in');
}
+ // A bare host:port parses with a nonsense scheme and would match nothing.
+ for (const uri of redirectUris(cfg.auth.local.redirect_uris, cfg.server.public_url)) {
+ if (uri === '*') continue;
+ let parsed = null;
+ try {
+ parsed = new URL(uri);
+ } catch { /* reported below */ }
+
+ if (!parsed || !parsed.protocol.startsWith('http')) {
+ errors.push(
+ `auth.local.redirect_uris entry ${JSON.stringify(uri)} must be an absolute http or https URL, `
+ + 'such as https://ci.example.com, or * to allow any origin'
+ );
+ }
+ }
+
try {
new URL(cfg.server.public_url);
} catch {
diff --git a/test/config.test.js b/test/config.test.js
@@ -122,6 +122,20 @@ test('redirect uris are split on commas and lose trailing slashes', async () =>
});
});
+test('a star allows any origin, and absorbs anything listed beside it', async () => {
+ await withEnv({ CONDUCTOR_LOCAL_REDIRECT_URIS: 'https://app.example.com,*' }, () => {
+ assert.deepEqual(loadConfig().auth.local.redirect_uris, ['*']);
+ });
+});
+
+// A bare host:port parses as a URL with a nonsense scheme, so it would
+// silently match nothing rather than failing.
+test('a redirect uri without a scheme is rejected at startup', async () => {
+ await withEnv({ CONDUCTOR_LOCAL_REDIRECT_URIS: '127.0.0.1:18080,localhost:18080' }, () => {
+ assert.throws(() => loadConfig(), /must be an absolute http or https URL/);
+ });
+});
+
test('token lifetimes are read from the environment as integers', async () => {
await withEnv({
CONDUCTOR_ACCESS_TTL: '300',
diff --git a/test/helpers/harness.js b/test/helpers/harness.js
@@ -108,6 +108,9 @@ export async function startHarness(options = {}) {
: []),
'auth:',
' session_secret: test-session-secret-not-for-real-use',
+ ...(options.redirectUris
+ ? [' local:', ` redirect_uris: ${JSON.stringify(options.redirectUris)}`]
+ : []),
...(options.oidcDiscoveryUrl
? [
' oidc:',
diff --git a/test/provider.test.js b/test/provider.test.js
@@ -148,6 +148,63 @@ test('an unregistered redirect uri is refused without redirecting', async () =>
});
});
+// localhost and 127.0.0.1 are different origins, which is the mistake this
+// message has to be able to explain.
+test('a refused redirect uri names what is registered', async () => {
+ await withConductor(async (c) => {
+ const query = authorizeQuery({ redirect_uri: 'http://localhost:8080/' });
+ const res = await c.app.inject({ method: 'GET', url: `/auth/authorize?${query}` });
+
+ assert.equal(res.statusCode, 400);
+ assert.match(res.body, /localhost:8080/);
+ assert.match(res.body, /Registered: http:\/\/conductor\.test/);
+ assert.match(res.body, /CONDUCTOR_LOCAL_REDIRECT_URIS/);
+ });
+});
+
+test('several origins may be registered at once', async () => {
+ const c = await startHarness({
+ bootstrap: true,
+ publicUrl: PUBLIC_URL,
+ redirectUris: 'http://127.0.0.1:18080,http://localhost:18080',
+ });
+ try {
+ for (const origin of ['http://127.0.0.1:18080', 'http://localhost:18080']) {
+ const query = authorizeQuery({ redirect_uri: `${origin}/` });
+ const res = await c.app.inject({ method: 'GET', url: `/auth/authorize?${query}` });
+ assert.equal(res.statusCode, 200, `${origin} should be registered`);
+ }
+
+ // Anything outside the list is still refused.
+ for (const origin of ['http://127.0.0.1:19999', 'https://evil.example']) {
+ const query = authorizeQuery({ redirect_uri: `${origin}/` });
+ const res = await c.app.inject({ method: 'GET', url: `/auth/authorize?${query}` });
+ assert.equal(res.statusCode, 400, `${origin} should be refused`);
+ }
+ } finally {
+ await c.stop();
+ }
+});
+
+test('a star registration allows any origin but still refuses nonsense', async () => {
+ const c = await startHarness({ bootstrap: true, publicUrl: PUBLIC_URL, redirectUris: '*' });
+ try {
+ for (const uri of ['http://localhost:3000/', 'https://anything.example/callback']) {
+ const query = authorizeQuery({ redirect_uri: uri });
+ const res = await c.app.inject({ method: 'GET', url: `/auth/authorize?${query}` });
+ assert.equal(res.statusCode, 200, `${uri} should be allowed`);
+ }
+
+ for (const uri of ['javascript:alert(1)', 'data:text/html,x', '']) {
+ const query = authorizeQuery({ redirect_uri: uri });
+ const res = await c.app.inject({ method: 'GET', url: `/auth/authorize?${query}` });
+ assert.equal(res.statusCode, 400, `${JSON.stringify(uri)} must still be refused`);
+ }
+ } finally {
+ await c.stop();
+ }
+});
+
test('a bad request with a good redirect uri comes back as an oauth error', async () => {
await withConductor(async (c) => {
for (const [override, error] of [
