-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix boundary value extraction for form-data requests #7518
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
Open
michaelwolz
wants to merge
19
commits into
ionic-team:main
Choose a base branch
from
michaelwolz:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+68
−5
Open
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
58045b0
fix(ios): fix boundary value extraction for form-data requests
michaelwolz bc6a6e0
fix(android): fix boundary value extraction for form-data requests
michaelwolz 459e60c
fix(android): add fallback for form-data boundary
michaelwolz 792ea5b
Merge branch 'main' into main
michaelwolz d9195d5
Merge branch 'main' into main
michaelwolz bbd9ab4
Merge branch 'main' into main
michaelwolz 1fd8bbf
Merge branch 'main' into main
michaelwolz 1206b8b
Merge branch 'main' into main
michaelwolz 91368fa
Merge branch 'main' into main
michaelwolz 3469626
Merge branch 'main' into main
michaelwolz f186f0f
Merge branch 'main' into main
michaelwolz 7878a4c
Merge branch 'main' into main
michaelwolz 46a00dc
Merge branch 'main' into main
michaelwolz 49f824a
Merge branch 'main' into main
michaelwolz 2583c09
Merge branch 'main' into main
michaelwolz 683e7a4
Merge branch 'main' into main
michaelwolz fdcbf46
Merge branch 'main' into main
michaelwolz 2009925
Merge branch 'main' into main
michaelwolz d834600
Merge branch 'main' into main
michaelwolz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ | |
import java.util.List; | ||
import java.util.Locale; | ||
import java.util.Map; | ||
import java.util.UUID; | ||
import javax.net.ssl.HttpsURLConnection; | ||
import javax.net.ssl.SSLSocketFactory; | ||
import org.json.JSONException; | ||
|
@@ -224,7 +225,16 @@ public void setRequestBody(PluginCall call, JSValue body, String bodyType) throw | |
this.writeRequestBody(body.toString()); | ||
} | ||
} else if (bodyType != null && bodyType.equals("formData")) { | ||
this.writeFormDataRequestBody(contentType, body.toJSArray()); | ||
String boundary = extractBoundaryFromContentType(contentType); | ||
if (boundary == null) { | ||
// 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
connection.setRequestProperty("Content-Type", "multipart/form-data; boundary=" + boundary); | ||
} | ||
|
||
this.writeFormDataRequestBody(boundary, body.toJSArray()); | ||
} else { | ||
this.writeRequestBody(body.toString()); | ||
} | ||
|
@@ -260,9 +270,8 @@ private void writeObjectRequestBody(JSObject object) throws IOException, JSONExc | |
} | ||
} | ||
|
||
private void writeFormDataRequestBody(String contentType, JSArray entries) throws IOException, JSONException { | ||
private void writeFormDataRequestBody(String boundary, JSArray entries) throws IOException, JSONException { | ||
try (DataOutputStream os = new DataOutputStream(connection.getOutputStream())) { | ||
String boundary = contentType.split(";")[1].split("=")[1]; | ||
String lineEnd = "\r\n"; | ||
String twoHyphens = "--"; | ||
|
||
|
@@ -303,6 +312,39 @@ private void writeFormDataRequestBody(String contentType, JSArray entries) throw | |
} | ||
} | ||
|
||
/** | ||
* Extracts the boundary value from the `Content-Type` header for multipart/form-data requests, if provided. | ||
* | ||
* The boundary value might be surrounded by double quotes (") which will be stripped away. | ||
* | ||
* @param contentType The `Content-Type` header string. | ||
* @return The boundary value if found, otherwise `null`. | ||
*/ | ||
public static String extractBoundaryFromContentType(String contentType) { | ||
String boundaryPrefix = "boundary="; | ||
int boundaryIndex = contentType.indexOf(boundaryPrefix); | ||
if (boundaryIndex == -1) { | ||
return null; | ||
} | ||
|
||
// Extract the substring starting right after "boundary=" | ||
String boundary = contentType.substring(boundaryIndex + boundaryPrefix.length()); | ||
|
||
// Find the end of the boundary value by looking for the next ";" | ||
int endIndex = boundary.indexOf(";"); | ||
if (endIndex != -1) { | ||
boundary = boundary.substring(0, endIndex); | ||
} | ||
|
||
// Remove surrounding double quotes if present | ||
boundary = boundary.trim(); | ||
if (boundary.startsWith("\"") && boundary.endsWith("\"")) { | ||
boundary = boundary.substring(1, boundary.length() - 1); | ||
} | ||
|
||
return boundary; | ||
} | ||
|
||
/** | ||
* Opens a communications link to the resource referenced by this | ||
* URL, if such a connection has not already been established. | ||
|
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
if
condition on line 197 is never reached maybe only on xhr requests or on both.In native-bridge.js, it seems the Content-Type header is not being set. Could this be related to the issue?
Additionally, I am encountering a problem where my POST requests with (angular httpClient) multipart/form-data result in an empty body. This might be relevant to the bug described here: #7579.
android
https://github.com/ionic-team/capacitor/blob/main/android/capacitor/src/main/assets/native-bridge.js#L129
Called here (fetch proxy possible working) -> https://github.com/ionic-team/capacitor/blob/main/android/capacitor/src/main/assets/native-bridge.js#L493
Called here (xhr proxy bugged) -> https://github.com/ionic-team/capacitor/blob/main/android/capacitor/src/main/assets/native-bridge.js#L623
ios
https://github.com/ionic-team/capacitor/blob/main/ios/Capacitor/Capacitor/assets/native-bridge.js#L129
called here (fetch proxy maybe bugged) -> https://github.com/ionic-team/capacitor/blob/main/ios/Capacitor/Capacitor/assets/native-bridge.js#L493
called here (xhr proxy bugged) -> https://github.com/ionic-team/capacitor/blob/main/ios/Capacitor/Capacitor/assets/native-bridge.js#L623
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is definitely reached for regular fetch requests. Please feel free to checkout the provided example in the PR description which shows a minimal example using a fetch request. I also just updated the example to the latest capacitor version.
I am not sure that your issue is really related to the bug I tried to address here. This PR is only about wrong handling of form data boundaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, never mind—I found the issue. Sorry for the interruption.
If you don’t set the Content-Type header explicitly, the CapacitorHttp proxy will not automatically add a default Content-Type header and ignore the body.
In web the browser will handle this and add a default Content-Type...
I tested this behavior using your example app.