Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Order is not enforced when making a multipart/form-data request #1382

Closed
kayleewright-bluescape opened this issue Apr 1, 2020 · 23 comments
Closed
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 new-http issues that would require (or benefit from) a new HTTP API

Comments

@kayleewright-bluescape
Copy link

kayleewright-bluescape commented Apr 1, 2020

As an intermediate step to testing an API flows I need to upload to s3 using their POST API:
https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectPOST.html
The api docs dictate that file MUST be the last field in the form data.

I have setup my payload like this:

          const s3Payload = {
            key: key,
            bucket: bucket,
            'X-Amz-Algorithm': algorithm,
            'X-Amz-Credential': credential,
            'X-Amz-Date': date,
            Policy: policy,
            'X-Amz-Signature': signature,
            file: http.file(file, fileName, 'application/msword'),
          };
          const uploadToS3Res = http.post(s3Url, s3Payload);

Some background:
API1 (not shown here) is run to get all the payload values and the s3Url.
We use that response to actually upload to s3 (http.post(s3Url, s3Payload))
When S3 is done we call some more internal APIs to handle the newly uploaded file

We want to do this process as part of a test but when when try to post to the S3 api we get the following error:
"<Error><Code>InvalidArgument</Code><Message>Bucket POST must contain a field named 'key'. If it is specified, please check the order of the fields.</Message><ArgumentName>key</ArgumentName><ArgumentValue></ArgumentValue><RequestId>B235CB11F0A582FA</RequestId><HostId></HostId></Error>"
or similar errors complaining about other fields being missing.

I have tested this in postman by changing the field order and anything below the file prop is reported as missing. In postman once putting file as the last parameter using all the same values passed through the K6 test, it succeeds.
In Node.js as of ES2015 this doesn't seem to happen as objects retain their keyed order (numeric keys in order added, followed by string keys in order added, followed by Symbol keys in order added). I'm guessing this is not the case once things end up down at the golang level.

Environment

  • k6 version: k6 v0.26.2 (2020-03-18T12:06:57+0000/v0.26.2-0-g459da79e, go1.13.8, linux/amd64)
  • OS and version: Ubuntu 18.04.2 LTS
  • Docker version and image, if applicable:

Expected Behavior

Form data send in the order specified in test

Actual Behavior

Form data sent out of order and api call fails.

@imiric
Copy link
Contributor

imiric commented Apr 2, 2020

Hi, thanks for reporting this.

This is an unfortunate side-effect of the current HTTP API, which is in plans of being improved in the near future, so we'll make sure to address this as well.

In the meantime, you should be able to workaround this by manually building the request payload as a string (something like this) and passing that to http.post() instead of an object.

The only tricky thing would be serializing the binary file, which you'd need to base64 encode first. Unfortunately, the k6 b64encode() function doesn't handle binary input, so you'll need to use a shim like https://gist.github.com/jonleighton/958841.

Then you'll be able to do something like:

let file = http.file(binFile, fileName, 'application/msword');
let b64 = base64ArrayBuffer(file.data);

... and append that to the body.

We realize this is unwieldy, but it's the best workaround until this is improved.

EDIT: Correction: b64encode does handle an array of integers so you wouldn't need the above shim and encoding.b64encode(file.data) works just as well.

@kayleewright-bluescape
Copy link
Author

kayleewright-bluescape commented Apr 2, 2020

Thanks @imiric! This worked wonderfully!

It's ugly but we only need it for s3 uploads so we can just hide it away in a utility for now. Was tearing my hair out looking for any workaround or solution 😊

@landon-martin
Copy link

Hey @imiric this approach did take us a step further, but still with no luck. The upload successfully goes through, and we can see the file in the S3 bucket, but it becomes corrupted. We've deduced the S3 requires either a Buffer or a UTF8 String, not base64. Neither which k6 has native support for. So we end up trying to upload a word doc, it shows up in the bucket as a word document filled with the base64 of the corrupted file.

Any suggestions on a work around for this?

@imiric
Copy link
Contributor

imiric commented Apr 8, 2020

Hi, this is likely because S3 is storing the raw base64 string which you'll need to decode when you download it, but the data shouldn't be corrupted.

Alternatively, try uploading a string converted from the byte array with something like:

let file = http.file(binFile, fileName, 'application/msword');
let fileStr = String.fromCharCode(...file.data);

And set the Content-Type: application/msword header for that part in the payload. That should produce a correct binary in S3, but I haven't tested this so let us know if it works.

@landon-martin
Copy link

@imiric Nope, that didn't do it either. For a little more context, here's what we have so far.

