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

robertying · 2022-10-24T09:34:13Z

Summary

Fix #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

Clone the react-native repo.
Build the rn-tester app.

Prepare tests

Add android:usesCleartextTraffic="true" to AndroidManifest.xml

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);

Run adb reverse tcp:3000 tcp:3000 to connect the 3000 port on the emulator if necessary.

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);
    });
  }, []);

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 · 2022-10-24T16:26:29Z

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

cipolleschi · 2022-10-25T09:09:26Z

Hi @robertying, Thank you for doing this.
The filtering has been introduced by @dryganets in this PR #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 · 2022-10-25T19:51:19Z

@cipolleschi Thanks for taking a look. The detail was discussed in this issue #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 square/okhttp#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 #21231 in response to the issue square/okhttp#2802. The issue has been discussed in another one square/okhttp#3876, which was fixed by square/okhttp#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.

dryganets

The change looks good to me.
The behavior hasn't been handled by okhttp at the moment.

robertying · 2022-10-28T09:17:09Z

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

cipolleschi · 2022-11-08T09:43:23Z

@robertying Sorry for the delay, busy days.

Can I ask two favours, first:

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
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 · 2022-11-08T19:54:58Z

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

Base commit: b7a85b5
Branch: main

pull-bot · 2022-11-08T20:04:08Z

PR build artifact for 5d31b62 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 · 2022-11-08T20:37:23Z

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: 92b8981
Branch: main

pull-bot · 2022-11-08T20:44:11Z

PR build artifact for 33711f2 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 · 2022-11-08T21:37:53Z

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

facebook-github-bot · 2022-11-14T15:44:14Z

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

cipolleschi · 2022-11-15T16:02:53Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java

      if (headerName == null || headerValue == null) {
        return null;
      }
-      headersBuilder.add(headerName, headerValue);
+      headersBuilder.addUnsafeNonAscii(headerName, headerValue);


We have internal failures due to this line. The internal system is not able to find it.
I'm not an Android expert and I couldn't find from which version of OkHTTP this is available.
Could you help me out a bit here? 🙏

It was added in 3.12.0: https://square.github.io/okhttp/changelogs/changelog_3x/#version-3120
RN is using v4 of okhttp now:

react-native/ReactAndroid/gradle.properties

Line 17 in 5fda72f

OKHTTP_VERSION=4.9.2

Hi @cipolleschi hope you had a great week!

Is there any update on this?

All non-internal builds passed so I suppose the code should have built without problems.

Is the internal build using a different okhttp version than the one on GitHub? If you could attach logs (without sensitive info of course), it would be easier for me to help.

Also, I saw the Linter complained about a warning (not an error). Does this mean it is still mergeable?

Thank you.

Jumping on this.

The build still fails internally. For your reference with this error:

ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java:754: error: cannot find symbol [CONTEXT] headersBuilder.addUnsafeNonAscii(headerName, headerValue); [CONTEXT] ^ [CONTEXT] symbol: method addUnsafeNonAscii(java.lang.String,java.lang.String) [CONTEXT] location: variable headersBuilder of type okhttp3.Headers.Builder

Also, I saw the Linter complained about a warning (not an error). Does this mean it is still mergeable?

Currenty we can't merge this as it is, as our internal builds are red (not sure why they don't show up on Github though).

Thank you @cortinico for looking into this!

So the method couldn't be found when compiled...

The code builds fine on CircleCI though. Is there any particularly special setup for internal builds?

For example, was the build that failed configured with a lower version of OkHttp?

Since I don't have access to the build logs and configurations, what's the best approach to the issue if the code builds fine in public but fails internally?

Thanks!

Edit: I synced with the main branch again to force rerun the jobs, hoping they all passes so it triggers the internal one again. I find the CI sometimes fails abruptly due to broken environments.

pull-bot · 2022-11-28T09:09:01Z

PR build artifact for b5749b6 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 · 2022-11-28T11:59:27Z

PR build artifact for 7ed0489 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 · 2022-11-28T17:43:42Z

