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(http): media requests route to custom handlers #6540

Closed
wants to merge 18 commits into from

Conversation

ItsChaceD
Copy link
Contributor

@ItsChaceD ItsChaceD commented Apr 26, 2023

Addresses: #6123 and #6126

Test app: https://github.com/ItsChaceD/capacitor-http-wasm-issue

This PR routes media/non-text requests (files, blobs, WASM, etc.) to custom native protocol handlers so the responses don't have to be encoded/decoded through the bridge.

On iOS: we change the protocol from https to capacitor and it is handled by the URL scheme handler
On Android: we take a different approach to avoid cleartext traffic issues. Instead we prepend a Capacitor Media string to the URL so that shouldInterceptRequest can detect that the request should be intercepted. We cannot do this on iOS since it is not allowed to intercept requests on the https protocol.

@markemer
Copy link
Member

markemer commented May 1, 2023

Looks like lint is failing, BTW.

@markemer
Copy link
Member

markemer commented May 3, 2023

Core tests are failing in native-bridge.ts not 100% why.

@jcesarmobile
Copy link
Member

jcesarmobile commented Jun 20, 2023

Why is this change applied for media files only? I think the problem is present for all file types, not just for media files, so the current change will only partly fix the issue.

Also, why use a second scheme handler? We used to have multiple and we ended using only one because it had some problems (which I don't remember at the moment).
While it's true that we can't intercept http/https calls, we can still use the existing scheme handler turning the urls into something like

if (platform === 'ios') {
    url.protocol = 'capacitor:'; // it's just an example, shouldn't be hardcoded as users can change, get the actual value from WEBVIEW_SERVER_URL
} 
url.pathname += '/_capacitor_media_'; // do this for both platforms 

And then in the existing handler check for the _capacitor_media_ for needed customizations.
Also, I think the /_capacitor_media_ should be prepended to the pathname, not appended.

@ItsChaceD
Copy link
Contributor Author

@jcesarmobile "Why is this change applied for media files only? I think the problem is present for all file types, not just for media files, so the current change will only partly fix the issue."

This change is for any MIME type that is not text/JSON.

const mediaContentTypes = [
        'application/*',
        'audio/*',
        'image/*',
        'model/*',
        'video/*',
      ];
      
const textContentTypes = [
        'text/*',
        'application/json',
        'application/x-www-form-urlencoded',
        'multipart/form-data',
      ];
      
 if(isMediaType && !isTextType) ...

So this includes all other content types (Files, Blobs, etc.). The tricky part is that there is no way to guarantee what response type we are expecting before the request is actually executed. So to account for this, we check if 1 of these conditions are met:

  1. Is X-Capacitor-Force-Media-Request set on our Request Headers. (Just Added + Needs Documentation).
  2. Is the Content-Type set on our Request Headers & is it in mediaContentTypes but not in textContentTypes
  3. Is the file extension on the URL in fileExtensions
  4. Is our responseType in responseTypes (ONLY FOR XHR)

@jcesarmobile
Copy link
Member

But why not do it for text files too? I think we should do it for all regular GET requests (not for POST because in Android it's not possible to get the body), so exclude only application/x-www-form-urlencoded and multipart/form-data, but not text/JSON files. So we can remove the whole mime type checking.

We have another issue about a XHR call returning an Object for a JSON file instead of string and using the approach in this PR (removing the string type check) I get the string properly instead of the Object.

I don't think adding the CORS headers are not needed anymore since we are using capacitor:// scheme on all requests now, it's hard to tell with the provided app, but looks like it's working fine if I remove the code.

@markemer
Copy link
Member

markemer commented Jul 6, 2023

Shouldn't 'text/*' cover text/plain for text files?

@@ -43,7 +48,7 @@ internal class WebViewAssetHandler: NSObject, WKURLSchemeHandler {
]

// if using live reload, then set CORS headers
if self.serverUrl != nil && self.serverUrl?.scheme != localUrl.scheme {
if self.serverUrl != nil && self.serverUrl?.scheme != localUrl.scheme {
Copy link
Member

Choose a reason for hiding this comment

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

👍

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.

4 participants