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

Fix boundary value extraction for form-data requests #7518

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

michaelwolz
Copy link
Contributor

@michaelwolz michaelwolz commented Jun 18, 2024

The changes introduced in #7306 fixed several bugs with multipart/form-data requests on iOS. However, the extraction of the actual boundary value might fail due to the value being surrounded by double quotes, which is allowed and happens occasionally (See MDN Reference which utilizes double quotes for the boundary in their example: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/POST#example).

In addition, the Content-Type header might include other keys such as charset.

This change extracts the boundary value regardless of the order of the individual keys within the Content-Type header.

Example Code

(Assuming that the plugin override is enable in capacitor.config.ts)

const body = new FormData();
body.append('key', 'value');

await fetch(url, { method: 'POST', body, 
  headers: {
    'Content-Type': 'multipart/form-data; boundary="boundary"; charset=utf8'
    //  'Content-Type': 'multipart/form-data; boundary="boundary"'
  }
});

The current implementation only considers the last string after any equal sign (contentType.components(separatedBy: "=").last). This results in --utf8 being set as the boundary in the request body. Even if there is no other key involved in the request it will include double quotes " in the request body, leading to a malformed request.

Working example including the changes can be found here:
https://github.com/michaelwolz/multipart-form-data-not-working-on-ios

The changes introduced in ionic-team#7306 fixed several bugs with multipart/form-data requests on iOS.
However, the extraction of the actual boundary value might fail due to the value
being surrounded by double quotes, which is allowed and happens occasionally.
Additionally, the `Content-Type` header might include other keys such as `charset`.
This change extracts the boundary value regardless of the order of the individual keys within
the `Content-Type` header.
@michaelwolz
Copy link
Contributor Author

michaelwolz commented Jun 19, 2024

Actually I just saw that the same issue exists with Android. I will try to also provide a fix for this :).

Edit: Done in bc6a6e0, example app also updated to include android platform

Applies changes made to iOS in 58045b0 to android platform
@michaelwolz michaelwolz changed the title Fix boundary value extraction for form-data requests on iOS Fix boundary value extraction for form-data requests on iOS / Android Aug 14, 2024
@michaelwolz michaelwolz changed the title Fix boundary value extraction for form-data requests on iOS / Android Fix boundary value extraction for form-data requests Aug 14, 2024
// If no boundary is provided, generate a random one and set the Content-Type header accordingly
// or otherwise servers will not be able to parse the request body. Browsers do this automatically
// but here we need to do this manually in order to comply with browser api behavior.
boundary = UUID.randomUUID().toString();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this part was missing in the iOS implementation but present for Android already.

@markemer markemer self-assigned this Aug 19, 2024
@fobaz

This comment was marked as abuse.

@barillax
Copy link

barillax commented Dec 2, 2024

thanks @michaelwolz ! This fix worked to unblock us. Looking forward to it getting upstream 🤞

@heath-pack
Copy link

@michaelwolz We used these changes to help unblock us as well, thanks!

We also needed to add a change to the convertFormData function of the native-bridge code.
It has a check on if (value instanceof File) { to correctly encode a file and add it as formData.

We ended up changing it to something like if (value instanceof File || value instanceof Blob) { so that blobs are also correctly encoded and added to the formData.

I think it would be great if that change could be included with your PR as well so that we really do end up having full support for form-data requests after this gets merged! WDYT?

@michaelwolz
Copy link
Contributor Author

@heath-pack In my opinion this is a different issue. I would like to get some official feedback on the PR by the capacitor team first. But you can create an issue (and PR) for this on your own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants