Skip to content

Commit 5db9a4d

Browse files
committed
fix: handle JSON primitives in dataToQueryParams for GET requests
parseDataBody can return null, booleans, or numbers from JSON.parse (hidden behind an `as` cast). These fall through the string and array checks in dataToQueryParams and crash on Object.entries(null). Guard against all non-object primitives with a clear ValidationError. Extract URL-encoded parsing to a helper to stay under complexity limit.
1 parent c22c4a8 commit 5db9a4d

File tree

3 files changed

+66
-34
lines changed

3 files changed

+66
-34
lines changed

AGENTS.md

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -635,18 +635,18 @@ mock.module("./some-module", () => ({
635635
* **cli.sentry.dev is served from gh-pages branch via GitHub Pages**: \`cli.sentry.dev\` is served from gh-pages branch via GitHub Pages. Craft's gh-pages target runs \`git rm -r -f .\` before extracting docs — persist extra files via \`postReleaseCommand\` in \`.craft.yml\`. Install script supports \`--channel nightly\`, downloading from the \`nightly\` release tag directly. version.json is only used by upgrade/version-check flow.
636636
637637
<!-- lore:019cbe93-19b8-7776-9705-20bbde226599 -->
638-
* **Nightly delta upgrade buildNightlyPatchGraph fetches ALL patch tags — O(N) HTTP calls**: Delta upgrade in \`src/lib/delta-upgrade.ts\` supports stable (GitHub Releases) and nightly (GHCR) channels. \`filterAndSortChainTags\` filters \`patch-\*\` tags by version range using \`Bun.semver.order()\`, fetches only 1-2 chain manifests. GHCR uses \`fetchWithRetry\` (10s timeout + 1 retry; blobs 30s) with optional \`signal?: AbortSignal\` combined via \`AbortSignal.any()\`. \`isExternalAbort(error, signal)\` skips retries for external aborts — critical for background prefetch where \`abortPendingVersionCheck()\` fires on exit. Patch pre-fetch downloads ~50-80KB patches to \`~/.sentry/patch-cache/\` (file-based, not SQLite) with 7-day TTL. \`loadCachedChain\` stitches patches across runs for multi-hop offline upgrades. \`delta.source\` span attribute distinguishes \`cache\` vs \`network\`.
638+
* **Nightly delta upgrade buildNightlyPatchGraph fetches ALL patch tags — O(N) HTTP calls**: Delta upgrade in \`src/lib/delta-upgrade.ts\` supports stable (GitHub Releases) and nightly (GHCR) channels. \`filterAndSortChainTags\` filters \`patch-\*\` tags by version range using \`Bun.semver.order()\`. GHCR uses \`fetchWithRetry\` (10s timeout + 1 retry; blobs 30s) with optional \`signal?: AbortSignal\` combined via \`AbortSignal.any()\`. \`isExternalAbort(error, signal)\` skips retries for external aborts — critical for background prefetch. Patches cached to \`~/.sentry/patch-cache/\` (file-based, 7-day TTL). \`loadCachedChain\` stitches patches for multi-hop offline upgrades.
639639
640640
<!-- lore:2c3eb7ab-1341-4392-89fd-d81095cfe9c4 -->
641641
* **npm bundle requires Node.js >= 22 due to node:sqlite polyfill**: The npm package (dist/bin.cjs) requires Node.js >= 22 because the bun:sqlite polyfill uses \`node:sqlite\`. A runtime version guard in the esbuild banner catches this early. When writing esbuild banner strings in TS template literals, double-escape: \`\\\\\\\n\` in TS → \`\\\n\` in output → newline at runtime. Single \`\\\n\` produces a literal newline inside a JS string, causing SyntaxError.
642642
643643
<!-- lore:019c972c-9f0f-75cd-9e24-9bdbb1ac03d6 -->
644-
* **Numeric issue ID resolution returns org:undefined despite API success**: Numeric issue ID resolution in \`resolveNumericIssue()\`: (1) try DSN/env/config for org, (2) if found use \`getIssueInOrg(org, id)\` with region routing, (3) else fall back to unscoped \`getIssue(id)\`, (4) extract org from \`issue.permalink\` via \`parseSentryUrl\` as final fallback. \`parseSentryUrl\` handles both path-based (\`/organizations/{org}/...\`) and subdomain-style (\`https://{org}.sentry.io/issues/123/\`) URLs. \`matchSubdomainOrg()\` filters region subdomains by requiring slug length > 2. Supports \`/issues/{id}/\`, \`/issues/{id}/events/{eventId}/\`, and \`/traces/{traceId}/\` paths. Self-hosted uses path-based only.
644+
* **Numeric issue ID resolution returns org:undefined despite API success**: Numeric issue ID resolution in \`resolveNumericIssue()\`: (1) try DSN/env/config for org, (2) if found use \`getIssueInOrg(org, id)\` with region routing, (3) else fall back to unscoped \`getIssue(id)\`, (4) extract org from \`issue.permalink\` via \`parseSentryUrl\` as final fallback. \`parseSentryUrl\` handles path-based (\`/organizations/{org}/...\`) and subdomain-style URLs. \`matchSubdomainOrg()\` filters region subdomains by requiring slug length > 2. Self-hosted uses path-based only.
645645
646646
### Decision
647647
648648
<!-- lore:019cc2ef-9be5-722d-bc9f-b07a8197eeed -->
649-
* **All view subcommands should use \<target> \<id> positional pattern**: All \`\* view\` subcommands (\`event view\`, \`log view\`, \`trace view\`, etc.) should follow a consistent \`\<target> \<id>\` positional argument pattern where target is the optional \`org/project\` specifier. During migration, use opportunistic argument swapping with a stderr warning when the user provides args in the wrong order. This follows the CLI UX auto-correction pattern: safe when input is already invalid, no ambiguity, and warning goes to stderr.
649+
* **All view subcommands should use \<target> \<id> positional pattern**: All \`\* view\` subcommands should follow a consistent \`\<target> \<id>\` positional argument pattern where target is the optional \`org/project\` specifier. During migration, use opportunistic argument swapping with a stderr warning when args are in wrong order. This is an instance of the broader CLI UX auto-correction pattern: safe when input is already invalid, correction is unambiguous, warning goes to stderr. Normalize at command level, keep parsers pure. Model after \`gh\` CLI conventions.
650650
651651
<!-- lore:019c99d5-69f2-74eb-8c86-411f8512801d -->
652652
* **Raw markdown output for non-interactive terminals, rendered for TTY**: Markdown-first output pipeline: custom renderer in \`src/lib/formatters/markdown.ts\` walks \`marked\` tokens to produce ANSI-styled output. Commands build CommonMark using helpers (\`mdKvTable()\`, \`mdRow()\`, \`colorTag()\`, \`escapeMarkdownCell()\`, \`safeCodeSpan()\`) and pass through \`renderMarkdown()\`. \`isPlainOutput()\` precedence: \`SENTRY\_PLAIN\_OUTPUT\` > \`NO\_COLOR\` > \`FORCE\_COLOR\` > \`!isTTY\`. \`--json\` always outputs JSON. Colors defined in \`COLORS\` object in \`colors.ts\`. Tests run non-TTY so assertions match raw CommonMark; use \`stripAnsi()\` helper for rendered-mode assertions.
@@ -659,21 +659,21 @@ mock.module("./some-module", () => ({
659659
<!-- lore:019c8ab6-d119-7365-9359-98ecf464b704 -->
660660
* **@sentry/api SDK passes Request object to custom fetch — headers lost on Node.js**: @sentry/api SDK calls \`\_fetch(request)\` with no init object. In \`authenticatedFetch\`, \`init\` is undefined so \`prepareHeaders\` creates empty headers — on Node.js this strips Content-Type (HTTP 415). Fix: fall back to \`input.headers\` when \`init\` is undefined. Use \`unwrapPaginatedResult\` (not \`unwrapResult\`) to access the Response's Link header for pagination. \`per\_page\` is not in SDK types; cast query to pass it at runtime.
661661
662+
<!-- lore:019cc484-f0e1-7016-a851-177fb9ad2cc4 -->
663+
* **AGENTS.md must be excluded from markdown linters**: AGENTS.md is auto-managed by lore and uses \`\*\` list markers and long lines that violate typical remark-lint rules (unordered-list-marker-style, maximum-line-length). When a project uses remark with \`--frail\` (warnings become errors), AGENTS.md will fail CI. Fix: add \`AGENTS.md\` to \`.remarkignore\`. This applies to any lore-managed project with markdown linting.
664+
662665
<!-- lore:019c9e98-7af4-7e25-95f4-fc06f7abf564 -->
663666
* **Bun binary build requires SENTRY\_CLIENT\_ID env var**: The build script (\`script/bundle.ts\`) requires \`SENTRY\_CLIENT\_ID\` environment variable and exits with code 1 if missing. When building locally, use \`bun run --env-file=.env.local build\` or set the env var explicitly. The binary build (\`bun run build\`) also needs it. Without it you get: \`Error: SENTRY\_CLIENT\_ID environment variable is required.\`
664667
665668
<!-- lore:019cc40e-e56e-71e9-bc5d-545f97df732b -->
666669
* **Consola prompt cancel returns truthy Symbol, not false**: When a user cancels a \`consola\` / \`@clack/prompts\` confirmation prompt (Ctrl+C), the return value is \`Symbol(clack:cancel)\`, not \`false\`. Since Symbols are truthy in JavaScript, checking \`!confirmed\` will be \`false\` and the code falls through as if the user confirmed. Fix: use \`confirmed !== true\` (strict equality) instead of \`!confirmed\` to correctly handle cancel, false, and any other non-true values.
667670
668671
<!-- lore:019c9776-e3dd-7632-88b8-358a19506218 -->
669-
* **GitHub immutable releases prevent rolling nightly tag pattern**: getsentry/cli has immutable GitHub releases — assets can't be modified and tags can NEVER be reused. Nightly builds are published to GHCR with versioned tags like \`nightly-0.14.0-dev.1772661724\`, NOT to GitHub Releases or npm. \`versionExists()\` routes nightly versions to GHCR manifest checks. \`downloadNightlyToPath()\` accepts optional \`version\` param for pinned versioned tags vs rolling \`:nightly\`. \`fetchManifest()\` throws \`UpgradeError("network\_error")\` for both network failures and non-200 — callers must check message for HTTP 404/403 to distinguish "not found" from real errors. Craft with no \`preReleaseCommand\` silently skips \`bump-version.sh\` if only target is \`github\` — must explicitly set it.
672+
* **GitHub immutable releases prevent rolling nightly tag pattern**: getsentry/cli has immutable GitHub releases — assets can't be modified and tags can NEVER be reused. Nightly builds publish to GHCR with versioned tags like \`nightly-0.14.0-dev.1772661724\`, not GitHub Releases or npm. \`fetchManifest()\` throws \`UpgradeError("network\_error")\` for both network failures and non-200 — callers must check message for HTTP 404/403. Craft with no \`preReleaseCommand\` silently skips \`bump-version.sh\` if only target is \`github\`.
670673
671674
<!-- lore:019cb8c2-d7b5-780c-8a9f-d20001bc198f -->
672675
* **Install script: BSD sed and awk JSON parsing breaks OCI digest extraction**: The install script parses OCI manifests with awk (no jq). Key trap: BSD sed \`\n\` is literal, not newline. Fix: single awk pass tracking last-seen \`"digest"\`, printing when \`"org.opencontainers.image.title"\` matches target. The config digest (\`sha256:44136fa...\`) is a 2-byte \`{}\` blob — downloading it instead of the real binary causes \`gunzip: unexpected end of file\`.
673676
674-
<!-- lore:019cb963-cb63-722d-9365-b34336f4766d -->
675-
* **macOS SIGKILL on MAP\_SHARED mmap of signed Mach-O binaries**: Bun.mmap() always opens files with PROT\_WRITE/O\_RDWR regardless of the \`shared\` flag. This kills the process on the running binary: macOS sends SIGKILL (AMFI rejects writable mappings on signed Mach-O), Linux returns ETXTBSY. Fix: use \`new Uint8Array(await Bun.file(oldPath).arrayBuffer())\` unconditionally in bspatch.ts. Costs ~100MB heap but is the only cross-platform approach.
676-
677677
<!-- lore:019c969a-1c90-7041-88a8-4e4d9a51ebed -->
678678
* **Multiple mockFetch calls replace each other — use unified mocks for multi-endpoint tests**: Bun test mocking gotchas: (1) \`mockFetch()\` replaces \`globalThis.fetch\` — calling it twice replaces the first mock. Use a single unified fetch mock dispatching by URL pattern. (2) \`mock.module()\` pollutes the module registry for ALL subsequent test files. Tests using it must live in \`test/isolated/\` and run via \`test:isolated\`. This also causes \`delta-upgrade.test.ts\` to fail when run alongside \`test/isolated/delta-upgrade.test.ts\` — the isolated test's \`mock.module()\` replaces \`CLI\_VERSION\` for all subsequent files. (3) For \`Bun.spawn\`, use direct property assignment in \`beforeEach\`/\`afterEach\`.
679679
@@ -695,13 +695,8 @@ mock.module("./some-module", () => ({
695695
* **Shared pagination infrastructure: buildPaginationContextKey and parseCursorFlag**: List commands with cursor pagination use \`buildPaginationContextKey(type, identifier, flags)\` for composite context keys and \`parseCursorFlag(value)\` accepting \`"last"\` magic value. Critical: \`resolveCursor()\` must be called inside the \`org-all\` override closure, not before \`dispatchOrgScopedList\` — otherwise cursor validation errors fire before the correct mode-specific error.
696696
697697
<!-- lore:019cbd5f-ec35-7e2d-8386-6d3a67adf0cf -->
698-
* **Telemetry instrumentation pattern: withTracingSpan + captureException for handled errors**: For graceful-fallback operations, use \`withTracingSpan\` from \`src/lib/telemetry.ts\` for child spans and \`captureException\` from \`@sentry/bun\` (named import — Biome forbids namespace imports) with \`level: 'warning'\` for non-fatal errors. \`withTracingSpan\` uses \`onlyIfParent: true\` so it's a no-op without active transaction. When returning \`withTracingSpan(...)\` directly, drop \`async\` and use \`Promise.resolve(null)\` for early returns. User-visible fallbacks should use \`log.warn()\` not \`log.debug()\` — debug is invisible at default level. Also: several commands bypass telemetry by importing \`buildCommand\` from \`@stricli/core\` directly instead of \`../../lib/command.js\`. Affected: trace/list, trace/view, log/view, api.ts, help.ts.
698+
* **Telemetry instrumentation pattern: withTracingSpan + captureException for handled errors**: For graceful-fallback operations, use \`withTracingSpan\` from \`src/lib/telemetry.ts\` for child spans and \`captureException\` from \`@sentry/bun\` (named import — Biome forbids namespace imports) with \`level: 'warning'\` for non-fatal errors. \`withTracingSpan\` uses \`onlyIfParent: true\` no-op without active transaction. User-visible fallbacks use \`log.warn()\` not \`log.debug()\`. Several commands bypass telemetry by importing \`buildCommand\` from \`@stricli/core\` directly instead of \`../../lib/command.js\` (trace/list, trace/view, log/view, api.ts, help.ts).
699699
700700
<!-- lore:019cc43d-e651-7154-a88e-1309c4a2a2b6 -->
701701
* **Testing Stricli command func() bodies via spyOn mocking**: To unit-test a Stricli command's \`func()\` body: (1) \`const func = await cmd.loader()\`, (2) \`func.call(mockContext, flags, ...args)\` with mock \`stdout\`, \`stderr\`, \`cwd\`, \`setContext\`. (3) \`spyOn\` namespace imports to mock dependencies (e.g., \`spyOn(apiClient, 'getLogs')\`). The \`loader()\` return type union causes \`.call()\` LSP errors — these are false positives that pass \`tsc --noEmit\`. When API functions are renamed (e.g., \`getLog\`\`getLogs\`), update both spy target name AND mock return shape (single → array). Slug normalization (\`normalizeSlug\`) replaces underscores with dashes but does NOT lowercase — test assertions must match original casing (e.g., \`'CAM-82X'\` not \`'cam-82x'\`).
702-
703-
### Preference
704-
705-
<!-- lore:019c9700-0fc3-730c-82c3-a290d5ecc2ea -->
706-
* **CI scripts: prefer jq/sed over node -e for JSON manipulation**: Prefer \`jq\`/\`sed\`/\`awk\` over \`node -e\` for JSON manipulation in CI scripts. Example: \`jq -r .version package.json\` to read, \`jq --arg v "$NEW" '.version = $v' package.json > tmp && mv tmp package.json\` to write.
707702
<!-- End lore-managed section -->

src/commands/api.ts

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -626,6 +626,27 @@ export function parseDataBody(
626626
}
627627
}
628628

629+
/**
630+
* Parse a URL-encoded string into a query parameter map.
631+
* Duplicate keys are collected into arrays.
632+
*/
633+
function parseUrlEncodedParams(
634+
data: string
635+
): Record<string, string | string[]> {
636+
const params: Record<string, string | string[]> = {};
637+
for (const [key, value] of new URLSearchParams(data)) {
638+
const existing = params[key];
639+
if (existing !== undefined) {
640+
params[key] = Array.isArray(existing)
641+
? [...existing, value]
642+
: [existing, value];
643+
} else {
644+
params[key] = value;
645+
}
646+
}
647+
return params;
648+
}
649+
629650
/**
630651
* Convert `--data` content to query parameters for bodyless HTTP methods
631652
* (GET, HEAD, OPTIONS).
@@ -638,35 +659,23 @@ export function parseDataBody(
638659
*
639660
* @param data - Parsed output from {@link parseDataBody}
640661
* @returns Query parameter map suitable for `rawApiRequest`'s `params` option
641-
* @throws {ValidationError} When data is a JSON array (cannot be represented as query params)
662+
* @throws {ValidationError} When data is a JSON array or primitive (cannot be query params)
642663
* @internal Exported for testing
643664
*/
644665
export function dataToQueryParams(
645666
data: Record<string, unknown> | unknown[] | string
646667
): Record<string, string | string[]> {
647-
// String data: parse as URL-encoded query string
648668
if (typeof data === "string") {
649-
const params: Record<string, string | string[]> = {};
650-
const searchParams = new URLSearchParams(data);
651-
for (const [key, value] of searchParams) {
652-
const existing = params[key];
653-
if (existing !== undefined) {
654-
params[key] = Array.isArray(existing)
655-
? [...existing, value]
656-
: [existing, value];
657-
} else {
658-
params[key] = value;
659-
}
660-
}
661-
return params;
669+
return parseUrlEncodedParams(data);
662670
}
663671

664-
// JSON arrays can't be represented as query params
665-
if (Array.isArray(data)) {
672+
// JSON arrays and primitives (null, boolean, number) can't be query params.
673+
// parseDataBody uses `as` to narrow JSON.parse output, but primitives slip through.
674+
if (data === null || typeof data !== "object" || Array.isArray(data)) {
666675
throw new ValidationError(
667-
"Cannot use --data with a JSON array for GET requests. " +
668-
"JSON arrays cannot be converted to query parameters. " +
669-
"Use --method POST or pass individual values with --field/-F.",
676+
"Cannot use --data with a JSON primitive or array for GET requests. " +
677+
"Only JSON objects and URL-encoded strings can be converted to query parameters. " +
678+
"Use --method POST to send this data as a request body.",
670679
"data"
671680
);
672681
}

test/commands/api.test.ts

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1609,6 +1609,16 @@ describe("resolveBody", () => {
16091609
).rejects.toThrow(/cannot.*query parameters/i);
16101610
});
16111611

1612+
test("GET --data with JSON primitive throws ValidationError", async () => {
1613+
const stderr = createMockWriter();
1614+
await expect(
1615+
resolveBody({ method: "GET", data: "null" }, MOCK_STDIN, stderr)
1616+
).rejects.toThrow(ValidationError);
1617+
await expect(
1618+
resolveBody({ method: "GET", data: "42" }, MOCK_STDIN, stderr)
1619+
).rejects.toThrow(ValidationError);
1620+
});
1621+
16121622
test("POST --data still returns body (regression guard)", async () => {
16131623
const stderr = createMockWriter();
16141624
const result = await resolveBody(
@@ -1658,7 +1668,25 @@ describe("dataToQueryParams", () => {
16581668
test("throws on JSON array", () => {
16591669
expect(() => dataToQueryParams([1, 2, 3])).toThrow(ValidationError);
16601670
expect(() => dataToQueryParams([1, 2, 3])).toThrow(
1661-
/cannot.*JSON array.*query parameters/i
1671+
/cannot.*JSON primitive or array.*query parameters/i
16621672
);
16631673
});
1674+
1675+
test("throws on null", () => {
1676+
expect(() =>
1677+
dataToQueryParams(null as unknown as Record<string, unknown>)
1678+
).toThrow(ValidationError);
1679+
});
1680+
1681+
test("throws on boolean", () => {
1682+
expect(() =>
1683+
dataToQueryParams(true as unknown as Record<string, unknown>)
1684+
).toThrow(ValidationError);
1685+
});
1686+
1687+
test("throws on number", () => {
1688+
expect(() =>
1689+
dataToQueryParams(42 as unknown as Record<string, unknown>)
1690+
).toThrow(ValidationError);
1691+
});
16641692
});

0 commit comments

Comments
 (0)