PR build artifact for 03fb462 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 · 2023-01-12T08:48:17Z

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 · 2023-01-19T10:51:32Z

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

robertying · 2023-03-09T08:29:18Z

@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 · 2023-03-09T10:49:27Z

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

robertying · 2023-03-10T06:35:20Z

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 · 2023-03-13T14:10:37Z

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.

facebook-github-bot · 2023-03-17T11:51:57Z

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

robertying · 2023-03-17T16:06:41Z

@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.

ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java

cipolleschi · 2023-03-18T19:56:22Z

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 · 2023-03-18T22:29:45Z

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

cipolleschi · 2023-03-20T10:26:00Z

@robertying, there are some internal tests failing, but these are snapshot tests because we changed the layout of HTTP, so they will be easy to fix for me as soon as we have a final version of the diff.

If you can:

rebase and solve conflicts (we are making react-native a proper monorepo, so we moved a bunch of files. Now react-native Core codebase resides in react-native/packages/react-native. You can read more here)
add caching

I can fix the tests and try to land it.

Thank you so much for your patience.

robertying · 2023-03-21T01:23:52Z

@cipolleschi woohoo! All tests passed.

I used the static constructor to initialize the reflected method. Let me know if you think this is okay.

cipolleschi · 2023-03-27T10:14:34Z

/rebase

cipolleschi · 2023-03-27T10:16:26Z

I used the static constructor to initialize the reflected method. Let me know if you think this is okay.

Hello! Changes looks good to me, I'm not a Java expert really, but I think it make sense and it could work!

facebook-github-bot · 2023-03-27T10:17:31Z

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

robertying · 2023-03-28T23:53:53Z

@cipolleschi Thanks! Also, usually the bot should have committed the code by now. Could you take a look when you have time?

cipolleschi · 2023-03-29T14:06:36Z

We are trying to understand if we can actually merge this. There could be performance implications in using reflection and the situation with OkHTTP is blocking other team internally, so we are trying to understand the best path forward.

I'm sorry that this is taking so long to merge. :(

robertying · 2023-03-29T14:36:26Z

We are trying to understand if we can actually merge this. There could be performance implications in using reflection and the situation with OkHTTP is blocking other team internally, so we are trying to understand the best path forward.

I'm sorry that this is taking so long to merge. :(

No worries. Please keep me posted.

cortinico · 2023-03-31T11:33:42Z

@robertying sorry for the delay.
I'm looking into understanding how to handle the OkHTTP version internally so we can merge this. Just to set expectations, I don't believe we'll be able to merge this in the near future. I'll keep on investigating and get back to you once we have a plan forward.

facebook-github-bot · 2023-09-18T10:52:37Z

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

github-actions · 2023-09-26T19:42:37Z

This pull request was successfully merged by @robertying in 7c7e9e6.

^{When will my fix make it into a release? | Upcoming Releases}

facebook-github-bot · 2023-09-26T19:42:41Z

@cortinico merged this pull request in 7c7e9e6.

cortinico · 2023-09-26T19:45:13Z

@robertying sorry for the delay, but we were finally able to ship this change.

robertying · 2023-09-26T22:02:59Z

@robertying sorry for the delay, but we were finally able to ship this change.

@cortinico no worries! Appreciate that you revisited this change and had it merged!

Does this mean there's still an old okhttp version somewhere internal and we have to use reflections as a workaround?

cortinico · 2023-09-27T12:52:37Z

Does this mean there's still an old okhttp version somewhere internal and we have to use reflections as a workaround?

Nope we were able to update the internal usages to OkHTTP 3.12.x

robertying · 2023-10-07T12:37:41Z

Does this mean there's still an old okhttp version somewhere internal and we have to use reflections as a workaround?

Nope we were able to update the internal usages to OkHTTP 3.12.x

Ah I see! I just realized the commit imported was the original one without the reflection workaround, not the current changes listed in this PR. All good now. 👍

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 24, 2022

react-native-bot added the Platform: Android Android applications. label Oct 24, 2022

