Skip to content

Commit f2b2554

Browse files
committed
fix: accept nullable user fields in OAuth token response (#468)
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'. - Make `name` and `email` nullable in `TokenResponseSchema` - Make `name` nullish in `SentryUserSchema` for consistency - Convert null → undefined at the UserInfo/LoginResult boundary - Add focused schema tests for nullable user fields Closes #468
1 parent 77603fc commit f2b2554

File tree

9 files changed

+339
-48
lines changed

9 files changed

+339
-48
lines changed

AGENTS.md

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

src/commands/auth/login.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,9 +175,9 @@ export const loginCommand = buildCommand({
175175
userId: user.id,
176176
email: user.email,
177177
username: user.username,
178-
name: user.name,
178+
name: user.name ?? undefined,
179179
});
180-
result.user = user;
180+
result.user = { ...user, name: user.name ?? undefined };
181181
} catch {
182182
// Non-fatal: user info is supplementary. Token remains stored and valid.
183183
}

src/commands/auth/whoami.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ export const whoamiCommand = buildCommand({
5959
userId: user.id,
6060
email: user.email,
6161
username: user.username,
62-
name: user.name,
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: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,8 @@ export async function runInteractiveLogin(
163163
try {
164164
setUserInfo({
165165
userId: user.id,
166-
email: user.email,
167-
name: user.name,
166+
email: user.email ?? undefined,
167+
name: user.name ?? undefined,
168168
});
169169
} catch (error) {
170170
// Report to Sentry but don't block auth - user info is not critical
@@ -178,7 +178,11 @@ export async function runInteractiveLogin(
178178
expiresIn: tokenResponse.expires_in,
179179
};
180180
if (user) {
181-
result.user = user;
181+
result.user = {
182+
id: user.id,
183+
name: user.name ?? undefined,
184+
email: user.email ?? undefined,
185+
};
182186
}
183187
return result;
184188
} 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: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ export const SentryUserSchema = z
215215
id: z.string(),
216216
email: z.string().optional(),
217217
username: z.string().optional(),
218-
name: z.string().optional(),
218+
name: z.string().nullish(),
219219
})
220220
.passthrough();
221221

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: 162 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,134 @@ 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 converted to undefined in result and 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+
expect(result!.user!.name).toBeUndefined();
187+
expect(result!.user!.email).toBe("user@example.com");
188+
expect(result!.user!.id).toBe("48168");
189+
190+
expect(setUserInfoSpy).toHaveBeenCalledWith({
191+
userId: "48168",
192+
email: "user@example.com",
193+
name: undefined,
194+
});
195+
});
196+
197+
test("null user.email is converted to undefined in result", async () => {
198+
performDeviceFlowSpy = spyOn(oauth, "performDeviceFlow").mockImplementation(
199+
async (callbacks) => {
200+
await callbacks.onUserCode(
201+
"EFGH",
202+
"https://sentry.io/auth/device/",
203+
"https://sentry.io/auth/device/?user_code=EFGH"
204+
);
205+
return makeTokenResponse({
206+
id: "123",
207+
name: "Jane Doe",
208+
email: null,
209+
});
210+
}
211+
);
212+
213+
const result = await runInteractiveLogin({ timeout: 1000 });
214+
215+
expect(result).not.toBeNull();
216+
expect(result!.user!.name).toBe("Jane Doe");
217+
expect(result!.user!.email).toBeUndefined();
218+
219+
expect(setUserInfoSpy).toHaveBeenCalledWith({
220+
userId: "123",
221+
email: undefined,
222+
name: "Jane Doe",
223+
});
224+
});
225+
226+
test("no user in token response: result.user is undefined, setUserInfo not called", async () => {
227+
performDeviceFlowSpy = spyOn(oauth, "performDeviceFlow").mockImplementation(
228+
async (callbacks) => {
229+
await callbacks.onUserCode(
230+
"WXYZ",
231+
"https://sentry.io/auth/device/",
232+
"https://sentry.io/auth/device/?user_code=WXYZ"
233+
);
234+
return makeTokenResponse(); // no user
235+
}
236+
);
237+
238+
const result = await runInteractiveLogin({ timeout: 1000 });
239+
240+
expect(result).not.toBeNull();
241+
expect(result!.user).toBeUndefined();
242+
expect(setUserInfoSpy).not.toHaveBeenCalled();
243+
});
244+
});

test/types/oauth.test.ts

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
/**
2+
* OAuth Schema Tests
3+
*
4+
* Validates that Zod schemas correctly handle real-world API responses,
5+
* including nullable fields that the Sentry API may return.
6+
*/
7+
8+
import { describe, expect, test } from "bun:test";
9+
import { TokenResponseSchema } from "../../src/types/oauth.js";
10+
11+
describe("TokenResponseSchema", () => {
12+
const baseTokenResponse = {
13+
access_token: "sntrys_abc123",
14+
refresh_token: "sntryr_def456",
15+
expires_in: 2_591_999,
16+
token_type: "Bearer",
17+
scope: "event:read event:write member:read org:read project:admin",
18+
};
19+
20+
test("accepts response with user.name as null (GH-468)", () => {
21+
const response = {
22+
...baseTokenResponse,
23+
expires_at: "2026-04-18T09:03:59.747189Z",
24+
user: { id: "48168", name: null, email: "user@example.com" },
25+
};
26+
27+
const result = TokenResponseSchema.safeParse(response);
28+
expect(result.success).toBe(true);
29+
if (result.success) {
30+
expect(result.data.user?.name).toBeNull();
31+
expect(result.data.user?.email).toBe("user@example.com");
32+
}
33+
});
34+
35+
test("accepts response with user.email as null", () => {
36+
const response = {
37+
...baseTokenResponse,
38+
user: { id: "48168", name: "Jane Doe", email: null },
39+
};
40+
41+
const result = TokenResponseSchema.safeParse(response);
42+
expect(result.success).toBe(true);
43+
if (result.success) {
44+
expect(result.data.user?.name).toBe("Jane Doe");
45+
expect(result.data.user?.email).toBeNull();
46+
}
47+
});
48+
49+
test("accepts response with both name and email as null", () => {
50+
const response = {
51+
...baseTokenResponse,
52+
user: { id: "48168", name: null, email: null },
53+
};
54+
55+
const result = TokenResponseSchema.safeParse(response);
56+
expect(result.success).toBe(true);
57+
});
58+
59+
test("accepts response without user field", () => {
60+
const result = TokenResponseSchema.safeParse(baseTokenResponse);
61+
expect(result.success).toBe(true);
62+
if (result.success) {
63+
expect(result.data.user).toBeUndefined();
64+
}
65+
});
66+
67+
test("accepts response with extra fields in user (passthrough)", () => {
68+
const response = {
69+
...baseTokenResponse,
70+
user: {
71+
id: "48168",
72+
name: "Jane",
73+
email: "jane@example.com",
74+
avatar_url: "https://example.com/avatar.png",
75+
},
76+
};
77+
78+
const result = TokenResponseSchema.safeParse(response);
79+
expect(result.success).toBe(true);
80+
});
81+
82+
test("rejects response missing access_token", () => {
83+
const { access_token: _, ...noToken } = baseTokenResponse;
84+
const result = TokenResponseSchema.safeParse(noToken);
85+
expect(result.success).toBe(false);
86+
});
87+
});

0 commit comments

Comments
 (0)