Repository navigation
fix: enforce required plain-text request bodies - #499
petebacondarwin wants to merge 3 commits into
Conversation
commit: |
|
| mean | stddev | min | max | |
|---|---|---|---|---|
| PR | 284.2 ms | 4.6 ms | 274.4 ms | 290.9 ms |
| base | 283.8 ms | 5.5 ms | 275.0 ms | 295.2 ms |
Δ (PR vs base): — +0.4 ms (+0.1%)
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.
![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
6b55e475.
This review, of 8c2435de
💬 Approved with comments — The raw-body requirement fix is correct and regenerated consistently. One WARNING: multipart commands still let an explicit empty --file through without validating it.
Findings: 1 warning
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #499.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] The multipart emitter still checks `argv.file` by truthiness. An explicit `--file ""` enters the multipart branch (the outer guard uses `!== undefined`) but skips `readFileForFlag`, so the command sends a FormData request with no payload field instead of rejecting the empty path. Raw uploads now reject it; use `argv.file !== undefined` here as well and regenerate.
- **File:** packages/cli/generator/plugin/emit/handler.ts:423
📋 Code Quality: ⚠️ 1 warning
The raw-body fix is coherent; the multipart branch still skips validation of an explicit empty --file.
Recommendation: 📝 Minor Comments
- [WARNING] The multipart emitter still checks
argv.fileby truthiness. An explicit--file ""enters the multipart branch (the outer guard uses!== undefined) but skipsreadFileForFlag, so the command sends a FormData request with no payload field instead of rejecting the empty path. Raw uploads now reject it; useargv.file !== undefinedhere as well and regenerate. (packages/cli/generator/plugin/emit/handler.ts:423)
View full review
Multipart --file "" is not validated (packages/cli/generator/plugin/emit/handler.ts:423; generated output such as packages/cli/src/commands/_generated/dns/records/import.ts:94)
This pull request changes fileUpload() to if (argv.file !== undefined), so readFileForFlag now rejects --file "" on raw uploads. The multipart emitter's outer guard already treats argv.file !== undefined as input, so --file "" enters that branch. Its inner if (argv.file) { is falsy, though, and --body is undefined, so the command sends a FormData request with no payload field instead of failing with the invalid-file error. That falls short of the stated goal of validating explicitly supplied file paths, and it leaves raw and multipart commands handling the shared --file flag differently.
Fix: emit if (argv.file !== undefined) { in the multipart branch, regenerate the commands, and add a multipart --file "" case to required-raw-bodies.test.ts.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: low. The patch changeset is appropriate for this fix.
🔒 Security: ✅ LGTM
No security issues. Missing required input is now rejected before authentication.
📖 AGENTS.md: ✅ LGTM
The generated output is regenerated rather than hand-edited, and the pull request includes a patch changeset, as the repository rules require.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: 8c2435de
|
LGTM |
|
No new concerns since the last review |
There was a problem hiding this comment.
Superseded by the review of
257de795.
This review, of 6b55e475
💬 Approved with comments — The raw-body requirement fix is correct and was regenerated consistently. One WARNING is still open: multipart commands let an explicit empty --file through without validating it.
Findings: 1 warning
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #499.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] Multipart emitter still checks `argv.file` by truthiness: an explicit `--file ""` enters the multipart branch but skips `readFileForFlag`, so it sends a FormData request with no payload instead of rejecting the empty path.
- **File:** packages/cli/generator/plugin/emit/handler.ts:423
📋 Code Quality: ⚠️ 1 warning
One open warning: multipart commands do not validate an explicit empty --file "".
Recommendation: 📝 Minor Comments
- [WARNING] Multipart emitter still checks
argv.fileby truthiness: an explicit--file ""enters the multipart branch but skipsreadFileForFlag, so it sends a FormData request with no payload instead of rejecting the empty path. (packages/cli/generator/plugin/emit/handler.ts:423)
View full review
Multipart --file "" is not validated (packages/cli/generator/plugin/emit/handler.ts:423; generated output such as packages/cli/src/commands/_generated/dns/records/import.ts:95)
- This pull request changes
fileUpload()toif (argv.file !== undefined), so raw uploads now reject--file ""throughreadFileForFlag. - The multipart emitter's outer guard (
handler.ts:410-411) already treatsargv.file !== undefinedas input, so--file ""enters the multipart branch. - The inner check at
handler.ts:423is stillif (argv.file) {. An empty string is falsy, so file validation is skipped. With--bodyunset, the command then sends a FormData request with no payload field.
This misses the stated goal of validating explicitly supplied file paths, and raw and multipart commands now treat the shared --file flag differently.
Fix: emit if (argv.file !== undefined) { at handler.ts:423, regenerate the commands, and add a multipart --file "" case to required-raw-bodies.test.ts.
The updated snapshot in kv/key.test.ts matches the generated kv/keys/put.ts error message.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. A patch changeset is appropriate for this fix.
🔒 Security: ✅ LGTM
No security issues found.
📖 AGENTS.md: ✅ LGTM
The pull request follows the repository rules: generated commands are regenerated rather than hand-edited, and a patch changeset is included.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: 6b55e475
There was a problem hiding this comment.
✅ Approved — The raw-body requirement fix is correct and consistently regenerated, and the earlier multipart --file "" warning is fixed and covered by a regression test. 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. A patch changeset is appropriate for this fix.
🔒 Security: ✅ LGTM
No security issues found.
📖 AGENTS.md: ✅ LGTM
The pull request follows the repository rules: generated commands are regenerated rather than hand-edited, and a patch changeset is included.
📋 Code Quality: ✅ LGTM
The multipart emitter at packages/cli/generator/plugin/emit/handler.ts:423 now emits if (argv.file !== undefined), so --file "" is validated through readFileForFlag the same way as on the raw upload path. No generated command still uses a truthiness check on argv.file. The regression test in required-raw-bodies.test.ts checks that the command rejects the empty path and that no request is sent. Sampled regenerated commands (analytics-engine/sql/query.ts, logs/query.ts) no longer have the bodyless typed fallback.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
No documentation issues.
Reviewed commit: 257de795
|
LGTM! |
Required plain-text request bodies could lose their input requirement during generation and fall through to a bodyless typed request. This is the regression reported for
cf zaraz workflow updatein the review on #477.Preserve the requirement for non-JSON bodies and reject missing
--body/--fileinput before authentication. Keep valid empty multipart requests supported, send explicitly supplied empty raw bodies, and validate explicitly supplied file paths. Regenerate the tracked commands with the existing OpenAPI pin and include a patch changeset. Regenerating #477 with this plugin restores its workflow input guard.Validation:
pnpm checkpasses.