Repository navigation
fix: validate --body before confirmation prompts - #504
Conversation
Confirm-gated commands parsed --body only after authenticating and prompting, so a malformed or oversized body failed after the user had accepted a destructive prompt. #498 hoisted only the presence check; the parse now happens before auth and confirmation too. Also guard the --body branch with `!== undefined`, matching the dry run and the required-body check, so an empty JSON --body is rejected instead of skipped; show --file in raw-body dry runs; and import parseBody only for JSON bodies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
8ab6e389.
This review, of 2cd369f2
💬 Approved with comments — The fix is correct and consistent across the generator and the regenerated commands. The only note is an INFO: the raw-body early-parse branch has no test.
Findings: 1 info (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 info
The generator change and the regenerated output are correct. The raw-body early-parse branch has no test.
Recommendation: 💬 Optional Nits
- [INFO] The raw-body branch of
preparsedBodyhas no test and no generated example. (packages/cli/generator/plugin/emit/handler.ts:158)
View full review
- Generated output: it falls into four uniform patterns:
- the guard change to
argv.body !== undefined - the early
parsedBodyin confirm-gated leaves, reused by the bypass - the
parseBodyimport dropped from raw-body leaves - the
--filedry-run output
- the guard change to
- Ordering: in the confirm-gated leaves, the body is parsed after the dry-run and required-body checks, and before the client, auth and
confirmDelete. - Raw-body early-parse branch: the non-JSON branch of
preparsedBody(handler.ts:156-158) emitsresolveFileToken(argv.body, 'body', 'binary')unconditionally. No generated leaf uses it today. It is safe, becauseresolveFileTokenpassesundefinedthrough and the bypass re-checksargv.body !== undefined. Still, no test covers it.
✅ All other checks passed
🚀 Release: ✅ LGTM
Low risk. The changeset accompanies the user-facing fix. The tag and prior-release history check was not performed because that history is not available here.
🔒 Security: ✅ LGTM
Confirmation and the --force bypass are unchanged. The body is parsed earlier with the same input. The stricter guard closes a silent no-op on --body "".
📖 AGENTS.md: ✅ LGTM
The generated output comes from a generator change, not hand edits. A changeset is included.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: 2cd369f2
commit: |
|
| mean | stddev | min | max | |
|---|---|---|---|---|
| PR | 280.7 ms | 8.3 ms | 272.9 ms | 298.9 ms |
| base | 278.6 ms | 4.3 ms | 273.8 ms | 287.2 ms |
Δ (PR vs base): ↑ +2.1 ms (+0.8%)
Measured with hyperfine on ubuntu-latest (3 warmup runs, 20 timed runs). CI numbers carry ±a few ms of runner jitter; treat small deltas as noise.
|
LGTM! |
Generalise the early parse from confirm-gated commands to every command with --body, so a bad body or unreadable @file fails before auth and account lookup too. Multipart-only commands keep appending --body to the form as given, so they are excluded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Superseded by the review of
01b34012.
This review, of 8ab6e389
✅ Approved — The early body validation is correct and applied the same way across the generator and the regenerated commands. The new tests cover the raw-body branch noted in the last review, and there are no open findings.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset documents the fix as a patch.
🔒 Security: ✅ LGTM
The confirmation prompt and the --force bypass are unchanged. The same input is validated, only earlier. The stricter !== undefined guard closes the silent no-op on --body "".
📖 AGENTS.md: ✅ LGTM
The generated output comes from a generator change, not hand edits. A changeset is included.
📋 Code Quality: ✅ LGTM
Generator. preparsedBody runs after the dry-run and required-body checks and before auth and deleteConfirm. multipartTakesBody leaves multipart-only operations out. parseBody is imported only for JSON bodies.
Generated leaves. They match the generator. parseBody arguments such as item limits and redaction paths carry over unchanged.
Tests. required-raw-bodies.test.ts now covers the raw early-parse branch.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
No documentation issues in the changed files. The changeset describes the user-facing fix.
Reviewed commit: 8ab6e389
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Review: 1 findings. 🤖 I've done a thorough review of your PR. Moves body validation before authentication and confirmation.
|
|
LGTM! |
There was a problem hiding this comment.
Superseded by the review of
ea24beb2.
This review, of 01b34012
✅ Approved — No findings. --body is now validated before auth and before confirmation prompts. Precedence between --body, --file and multipart inputs is unchanged, and the regenerated commands match the generator.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset marks this fix as a patch release.
🔒 Security: ✅ LGTM
The confirmation prompt and the --force bypass are unchanged. The same input is validated as before, only earlier. The !== undefined guard stops --body "" from passing the required-body check and then exiting without doing anything.
📖 AGENTS.md: ✅ LGTM
The generated output comes from the generator change, not from hand edits, and a changeset is included.
📋 Code Quality: ✅ LGTM
- Ordering.
preparsedBodyruns after the dry-run and required-body checks and before auth anddeleteConfirm. - Precedence. The new
earlierBranchReadsguard skips the early parse when a multipart field or--filewill be used instead of--body. - Parse arguments.
parseBodyarguments such as redaction paths and item limits carry over unchanged. - Raw bodies. These resolve once with
resolveFileToken, and the dry run now shows--file. - Tests. Both oversized/malformed/empty bodies and
--fileprecedence are covered.
⚡ Performance: ✅ LGTM
No performance concerns. The body is parsed once.
📝 Documentation: ✅ LGTM
The changeset describes the user-facing fix.
Reviewed commit: 01b34012
There was a problem hiding this comment.
✅ Approved — No findings. --body is now validated before auth and before confirmation prompts. Precedence between --body, --file and multipart inputs is unchanged, and the regenerated commands match the generator.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset marks this fix as a patch release.
🔒 Security: ✅ LGTM
The confirmation prompt and the --force bypass are unchanged. The same input is validated as before, only earlier. The !== undefined guard stops --body "" from passing the required-body check and then exiting without doing anything.
📖 AGENTS.md: ✅ LGTM
The generated output comes from the generator change, not from hand edits, and a changeset is included.
📋 Code Quality: ✅ LGTM
- Ordering.
preparsedBodyruns after the dry-run and required-body checks and before auth anddeleteConfirm. - Precedence. The new
earlierBranchReadsguard skips the early parse when a multipart field or--filewill be used instead of--body. - Parse arguments.
parseBodyarguments such as redaction paths and item limits carry over unchanged. - Raw bodies. These resolve once with
resolveFileToken, and the dry run now shows--file. - Tests. Both oversized/malformed/empty bodies and
--fileprecedence are covered.
⚡ Performance: ✅ LGTM
No performance concerns. The body is parsed once.
📝 Documentation: ✅ LGTM
The changeset describes the user-facing fix.
Reviewed commit: ea24beb2 — same changes as 01b34012, so this is that commit's review (no new review was run; line numbers are as of 01b34012)
|
LGTM! |
Follow-up to #498, from the review of #477.
Generated commands parsed
--bodyonly after authenticating, and forconfirm-gated commands only after the prompt. A malformed or oversized body,
or an unreadable
@file, therefore failed after an account lookup or afterthe user had accepted a destructive prompt. On main this affected 51
confirm-gated leaves, including
kv bulk delete,cache purge,r2 objects bulk-delete,queues purgeandresource-sharing recipients|excluded-recipients update.Changes in
generator/plugin/emit/handler.ts:--bodynow parses (JSON)or resolves (raw) it before auth and any prompt, and the bypass reuses the
result (1043 leaves). Multipart-only commands are excluded because they
append
--bodyto the form as given.--body. The bypass guard wasif (argv.body), so--body ""passed the required-body check and then exited 0 without doinganything. It is now
!== undefined, matching the dry run, which alreadyrejected it. Raw bodies keep sending an empty body, as fix: enforce required plain-text request bodies #499 intended.
--file. Raw-body dry runs now show the--filepathinstead of
body: undefined.parseBodyimport. It is imported only for JSON bodies, whichremoves the unused import from 29 raw-body leaves (
_generated/**isn'tlinted).
On the regenerated output, no confirm-gated leaf parses after the prompt and
no leaf has an unused
parseBodyimport. Most of the generated diff is theone-line guard change.
Tests: body-limits covers oversized, malformed and empty bodies, each rejected
before auth and confirmation, for PUT/POST/PATCH/DELETE, plus a command with
no confirmation. required-raw-bodies covers an unreadable
@filerejectedbefore auth, the
--filedry run, and the absent import.🤖 Generated with Claude Code