[Android] Allow non-ascii header values & add utf-8 filename fallback

[robertying]

Summary

Fix https://github.com/facebook/react-native/issues/31537: [Android] React Native strips non-ASCII characters from HTTP headers

Changelog

[Android] [Changed] - Allow non-ascii header values on Android and add utf-8 filename fallback in FormData

Test Plan

1. Clone the react-native repo.
2. Build the rn-tester app.
3. Prepare tests
   1. Add android:usesCleartextTraffic="true" to AndroidManifest.xml
   2. Use the following code as a server:

      const http = require('http');
      
      const requestListener = function (req, res) {
          // raw header value
          console.log(req.headers['content-disposition']);
          // nodejs assumes the header value is ISO-8859-1 encoded
          console.log(Buffer.from(req.headers['content-disposition'], 'latin1').toString('utf-8'));
          // decode encoded header value if it's sent as UTF-8
          console.log(decodeURI(req.headers['content-disposition']));
          res.writeHead(200);
          res.end();
      };
      
      const server = http.createServer(requestListener);
      server.listen(3000);
      

   3. Run adb reverse tcp:3000 tcp:3000 to connect the 3000 port on the emulator if necessary.
4. Edit RNTesterAppShared.js to include test code:

     useEffect(() => {
       fetch('http://localhost:3000/', {
         headers: {
           'Content-Type': 'multipart/form-data; charset=utf-8',
           'Content-Disposition': `attachment; filename*=utf-8''${encodeURI(
             'filename测试abc.jpg',
           )}`,
         },
       }).then(res => {
         console.log(res.ok);
       });
       fetch('http://localhost:3000/', {
         headers: {
           'Content-Type': 'multipart/form-data; charset=utf-8',
           'Content-Disposition': `attachment; filename="filename测试abc.jpg"`,
         },
       }).then(res => {
         console.log(res.ok);
       });
     }, []);
   

5. Both requests should succeed; without the fix, the second request received by the server will not have the utf-8 characters "测试" in the header value.

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

Hi @robertying, Thank you for doing this. The filtering has been introduced by @dryganets in this PR https://github.com/facebook/react-native/pull/21231 4 years ago.

We are wondering whether this change could break some webservers that don’t understand the utf8- marker. @dryganets, could you help out with the review?

[robertying]

@cipolleschi Thanks for taking a look. The detail was discussed in this issue https://github.com/facebook/react-native/issues/31537 when I first opened it:

As I investigate, I find lines of code that should be responsible ebc89bf/ReactAndroid/src/main/java/com/facebook/react/modules/network/HeaderUtil.java.

These methods are to weaken okhttp's too strict rules on headers, but also bring this unwanted behavior.

FYI, okhttp has supported non-ASCII headers from https://github.com/square/okhttp/pull/4296.

These two stripping methods may need to be looked into and synced with the upstream to reflect okhttp's changes.

This change was brought by https://github.com/facebook/react-native/pull/21231 in response to the issue https://github.com/square/okhttp/issues/2802. The issue has been discussed in another one https://github.com/square/okhttp/issues/3876, which was fixed by https://github.com/square/okhttp/pull/4296.

The filtering was done before okhttp fixed the utf-8 thing. Since okhttp has already fixed the utf-8 header value handling, we should remove the workaround that is not invalid anymore.

---

As for whether it will break web servers, this usage is actually included in RFC 5987:

This specification suggests that a parameter using the extended syntax takes precedence. This would allow producers to use both formats without breaking recipients that do not understand the extended syntax yet.

Example:

 foo: bar; title="EURO exchange rates";
           title*=utf-8''%e2%82%ac%20exchange%20rates

There is also a note:

Note: at the time of this writing, many implementations failed to ignore the form they do not understand, or prioritize the ASCII form although the extended syntax was present.

However, the RFC was written in 2010 and I believe utf-8 support on the server side should be adequate now; and even if the server doesn't understand the extended syntax, it should be able to pick up the original filename as it used to.

[robertying]

@cipolleschi it seems we could move forward since Sergei has commented. Please let me know if there's anything else that needs addressing!

[cipolleschi]

@robertying Sorry for the delay, busy days.

Can I ask two favours, first:

1. Could you go to https://app.circleci.com and log into their website? This should restart the CI pipelines. I know it is annoying, I'm working with them so that we can automate this step
2. Could you rebase onto main?

After that, when the CI is green, I can reimport it!

Thank you so much and sorry again for the delay.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: b7a85b59b5798add4e9dbfb5f5f2fc62756e30b5 Branch: main

[pull-bot]

