Support for multipart/form-data in the request body #2321

ilanashapiro · 2023-07-03T20:36:39Z

Addresses #2008

Summary

Currently, validation always fails whenever a multipart/form-data content type is used in the request body. This prevents the normal usage of prism without any other proxy server (like nginx) on which such requests can be filtered out. Now, users can successfully use multipart/form-data content type in the request body.

Checklist

The basics
- I tested these changes manually in my local or dev environment
Tests
- Added or updated
- N/A
Event Tracking
- I added event tracking and followed the event tracking guidelines
- N/A
Error Reporting
- I reported errors and followed the error reporting guidelines
- N/A

Screenshots

Given the following file called examples/testapi.yaml:

openapi: '3.1.0'
paths:
  /path:
    post:
      responses:
        200:
          content:
            text/plain:
              example: ok
      requestBody:
        content:
          multipart/form-data:
            schema:
              type: object
              properties:
                testData:
                  type: string
              required:
                - "testData"

Writing a non-empty post request to http://127.0.0.1/path with included form-data in the request body now produces the following, successfully returning the response defined in the example:

In contrast, running this same command on the old version of Prism gives a 422 error:

… this test will be easily modified now once the correct behavior is implemented

ilanashapiro · 2023-07-03T21:10:56Z

@rattrayalex @RobertCraigie

chohmann

Thank you for submitting this PR!

The main issue we see with this PR is the test harness tests don't actually send multipart form data as expected.

Each of those tests needs to use the -F flag instead of --data in the curl command.

packages/http-server/src/__tests__/body-params-validation.spec.ts

chohmann · 2023-07-07T16:30:50Z

test-harness/specs/validate-body-params/multipart-allow-empty-value-2.oas2.txt

@@ -0,0 +1,28 @@
+====test====
+If I have an operation with a parameter with the allowEmptyValue set to false to false


Suggested change

If I have an operation with a parameter with the allowEmptyValue set to false to false

If I have an operation with a parameter with the allowEmptyValue set to false

chohmann · 2023-07-07T16:31:19Z

test-harness/specs/validate-body-params/multipart-allow-empty-value-2.oas2.txt

@@ -0,0 +1,28 @@
+====test====
+If I have an operation with a parameter with the allowEmptyValue set to false to false
+and I send a request with a correcly encoded parameter but with an empty key


Suggested change

and I send a request with a correcly encoded parameter but with an empty key

and I send a request with a correctly encoded parameter but with an empty key

chohmann · 2023-07-07T16:32:03Z

test-harness/specs/validate-body-params/multipart-allow-empty-value.oas2.txt

@@ -0,0 +1,30 @@
+====test====
+If I have an operation with a parameter with the allowEmptyValue set to false to false


Suggested change

If I have an operation with a parameter with the allowEmptyValue set to false to false

If I have an operation with a parameter with the allowEmptyValue set to true

chohmann · 2023-07-07T16:32:15Z

test-harness/specs/validate-body-params/multipart-allow-empty-value.oas2.txt

@@ -0,0 +1,30 @@
+====test====
+If I have an operation with a parameter with the allowEmptyValue set to false to false
+and I send a request with a correcly encoded parameter and with the parameter empty


Suggested change

and I send a request with a correcly encoded parameter and with the parameter empty

and I send a request with a correctly encoded parameter and with the parameter empty

chohmann · 2023-07-07T16:39:07Z

test-harness/specs/validate-body-params/multipart-form-data-allow-reserved-ok.oas3.txt

+====server====
+mock -p 4010 ${document}
+====command====
+curl -i -X POST http://localhost:4010/path -H "Content-Type: multipart/form-data" --data "reserved=:/?#[]@!$&'()*+,;"


For all of these curl requests you need to use -F instead of --data as that will actually pass form data as multipart.

…lidation

ilanashapiro · 2023-07-12T12:10:41Z

@chohmann All requested changes are now implemented. Thank you for pointing out that I needed to pass data with the -F flag rather than the --data flag! I initially misunderstood that setting "Content-type: multipart/form-data" was not sufficient. And apologies about the typos in tests I added to the test harness -- I copied these tests from existing files in the application/x-www-form-urlencoded test suite and didn't proofread the original files. I have now fixed all the typos in these as well as in the original files. I also added some regex parsing logic for multipart form data to handle additional cases. Please let me know if you'd like any further changes! Thanks!

EdVinyard

