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: capacitor errors reading response headers android #6604

Closed
tbscode opened this issue May 17, 2023 · 8 comments
Closed

bug: capacitor errors reading response headers android #6604

tbscode opened this issue May 17, 2023 · 8 comments

Comments

@tbscode
Copy link

tbscode commented May 17, 2023

Bug Report

Capacitor Version

Latest Dependencies:

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

Installed Dependencies:

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

Platform(s)

  • android

not tested on ios

Current Behavior

I am encountering an issue with capacitor-http and the response headers for fetch calls on Android. My fetch calls are working fine in the web app, but they throw a Capacitor error when running on Android.

(index):392 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'startsWith')
    at window.fetch ((index):392:80) <------- error in capacitor android
    at async g (index-e974cbdf3f503cbf.js:1:2107)

The line causing the error seems to be this one:

let data = !nativeResponse.headers['Content-Type'].startsWith('application/json')

Reproduction

In the browser

I am using Next.js, React, and Capacitor for the frontend of a cross-platform React app. The backend is a Django server set up with the REST framework.

For the frontend of the browser web application, I can use the following code:

const res = await fetch(`${process.env.NEXT_PUBLIC_BACKEND_URL}/api/user_data`, {
  method: 'POST',
  credentials: 'include',
  headers: {
    'X-CSRFToken': getCookiesAsObject().csrftoken,
    'Content-Type': 'application/json',
    Accept: 'application/json'
  },
  body: JSON.stringify({'some' : 'data'}),
});

When I run the web app, I get the expected 200 OK response with some user data.

These are the server headers:

HTTP/1.1 200 OK
date: Wed, 17 May 2023 21:37:55 GMT
server: uvicorn
content-type: application/json
vary: Accept, origin, Cookie
allow: POST, GET, OPTIONS
access-control-allow-credentials: true
access-control-allow-origin: http://localhost:3000
x-frame-options: DENY
content-length: 95
x-content-type-options: nosniff
referrer-policy: same-origin
cross-origin-opener-policy: same-origin

On Android:

I modify NEXT_PUBLIC_BACKEND_URL = "http://10.0.2.2:8000", and the app builds and starts correctly on Android. I see all requests coming through correctly in the logs of my Django server, data is corretly returned and response headers are set.

Using middleware I've checked my headers are set correctly, including the CSRF token and session ID, also making a call to my login api succeeds and capacitor correctly sets cookies, but then fails with the same error.

It seems like Capacitor fails when trying to load the request content. This happens during the fetch requests, and any subsequent const data = await res.json(); calls are not executed.

I'm confused about why the nativeResponse.headers are undefined, as I expected fetch to work out of the box with CapacitorHttp. I'm not sure if I'm misusing fetch in combination with Capacitor, or if this is an Android-related issue. I've searched for similar issues and experimented with various implementation modifications, but I haven't directly used CapacitorHttp.post. I was hoping to avoid that, if possible. It feels like I'm very close to getting the whole package (cookies + request) working using the standard fetch, so any help or confirmation that this is not a bug would be appreciated.

Capacitor setup

I made some modifications in the Android Manifest to allow text content requests:

 <application
	 ...
        android:usesCleartextTraffic="true">
	...

And my Capacitor config:

...
  server: {
    //androidScheme: 'https'
  },
  plugins: {
    CapacitorHttp: {
      enabled: true,
    },
    CapacitorCookies: {
      enabled: true
    }
  },
  android: {
    allowMixedContent: true
  },
....

Reproducing the issue

I believe the issue might be related to me misusing fetch or misunderstanding the HTTP plugin usage of Capacitor. If initial feedback suggests so, I can either move this to a discussion or create a reproduction repo (smaller than the one below). It might also be related to my server configuration, although I'm using fairly standard request headers and content on both sides.

The project I'm experiencing this issue with is here.

@tbscode
Copy link
Author

tbscode commented May 17, 2023

Ok i've decided to poke a little i the capacitor code and actually the bug seems to be related to the captialization of content-type in the response headers.

If I change the line to:

    let data = !nativeResponse.headers['content-type'].startsWith('application/json')

All works fine, so I assume django is changing the naming of the header? This should be an issue with other servers to I assume?