PR build artifact for 5d31b627c902df6a9ceda954e70c012d2f45dce9 is ready. To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,519,584	+172
android	hermes	armeabi-v7a	7,835,878	+171
android	hermes	x86	8,998,729	+178
android	hermes	x86_64	8,854,890	+175
android	jsc	arm64-v8a	9,140,777	+213
android	jsc	armeabi-v7a	8,332,995	+218
android	jsc	x86	9,194,447	+213
android	jsc	x86_64	9,453,529	+219

Base commit: 92b898149956a301a44f99019f5c7500335c5553 Branch: main

[pull-bot]

PR build artifact for 33711f2e87b259fc15a17340fa1f0f185e468f7f is ready. To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[robertying]

@cipolleschi No problem! Appreciate the work you've been doing. I fixed some issue and all checks have passed.

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[pull-bot]

PR build artifact for b5749b659910284e6895520542d6a5cbb20eb4fb is ready. To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[pull-bot]

PR build artifact for 7ed0489454def48b3ab9c9f4088ae66038e78658 is ready. To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[pull-bot]

PR build artifact for 03fb4624a5aa433d539d88e8a0d524252970138b is ready. To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[robertying]

Want to bump this PR. @cipolleschi @cortinico would you mind importing this again to see how it behaves in internal builds?

The fix passed all external checks but the internal ones, making it impossible for me to find the problem myself.

This utf-8 fix, however, is important to developers in lots of non-English speaking countries and I wish it could be handled soon.

Thank you!

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[robertying]

@cipolleschi would you mind importing the diff again? Want to see if there's any internal update that might have solved the linter issue. Thank you!

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[robertying]

Ok so I'm convinced some Meta internal apps are still using an old version of okhttp that is not caught up with the open source version (v4.9.2).

To be honest, this feels so weird because it basically means we cannot use any newer features introduced in okhttp in open source React Native even if this repo has set its okhttp version to be v4.

I've done what I can, but only people at Meta can fix this issue.

[cipolleschi]

Hi @robertying, sorry for the long silence. We were discussing this feature internally and, while we are looking to try and understand what needs to be done to make it work, we were exploring alternatives.

Do you think would it be possible to make this feature an opt-in? 🤔

So, for example, if we switch a flag in the gradle.properties people can start using this feature.

How do you feel about that? This would make much easier to land this as we will be preserving the base default behavior (and internal system won't break) but supporting the use case presented here.

[robertying]

Hi @robertying, sorry for the long silence. We were discussing this feature internally and, while we are looking to try and understand what needs to be done to make it work, we were exploring alternatives.

Do you think would it be possible to make this feature an opt-in? 🤔

So, for example, if we switch a flag in the gradle.properties people can start using this feature.

How do you feel about that? This would make much easier to land this as we will be preserving the base default behavior (and internal system won't break) but supporting the use case presented here.

Thanks for writing this @cipolleschi. I agree this is the best approach for now before internal tests catch up.

I can try coming up with an improved fix. Cheers.

[robertying]

Since the usage of the new okhttp's method is inside React Native's network module, I can't think of a way for the downstream (apps that use RN) to simply change a flag to make it work.

Downstream needs to either switch to another build artifact (aar) that was pre-compiled, or compile RN from source.

Do you have any suggestion as to how to achieve this in an easier way?

[dryganets]

I'd compile from source. The alternative is to replace the module with your's implementation.

[robertying]

If compiling from source, would it be better for people who need this feature to build from source themselves and consume artifacts?

If we add a flag and somehow switch to compiling from source when the flag is enabled, it requires the same if not more work to adopt this change than a custom build.

[cipolleschi]

So, I investigated internally a bit and making it build from source is not a possibility.

Another approach we can take could be to use reflection: if the headersBuilder has the addUnsafeNonAscii we use it, otherwise, we fall back to the old behavior.

How do you feel about that. It would be maybe simpler to implement.

[robertying]

@cipolleschi great idea! I've updated the code. Can you try importing it to see how it goes internally?

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[robertying]

@cipolleschi I don't see any changes you requested. Did you mean to have dedicated functions to check?

Also, I'm not sure how internal tests are still failing. 😅 I tested all these GitHub checks with a lower version of okhttp v3.11.0 and they all passed.

[cipolleschi]

My bad, I probably closed the page before submitting the suggestions... 🤦 I added the comment now. We should cache what we can to avoid expensive operation. There could be potential multi-threading issues, so it could make sense to synchronize access on the new variables... But I'll first try this out.

[robertying]

@cipolleschi I see what you mean. In the meantime, can you check the linter error that was reported internally?