dryganets reviewed Oct 26, 2022

View reviewed changes

robertying force-pushed the fix-non-ascii-header-value branch from de5ccc9 to 5d31b62 Compare November 8, 2022 18:54

cipolleschi reviewed Nov 15, 2022

View reviewed changes

robertying force-pushed the fix-non-ascii-header-value branch from 33711f2 to b5749b6 Compare November 28, 2022 08:01

robertying force-pushed the fix-non-ascii-header-value branch 4 times, most recently from dd10a95 to 0460aad Compare March 9, 2023 07:36

cipolleschi reviewed Mar 18, 2023

View reviewed changes

ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java Outdated Show resolved Hide resolved

robertying force-pushed the fix-non-ascii-header-value branch 2 times, most recently from 5cf6090 to e869398 Compare March 20, 2023 21:38

cipolleschi approved these changes Mar 27, 2023

View reviewed changes

Allow non-ascii header values on Android & add utf-8 fallback

011075f

github-actions bot force-pushed the fix-non-ascii-header-value branch from e869398 to 011075f Compare March 27, 2023 10:17

facebook-github-bot closed this in 7c7e9e6 Sep 26, 2023

github-actions bot added the Merged This PR has been merged. label Sep 26, 2023

robertying deleted the fix-non-ascii-header-value branch October 7, 2023 12:37

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

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

Conversation

robertying commented Oct 24, 2022 • edited Loading

Summary

Changelog

Test Plan

facebook-github-bot commented Oct 24, 2022

cipolleschi commented Oct 25, 2022

robertying commented Oct 25, 2022 • edited Loading

dryganets left a comment

Choose a reason for hiding this comment

robertying commented Oct 28, 2022

cipolleschi commented Nov 8, 2022

analysis-bot commented Nov 8, 2022 • edited Loading

pull-bot commented Nov 8, 2022

analysis-bot commented Nov 8, 2022 • edited Loading

pull-bot commented Nov 8, 2022

robertying commented Nov 8, 2022

facebook-github-bot commented Nov 14, 2022

cipolleschi Nov 15, 2022

Choose a reason for hiding this comment

robertying Nov 15, 2022

Choose a reason for hiding this comment

robertying Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

cortinico Nov 28, 2022

Choose a reason for hiding this comment

robertying Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

pull-bot commented Nov 28, 2022

pull-bot commented Nov 28, 2022

pull-bot commented Nov 28, 2022

robertying commented Jan 12, 2023

facebook-github-bot commented Jan 19, 2023

robertying commented Mar 9, 2023

facebook-github-bot commented Mar 9, 2023

robertying commented Mar 10, 2023

cipolleschi commented Mar 13, 2023

facebook-github-bot commented Mar 17, 2023

robertying commented Mar 17, 2023

cipolleschi commented Mar 18, 2023

robertying commented Mar 18, 2023

cipolleschi commented Mar 20, 2023

robertying commented Mar 21, 2023

cipolleschi commented Mar 27, 2023

cipolleschi commented Mar 27, 2023

facebook-github-bot commented Mar 27, 2023

robertying commented Mar 28, 2023

cipolleschi commented Mar 29, 2023 • edited Loading

robertying commented Mar 29, 2023

cortinico commented Mar 31, 2023

facebook-github-bot commented Sep 18, 2023

github-actions bot commented Sep 26, 2023

facebook-github-bot commented Sep 26, 2023

cortinico commented Sep 26, 2023

robertying commented Sep 26, 2023

cortinico commented Sep 27, 2023

robertying commented Oct 7, 2023

robertying commented Oct 24, 2022 •

edited

Loading

robertying commented Oct 25, 2022 •

edited

Loading

analysis-bot commented Nov 8, 2022 •

edited

Loading

analysis-bot commented Nov 8, 2022 •

edited

Loading

robertying Nov 28, 2022 •

edited

Loading

robertying Nov 28, 2022 •

edited

Loading

cipolleschi commented Mar 29, 2023 •

edited

Loading