We really appreciate your update, but I think there are some cases the parseMultipartFormDataParams() function doesn't yet handle regarding the boundary between multipart entities.

EdVinyard · 2023-07-14T23:33:42Z

packages/http/src/validator/validators/body.ts

@@ -54,6 +54,15 @@ export function splitUriParams(target: string) {
  }, {});
 }

+export function parseMultipartFormDataParams(target: string) {
+  const pairs = target.split(/--.*\s*.*="/).filter(element => element);


I think this code assumes that the boundary between multipart entities is -- followed by any string. However, the boundary between multipart entities is defined, in each individual request, by the request's Content-Type header's Boundary parameter. This is because the client must choose a value that doesn't occur within any multipart entity that it will transmit. For example, consider the effect of the following example on your implementation.

curl -v -F a=b -F c='--="' <hostname>

Also consider any form value that includes \r\n\r\n.

RFC 2046 § 5.1.1 describes the boundary as...

a line consisting entirely of two hyphen characters ("-", decimal value 45) followed by the boundary parameter value from the Content-Type header field, optional linear whitespace, and a terminating CRLF.

I think you'll need to either:

implement this as the standard describes, with unit tests (not test-harness integration tests) to validate that it works with a variety of input, OR

rely on a library which parses multipart data according to the standard.

Hi Ed, thank you so much for your feedback. I discovered a library called parse-multiform-data which I'm installing as an npm package that seemed to be the most reliable method. I made changes according to this, such as parsing the boundary parameter from the Content-Type header field in order to pass it to the library for parsing the body. I left some comments on the recent changes that hopefully help clarify some of the decisions I made when implementing this. Thanks again and please let me know if you'd like any other changes!

der

ilanashapiro · 2023-07-18T10:39:49Z

packages/http/src/validator/index.ts

+      // parse boundary string from content-type in case media type is multipart/form-data
+      const multipart = require('parse-multipart-data');
+      const contentTypeHeader = headers.get('content-type');
+      const multipartBoundary = contentTypeHeader ? multipart.getBoundary(contentTypeHeader) : "";


Here, we parse the boundary parameter value from the Content-Type header field as specified by https://datatracker.ietf.org/doc/html/rfc2046#section-5.1.1. We do so using the parse-multipart-data library to ensure it's parsed correctly according to the standard. The boundary string is later passed into the parse-multipart-data library to correctly parse the body in the function parseMultipartFormDataParams.

ilanashapiro · 2023-07-18T10:41:39Z

packages/http/src/mocker/index.ts

+
+  // parse boundary string from content-type in case media type is multipart/form-data
+  const multipart = require('parse-multipart-data');
+  const multipartBoundary = multipart.getBoundary(contentTypeHeader);


See comment in packages/http/src/validator/index.ts

ilanashapiro · 2023-07-18T10:43:40Z

packages/http/src/validator/index.ts

+      const multipart = require('parse-multipart-data');
+      const contentTypeHeader = headers.get('content-type');
+      const multipartBoundary = contentTypeHeader ? multipart.getBoundary(contentTypeHeader) : "";
+      const mediaType = contentTypeHeader ? contentTypeHeader.replace(new RegExp(";\\s*boundary=" + multipartBoundary), "") : contentTypeHeader;


After end-end and unit testing, the content-type header appeared in both formats: "multipart/form-data;boundary=--...." and "multipart/form-data; boundary=--...." (with the space after the semicolon). Thus, I'm handling possible whitespace between the media type and the boundary string.

I'm concerned about your use of a regular expression here. Notice that the boundary may contain any 7-bit ASCII character, which I think includes many characters with special meanings inside a regular expression (e.g., ., *, (). Consider using a library to parse the Content-Type header in its entirety. Here's a very simple example using whatwg-mimetype:

import MIMEType from 'whatwg-mimetype'; const t = new MIMEType('multipart/form-data; boundary=.*.?.+zzz'); console.log(t?.essence); console.log(t.parameters.get('boundary'));

It looks like this block of code is a duplicate of what you added in mocker/index.ts. Could it be factored out?

…g are parsed from content-type header

ilanashapiro · 2023-07-18T10:46:29Z

packages/http/src/mocker/index.ts

+  // parse boundary string from content-type in case media type is multipart/form-data
+  const multipart = require('parse-multipart-data');
+  const multipartBoundary = multipart.getBoundary(contentTypeHeader);
+  const mediaType = contentTypeHeader.replace(new RegExp(";\\s*boundary=" + multipartBoundary), "");


See comment in packages/http/src/validator/index.ts

ilanashapiro · 2023-07-18T10:48:28Z

packages/http/src/validator/validators/body.ts

+  const multipart = require('parse-multipart-data');
+  const bufferBody = Buffer.from(target, "utf-8");
+
+  // the parse-multipart-data package requires that the body is passed in as a buffer, not a string


See: hovissimo's comment on nachomazzara/parse-multipart-data#14 (I was getting the same error as him)

And also the source code: https://github.com/nachomazzara/parse-multipart-data/blob/master/src/multipart.ts#L34C80-L34C80

ilanashapiro · 2023-07-18T10:49:46Z

packages/http/src/validator/validators/body.ts

+  const parts = multipart.parse(bufferBody, multipartBoundary);
+
+  return parts.reduce((result: Dictionary<string>, pair: string) => {
+    result[pair['name']] = pair['data'].toString();


then convert back from Buffer to String for further processing by Prism

ilanashapiro · 2023-07-18T10:55:25Z

packages/http/src/validator/validators/body.ts

@@ -54,6 +55,30 @@ export function splitUriParams(target: string) {
  }, {});
 }

+export function parseMultipartFormDataParams(target: string, multipartBoundary?: string) {
+  if(!multipartBoundary) {
+    const error = "Boundary parameter for multipart/form-data is not defined or generated in the request header. Try removing manually defined content-type from your request header if it exists.";


The boundary string MUST be non-empty if this is multipart/form-data (which it is if we're in this function). Thus, we return with error. According to https://bobbyhadz.com/blog/error-multipart-boundary-not-found-in-express-js, postmanlabs/postman-app-support#191, https://stackoverflow.com/questions/40351429/formdata-how-to-get-or-set-boundary-in-multipart-form-data-angular, this can occur when the user is manually setting the content-type, which means the server is not generating it (and the boundary string). Thus, the possible fix is suggested in the error message.

(Returning an error seemed to me a better solution than just returning an empty dictionary as it keeps the user more informed. Thus, the return type is Either).

EdVinyard · 2023-07-21T17:05:23Z

packages/http/src/validator/index.ts

@@ -75,17 +76,22 @@ const validateInputBody = (
    checkRequiredBodyIsProvided(requestBody, body),
    E.map(b => [...b, caseless(headers || {})] as const),
    E.chain(([requestBody, body, headers]) => {
+      // parse boundary string from content-type in case media type is multipart/form-data
+      const multipart = require('parse-multipart-data');


We'd prefer to use import statements rather than dynamic require whenever possible.

EdVinyard · 2023-07-21T17:36:37Z

packages/http/src/validator/index.ts

+      const multipart = require('parse-multipart-data');
+      const contentTypeHeader = headers.get('content-type');
+      const multipartBoundary = contentTypeHeader ? multipart.getBoundary(contentTypeHeader) : "";
+      const mediaType = contentTypeHeader ? contentTypeHeader.replace(new RegExp(";\\s*boundary=" + multipartBoundary), "") : contentTypeHeader;


I'm concerned about your use of a regular expression here. Notice that the boundary may contain any 7-bit ASCII character, which I think includes many characters with special meanings inside a regular expression (e.g., ., *, (). Consider using a library to parse the Content-Type header in its entirety. Here's a very simple example using whatwg-mimetype:

import MIMEType from 'whatwg-mimetype'; const t = new MIMEType('multipart/form-data; boundary=.*.?.+zzz'); console.log(t?.essence); console.log(t.parameters.get('boundary'));

It looks like this block of code is a duplicate of what you added in mocker/index.ts. Could it be factored out?

…empty boundary string in multipart data, modularize MIME header parsing

…ed files

ilanashapiro · 2023-07-25T08:49:04Z

packages/http/src/validator/validators/body.ts

@@ -47,11 +49,35 @@ export function deserializeFormBody(
 }

 export function splitUriParams(target: string) {
-  return target.split('&').reduce((result: Dictionary<string>, pair: string) => {
+  return E.right(target.split('&').reduce((result: Dictionary<string>, pair: string) => {


Wrapping in Either monad in order to match the return type of parseMultipartFormDataParams, which is of type Either as it may return an error.

ilanashapiro · 2023-07-25T08:52:41Z

packages/http/src/validator/validators/body.ts


  return pipe(
-    validateAgainstReservedCharacters(encodedUriParams, encodings, prefix),
+    content.mediaType === "multipart/form-data" ? parseMultipartFormDataParams(target, multipartBoundary) : splitUriParams(target),


Since parseMultipartFormDataParams returns an error wrapped in E.left if the boundary string is empty, the return type is Either, and I wrapped the result of splitUriParams in E.Right to match the return type (Either) of parseMultipartFormDataParams. Thus, we move this statement into the expression pipeline.

ilanashapiro · 2023-07-25T08:57:16Z

packages/http/src/mocker/index.ts

+
+  const requestBody = request.body as string;
+  const encodedUriParams = pipe(
+    mediaType === "multipart/form-data" ? parseMultipartFormDataParams(requestBody, multipartBoundary) : splitUriParams(requestBody),


parseMultipartFormDataParams returns an error wrapped in E.Left if the boundary sting is empty. Here I convert that to a default value (i.e. empty dictionary) if that occurs. I'm not sure if I'm supposed to be doing anything else here -- this same behavior (unwrapping the monad using defaults) is implemented for findContentByMediaTypeOrFirst just below on line 169, so I attempted to emulate that.

ilanashapiro · 2023-07-25T15:15:35Z

Hi Ed, thanks for your feedback, I am now doing all Content-Type header parsing using the whatwg-mimetype library you recommended, and the dynamic require statements were changed to imports. As for the duplicate code block, I moved it to a small utility function called parseMIMEHeader I created in validators/headers.ts. Finally, I also attempted to improve the error reporting for empty multipart boundary strings by wrapping parseMultipartFormDataParams in the Either monad and returning a descriptive error (which meant I also needed to wrap splitUriParams in the Either monad to maintain equivalent types).

…ub.com/ilanashapiro/prism into feature/multipart-form-data-validation

packages/http/src/validator/index.ts

chohmann

Thank you for your contribution and for working through our reviews with us!

Review is stale

chohmann · 2023-07-28T18:29:40Z

This is included in version 5.2.0 of Prism.

rattrayalex · 2023-07-29T01:14:56Z

Hooray, thanks Ilana, Ed, and Chelsea!

ilanashapiro added 4 commits June 30, 2023 13:59

add test for (currently failing) multipart form data POST requests --…

f0f4fc0

… this test will be easily modified now once the correct behavior is implemented

implement support for multipart/form-data requests with working test

a541db8

added tests to the test harness for multipart/form-data

cbf87de

delete commented out code

9baa519

ilanashapiro changed the title ~~Support for multipart/form-data in the requestBody.~~ Support for multipart/form-data in the request body Jul 3, 2023

ilanashapiro marked this pull request as ready for review July 3, 2023 20:51

ilanashapiro requested a review from a team as a code owner July 3, 2023 20:51

ilanashapiro requested review from EdVinyard and removed request for a team July 3, 2023 20:51

chohmann suggested changes Jul 7, 2023

View reviewed changes

ilanashapiro added 4 commits July 11, 2023 07:46

Parse multiform data passed with -F flag rather than --data flag

84a6490

Parse multiform data passed with -F flag rather than --data flag

55364bd

Merge branch 'stoplightio:master' into feature/multipart-form-data-va…

51f6722

…lidation

resolve merge

c0410d0

ilanashapiro marked this pull request as draft July 11, 2023 14:51

ilanashapiro added 3 commits July 11, 2023 07:54

add test files and cleanup whitespace

b954b44

delete tests not relevant to multipart form data

57b53d9

resolve conflict in yarn.lock file

fffddeb

ilanashapiro marked this pull request as ready for review July 12, 2023 12:05

EdVinyard suggested changes Jul 14, 2023

View reviewed changes

ilanashapiro added 2 commits July 16, 2023 08:45

use npm package to parse boundary string and form data from multipart

b568f73

resolve yarn.lock conflict

4a0a530

ilanashapiro marked this pull request as draft July 16, 2023 15:50

ilanashapiro added 2 commits July 16, 2023 08:56

improve error message for missing boundary

2c9f3a3

add null checks for parsing boundary from content-type hea

2466aff

der

ilanashapiro commented Jul 18, 2023

View reviewed changes

handle whitespace in the other location media type and boundary strin…

c81993f

…g are parsed from content-type header

ilanashapiro commented Jul 18, 2023

View reviewed changes

ilanashapiro marked this pull request as ready for review July 18, 2023 10:57

EdVinyard previously requested changes Jul 21, 2023

View reviewed changes

ilanashapiro added 2 commits July 23, 2023 03:04

resolve merge conflict in yarn.lock

79dedc5

parse MIME header with library, improve error handling/add tests for …

53bfedd

…empty boundary string in multipart data, modularize MIME header parsing

ilanashapiro marked this pull request as draft July 24, 2023 12:17

attempt to resolve node install error on server by committing generat…

e209c1b

…ed files

ilanashapiro commented Jul 25, 2023

View reviewed changes

fix formatting

fb50bc7

ilanashapiro added 3 commits July 25, 2023 08:20

condense verbose code block

8a6a229

condense verbose code block

9818ba0

Merge branch 'feature/multipart-form-data-validation' of https://gith…

362a79d

…ub.com/ilanashapiro/prism into feature/multipart-form-data-validation

ilanashapiro marked this pull request as ready for review July 25, 2023 15:48

chohmann reviewed Jul 28, 2023

View reviewed changes

packages/http/src/validator/index.ts Outdated Show resolved Hide resolved

Merge branch 'master' into feature/multipart-form-data-validation

b2df062

chohmann approved these changes Jul 28, 2023

View reviewed changes

chohmann merged commit 0f46225 into stoplightio:master Jul 28, 2023

travisgosselin mentioned this pull request Sep 1, 2023

Allow Binary Requests To Succeed in Prism Proxy Mode #2376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for multipart/form-data in the request body #2321

Support for multipart/form-data in the request body #2321

ilanashapiro commented Jul 3, 2023 •

edited

Loading

ilanashapiro commented Jul 3, 2023

chohmann left a comment

chohmann Jul 7, 2023

chohmann Jul 7, 2023

chohmann Jul 7, 2023

chohmann Jul 7, 2023

chohmann Jul 7, 2023

ilanashapiro commented Jul 12, 2023 •

edited

Loading

EdVinyard left a comment

EdVinyard Jul 14, 2023

ilanashapiro Jul 18, 2023

ilanashapiro Jul 18, 2023 •

edited

Loading

ilanashapiro Jul 18, 2023 •

edited

Loading

ilanashapiro Jul 18, 2023

EdVinyard Jul 21, 2023

ilanashapiro Jul 18, 2023

ilanashapiro Jul 18, 2023 •

edited

Loading

ilanashapiro Jul 18, 2023

ilanashapiro Jul 18, 2023

ilanashapiro Jul 25, 2023 •

edited

Loading

EdVinyard Jul 21, 2023

EdVinyard Jul 21, 2023

ilanashapiro Jul 25, 2023 •

edited

Loading

ilanashapiro Jul 25, 2023 •

edited

Loading

ilanashapiro Jul 25, 2023

ilanashapiro commented Jul 25, 2023

chohmann left a comment

chohmann commented Jul 28, 2023

rattrayalex commented Jul 29, 2023

		@@ -0,0 +1,28 @@
		====test====
		If I have an operation with a parameter with the allowEmptyValue set to false to false

	and I send a request with a correcly encoded parameter but with an empty key
	and I send a request with a correctly encoded parameter but with an empty key

		@@ -0,0 +1,30 @@
		====test====
		If I have an operation with a parameter with the allowEmptyValue set to false to false

Support for multipart/form-data in the request body #2321

Support for multipart/form-data in the request body #2321

Conversation

ilanashapiro commented Jul 3, 2023 • edited Loading

ilanashapiro commented Jul 3, 2023

chohmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilanashapiro commented Jul 12, 2023 • edited Loading

EdVinyard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilanashapiro Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

ilanashapiro Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilanashapiro Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilanashapiro Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilanashapiro Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

ilanashapiro Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilanashapiro commented Jul 25, 2023

chohmann left a comment

Choose a reason for hiding this comment

chohmann commented Jul 28, 2023

rattrayalex commented Jul 29, 2023

ilanashapiro commented Jul 3, 2023 •

edited

Loading

ilanashapiro commented Jul 12, 2023 •

edited

Loading

ilanashapiro Jul 18, 2023 •

edited

Loading

ilanashapiro Jul 18, 2023 •

edited

Loading

ilanashapiro Jul 18, 2023 •

edited

Loading

ilanashapiro Jul 25, 2023 •

edited

Loading

ilanashapiro Jul 25, 2023 •

edited

Loading

ilanashapiro Jul 25, 2023 •

edited

Loading