margelo / margelo/react-native-nitro-fetch

Native multipart serializers do not escape names, filenames or validate MIME types (#229 revert)

Open
#238 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
997
Forks
50
Avg merge
1d 20h
Merged PRs (30d)
14

Description

Before submitting a new issue
  • I tested using the latest version of the library, as the bug might be already fixed.
  • I checked for possible duplicate issues, with possible answers.
Bug summary

Native multipart serialization interpolates name, filename and the file MIME type straight into part headers with no escaping and no validation, on both platforms. A quote breaks out of the quoted string; a CR/LF injects arbitrary extra part headers.

This is the defect #224 reported. It was fixed in #229, but #229 was reverted in 9408579 and the revert is on main and in v1.7.0, so the code is back to its pre-#229 shape. #224 is still marked closed.

packages/react-native-nitro-fetch/ios/HybridNitroFetchClient.swift:

let fileName = part.fileName ?? "file"
let mimeType = part.mimeType ?? "application/octet-stream"
body.append("Content-Disposition: form-data; name=\"\(part.name)\"; filename=\"\(fileName)\"\(crlf)".data(using: .utf8)!)
body.append("Content-Type: \(mimeType)\(crlf)\(crlf)".data(using: .utf8)!)

packages/react-native-nitro-fetch/android/.../HybridNitroFetchClient.kt is the same shape (lines 368–370). escapeMultipartParameter and normalizeMultipartLineBreaks appear 0 times in either file.

What is missing, relative to the WHATWG multipart/form-data algorithm:

on main today
" in field name / filename → %22 not escaped
CR/LF in field name / filename → %0D / %0A not escaped
CR/LF in file MIME type not rejected
field names and string values normalized to CRLF not normalized
Library version

main @ 82b7fd4, and v1.7.0.

Environment info

Affects the native serializers on both iOS (URLSession) and Android (Cronet). Not platform- or RN-version-specific — it is the shared buildMultipartBody path.

Steps to reproduce
  1. fd.append('photo', { uri, type: 'image/jpeg\r\nX-Injected: 1', name: 'test.jpg' })
  2. POST it with nitroFetch.
  3. The receiving server parses a clean upload carrying an extra X-Injected: 1 part header. A " in name or the filename instead corrupts the Content-Disposition line, and receivers drop the part.

Exposure depends on the app passing user- or server-controlled values into a FormData part's name, name (filename) or type, which is a normal thing to do for user-chosen file uploads.

Reproducible example repository

Not needed — visible by inspection at the lines above. #229 carried five harness cases covering exactly these shapes; the revert removed them, so nothing currently tests it.

Note for whoever picks this up

A straight revert-of-the-revert is not sufficient on iOS. #229's guard was:

guard !mimeType.contains("\r"), !mimeType.contains("\n") else { throw ... }

String.contains(_:) compares grapheme clusters and CRLF is a single cluster, so both checks are false for "image/jpeg\r\nX-Injected: 1" — the one payload shape that matters. iOS was therefore still unprotected for CRLF during the whole window #229 was on main; only a lone CR or LF was caught. Android was fine, because contains('\r') there takes a Char and is a plain UTF-16 scan.

Scanning unicode scalars catches all three forms:

guard !mimeType.unicodeScalars.contains(where: { $0 == "\r" || $0 == "\n" }) else { throw ... }

Separately, #229's harness case FormData filename with a quote survives the round trip asserted that the server hands back na"me.jpg. busboy 1.6.0 does not percent-decode filenames, so both platforms return the literal na%22me.jpg and that case fails — it was one of the two red jobs that preceded the revert. The encoder is right and the expectation was wrong: the spec requires "%22 and says the UA must not perform any other escapes, so the assertion should be na%22me.jpg.

Both of those were in #233, which I have closed since it no longer applies to main; the diff is still readable there if it is useful.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with packages/react-native-nitro-fetch/ios/HybridNitroFetchClient.swift and the Android HybridNitroFetchClient.kt multipart code around lines 368–370. Compare the reverted behavior with #229 and the readable diff from #233, then inspect the five harness cases removed by the revert. Done means quoted names and filenames are escaped, line breaks and MIME types are handled as specified on both platforms, and the filename expectation matches the server behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
kotlin, react-native, swift
Domain
api, mobile-dev, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.