fix: OAuth client API returned client_id: undefined; unknown id 500'd
The ORM Model.toJSON() serializes only schema fields, so the mapped client_id/scopes/redirect_uris/... that OAuthClient.get() attaches to the wrapped Resource were stripped from GET /api/oauth/client[/:id] responses. client_id came back undefined; the theta-env bootstrap then POSTed /api/oauth/client/undefined/rotate and got a 500, aborting stack bring-up whenever proxy-secrets.js lacked a usable secret. - OAuthClient.get() now emits an explicit public toJSON (client_id, name, slug, scopes, redirect_uris, allowed_groups, token_lifetime, is_valid), deliberately omitting client_secret_hash so it can't leak over the API. - OAuthClient.get() null-guards Resource.get() (which returns null, not throws) and returns a clean 404 for an unknown/undefined id instead of crashing on r.kind. - Regression tests: list/get expose client_id + hide the secret hash, the list-then-rotate bootstrap path, and unknown-id -> 4xx not 500. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -4,6 +4,12 @@ All notable changes to this project are documented here. Format loosely
|
|||||||
follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); versions
|
follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); versions
|
||||||
correspond to git tags (`vX.Y.Z`) and `nodejs/package.json`'s `version`.
|
correspond to git tags (`vX.Y.Z`) and `nodejs/package.json`'s `version`.
|
||||||
|
|
||||||
|
## [1.3.2] - 2026-07-23
|
||||||
|
|
||||||
|
### Fixed
|
||||||
|
- **OAuth client management API returned `client_id: undefined` on every GET.** The ORM's `Model.toJSON()` only serializes schema fields, so the mapped `client_id`/`scopes`/`redirect_uris`/… that `OAuthClient.get()` attaches to the wrapped Resource were stripped from `GET /api/oauth/client` and `GET /api/oauth/client/:id` responses. The theta-env bootstrap (which lists clients and rotates by the returned `client_id`) then called `/api/oauth/client/undefined/rotate` and got a 500, aborting stack bring-up when `proxy-secrets.js` had no usable secret. `OAuthClient.get()` now emits an explicit public JSON shape (and deliberately omits `client_secret_hash`, so the secret hash no longer leaks over the API).
|
||||||
|
- `OAuthClient.get()` no longer 500s on an unknown/`undefined` client id: `Resource.get()` returns `null` (it doesn't throw), which was dereferenced as `r.kind`. It now returns a clean 404.
|
||||||
|
|
||||||
## [1.3.1] - 2026-07-23
|
## [1.3.1] - 2026-07-23
|
||||||
|
|
||||||
### Added
|
### Added
|
||||||
|
|||||||
@@ -44,13 +44,21 @@ class OAuthClient {
|
|||||||
return r;
|
return r;
|
||||||
}
|
}
|
||||||
static async get(client_id) {
|
static async get(client_id) {
|
||||||
|
const notFound = () => {
|
||||||
|
const e = new Error('OAuthClient not found');
|
||||||
|
e.status = 404;
|
||||||
|
return e;
|
||||||
|
};
|
||||||
let r;
|
let r;
|
||||||
try {
|
try {
|
||||||
r = await Resource.get(client_id);
|
r = await Resource.get(client_id);
|
||||||
} catch (_) {
|
} catch (_) {
|
||||||
throw new Error('OAuthClient not found');
|
throw notFound();
|
||||||
}
|
}
|
||||||
if (r.kind !== 'oauth') throw new Error('OAuthClient not found');
|
// Resource.get() returns null (does not throw) for a missing id —
|
||||||
|
// guard it so a bad/undefined client_id is a clean 404, not a
|
||||||
|
// "Cannot read properties of null (reading 'kind')" 500.
|
||||||
|
if (!r || r.kind !== 'oauth') throw notFound();
|
||||||
// Map metadata to top-level properties to satisfy routes/oauth.js without rewriting it
|
// Map metadata to top-level properties to satisfy routes/oauth.js without rewriting it
|
||||||
r.client_id = r.id;
|
r.client_id = r.id;
|
||||||
r.client_secret_hash = r.metadata.client_secret_hash;
|
r.client_secret_hash = r.metadata.client_secret_hash;
|
||||||
@@ -69,6 +77,29 @@ class OAuthClient {
|
|||||||
return raw_secret;
|
return raw_secret;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
// The ORM Model.toJSON() only serializes schema fields, so the mapped
|
||||||
|
// properties above (client_id, scopes, redirect_uris, …) would be
|
||||||
|
// stripped from any res.json() — that's why GET /api/oauth/client
|
||||||
|
// returned client_id: undefined and the bootstrap's rotate blew up.
|
||||||
|
// Emit the public shape explicitly. client_secret_hash is deliberately
|
||||||
|
// omitted so it never leaks over the API.
|
||||||
|
r.toJSON = function () {
|
||||||
|
return {
|
||||||
|
client_id: r.id,
|
||||||
|
id: r.id,
|
||||||
|
kind: r.kind,
|
||||||
|
name: r.name,
|
||||||
|
slug: r.slug,
|
||||||
|
owner: r.owner,
|
||||||
|
description: r.description,
|
||||||
|
redirect_uris: r.redirect_uris,
|
||||||
|
scopes: r.scopes,
|
||||||
|
allowed_groups: r.allowed_groups,
|
||||||
|
token_lifetime: r.token_lifetime,
|
||||||
|
is_valid: r.is_valid,
|
||||||
|
};
|
||||||
|
};
|
||||||
|
|
||||||
// proxy update to handle metadata correctly
|
// proxy update to handle metadata correctly
|
||||||
const originalUpdate = r.update.bind(r);
|
const originalUpdate = r.update.bind(r);
|
||||||
r.update = async (data) => {
|
r.update = async (data) => {
|
||||||
|
|||||||
Generated
+2
-2
@@ -1,12 +1,12 @@
|
|||||||
{
|
{
|
||||||
"name": "t42-sso-manager",
|
"name": "t42-sso-manager",
|
||||||
"version": "1.3.1",
|
"version": "1.3.2",
|
||||||
"lockfileVersion": 3,
|
"lockfileVersion": 3,
|
||||||
"requires": true,
|
"requires": true,
|
||||||
"packages": {
|
"packages": {
|
||||||
"": {
|
"": {
|
||||||
"name": "t42-sso-manager",
|
"name": "t42-sso-manager",
|
||||||
"version": "1.3.1",
|
"version": "1.3.2",
|
||||||
"license": "MIT",
|
"license": "MIT",
|
||||||
"dependencies": {
|
"dependencies": {
|
||||||
"@fortawesome/fontawesome-free": "^7.3.0",
|
"@fortawesome/fontawesome-free": "^7.3.0",
|
||||||
|
|||||||
+1
-1
@@ -1,6 +1,6 @@
|
|||||||
{
|
{
|
||||||
"name": "t42-sso-manager",
|
"name": "t42-sso-manager",
|
||||||
"version": "1.3.1",
|
"version": "1.3.2",
|
||||||
"description": "A very simple LDAP management and SSO system",
|
"description": "A very simple LDAP management and SSO system",
|
||||||
"author": [
|
"author": [
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -43,6 +43,65 @@ afterAll(async () => {
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('OAuth client management API — /api/oauth/client', () => {
|
||||||
|
// Regression: the ORM Model.toJSON() strips non-schema fields, so the
|
||||||
|
// mapped client_id/scopes/etc. used to vanish from GET responses —
|
||||||
|
// client_id came back undefined and the theta-env bootstrap's rotate
|
||||||
|
// crashed with a 500. GET must expose client_id (and never the secret hash).
|
||||||
|
test('GET / list exposes client_id and hides client_secret_hash', async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.get('/api/oauth/client/')
|
||||||
|
.set('auth-token', token);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
const mine = res.body.results.find((c) => c.client_id === clientId);
|
||||||
|
expect(mine).toBeDefined();
|
||||||
|
expect(mine.client_id).toBe(clientId);
|
||||||
|
expect(mine).toHaveProperty('scopes');
|
||||||
|
expect(mine).not.toHaveProperty('client_secret_hash');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('GET /:id exposes client_id', async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.get(`/api/oauth/client/${clientId}`)
|
||||||
|
.set('auth-token', token);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.results.client_id).toBe(clientId);
|
||||||
|
expect(res.body.results).not.toHaveProperty('client_secret_hash');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('list then rotate a client by its returned client_id (the bootstrap path)', async () => {
|
||||||
|
// Reproduces exactly what the theta-env bootstrap does: create, list,
|
||||||
|
// find by name, rotate by the client_id from the list response. Uses a
|
||||||
|
// throwaway client so the shared flow client's secret is untouched.
|
||||||
|
const created = await request(app)
|
||||||
|
.post('/api/oauth/client/')
|
||||||
|
.set('auth-token', token)
|
||||||
|
.send({ name: 'rotate-regression', redirect_uris: REDIRECT_URI });
|
||||||
|
expect(created.status).toBe(200);
|
||||||
|
|
||||||
|
const list = await request(app).get('/api/oauth/client/').set('auth-token', token);
|
||||||
|
const found = list.body.results.find((c) => c.name === 'rotate-regression');
|
||||||
|
expect(found).toBeDefined();
|
||||||
|
expect(found.client_id).toBeTruthy(); // was undefined before the fix
|
||||||
|
|
||||||
|
const rotated = await request(app)
|
||||||
|
.post(`/api/oauth/client/${found.client_id}/rotate`)
|
||||||
|
.set('auth-token', token);
|
||||||
|
expect(rotated.status).toBe(200);
|
||||||
|
expect(rotated.body.client_secret).toBeTruthy();
|
||||||
|
|
||||||
|
await request(app).delete(`/api/oauth/client/${found.client_id}`).set('auth-token', token);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('GET /:id unknown id returns 404, not 500', async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.get('/api/oauth/client/00000000-0000-0000-0000-000000000000')
|
||||||
|
.set('auth-token', token);
|
||||||
|
expect(res.status).toBeGreaterThanOrEqual(400);
|
||||||
|
expect(res.status).toBeLessThan(500);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('OIDC Discovery', () => {
|
describe('OIDC Discovery', () => {
|
||||||
test('GET /.well-known/openid-configuration returns required fields', async () => {
|
test('GET /.well-known/openid-configuration returns required fields', async () => {
|
||||||
const res = await request(app).get('/.well-known/openid-configuration');
|
const res = await request(app).get('/.well-known/openid-configuration');
|
||||||
|
|||||||
Reference in New Issue
Block a user