/*
  const s3Payload = {
    key,
    bucket,
    'X-Amz-Algorithm',
    'X-Amz-Credential',
    'X-Amz-Date',
    Policy:,
    'X-Amz-Signature',
  };
*/
  // separate form elements
  const boundary = 'nVenJ7H4puv';
  let body = '';
  // build body manually to guarantee order (file must be last)
  Object.keys(s3Payload).forEach(key => {
    body += `--${boundary}\r\n`;
    body += `Content-Disposition: form-data; name="${key}"\r\n`;
    body += `Content-Type: ${typeof s3Payload[key]}\r\n\r\n`;
    body += `${s3Payload[key]}\r\n`;
  });

  // get s3 file data
  const s3file = http.file(fileData, fileName, 'application/msword');
  // use s3file object to add the file field
  body += `--${boundary}\r\n`;
  body += `Content-Disposition: form-data; name="file"; filename="${s3file.filename}"\r\n`;
  body += `Content-Type: ${s3file.content_type}\r\n`;
  body += '\r\n';
  body += `${String.fromCharCode(...s3file.data)}\r\n`;
  body += `--${boundary}--`;
  const headers = {
    'Content-Type': `multipart/form-data; boundary=${boundary}`,
    'Content-Length': body.length,
  };

  const res = http.post(s3Url, body, {headers});

Where the s3Payload has actual values.

When a doc file is uploaded using this, it actually is more "corrupt" than the other method LibreOffice tries to recover it and fails. Where before the word doc would open, but just be filled with base64.

