fix: apply per-field Content-Type in multipart/form-data serialization#7863
Open
dfcz1314 wants to merge 5 commits intousebruno:mainfrom
Open
fix: apply per-field Content-Type in multipart/form-data serialization#7863dfcz1314 wants to merge 5 commits intousebruno:mainfrom
dfcz1314 wants to merge 5 commits intousebruno:mainfrom
Conversation
When a multipart form field has a contentType set (e.g. application/json;
charset=utf-8), the per-field Content-Type header was missing from the
serialized request body because the params array was passed directly to
axios without building a FormData instance.
Build FormData explicitly so each field's contentType is passed via
form.append(name, value, { contentType }) and emitted in the part header.
File fields preserve large-file streaming and filename inference.
Fixes: multipart requests where servers parse per-part Content-Type headers
(e.g. APIs expecting the "message" field to be application/json).
dfcz1314
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPrepare-request handlers (CLI & Electron) now build real multipart FormData via Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(0,128,255,0.5)
Participant Client
end
rect rgba(0,200,83,0.5)
Participant RequestPreparer
end
rect rgba(255,193,7,0.5)
Participant FileSystem
end
rect rgba(156,39,176,0.5)
Participant FormDataLib
end
rect rgba(244,67,54,0.5)
Participant Axios
end
Client->>RequestPreparer: prepare multipart request
RequestPreparer->>RequestPreparer: filter enabled params
RequestPreparer->>FileSystem: resolve paths & stat sizes
alt file is large
RequestPreparer->>FileSystem: createReadStream(file)
else small file
RequestPreparer->>FileSystem: readFileSync(file)
end
RequestPreparer->>FormDataLib: append parts (files + fields)
FormDataLib-->>RequestPreparer: form object + getHeaders()
RequestPreparer->>Axios: merge headers + axiosRequest.data = form
Client->>Axios: send request
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/bruno-cli/src/runner/prepare-request.js (1)
405-432: Tiny nit — duplicated multipart logic across CLI and Electron is becoming non-trivial.This block is now ~25 lines virtually identical to
packages/bruno-electron/src/ipc/network/prepare-request.js:475-504. Not a blocker for this PR (per the "rule of three" we're at exactly two callsites), but if we ever touch this a third time, extracting a small helper likebuildMultipartForm(enabledParams, { collectionPath, threshold })into a shared util would be very welcome.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/src/runner/prepare-request.js` around lines 405 - 432, Extract the multipart form building logic into a shared helper (e.g., buildMultipartForm) and replace the duplicated blocks in prepare-request.js with a call to that helper; the helper should accept enabledParams and an options object (e.g., { collectionPath, threshold: STREAMING_FILE_SIZE_THRESHOLD }) and perform the same steps currently done here (filtering enabled params, using isLargeFile, fs.createReadStream vs fs.readFileSync, setting filename/contentType opts, appending fields/files to a FormData instance, and returning { form, headers } so callers can set axiosRequest.data and Object.assign(axiosRequest.headers, headers)). Ensure the helper uses the existing symbols isLargeFile and STREAMING_FILE_SIZE_THRESHOLD and preserves behavior for file vs non-file params and opts merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-cli/src/runner/prepare-request.js`:
- Around line 409-429: The multipart file-param branch in prepareRequest
currently calls isLargeFile and fs.readFileSync without error handling and
doesn't resolve relative paths against collectionPath, causing crashes and wrong
resolution; update the file-handling block (the each loop that checks param.type
=== 'file') to first resolve filePath relative to collectionPath like the mode
=== 'file' branch does, wrap the isLargeFile/readFileSync/fs.createReadStream
operations in a try/catch, and on error log the failure and skip appending that
file (mirroring the graceful logging/skipping behavior used in the mode ===
'file' branch).
In `@packages/bruno-electron/src/ipc/network/prepare-request.js`:
- Around line 500-503: The code conditionally skips merging multipart headers
when contentTypeDefined is true, causing missing boundary parameters; always
merge form.getHeaders() into axiosRequest.headers before setting
axiosRequest.data so the Content-Type header includes the correct boundary even
if a user-provided Content-Type exists—update the logic around
contentTypeDefined/axiosRequest.headers to Object.assign(axiosRequest.headers,
form.getHeaders()) unconditionally, then set axiosRequest.data = form.
- Around line 482-494: The loop that calls isLargeFile and fs.readFileSync
inside prepareRequest's multipart branch can throw and isn't mirroring the safe
behavior in the mode === 'file' branch: resolve relative filePath against
collectionPath, wrap each file handling (the isLargeFile check,
createReadStream/readFileSync and form.append) in a try/catch, and on error log
the problem and continue (do not throw); use the same logging approach as the
mode === 'file' branch so bad or missing paths (isLargeFile throwing Error("File
... is not a file")) are skipped instead of aborting the whole prepareRequest.
Ensure you reference the same STREAMING_FILE_SIZE_THRESHOLD, isLargeFile,
form.append and collectionPath logic while implementing this change.
---
Nitpick comments:
In `@packages/bruno-cli/src/runner/prepare-request.js`:
- Around line 405-432: Extract the multipart form building logic into a shared
helper (e.g., buildMultipartForm) and replace the duplicated blocks in
prepare-request.js with a call to that helper; the helper should accept
enabledParams and an options object (e.g., { collectionPath, threshold:
STREAMING_FILE_SIZE_THRESHOLD }) and perform the same steps currently done here
(filtering enabled params, using isLargeFile, fs.createReadStream vs
fs.readFileSync, setting filename/contentType opts, appending fields/files to a
FormData instance, and returning { form, headers } so callers can set
axiosRequest.data and Object.assign(axiosRequest.headers, headers)). Ensure the
helper uses the existing symbols isLargeFile and STREAMING_FILE_SIZE_THRESHOLD
and preserves behavior for file vs non-file params and opts merging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c0c4803-20d6-43a6-b5da-920073813492
📒 Files selected for processing (2)
packages/bruno-cli/src/runner/prepare-request.jspackages/bruno-electron/src/ipc/network/prepare-request.js
…params - Resolve filePath against collectionPath when not absolute, mirroring the mode === 'file' branch behavior in both CLI and Electron - Wrap isLargeFile/readFileSync/createReadStream in try/catch and log errors instead of crashing, so bad or missing paths are skipped - Always merge form.getHeaders() unconditionally in Electron so the Content-Type boundary is present even when contentTypeDefined is true
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-electron/src/ipc/network/prepare-request.js (1)
475-509: Consider consolidating multipart construction into a shared helper. 🧰This block is now a near-duplicate of
packages/bruno-cli/src/runner/prepare-request.js(lines 405–439) and overlaps significantly with the existingcreateFormDatainpackages/bruno-electron/src/utils/form-data.js(which is missing the streaming threshold + try/catch parity that this PR adds). Extracting a singlebuildMultipartFormData(params, collectionPath)helper would prevent these three sites from drifting next time someone tweaks streaming, path resolution, or per-part headers.Not blocking — happy to defer if you'd rather keep the diff tight. As per coding guidelines, "Avoid abstractions unless the exact same code is being used in more than 3 places" — this is right at that threshold.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/ipc/network/prepare-request.js` around lines 475 - 509, The multipart form construction in this block duplicates logic in createFormData and the CLI prepare-request; extract a shared helper function buildMultipartFormData(params, collectionPath, streamThreshold) that encapsulates the file path resolution, streaming threshold check (isLargeFile with STREAMING_FILE_SIZE_THRESHOLD), try/catch around file reads, per-part contentType/filename opts, and returns { form, headers } (or sets headers via form.getHeaders()); replace the current inline logic in prepare-request.js and the CLI prepare-request and update createFormData to call this new helper so all three sites use the same implementation and parity for streaming and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bruno-electron/src/ipc/network/prepare-request.js`:
- Around line 475-509: The multipart form construction in this block duplicates
logic in createFormData and the CLI prepare-request; extract a shared helper
function buildMultipartFormData(params, collectionPath, streamThreshold) that
encapsulates the file path resolution, streaming threshold check (isLargeFile
with STREAMING_FILE_SIZE_THRESHOLD), try/catch around file reads, per-part
contentType/filename opts, and returns { form, headers } (or sets headers via
form.getHeaders()); replace the current inline logic in prepare-request.js and
the CLI prepare-request and update createFormData to call this new helper so all
three sites use the same implementation and parity for streaming and error
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de10777f-911e-4092-80f2-cea87a899048
📒 Files selected for processing (2)
packages/bruno-cli/src/runner/prepare-request.jspackages/bruno-electron/src/ipc/network/prepare-request.js
…Data helper Extract shared buildMultipartFormData logic from both prepare-request.js files into the existing createFormData utility in each package's form-data.js. Changes per package (cli + electron): - form-data.js: add optional streamThreshold param, isLargeFile streaming decision, try/catch on file reads, and handle single-string file values in addition to arrays - prepare-request.js: replace 30-line inline FormData block with a single createFormData(enabledParams, collectionPath, STREAMING_FILE_SIZE_THRESHOLD) call, eliminating duplication across CLI, Electron, and the existing createFormData sites (axios-instance.js, index.js) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-electron/src/utils/form-data.js (1)
64-104:⚠️ Potential issue | 🟡 MinorUpdate
formatMultipartData()to render repeated parts and per-partContent-Typeheaders.The formatter is used in request preview (sent to the UI via IPC at
network/index.js:859and1535). However,createFormData()now appends repeated parts for array values and can include per-partContent-Typeheaders, whileformatMultipartData()still collapses arrays and omits per-part headers. Users will see a preview that doesn't match the actual payload sent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/utils/form-data.js` around lines 64 - 104, formatMultipartData currently collapses array fields and omits per-part Content-Type, producing a preview that doesn't match createFormData's actual payload; update formatMultipartData to mirror createFormData's behavior by rendering repeated parts for array values (i.e., emit one multipart part per array element rather than joining) and include each part's Content-Type when a datum.contentType is present, also showing per-file filename/filename options for file parts so the preview matches the FormData produced by createFormData.
🧹 Nitpick comments (1)
packages/bruno-cli/src/utils/form-data.js (1)
9-49: Add a regression test for per-partContent-Type.This helper now drives both the initial multipart request path and multipart recreation after retries. Please lock this down with a test that asserts a text field with
contentType: 'application/json; charset=utf-8'produces a per-partContent-Typeheader, so this fix does not regress quietly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/src/utils/form-data.js` around lines 9 - 49, Add a regression test that calls createFormData with a text field object containing contentType: 'application/json; charset=utf-8' (use a sample name/value and collectionPath) and then inspects the generated multipart payload to assert the per-part header "Content-Type: application/json; charset=utf-8" appears for that part; locate createFormData in the utils/form-data.js module and exercise it (use FormData methods like getBuffer()/getBoundary() or getHeaders() to build/parse the multipart body) and fail the test if the per-part Content-Type is missing or different so the multipart recreation for retries remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/bruno-electron/src/utils/form-data.js`:
- Around line 64-104: formatMultipartData currently collapses array fields and
omits per-part Content-Type, producing a preview that doesn't match
createFormData's actual payload; update formatMultipartData to mirror
createFormData's behavior by rendering repeated parts for array values (i.e.,
emit one multipart part per array element rather than joining) and include each
part's Content-Type when a datum.contentType is present, also showing per-file
filename/filename options for file parts so the preview matches the FormData
produced by createFormData.
---
Nitpick comments:
In `@packages/bruno-cli/src/utils/form-data.js`:
- Around line 9-49: Add a regression test that calls createFormData with a text
field object containing contentType: 'application/json; charset=utf-8' (use a
sample name/value and collectionPath) and then inspects the generated multipart
payload to assert the per-part header "Content-Type: application/json;
charset=utf-8" appears for that part; locate createFormData in the
utils/form-data.js module and exercise it (use FormData methods like
getBuffer()/getBoundary() or getHeaders() to build/parse the multipart body) and
fail the test if the per-part Content-Type is missing or different so the
multipart recreation for retries remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f287492-852c-41a1-97e3-83c9816cf061
📒 Files selected for processing (4)
packages/bruno-cli/src/runner/prepare-request.jspackages/bruno-cli/src/utils/form-data.jspackages/bruno-electron/src/ipc/network/prepare-request.jspackages/bruno-electron/src/utils/form-data.js
- formatMultipartData: render each array value as a separate part (was incorrectly joining with ', '), include per-part Content-Type when datum.contentType is set, and fix duplicate boundary markers for file fields — preview now matches the actual multipart payload sent by createFormData - electron prepare-request: preserve _originalMultipartData and collectionPath on axiosRequest so the UI preview (parseDataFromRequest) and FormData recreation on redirects (axios-instance) continue to work now that prepare-request builds FormData directly - Add form-data.spec.js regression tests to both packages covering per-part Content-Type, array expansion, and single-string file paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/bruno-cli/tests/utils/form-data.spec.js (1)
13-13: Use OS-aware temp directory instead of hardcoded/tmpHardcoding
'/tmp'is not cross-platform-safe. Useos.tmpdir()once and reuse it across tests.Proposed patch
const { createFormData } = require('../../src/utils/form-data'); +const os = require('os'); + +const collectionPath = os.tmpdir(); @@ - const form = createFormData(data, '/tmp'); + const form = createFormData(data, collectionPath); @@ - const form = createFormData(data, '/tmp'); + const form = createFormData(data, collectionPath); @@ - const form = createFormData(data, '/tmp'); + const form = createFormData(data, collectionPath); @@ - expect(() => createFormData(data, '/tmp')).not.toThrow(); + expect(() => createFormData(data, collectionPath)).not.toThrow();As per coding guidelines: "Bruno is a cross-platform Electron desktop app... Use
os.tmpdir()instead of hardcoding/tmp."Also applies to: 20-20, 29-29, 40-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/tests/utils/form-data.spec.js` at line 13, Tests in form-data.spec.js hardcode '/tmp' when calling createFormData (and in other assertions on lines noted), which is not cross-platform; import Node's os module, call os.tmpdir() once (e.g., const tmpDir = os.tmpdir()) at the top of the spec, replace every hardcoded '/tmp' argument and assertion with tmpDir, and reuse tmpDir across the createFormData(...) calls and related expectations so the tests work on Windows and other environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-cli/tests/utils/form-data.spec.js`:
- Around line 18-25: The test "omits Content-Type header for text field without
contentType" doesn't assert the absence of a Content-Type header, so add a
negative assertion when using createFormData(data, '/tmp') and form.getBuffer():
verify the serialized buffer contains the field name and value (e.g.,
'name="field"' and 'hello') and also assert it does NOT contain a
'Content-Type:' line for that field; use createFormData and form.getBuffer() as
the reference symbols to locate the test and ensure the negative assertion
checks the raw multipart section for the text field.
- Around line 35-42: The test "handles single-string file value (not wrapped in
array)" currently only asserts createFormData doesn't throw but doesn't verify
fallback logging; update the test to assert that console.error was called when
the file read fails by using the existing jest.spyOn(console, 'error') mock and
replacing expect(...).not.toThrow() with both the no-throw assertion and an
expectation like expect(consoleSpy).toHaveBeenCalled(); keep the mockRestore()
call to restore console.error; target the createFormData invocation and the
consoleSpy variable to implement this strengthened degraded-path assertion.
In `@packages/bruno-electron/tests/utils/form-data.spec.js`:
- Around line 48-53: Update the test for formatMultipartData to assert
conditional emission of per-part Content-Type by creating a mixed payload with
two parts (e.g., one text field that includes contentType and one text field
that does not), call formatMultipartData(boundary), then assert the output
contains the specific Content-Type string for the field that set contentType and
does NOT contain any Content-Type header for the other field; use unique
identifiers (field names like 'messageWithType' and 'messageWithoutType') so
assertions target the correct part.
---
Nitpick comments:
In `@packages/bruno-cli/tests/utils/form-data.spec.js`:
- Line 13: Tests in form-data.spec.js hardcode '/tmp' when calling
createFormData (and in other assertions on lines noted), which is not
cross-platform; import Node's os module, call os.tmpdir() once (e.g., const
tmpDir = os.tmpdir()) at the top of the spec, replace every hardcoded '/tmp'
argument and assertion with tmpDir, and reuse tmpDir across the
createFormData(...) calls and related expectations so the tests work on Windows
and other environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9484c2bc-e57f-4c9a-8b0a-a9fdd0b0369d
📒 Files selected for processing (4)
packages/bruno-cli/tests/utils/form-data.spec.jspackages/bruno-electron/src/ipc/network/prepare-request.jspackages/bruno-electron/src/utils/form-data.jspackages/bruno-electron/tests/utils/form-data.spec.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-electron/src/ipc/network/prepare-request.js
- packages/bruno-electron/src/utils/form-data.js
- cli form-data.spec.js: replace hardcoded '/tmp' with os.tmpdir() for cross-platform safety, add negative Content-Type assertion in the 'omits Content-Type' test, and assert consoleSpy.toHaveBeenCalled() in the degraded-path file test - electron form-data.spec.js: add mixed-payload test to verify Content-Type is emitted only for the field that defines contentType and not globally Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When using
body:multipart-formwith a text field that hascontentTypeset (e.g.application/json; charset=utf-8), the per-fieldContent-Typeheader is missing from the actual multipart body sentto the server.
Servers that inspect individual part headers to determine the encoding of that field (common in APIs
that accept JSON in a multipart field alongside binary files) will fail to parse the field and return
errors.
Actual part in the sent body:
Content-Disposition: form-data; name="message"
{"key":"value"}
Expected part in the sent body:
Content-Disposition: form-data; name="message"
Content-Type: application/json; charset=utf-8
{"key":"value"}
Root Cause
In both
bruno-cliandbruno-electronprepare-request.js, the filteredmultipartFormparamsarray was passed directly to
axiosRequest.data. axios/form-data does not read thecontentTypeproperty from plain param objects when serializing, so per-part
Content-Typeheaders are neveremitted.
Fix
Build a
FormDatainstance explicitly and useform.append(name, value, { contentType })for eachfield.
form-data(already a dependency at4.0.4) supports per-part content type via the thirdoptionsargument. File fields preserve the existing large-file streaming and filename inferencebehavior.
Verification
Verified with a local echo server capturing the raw multipart body:
Before:
Content-Disposition: form-data; name="message"
{"MessageType":4,...}
After:
Content-Disposition: form-data; name="message"
Content-Type: application/json; charset=utf-8
{"MessageType":4,...}
Summary by CodeRabbit