Skip to content

Commit 28ab647

Browse files
authored
fix: accept nullable user fields in OAuth token response (#470)
The Sentry API can return `null` for `user.name` in the OAuth token response. The Zod schema had `name: z.string()` which rejected null, causing `safeParse()` to fail and login to throw 'Unexpected response from token endpoint'. ## Changes - Make `name` and `email` nullable in `TokenResponseSchema` (root cause) - Make `name` nullish in `SentryUserSchema` for consistency - Convert null → undefined at the UserInfo/LoginResult boundary in `interactive-login.ts`, `auth/login.ts`, and `auth/whoami.ts` - Add focused schema tests for nullable user fields Closes #468
1 parent 2ade5c4 commit 28ab647

File tree

9 files changed

+372
-55
lines changed

9 files changed

+372
-55
lines changed

AGENTS.md

Lines changed: 46 additions & 35 deletions
Large diffs are not rendered by default.

src/commands/auth/login.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@ import {
1919
} from "../../lib/formatters/human.js";
2020
import { CommandOutput } from "../../lib/formatters/output.js";
2121
import type { LoginResult } from "../../lib/interactive-login.js";
22-
import { runInteractiveLogin } from "../../lib/interactive-login.js";
22+
import {
23+
runInteractiveLogin,
24+
toLoginUser,
25+
} from "../../lib/interactive-login.js";
2326
import { logger } from "../../lib/logger.js";
2427
import { clearResponseCache } from "../../lib/response-cache.js";
2528

@@ -173,11 +176,11 @@ export const loginCommand = buildCommand({
173176
const user = await getCurrentUser();
174177
setUserInfo({
175178
userId: user.id,
176-
email: user.email,
177-
username: user.username,
178-
name: user.name,
179+
email: user.email ?? undefined,
180+
username: user.username ?? undefined,
181+
name: user.name ?? undefined,
179182
});
180-
result.user = user;
183+
result.user = toLoginUser(user);
181184
} catch {
182185
// Non-fatal: user info is supplementary. Token remains stored and valid.
183186
}

src/commands/auth/whoami.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,9 @@ export const whoamiCommand = buildCommand({
5757
try {
5858
setUserInfo({
5959
userId: user.id,
60-
email: user.email,
61-
username: user.username,
62-
name: user.name,
60+
email: user.email ?? undefined,
61+
username: user.username ?? undefined,
62+
name: user.name ?? undefined,
6363
});
6464
} catch {
6565
// Cache update failure is non-essential — user identity was already fetched.

src/lib/interactive-login.ts

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,23 @@ export type LoginResult = {
3131
expiresIn?: number;
3232
};
3333

34+
/**
35+
* Build a clean user object for {@link LoginResult}, omitting falsy
36+
* fields (null, undefined, empty string) so they don't leak into output.
37+
*
38+
* Empty-string names/emails are semantically absent for display purposes.
39+
*/
40+
export function toLoginUser(user: {
41+
id: string;
42+
name?: string | null;
43+
email?: string | null;
44+
username?: string | null;
45+
}): NonNullable<LoginResult["user"]> {
46+
return Object.fromEntries(
47+
Object.entries(user).filter(([k, v]) => k === "id" || Boolean(v))
48+
) as NonNullable<LoginResult["user"]>;
49+
}
50+
3451
/** Options for the interactive login flow */
3552
export type InteractiveLoginOptions = {
3653
/** Timeout for OAuth flow in milliseconds (default: 900000 = 15 minutes) */
@@ -163,8 +180,8 @@ export async function runInteractiveLogin(
163180
try {
164181
setUserInfo({
165182
userId: user.id,
166-
email: user.email,
167-
name: user.name,
183+
email: user.email ?? undefined,
184+
name: user.name ?? undefined,
168185
});
169186
} catch (error) {
170187
// Report to Sentry but don't block auth - user info is not critical
@@ -178,7 +195,7 @@ export async function runInteractiveLogin(
178195
expiresIn: tokenResponse.expires_in,
179196
};
180197
if (user) {
181-
result.user = user;
198+
result.user = toLoginUser(user);
182199
}
183200
return result;
184201
} catch (err) {

src/types/oauth.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ export const TokenResponseSchema = z
3434
user: z
3535
.object({
3636
id: z.string(),
37-
name: z.string(),
38-
email: z.string(),
37+
name: z.string().nullable(),
38+
email: z.string().nullable(),
3939
})
4040
.passthrough()
4141
.optional(),

src/types/sentry.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -210,12 +210,20 @@ export type UserRegionsResponse = z.infer<typeof UserRegionsResponseSchema>;
210210

211211
// User
212212

213+
/**
214+
* Minimal user schema for the `/auth/` endpoint response.
215+
*
216+
* All optional fields use `.nullish()` (accepts both `null` and `undefined`)
217+
* because the Sentry API can return `null` for any of these.
218+
* Note: `@sentry/api` doesn't export types for the `/auth/` endpoint —
219+
* it's undocumented, so we define this schema manually.
220+
*/
213221
export const SentryUserSchema = z
214222
.object({
215223
id: z.string(),
216-
email: z.string().optional(),
217-
username: z.string().optional(),
218-
name: z.string().optional(),
224+
email: z.string().nullish(),
225+
username: z.string().nullish(),
226+
name: z.string().nullish(),
219227
})
220228
.passthrough();
221229

test/commands/auth/login.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,37 @@ describe("loginCommand.func --token path", () => {
182182
expect(out).toContain("Jane Doe");
183183
});
184184

185+
test("--token: null user.name is converted to undefined in setUserInfo", async () => {
186+
isAuthenticatedSpy.mockResolvedValue(false);
187+
setAuthTokenSpy.mockResolvedValue(undefined);
188+
getUserRegionsSpy.mockResolvedValue([]);
189+
getCurrentUserSpy.mockResolvedValue({
190+
id: "5",
191+
name: null,
192+
email: "x@y.com",
193+
username: "xuser",
194+
});
195+
setUserInfoSpy.mockReturnValue(undefined);
196+
197+
const { context, getStdout } = createContext();
198+
await func.call(context, {
199+
token: "valid-token",
200+
force: false,
201+
timeout: 900,
202+
});
203+
204+
expect(setUserInfoSpy).toHaveBeenCalledWith({
205+
userId: "5",
206+
email: "x@y.com",
207+
username: "xuser",
208+
name: undefined,
209+
});
210+
const out = getStdout();
211+
expect(out).toContain("Authenticated");
212+
// With null name, formatUserIdentity falls back to email
213+
expect(out).toContain("x@y.com");
214+
});
215+
185216
test("--token: invalid token clears auth and throws AuthError", async () => {
186217
isAuthenticatedSpy.mockResolvedValue(false);
187218
setAuthTokenSpy.mockResolvedValue(undefined);

test/lib/interactive-login.test.ts

Lines changed: 164 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,40 @@
11
/**
2-
* Tests for buildDeviceFlowDisplay — the extracted display logic from the
3-
* interactive login flow's onUserCode callback.
2+
* Tests for interactive login flow.
3+
*
4+
* - buildDeviceFlowDisplay: extracted display logic (pure function)
5+
* - runInteractiveLogin: full OAuth device flow with mocked dependencies
46
*
57
* Uses SENTRY_PLAIN_OUTPUT=1 to get predictable raw markdown output
68
* (no ANSI codes) for string assertions.
79
*/
810

9-
import { afterAll, beforeAll, describe, expect, test } from "bun:test";
10-
import { buildDeviceFlowDisplay } from "../../src/lib/interactive-login.js";
11+
import {
12+
afterAll,
13+
afterEach,
14+
beforeAll,
15+
beforeEach,
16+
describe,
17+
expect,
18+
spyOn,
19+
test,
20+
} from "bun:test";
21+
// biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking
22+
import * as browser from "../../src/lib/browser.js";
23+
// biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking
24+
import * as clipboard from "../../src/lib/clipboard.js";
25+
// biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking
26+
import * as dbInstance from "../../src/lib/db/index.js";
27+
// biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking
28+
import * as dbUser from "../../src/lib/db/user.js";
29+
import {
30+
buildDeviceFlowDisplay,
31+
runInteractiveLogin,
32+
} from "../../src/lib/interactive-login.js";
33+
// biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking
34+
import * as oauth from "../../src/lib/oauth.js";
35+
// biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking
36+
import * as qrcode from "../../src/lib/qrcode.js";
37+
import type { TokenResponse } from "../../src/types/index.js";
1138

1239
// Force plain output for predictable string matching
1340
let origPlain: string | undefined;
@@ -84,3 +111,136 @@ describe("buildDeviceFlowDisplay", () => {
84111
expect(withoutBrowser.length).toBeGreaterThan(withBrowser.length);
85112
});
86113
});
114+
115+
describe("runInteractiveLogin", () => {
116+
let performDeviceFlowSpy: ReturnType<typeof spyOn>;
117+
let completeOAuthFlowSpy: ReturnType<typeof spyOn>;
118+
let openBrowserSpy: ReturnType<typeof spyOn>;
119+
let generateQRCodeSpy: ReturnType<typeof spyOn>;
120+
let setupCopyKeyListenerSpy: ReturnType<typeof spyOn>;
121+
let setUserInfoSpy: ReturnType<typeof spyOn>;
122+
let getDbPathSpy: ReturnType<typeof spyOn>;
123+
124+
/** Helper to build a mock TokenResponse with a user whose fields may be null. */
125+
function makeTokenResponse(user?: {
126+
id: string;
127+
name: string | null;
128+
email: string | null;
129+
}): TokenResponse {
130+
return {
131+
access_token: "sntrys_test_token",
132+
token_type: "Bearer",
133+
expires_in: 3600,
134+
...(user ? { user } : {}),
135+
};
136+
}
137+
138+
beforeEach(() => {
139+
completeOAuthFlowSpy = spyOn(oauth, "completeOAuthFlow").mockResolvedValue(
140+
undefined
141+
);
142+
openBrowserSpy = spyOn(browser, "openBrowser").mockResolvedValue(false);
143+
generateQRCodeSpy = spyOn(qrcode, "generateQRCode").mockResolvedValue(
144+
"[QR]"
145+
);
146+
setupCopyKeyListenerSpy = spyOn(
147+
clipboard,
148+
"setupCopyKeyListener"
149+
).mockReturnValue(() => {
150+
// no-op cleanup
151+
});
152+
setUserInfoSpy = spyOn(dbUser, "setUserInfo").mockReturnValue(undefined);
153+
getDbPathSpy = spyOn(dbInstance, "getDbPath").mockReturnValue("/tmp/db");
154+
});
155+
156+
afterEach(() => {
157+
performDeviceFlowSpy.mockRestore();
158+
completeOAuthFlowSpy.mockRestore();
159+
openBrowserSpy.mockRestore();
160+
generateQRCodeSpy.mockRestore();
161+
setupCopyKeyListenerSpy.mockRestore();
162+
setUserInfoSpy.mockRestore();
163+
getDbPathSpy.mockRestore();
164+
});
165+
166+
test("null user.name is omitted from result and stored as undefined in setUserInfo", async () => {
167+
performDeviceFlowSpy = spyOn(oauth, "performDeviceFlow").mockImplementation(
168+
async (callbacks) => {
169+
await callbacks.onUserCode(
170+
"ABCD",
171+
"https://sentry.io/auth/device/",
172+
"https://sentry.io/auth/device/?user_code=ABCD"
173+
);
174+
return makeTokenResponse({
175+
id: "48168",
176+
name: null,
177+
email: "user@example.com",
178+
});
179+
}
180+
);
181+
182+
const result = await runInteractiveLogin({ timeout: 1000 });
183+
184+
expect(result).not.toBeNull();
185+
expect(result!.user).toBeDefined();
186+
// name must be absent from the result object (not present as undefined)
187+
expect("name" in result!.user!).toBe(false);
188+
expect(result!.user!.email).toBe("user@example.com");
189+
expect(result!.user!.id).toBe("48168");
190+
191+
expect(setUserInfoSpy).toHaveBeenCalledWith({
192+
userId: "48168",
193+
email: "user@example.com",
194+
name: undefined,
195+
});
196+
});
197+
198+
test("null user.email is omitted from result", async () => {
199+
performDeviceFlowSpy = spyOn(oauth, "performDeviceFlow").mockImplementation(
200+
async (callbacks) => {
201+
await callbacks.onUserCode(
202+
"EFGH",
203+
"https://sentry.io/auth/device/",
204+
"https://sentry.io/auth/device/?user_code=EFGH"
205+
);
206+
return makeTokenResponse({
207+
id: "123",
208+
name: "Jane Doe",
209+
email: null,
210+
});
211+
}
212+
);
213+
214+
const result = await runInteractiveLogin({ timeout: 1000 });
215+
216+
expect(result).not.toBeNull();
217+
expect(result!.user!.name).toBe("Jane Doe");
218+
// email must be absent from the result object
219+
expect("email" in result!.user!).toBe(false);
220+
221+
expect(setUserInfoSpy).toHaveBeenCalledWith({
222+
userId: "123",
223+
email: undefined,
224+
name: "Jane Doe",
225+
});
226+
});
227+
228+
test("no user in token response: result.user is undefined, setUserInfo not called", async () => {
229+
performDeviceFlowSpy = spyOn(oauth, "performDeviceFlow").mockImplementation(
230+
async (callbacks) => {
231+
await callbacks.onUserCode(
232+
"WXYZ",
233+
"https://sentry.io/auth/device/",
234+
"https://sentry.io/auth/device/?user_code=WXYZ"
235+
);
236+
return makeTokenResponse(); // no user
237+
}
238+
);
239+
240+
const result = await runInteractiveLogin({ timeout: 1000 });
241+
242+
expect(result).not.toBeNull();
243+
expect(result!.user).toBeUndefined();
244+
expect(setUserInfoSpy).not.toHaveBeenCalled();
245+
});
246+
});

0 commit comments

Comments
 (0)