Replacing ``${String.fromCharCode(...s3file.data)}` with a random string, literally converts to a word document with that string in it.

@landon-martin
Copy link

The only successful attempt I have completed is by reverting back to the original method using the JSON object, and file: http.file() originally in @kylewright-bluescape 's post. But the preserving ordering issue remains. So it only completes successfully, randomly based on if key was read first.

@kayleewright-bluescape
Copy link
Author

Hi, this is likely because S3 is storing the raw base64 string which you'll need to decode when you download it

Unfortunately this isn't possible as this is part of a multi part system. When the system pulls the file from S3 it expects to be the exact file to be the format described without decoding being necessary (like it is when we do a regular form data post outside of k6).

@imiric
Copy link
Contributor

imiric commented Apr 8, 2020

Hi, sorry, I'm not familiar with the S3 API at this level, and we currently don't have the resources to implement a working example that uses formdata.

I've seen this discussion which also brings up the corruption issue and suggests uploading only the file portion, but that was 5 years ago, so not sure how valid it might be.

Alternatively give this library for the V4 API a try: https://gist.github.com/MStoykov/38cc1293daa9080b11e26053589a6865 , which you should be able to use without the manual form building.

Also consider posting on the community forum in case someone has already done this before.

@mstoykov
Copy link
Contributor

mstoykov commented Apr 8, 2020

If you can provide fuller code example where I only need to put in my keys/files, maybe I could take 30 minutes looking into it, but currently not up to once again reading AWS documentation on signing requests :D

@landon-martin
Copy link

Hey @mstoykov, thanks for offering. I don't blame you for not wanting to re-read those docs 😁

Bit of a tricky situation since there may be some confidential information in that payload. But maybe we could simplify this away from S3 for a second.

One of two things would solve this problem.

  1. Using JSON - Preserving order in the payload (would be lovely if http.post() had an option to pass an ordered list and have the GO code create the payload object order, or something of that nature)
  2. Using FormData - Finding a way to encode the file data so that it is accepted by S3 as a valid word doc content. Which I believe could possibly be done in k6, if the right combination of conversions is found.

When using form data. The file is read using:

const fileData = open(file, 'b');
const s3file = http.file(fileData, fileName, 'application/msword);
const contents = String.fromCharCode(...s3file.data);

The funny thing is, the word doc then contains the text from the doc unformatted along with some gibberish header and footer that is likely the contents of the file.

I believe this is because the formdata request is literally looking for the contents of the file. When using the JSON payload, we can pass the Buffer/stream (however k6 handles that).

If neither of those things are possible, that's just how it is! We'll check-out/post-on the community forums as well.

@mstoykov
Copy link
Contributor

mstoykov commented Apr 9, 2020

@landon-martin
I just wanted the code that does the signing and body encoding, not the actual keys or like your s3 bucket or the document or something any of your actual data ... The problem is that even if I come up with some way that it works for some server it may still not be supported by S3.
And yes ... I also think that you can make it work with the correct amount of due diligence and working with arrays of ints (at least that is what I would expect will work accurately), unfortunately, I don't have the time currently to try to work both S3 signing and this out, so unless I get some help from someone this will likely wait in "the backlog" ;)

Also, the JS VM, goja, that we use currently is getting a lot of work on it which will likely turn out to fix this, so, me trying to work a hack around it now might not be very productive. Although we probably won't start using that new version for some time more, especially given its experimental status :(

@kayleewright-bluescape
Copy link
Author

kayleewright-bluescape commented Apr 9, 2020

@mstoykov here what we were trying:

  // upload file to amazon s3
  const s3Url = url;
  console.debug(`Uploading to s3 with url: ${s3Url}`);
  // payload minus file -- need to special handle it
  const s3Payload = {
    key: data.key,
    bucket: data.fields.bucket,
    'X-Amz-Algorithm': data.fields['X-Amz-Algorithm'],
    'X-Amz-Credential': data.fields['X-Amz-Credential'],
    'X-Amz-Date': data.fields['X-Amz-Date'],
    Policy: data.fields.Policy,
    'X-Amz-Signature': data.fields['X-Amz-Signature'],
  };
  // separate form elements
  const boundary = 'nVenJ7H4puv';
  let body = '';
  // build body manually to guarantee order (file must be last)
  Object.keys(s3Payload).forEach(key => {
    body += `--${boundary}\r\n`;
    body += `Content-Disposition: form-data; name="${key}"\r\n`;
    body += `Content-Type: ${typeof s3Payload[key]}\r\n\r\n`;
    body += `${s3Payload[key]}\r\n`;
  });

  // get s3 file data
  const s3file = http.file(fileData, fileName, fileType.contentType);
  // use s3file object to add the file field
  body += `--${boundary}\r\n`;
  body += `Content-Disposition: form-data; name="file"; filename="${s3file.filename}"\r\n`;
  body += `Content-Type: ${s3file.content_type}\r\n`;
  body += '\r\n';
  body += `${fileData}\r\n`;
  body += `--${boundary}--\r\n`;
  const headers = {
    'Content-Type': `multipart/form-data; boundary=${boundary}`,
  };
  const res = http.post(s3Url, body, {headers});

For this part:
body += `${fileData}\r\n`;
We've also tried:
body += `${s3file.data}\r\n`;
and
body += `${encoding.b64encode(s3file.data)}\r\n`;

we've tried opening the file both with and without 'b'.

This is the api ref:
https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectPOST.html

@mstoykov
Copy link
Contributor

@kylewright-bluescape and doing encoding.b64encode(fileData) with Content-Encoding: base64 after the Content-Type: for the file doesn't work ?

@kayleewright-bluescape
Copy link
Author

@mstoykov correct had that in there before.

@kayleewright-bluescape
Copy link
Author

@mstoykov @imiric

The best solution for us turned out to be hacking together something into k6 source that allows us to specify field order in the params obj and then building our own binary.

That change lets us do this:

  const res = http.post(s3Url, s3Payload, {
    payloadKeyOrder: Object.keys(s3Payload),
  });

Since ES2015 outputs keys in order of insertion (if they are all strings) it makes it the cleanest way for us to do this for now.

My golang is pretty rudimentary/bad but I can post a PR (or a patch file) if you like. Not sure how this would fit into your current plans for the http rework.

-Kyle

@rtrive
Copy link

rtrive commented Nov 19, 2020

is there any update on this bug? we're facing this problem because we're using this library to manage https://github.com/jaydenseric/graphql-upload and follow their suggested pattern (https://github.com/jaydenseric/graphql-multipart-request-spec) we need to follow the order that they suggest to avoid errors.

Thanks

@landon-martin
Copy link

@rtrive We never got a response for a fix, ended up just applying a patch and building the binary ourselves.

0001-Add-order-support-for-form-data.txt

@na--
Copy link
Member

na-- commented Nov 20, 2020

Fixing this properly in k6 is probably not going to happen with the current k6/http API, sorry... 😞 We plan to eventually make a new one, where we won't repeat the same mistake. I skimmed the patch @landon-martin posted and I didn't spot any issues, but we are probably not going to merge something like it in k6 for two reasons. First, it's not addressing the multiple other issues with multipart/form-data requests the current k6/http API has (#1571, #843, #747). To address them, we simply need a better HTTP request body abstraction, which we can't get in the current API without breaking changes. Second, any new parameter to the current HTTP API is one more thing we're going to have to re-implement in the new API or, rather, the shim layer we plan to make so we also continue supporting the current API without breaking changes.

I don't have an estimate when the new HTTP API is going to land, I've even procrastinated writing the overarching issue for it for around a year now... 😞 Mostly because of the horrific #1007 delays and a bunch of other urgent priorities, but also because we've continued to gather more and more issues with the current API that need fixing. I'll make the issue soon (:tm: :sweat_smile:), but one piece of good news is that we'll likely develop the new HTTP API as an xk6 extension (more info). So, it will take shape gradually and be available for testing much earlier than otherwise, if we tried to make it directly in k6 core.

For a workaround until then, we plan to soon (either in k6 v0.30.0 or k6 v0.31.0) add better support for something else the current k6 doesn't deal well with - binary data (#1020). Specifically, we would support passing an ArrayBuffer as the HTTP request body, even in the current k6/http API. This, combined with the fact that recent k6 versions support typed arrays better, should allow us to write something like a FormData polyfill with extras that works with k6, with none of the issues encountered in #1382 (comment).

@imiric
Copy link
Contributor

imiric commented Feb 12, 2021

Hey guys, just a heads-up: multipart requests should be simpler to make now with the FormData polyfill and k6 v0.30.0 (make sure you're using at least this version). See the documentation.

@landon-martin
Copy link

Hey guys, just a heads-up: multipart requests should be simpler to make now with the FormData polyfill and k6 v0.30.0 (make sure you're using at least this version). See the documentation.

Thanks for updating the thread @imiric. I'm failing to see how this solves the problem we're encountering here, but maybe I'm missing something. It seems like the FormData library at https://jslib.k6.io/formdata/0.0.1/index.js is only addressing order of files, not payload keys of any other type.

For example, we are wanting something like:

    const fd = new FormData();
    fd.append('key', uploadData.key);
    fd.append('bucket', uploadData.fields.bucket);
    fd.append('X-Amz-Algorithm', uploadData.fields['X-Amz-Algorithm']);
    fd.append('X-Amz-Credential', uploadData.fields['X-Amz-Credential']);
    fd.append('X-Amz-Date', uploadData.fields['X-Amz-Date']);
    fd.append('Policy', uploadData.fields.Policy);
    fd.append('X-Amz-Signature', uploadData.fields['X-Amz-Signature']);
    fd.append('file', http.file(fileData, fileName, this.contentType));

    const params = {
      headers: {'Content-Type': `multipart/form-data; boundary=${fd.boundary}`},
    };

    const res = http.post(url, fd.body(), params);

Where everything but 'file' is a string.
But since fd.append() assumes that you're passing a file, it fails getting data from the file and we end up with a fd.body() value equal to just:

fd.append('file', http.file(fileData, fileName, this.contentType));

Since the order of the key value needs to be first in the payload, we still can't guarantee that with this method.

@mstoykov
Copy link
Contributor

Hi @landon-martin ,
It technically is taking an object which has data and optionally filename and content_type fields so

import http from 'k6/http';
import { FormData } from 'https://jslib.k6.io/formdata/0.0.1/index.js';

const fileData = open('file.dat', 'b');
export default function() {
	const uploadData = {
		key: "1234161",
		fields:{ 
			bucket: "mybucket",
			'X-Amz-Algorithm': "something",
		},
	}
	const fd = new FormData();
	fd.append('key', {data:uploadData.key});
	fd.append('bucket', {data:uploadData.fields.bucket});
	fd.append('X-Amz-Algorithm', {data:uploadData.fields['X-Amz-Algorithm'], content_type: "my custome type"});
	//fd.append('X-Amz-Credential', uploadData.fields['X-Amz-Credential']);
	//fd.append('X-Amz-Date', uploadData.fields['X-Amz-Date']);
	//fd.append('Policy', uploadData.fields.Policy);
	//fd.append('X-Amz-Signature', uploadData.fields['X-Amz-Signature']);
	fd.append('file', http.file(fileData, "something", 'application/mine'));

	const params = {
		headers: {'Content-Type': `multipart/form-data; boundary=${fd.boundary}`},
	};

	const res = http.post("https://httpbin.test.k6.io/post", fd.body(), params);
	console.log(JSON.stringify(res, null, "  "));
}

Does work as you will expect.

But I see how this isn't obvious from the docs as the only code that shows that is in the jslib tests and that still uses all the fields, so in practice, only the documentation inside the polyfill is mentioning they are optional.

@landon-martin
Copy link

Hi @mstoykov,

You're right, I was expecting it to act more like the web API class that this is mimicking, for example: https://developer.mozilla.org/en-US/docs/Web/API/FormData/Using_FormData_Objects

Thanks for the tip, quickly changed over to that and it worked as expected 👍

@na-- na-- added evaluation needed proposal needs to be validated or tested before fully implementing it in k6 new-http issues that would require (or benefit from) a new HTTP API and removed bug labels Dec 17, 2021
@imiric
Copy link
Contributor

imiric commented Mar 28, 2023

The FormData polyfill resolves this issue, and is the recommended way to submit multipart requests, so I'll close this issue.

In the future, we might integrate FormData into k6 as part of the new HTTP API (initial design document), but we haven't decided on the details yet.

@imiric imiric closed this as completed Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 new-http issues that would require (or benefit from) a new HTTP API
Projects
None yet
Development

No branches or pull requests

6 participants