Skip to content

Commit 22d5f39

Browse files
authored
fix: improve error quality and prevent token leak in telemetry (#279)
## Summary Fixes three of the highest-impact issues from the 0.11.0 release (sourced from Sentry production data). ### fix(issue): actionable error when issue not found by numeric ID (CLI-N) **118 events / 41 users** `getIssue()` surfaces a bare 404 as a generic message with no indication of which ID failed. Users often confuse numeric Sentry group IDs (which require access) with short-ID suffixes (which are human-readable). Now `resolveIssue()` catches the 404 in both the `numeric` and `explicit-org-numeric` cases and re-throws a `ContextError` with: - The exact ID that wasn't found - A note that access may be missing or the issue may be deleted - A suggestion to try the `<project>-<id>` short-ID format ### fix(telemetry): redact `--token` value from telemetry tags (CLI-19) **Security — token values were being sent to Sentry** `setFlagContext()` in `telemetry.ts` was forwarding the raw value of every flag as a `flag.<name>` tag. For `sentry auth login --token sntrys_...`, this meant the actual API token was appearing in the `flag.token` tag on every telemetry event. Adds a `SENSITIVE_FLAGS` set (currently: `token`) and replaces those tag values with `[REDACTED]` before the tag is set. ### fix(issue list): validate `--cursor` format before hitting the API (CLI-7H/7P/7B) **21 events / 12 users — 400 Bad Request from API** `--cursor 100` (a plain integer) was forwarded directly to the API, which rejects it. Cursor values are opaque strings like `1735689600:0:0`. Plain-integer inputs are now rejected at parse time with a message explaining the expected format. Note: the `--limit` part of this bug is already addressed on main via auto-pagination (#274).
1 parent 3ec9dd9 commit 22d5f39

File tree

7 files changed

+208
-6
lines changed

7 files changed

+208
-6
lines changed

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ docs/dist
4040
docs/node_modules
4141
docs/.astro
4242

43+
# local planning notes (not for version control)
44+
.plans
45+
4346
# IntelliJ based IDEs
4447
.idea
4548

src/commands/issue/list.ts

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,9 @@ const VALID_SORT_VALUES: SortValue[] = ["date", "new", "freq", "user"];
8383
/** Usage hint for ContextError messages */
8484
const USAGE_HINT = "sentry issue list <org>/<project>";
8585

86+
/** Matches strings that are all digits — used to detect invalid cursor values */
87+
const ALL_DIGITS_RE = /^\d+$/;
88+
8689
/**
8790
* Maximum --limit value (user-facing ceiling for practical CLI response times).
8891
* Auto-pagination can theoretically fetch more, but 1000 keeps responses reasonable.
@@ -733,7 +736,21 @@ export const listCommand = buildCommand({
733736
json: LIST_JSON_FLAG,
734737
cursor: {
735738
kind: "parsed",
736-
parse: String,
739+
parse: (value: string) => {
740+
// "last" is the magic keyword to resume from the saved cursor
741+
if (value === "last") {
742+
return value;
743+
}
744+
// Sentry pagination cursors are opaque strings like "1735689600:0:0".
745+
// Plain integers are not valid cursors — catch this early so the user
746+
// gets a clear error rather than a cryptic 400 from the API.
747+
if (ALL_DIGITS_RE.test(value)) {
748+
throw new Error(
749+
`'${value}' is not a valid cursor. Cursors look like "1735689600:0:0". Use "last" to continue from the previous page.`
750+
);
751+
}
752+
return value;
753+
},
737754
// Issue-specific cursor brief: cursor only works in <org>/ mode
738755
brief:
739756
'Pagination cursor — only for <org>/ mode (use "last" to continue)',

src/commands/issue/utils.ts

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {
1414
import { parseIssueArg } from "../../lib/arg-parsing.js";
1515
import { getProjectByAlias } from "../../lib/db/project-aliases.js";
1616
import { detectAllDsns } from "../../lib/dsn/index.js";
17-
import { ContextError } from "../../lib/errors.js";
17+
import { ApiError, ContextError } from "../../lib/errors.js";
1818
import { getProgressMessage } from "../../lib/formatters/seer.js";
1919
import { expandToFullShortId, isShortSuffix } from "../../lib/issue-id.js";
2020
import { poll } from "../../lib/polling.js";
@@ -255,8 +255,21 @@ export async function resolveIssue(
255255
switch (parsed.type) {
256256
case "numeric": {
257257
// Direct fetch by numeric ID - no org context
258-
const issue = await getIssue(parsed.id);
259-
return { org: undefined, issue };
258+
try {
259+
const issue = await getIssue(parsed.id);
260+
return { org: undefined, issue };
261+
} catch (err) {
262+
if (err instanceof ApiError && err.status === 404) {
263+
// Improve on the generic "Issue not found" message by including the ID
264+
// and suggesting the short-ID format, since users often confuse numeric
265+
// group IDs with short-ID suffixes.
266+
throw new ContextError(`Issue ${parsed.id}`, commandHint, [
267+
`No issue with numeric ID ${parsed.id} found — you may not have access, or it may have been deleted.`,
268+
`If this is a short ID suffix, try: sentry issue ${command} <project>-${parsed.id}`,
269+
]);
270+
}
271+
throw err;
272+
}
260273
}
261274

262275
case "explicit": {
@@ -268,8 +281,18 @@ export async function resolveIssue(
268281

269282
case "explicit-org-numeric": {
270283
// Org + numeric ID
271-
const issue = await getIssue(parsed.numericId);
272-
return { org: parsed.org, issue };
284+
try {
285+
const issue = await getIssue(parsed.numericId);
286+
return { org: parsed.org, issue };
287+
} catch (err) {
288+
if (err instanceof ApiError && err.status === 404) {
289+
throw new ContextError(`Issue ${parsed.numericId}`, commandHint, [
290+
`No issue with numeric ID ${parsed.numericId} found in org '${parsed.org}' — you may not have access, or it may have been deleted.`,
291+
`If this is a short ID suffix, try: sentry issue ${command} <project>-${parsed.numericId}`,
292+
]);
293+
}
294+
throw err;
295+
}
273296
}
274297

275298
case "explicit-org-suffix":

src/lib/telemetry.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,12 @@ export function setOrgProjectContext(orgs: string[], projects: string[]): void {
362362
}
363363
}
364364

365+
/**
366+
* Flag names whose values must never be sent to telemetry.
367+
* Values for these flags are replaced with "[REDACTED]" regardless of content.
368+
*/
369+
const SENSITIVE_FLAGS = new Set(["token"]);
370+
365371
/**
366372
* Set command flags as telemetry tags.
367373
*
@@ -373,6 +379,9 @@ export function setOrgProjectContext(orgs: string[], projects: string[]): void {
373379
* - String/number flags: only when defined and non-empty
374380
* - Array flags: only when non-empty
375381
*
382+
* Sensitive flags (e.g., `--token`) have their values replaced with
383+
* "[REDACTED]" to prevent secrets from reaching telemetry.
384+
*
376385
* Call this at the start of command func() to instrument flag usage.
377386
*
378387
* @param flags - The parsed flags object from Stricli
@@ -410,6 +419,12 @@ export function setFlagContext(flags: Record<string, unknown>): void {
410419
// Convert camelCase to kebab-case for consistency with CLI flag names
411420
const kebabKey = key.replace(/([A-Z])/g, "-$1").toLowerCase();
412421

422+
// Redact sensitive flag values (e.g., API tokens) — never send secrets to telemetry
423+
if (SENSITIVE_FLAGS.has(kebabKey)) {
424+
Sentry.setTag(`flag.${kebabKey}`, "[REDACTED]");
425+
continue;
426+
}
427+
413428
// Set the tag with flag. prefix
414429
// For booleans, just set "true"; for other types, convert to string
415430
const tagValue =

test/commands/issue/list.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -635,3 +635,30 @@ describe("issue list: org-all mode (cursor pagination)", () => {
635635
);
636636
});
637637
});
638+
639+
describe("issue list: cursor flag parse validation", () => {
640+
// Access the parse function directly from the command's flag definition.
641+
// This tests the validation without needing a full command invocation.
642+
const parseCursor = (
643+
listCommand.parameters.flags!.cursor as { parse: (v: string) => string }
644+
).parse;
645+
646+
test('accepts "last" keyword', () => {
647+
expect(parseCursor("last")).toBe("last");
648+
});
649+
650+
test("accepts valid opaque cursor strings", () => {
651+
expect(parseCursor("1735689600:0:0")).toBe("1735689600:0:0");
652+
expect(parseCursor("1735689600:0:1")).toBe("1735689600:0:1");
653+
expect(parseCursor("abc:def:ghi")).toBe("abc:def:ghi");
654+
});
655+
656+
test("rejects plain integer cursors with descriptive error", () => {
657+
expect(() => parseCursor("100")).toThrow("not a valid cursor");
658+
expect(() => parseCursor("100")).toThrow("1735689600:0:0");
659+
});
660+
661+
test("error message includes the invalid value passed", () => {
662+
expect(() => parseCursor("5000")).toThrow("'5000'");
663+
});
664+
});

test/commands/issue/utils.test.ts

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,13 @@ import {
99
buildCommandHint,
1010
ensureRootCauseAnalysis,
1111
pollAutofixState,
12+
resolveIssue,
1213
resolveOrgAndIssueId,
1314
} from "../../../src/commands/issue/utils.js";
1415
import { DEFAULT_SENTRY_URL } from "../../../src/lib/constants.js";
1516
import { setAuthToken } from "../../../src/lib/db/auth.js";
1617
import { setOrgRegion } from "../../../src/lib/db/regions.js";
18+
import { ContextError } from "../../../src/lib/errors.js";
1719
import { useTestConfigDir } from "../../helpers.js";
1820

1921
describe("buildCommandHint", () => {
@@ -1267,3 +1269,97 @@ describe("ensureRootCauseAnalysis", () => {
12671269
expect(stderrOutput).toContain("root cause analysis");
12681270
});
12691271
});
1272+
1273+
describe("resolveIssue: numeric 404 error handling", () => {
1274+
const getResolveIssueConfigDir = useTestConfigDir("test-resolve-issue-");
1275+
1276+
let savedFetch: typeof globalThis.fetch;
1277+
1278+
beforeEach(async () => {
1279+
savedFetch = globalThis.fetch;
1280+
await setAuthToken("test-token");
1281+
await setOrgRegion("my-org", DEFAULT_SENTRY_URL);
1282+
});
1283+
1284+
afterEach(() => {
1285+
globalThis.fetch = savedFetch;
1286+
});
1287+
1288+
test("numeric 404 throws ContextError with ID and short-ID hint", async () => {
1289+
// @ts-expect-error - partial mock
1290+
globalThis.fetch = async () =>
1291+
new Response(JSON.stringify({ detail: "Issue not found" }), {
1292+
status: 404,
1293+
headers: { "Content-Type": "application/json" },
1294+
});
1295+
1296+
const err = await resolveIssue({
1297+
issueArg: "123456789",
1298+
cwd: getResolveIssueConfigDir(),
1299+
command: "view",
1300+
}).catch((e) => e);
1301+
1302+
expect(err).toBeInstanceOf(ContextError);
1303+
// Message includes the numeric ID
1304+
expect(String(err)).toContain("123456789");
1305+
// Suggests the short-ID format
1306+
expect(String(err)).toContain("project>-123456789");
1307+
});
1308+
1309+
test("numeric non-404 error propagates unchanged", async () => {
1310+
// @ts-expect-error - partial mock
1311+
globalThis.fetch = async () =>
1312+
new Response(JSON.stringify({ detail: "Internal Server Error" }), {
1313+
status: 500,
1314+
headers: { "Content-Type": "application/json" },
1315+
});
1316+
1317+
await expect(
1318+
resolveIssue({
1319+
issueArg: "123456789",
1320+
cwd: getResolveIssueConfigDir(),
1321+
command: "view",
1322+
})
1323+
).rejects.not.toBeInstanceOf(ContextError);
1324+
});
1325+
1326+
test("explicit-org-numeric 404 throws ContextError with org and ID", async () => {
1327+
// @ts-expect-error - partial mock
1328+
globalThis.fetch = async () =>
1329+
new Response(JSON.stringify({ detail: "Issue not found" }), {
1330+
status: 404,
1331+
headers: { "Content-Type": "application/json" },
1332+
});
1333+
1334+
const err = await resolveIssue({
1335+
issueArg: "my-org/999999999",
1336+
cwd: getResolveIssueConfigDir(),
1337+
command: "view",
1338+
}).catch((e) => e);
1339+
1340+
expect(err).toBeInstanceOf(ContextError);
1341+
// Message includes the numeric ID
1342+
expect(String(err)).toContain("999999999");
1343+
// Message mentions the org
1344+
expect(String(err)).toContain("my-org");
1345+
// Suggests the short-ID format
1346+
expect(String(err)).toContain("project>-999999999");
1347+
});
1348+
1349+
test("explicit-org-numeric non-404 error propagates unchanged", async () => {
1350+
// @ts-expect-error - partial mock
1351+
globalThis.fetch = async () =>
1352+
new Response(JSON.stringify({ detail: "Unauthorized" }), {
1353+
status: 401,
1354+
headers: { "Content-Type": "application/json" },
1355+
});
1356+
1357+
await expect(
1358+
resolveIssue({
1359+
issueArg: "my-org/999999999",
1360+
cwd: getResolveIssueConfigDir(),
1361+
command: "view",
1362+
})
1363+
).rejects.not.toBeInstanceOf(ContextError);
1364+
});
1365+
});

test/lib/telemetry.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -411,6 +411,27 @@ describe("setFlagContext", () => {
411411
expect(setTagSpy).toHaveBeenCalledTimes(1);
412412
expect(setTagSpy).toHaveBeenCalledWith("flag.long-flag", "x".repeat(200));
413413
});
414+
415+
test("redacts sensitive flag values (token)", () => {
416+
setFlagContext({
417+
token: "sntrys_eyJpYXQiOjE3MDAwMDAwMDAsImF1dGhUb2tlbiI6InNlY3JldCJ9",
418+
});
419+
expect(setTagSpy).toHaveBeenCalledTimes(1);
420+
expect(setTagSpy).toHaveBeenCalledWith("flag.token", "[REDACTED]");
421+
});
422+
423+
test("still sets the tag when token flag is present (presence is useful signal)", () => {
424+
setFlagContext({ token: "any-token-value" });
425+
// The tag is set (so we know --token was used), but the value is redacted
426+
expect(setTagSpy).toHaveBeenCalledWith("flag.token", "[REDACTED]");
427+
});
428+
429+
test("does not redact non-sensitive flags alongside token", () => {
430+
setFlagContext({ token: "secret", format: "json" });
431+
expect(setTagSpy).toHaveBeenCalledTimes(2);
432+
expect(setTagSpy).toHaveBeenCalledWith("flag.token", "[REDACTED]");
433+
expect(setTagSpy).toHaveBeenCalledWith("flag.format", "json");
434+
});
414435
});
415436

416437
describe("setArgsContext", () => {

0 commit comments

Comments
 (0)