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

OkHttp is more strict than other http libraries. #21231

Conversation

dryganets
Copy link
Contributor

It crashes with IllegalStateException in case you pass a wrong URL.
It crashes if it meets unexpected symbols in the header name and value,
while standard says it is not recommended to use those symbols not that
they are prohibited.

The headers handing is a special use case as a client might have an auth token in the header. In this case, we want to get 401 error response
from the server to find out that token is wrong. In case of the onerror
client will continue to retry with an existing token.

Test Plan:

Verify that android app receives an error instead of crashing in case of following network request:
"contacts/v2/users/"
Verify the case with passing the headers with

Release Notes:

[ANDROID] [BUGFIX] [Networking] - Passing invalid URL not crashes the app instead onerror callback of HttpClient is called. Invalid symbols are stripped from the headers to allow HTTP query to fail with 401 error code in case of the broken token.

It crashes with IllegalStateException in case you pass wrong URL.
It crashes if it meets unexpected symbols in the header name and value,
while standard says it is not recommneded to use those symbols not that
they are prohibited.

The headers handing is a special use case as client might have an auth
tokens in the header. In this case we want to get 401 error response
from the server to find out that token is wrong. In case of the onerror
client will continue to retry with an existing token.

Verify that android app receives an error instead of crashing in case of following network request:
"contacts/v2/users/"
Verify the case with passing the headers with

Release Notes
[ANDROID] [BUGFIX] [Networking] - Passing invalid url not crashes the app instead onerror callback of HttpClient is called. Invalid symbols are stripped from the headers to allow http query to fail with 401 errorcode in case of the broken token.

@MsSourceId: ede64de66f9bb19ff1253eabe872f7d3fd60c98c
@facebook-github-bot 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 Sep 20, 2018
@dryganets
Copy link
Contributor Author

This is a workaround for the problem.

You can refer to this okhttp issue:
square/okhttp#2802
Square is not going to fix it.

The ugly part of this PR comes from the validation of the headers in okhttp.
Other http libraries are able to send HTTP request to the server. As token is corrupt server returns 401 and everything is handled correctly on js side as a result.
In case of rejecting the promise js code won't be able to handle the situation correctly.

@react-native-bot react-native-bot added 🌐Networking Related to a networking API. Core Team labels Sep 20, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@hramos hramos added Partner Contributor A React Native contributor. and removed Core Team labels Mar 8, 2019
@dryganets
Copy link
Contributor Author

@hramos, did it merged to the master?
looks like you've been trying to import it but PR is still open.

@hramos
Copy link
Contributor

hramos commented Mar 21, 2019

I looked at this and it's unclear why it wasn't landed. It was approved back in October. I'm going to rebase and run it through our tests again, then land.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @dryganets in aad4dbb.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Mar 21, 2019
kelset pushed a commit that referenced this pull request Mar 29, 2019
Summary:
It crashes with IllegalStateException in case you pass a wrong URL.
It crashes if it meets unexpected symbols in the header name and value,
while standard says it is not recommended to use those symbols not that
they are prohibited.

The headers handing is a special use case as a client might have an auth token in the header. In this case, we want to get 401 error response
from the server to find out that token is wrong. In case of the onerror
client will continue to retry with an existing token.

[ANDROID][Fixed] - Networking: Passing invalid URL not crashes the app instead onerror callback of HttpClient is called. Invalid symbols are stripped from the headers to allow HTTP query to fail with 401 error code in case of the broken token.

Pull Request resolved: #21231

Reviewed By: axe-fb

Differential Revision: D10222129

Pulled By: hramos

fbshipit-source-id: b23340692d0fb059a90e338fa85ad4d9612275f2
@TheSavior TheSavior added p: Microsoft Partner: Microsoft and removed Partner p: Microsoft Partner: Microsoft labels Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. 🌐Networking Related to a networking API. p: Microsoft Partner: Microsoft Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants