libredb / libredb/libredb-studio
Every OIDC sign-in failure renders the same message, and the login route classifies every cause as oidc_config
- Dominant language
- TypeScript
- Stars
- 726
- Forks
- 119
- Avg merge
- 7h 47m
- Merged PRs (30d)
- 265
Description
Every way an SSO sign-in can fail renders the same eleven words, and the login route labels every cause `oidc_config` whether or not the configuration is at fault.
An operator bringing up OIDC for the first time gets no signal at all about what is wrong, and the code that would have told them is computed and then discarded.
## The two halves
**The login route classifies nothing.** `src/app/api/auth/oidc/login/route.ts:32-36` catches everything and redirects to `?error=oidc_config`:
```ts
} catch (error) {
logger.error("OIDC login error", error, { route: "GET /api/auth/oidc/login" });
const origin = getPublicOrigin(request);
return NextResponse.redirect(`${origin}${withBasePath("/login")}?error=oidc_config`);
}
```
A missing `OIDC_CLIENT_SECRET`, an issuer that does not resolve, a TLS failure, an issuer that is up but returns a malformed discovery document and a refusal to speak plain http all arrive at the same line and all come out as `oidc_config`.
The callback route already does this properly one file over, distinguishing by error type rather than by message (`src/app/api/auth/oidc/callback/route.ts:120`), and its own comment records that substring matching on `error.message` was the bug that made it misclassify itself:
```ts
const errorCode = error instanceof AuthConfigError ? "oidc_config" : "oidc_failed";
```
**The login page discards whatever it is told.** `src/app/login/login-form.tsx:39` reads the code, and `:250-254` renders one fixed sentence for every value of it:
```tsx
{oidcError && (
Authentication failed. Please try again.
)}
```
`oidc_config`, `oidc_failed`, `oidc_state_missing` and everything else are indistinguishable on screen, and "Please try again" is actively wrong advice for the configuration cases, where trying again will fail identically forever.
## The worked example that found this
`openid-client` v6 refuses a plain-http issuer, on localhost included. Bringing up a local Keycloak the ordinary way (`start-dev`, `http://localhost:8080/realms/`) and pointing `OIDC_ISSUER` at it gives, on stdout:
```
[ERROR] {route=GET /api/auth/oidc/login} OIDC login error | ClientError: only requests to HTTPS are allowed
```
and, on screen, "Authentication failed. Please try again."
The configuration is complete and correct; the scheme is the problem. `discoverProvider()` (`src/lib/oidc.ts:66-82`) passes no execute options, and `allowInsecureRequests` is the documented opt-out: https://github.com/panva/openid-client/blob/main/docs/functions/allowInsecureRequests.md
The library marks it deprecated deliberately, to make it stand out as something you should not need, "possibly only for local development and testing against non-TLS secured environments". That framing is the right one for us too.
## What to change
Three changes, and they are independent enough to land separately if that is easier.
1. **Classify in the login route the way the callback route already does.** Use the error type, not the message. `AuthConfigError` stays `oidc_config`. Anything else is a discovery or transport failure and should get its own code, `oidc_discovery` or similar, so the two are distinguishable in the audit trail as well as on screen. `src/lib/audit.ts:82-83` is where the event type union lives; extend it there.
2. **Render one message per class on the login page.** Keep them short and keep them free of anything the issuer told us: the login page is unauthenticated and must not become a reconnaissance surface for the IdP. Naming the class is not leaking. Something along the lines of: a configuration problem says the server's SSO configuration is incomplete and to contact the administrator, with no retry advice; a discovery or transport problem says the identity provider could not be reached; the state and exchange failures keep today's "try again", which is correct advice for those. Do not interpolate `error.message` into the page under any circumstance.
3. **Decide the plain-http question and document the answer.** Either leave the refusal in place and say so in `docs/OIDC.md`, telling anyone testing against a local IdP to give it TLS, or add an explicit, loudly named opt-out (an env var, off by default, refused when `NODE_ENV=production`) wired to `client.allowInsecureRequests` in `discoverProvider()`. My preference is the first: it costs nothing to run a local Keycloak with a self-signed certificate plus `NODE_EXTRA_CA_CERTS`, and an insecure-transport switch is a thing someone will eventually set in production. Whoever picks this up should argue for one and not silently ship both.
## Done when
- A discovery or transport failure and a missing-variable failure produce different `error` codes from `GET /api/auth/oidc/login`, decided by type.
- The login page renders a different message for each class, none of which contains issuer-supplied text.
- Tests cover each class, including the negative: assert the rendered text does not contain the underlying error message. A test that only asserts "some error is shown" would pass today and is worth nothing here.
- `docs/OIDC.md` states whether a plain-http issuer is supported, and if not, how to test against a local IdP.
- 100% line coverage on touched files.
## Not in scope
Changing what `discoverProvider()` caches, the 5 minute discovery TTL, or the OIDC flow itself. This issue is about how a failure is classified and reported, not about the flow that failed.
Contributor guide
Research direction
Start with the login route, callback route, login form, audit event union, and discoverProvider() in the files named in the issue; compare the login and callback error handling before running the existing OIDC tests. Add coverage for distinct error classes and safe rendered messages, then update docs/OIDC.md to state the plain-HTTP decision and verify touched-file coverage reaches 100%.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- authentication, backend, documentation, frontend, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100