Skip to content

Commit 6060a17

Browse files
committed
fix(oauth): Prevent validation errors from orphaned client tokens
Because: * Sentry showed ValidationError: "[0].name" is required * OAuth token queries use LEFT OUTER JOIN with clients table * When a client is deleted but tokens remain (orphaned), the JOIN returns NULL * This may be converted to undefined, which fails Joi validation This commit: * Add nullish coalescing (?? null) in authorized_clients.js to convert undefined → null at source * Add nullish coalescing in factories.ts when merging OAuth client names * Fix shared reference bug in getDefaultClientFields() to return copy of defaults * Add Sentry warning reporting for orphaned tokens with clientId, tokenId, and timestamps * Add regression test for undefined client_name handling Closes #FXA-13132
1 parent 7667c0b commit 6060a17

3 files changed

Lines changed: 39 additions & 3 deletions

File tree

packages/fxa-auth-server/lib/oauth/authorized_clients.js

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,33 @@
55
const { OauthError } = require('@fxa/accounts/errors');
66
const oauthDB = require('./db');
77
const ScopeSet = require('fxa-shared').oauth.scopes;
8+
const { reportSentryMessage } = require('../sentry');
89

910
// Helper function to render each returned record in the expected form.
1011
function serialize(clientIdHex, token) {
1112
const createdTime = token.createdAt.getTime();
1213
const lastAccessTime = token.lastUsedAt.getTime();
14+
15+
// Report orphaned tokens to Sentry (tokens whose client was deleted)
16+
// This can happen when a client is deleted without first deleting its tokens.
17+
if (token.clientName === null || token.clientName === undefined) {
18+
reportSentryMessage('Orphaned OAuth token detected', {
19+
level: 'warning',
20+
extra: {
21+
clientId: clientIdHex,
22+
tokenId: token.tokenId ? token.tokenId.toString('hex') : 'unknown',
23+
tokenType: token.tokenId ? 'refresh' : 'access',
24+
createdAt: createdTime,
25+
lastAccessTime: lastAccessTime,
26+
},
27+
});
28+
}
29+
1330
return {
1431
client_id: clientIdHex,
1532
refresh_token_id: token.tokenId ? token.tokenId.toString('hex') : undefined,
16-
client_name: token.clientName,
33+
// clientName may be null if the client was deleted
34+
client_name: token.clientName ?? null,
1735
created_time: createdTime,
1836
last_access_time: lastAccessTime,
1937
// Sort the scopes alphabetically, for consistent output.

packages/fxa-shared/connected-services/factories.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,7 @@ export class ConnectedServicesFactory {
257257
// We fill in a default device name from the OAuth client name,
258258
// but individual clients can override this in their device record registration.
259259
if (!client.name) {
260-
client.name = oauthClient.client_name;
260+
client.name = oauthClient.client_name ?? null;
261261
}
262262
// For now we assume that all oauth clients that register a device record are mobile apps.
263263
// Ref https://github.com/mozilla/fxa/issues/449
@@ -292,6 +292,6 @@ export class ConnectedServicesFactory {
292292
}
293293

294294
protected getDefaultClientFields(): AttachedClient {
295-
return attachedClientsDefaults;
295+
return { ...attachedClientsDefaults };
296296
}
297297
}

packages/fxa-shared/test/connected-services/factories.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,5 +193,23 @@ describe('connected-services/factories', () => {
193193
Sinon.assert.calledOnce(bStubbed.oauthClients);
194194
Sinon.assert.calledOnce(bStubbed.sessions);
195195
});
196+
197+
it('handles undefined client_name without validation errors', async () => {
198+
oauthClients = [
199+
{
200+
refresh_token_id: 'test-oauth',
201+
created_time: Date.now(),
202+
last_access_time: Date.now(),
203+
client_name: undefined as any, // Simulate undefined from database
204+
} as AttachedOAuthClient,
205+
];
206+
deviceList = [];
207+
sessions = [];
208+
209+
const results = await factory.build('1234', 'en');
210+
211+
// Verify name is null, not undefined (required for validation)
212+
assert.strictEqual(results[0].name, null);
213+
});
196214
});
197215
});

0 commit comments

Comments
 (0)