@tbscode
Copy link
Author

tbscode commented May 17, 2023

So this does seem to be a bug, the simple update 'Content-Type' -> 'content-type' fixes all my issues everything works as expected now.

I'm not sure what causes the content-type header to be lowercase I assume this is prob django, though if this is an allowed shema it should prob also be supported right?

@ghost
Copy link

ghost commented Jun 3, 2023

@tbscode indeed. it should work with content-type as well as Content-Type. There probably needs to be a wider look at all header usage to make sure all reads are case-insensitive.

@gerhardcit
Copy link

This issue makes HMR with on the phone completely unusable. When can we expect this to be fixed?

Screenshot 2023-06-14 at 11 57 38

@tbscode
Copy link
Author

tbscode commented Jun 15, 2023

I could imagine making a PR for this but I haven't got too much experience ( also not with capacitor ;P ).

I'm not sure what the desired fix here is OR what side effects my fix might have.

Judging from comment and references as provided here: https://stackoverflow.com/a/41169947

I'd say since keys are supposed to be case-insensitive, we should just check lowercase everywhere and just automatically make all header keys lowercase.'

This would potentially break:

  • all code that assumes case-sensitive headers
  • potentially other locations in the code that use case sensitive header look-ups ( though these should all just be updated to do a lowercase check )

I see the following options for a fix:

Option 1:

  • fixing this issue globally by patching CapacitorHttp Java/Swift implementation to always return lowercase headers.
  • Then update all code to always reference lowercase headers.

As far as I can tell on the android side of things this would mean updating the code here:

public static JSObject buildResponseHeaders(CapacitorHttpUrlConnection connection) {

Just to:

    public static JSObject buildResponseHeaders(CapacitorHttpUrlConnection connection) {
        JSObject output = new JSObject();

        for (Map.Entry<String, List<String>> entry : connection.getHeaderFields().entrySet()) {
            String valuesString = TextUtils.join(", ", entry.getValue());
            output.put(entry.getKey().toLowerCase(), valuesString);
        }

        return output;
    }

I've newer worked with swift before but I'd assume the fix for the ios side would be around here:

public static func buildResponse(_ data: Data?, _ response: HTTPURLResponse, responseType: ResponseType = .default) -> [String: Any] {

Option 2: ( fixing only the android bridge )

Just update native-bridge.js to make headers lowercase:

nativeResponse.headers = Object.entries(nativeResponse.headers).reduce((acc, [key, value]) => {
  acc[key.toLowerCase()] = value;
  return acc;
}, {});

...

let data = !nativeResponse.headers['content-type'].startsWith('application/json')

I'm not sure how covering the test are but a PR should prob also include some tests for the heads on android and ios?

Option 2 has lower potential for side effects but might also not fix the whole issue.

Any tips or help welcome here, I'd love to use capacitor for building my PWA.
This fix is required for it!

@tbscode
Copy link
Author

tbscode commented Jun 16, 2023

So my current workaround is to apply Option 2 on the fly.

I simply made a fork of capacitor and patched native-bridge.js as described above.

Then I apply the patch - by overwriting native-bridge.js via postinstall in my package.json:

"postinstall": "curl -o node_modules/@capacitor/android/capacitor/src/main/assets/native-bridge.js https://raw.githubusercontent.com/tbscode/tims-capacitor/main/android/capacitor/src/main/assets/native-bridge.js"

This approach also works on CI since the post-install script automatically is executed by npm.

I've also tried to rebuild the android package with the fix described in Option 1 though the build process is currently a little to involved for me.

tbscode added a commit to tbscode/tims-stack-anystack that referenced this issue Jun 26, 2023
@kjr-lh
Copy link
Contributor

kjr-lh commented Jun 30, 2023

Just run into this, had to patch our version of capacitor even further (already using #6206 )

Seems really weird that the java code is parsing the json only for the bridge code to stringify it again 🤷

@jcesarmobile
Copy link
Member

should be fixed by #6206

if people is still facing the issue, please, create a new issue and provide a sample app that reproduces the issue

@ionic-team ionic-team locked as resolved and limited conversation to collaborators Jul 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants