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

[Bug]: Percent-encoding of url search parameters broken for patched fetch / xhr requests through CapacitorHttp #7523

Closed
2 of 3 tasks
michaelwolz opened this issue Jun 20, 2024 · 2 comments · Fixed by #7527
Labels
plugin: http type: bug A confirmed bug report

Comments

@michaelwolz
Copy link
Contributor

michaelwolz commented Jun 20, 2024

Capacitor Version

💊   Capacitor Doctor  💊 

Latest Dependencies:

  @capacitor/cli: 6.1.0
  @capacitor/core: 6.1.0
  @capacitor/android: 6.1.0
  @capacitor/ios: 6.1.0

Installed Dependencies:

  @capacitor/cli: 6.1.0
  @capacitor/core: 6.1.0
  @capacitor/android: 6.1.0
  @capacitor/ios: 6.1.0

Other API Details

No response

Platforms Affected

  • iOS
  • Android
  • Web

Current Behavior

All patched fetch and xhr requests using any of the request methods GET, HEAD, OPTIONS and TRACE do not correctly consider percent-encoding of url search parameters. Especially it breaks the encoding of properly encoded url search parameters.

Example

Let's consider the following fetch call:
await fetch(`https://example.com?param=${encodeURIComponent(";:@&=+$,/?%#[]")}`);

The resulting URL to call will then look like this on the Javascript side and work fine in the browser:
https://example.com?param=%3B%3A%40%26%3D%2B%24%2C%2F%3F%25%23%5B%5D

On Android the called URL will look like this though:
https://example.com/?param=!'();:@&=+$,/?%

and on iOS like this (oddly enough, iOS somehow managed to encode the "%" character itself correctly, but I assume Swift handles this somehow?):
https://example.com/?param=!'();:@&=+$,/?%25

See intercepted network traffic via mitmproxy:
image

Expected Behavior

Patched fetch and xhr request should respect url encoding.

Project Reproduction

https://github.com/michaelwolz/capacitor-tests

Additional Information

I tried to investigate the root cause of this problem and found that PR #7273 introduced some logic to correctly handle the colon character : in hostnames for proxied urls in order to be able to correctly use it with different ports. For this the hostname of the requested url is encoded in the createProxyUrl method in the native-bridge.

When we enable the CapacitorHttp plugin all requests of type GET, HEAD, OPTIONS and TRACE will go through the monkey patched version of fetch (or xhr, the behavior is the applies to both) which will then create the proxy url and pass it on to the original browser fetch method (via win.CapacitorWebFetch). This request is then being intercepted and processed by the native code in WebViewLocalServer on Android and WebViewAssetHandler on iOS.

The actual issue happens then in the respective handler function (handleCapacitorHttpRequest) here and here.
At this point, the entire URL string is decoded, while in the native bridge on the JS side, only the host was encoded. This will remove any previous encoding applied to the URL except for the host. I think a solution could be encoding the entire URL in the createProxyUrl method of the native-bridge. This might double encode search params, but decoding should give the correct URL again, assuming the native decoder functions are compatible with the JS encode version. I will test this and possibly submit a PR (PR: #7527), but I'm not sure if there might be any side effects which I haven't considered yet and would appreciate any feedback.

Edit: Actually this will most certainly also break encoded pathnames and the url hash because the same logic applies here.

Copy link

ionitron-bot bot commented Jun 24, 2024

This issue has been labeled as type: bug. This label is added to issues that that have been reproduced and are being tracked in our internal issue tracker.

Copy link

ionitron-bot bot commented Sep 4, 2024

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Capacitor, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
plugin: http type: bug A confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants