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

iOS - Duplicate HTTP requests occur when an authorisation challenge is returned (401 with www-authenticate header) #34883

Open
liamjones opened this issue Oct 6, 2022 · 4 comments
Labels
Needs: Triage 🔍 🌐Networking Related to a networking API. Never gets stale Prevent those issues and PRs from getting stale Platform: iOS iOS applications.

Comments

@liamjones
Copy link
Contributor

Description

RN's iOS code is not working with authentication challenges in the way the Foundation framework expects and this results in extraneous, duplicate requests to login when bad credentials are presented to the server. It seems this was first noticed by someone in 2015 but didn't go anywhere: #2266

In our instance, this is causing us issues with server-side brute force protection on login as iOS users get half the number of real-world attempts to log in, compared to Android users, before they hit our brute force limit.

The documentation for NSURLRequest states that, among others, Authorization is a reserved header and that:

If you set a value for one of these reserved headers, the system may ignore the value you set, or overwrite it with its own value, or simply not send it. Moreover, the exact behaviour may change over time. To avoid confusing problems like this, do not set these headers directly.

(see https://developer.apple.com/documentation/foundation/nsurlrequest?language=objc#1776617)

So, this header should not be set directly by RN but currently is.

Current code path

If an Authorization header is set in JS, the JS layer passes this on to RCTNetworking.sendRequest: https://github.com/facebook/react-native/blob/v0.70.2/Libraries/Network/XMLHttpRequest.js#L584

sendRequest then calls buildRequest and this copies all the headers onto the NSURLRequest (including all the reserved headers):

// Set supplied headers.

This NSURLRequest makes its way down to an NSURLSessionTask in RCTHTTPRequestHandler.mm and that task is eventually run:

RCTHTTPRequestHandler is an NSURLSessionDataDelegate but doesn't implement the authentication challenge delegate methods so the NSURLSession runs its default handling which results in the duplicate 401 request (seemingly because Foundation did not expect the Authorization header to have been set directly on the NSURLRequest).

Foundation's expected code path

What Foundation expects you to do is documented here: https://developer.apple.com/documentation/foundation/url_loading_system/handling_an_authentication_challenge?language=objc

Essentially:

  1. The reserved Authorization header (and other reserved headers) should not be populated on the NSURLRequest directly.
  2. The NSURLSessionTask runs and requests the specified URL with no credentials, it received a 401 response from the server.
  3. RCTHTTPRequestHandler (or another NSURLSessionDelegate) picks up the authentication challenge via URLSession:task:didReceiveChallenge:completionHandler: and provides an NSURLCredential for the completion handler to use.
  4. If this succeeds, all is good. If it still 401s then the URLSession:task:didReceiveChallenge:completionHandler: handler is called again but this time it has the context of challenge.proposedCredential and challenge.previousFailureCount so can detect that the NSURLCredential was denied and change its behaviour as a result (the handler does not get the context of a previous failure if a raw Authorization header was added).

Naive patch for this issue

I've implemented a naive patch in the PoC mentioned below: https://github.com/liamjones/RNDuplicateRequestsBug/blob/main/patches/react-native%2B0.70.2.patch

I'm pretty rusty on Objective-C so I took the simplest route (to me) to play with ways of fixing this. In summary the patch:

  1. Renames the Authorization header to Was-Authorization in buildRequest so it doesn't trigger a 401 with credentials on the server (it instead generates a 401 with no credentials - this stops it from recording a strike against our account brute force protection counter)
  2. Implements URLSession:task:didReceiveChallenge:completionHandler: in RCTHTTPRequestHandler to deal with the 401. This then goes through checking that we're dealing with a Basic auth request and, if we've not dealt with it already, grabs the Was-Authorization header off the request, base-64 decodes and splits it to retrieve the username and password in plaintext so it can use them to construct an NSURLCredential.

If URLSession:task:didReceiveChallenge:completionHandler: gets called a second time with the challenge.proposedCredential and challenge.previousFailureCount it uses these as a signal to abort because the credentials are wrong. It feels like what should happen is I call completionHandler(NSURLSessionAuthChallengeCancelAuthenticationChallenge, nil); but, with the current RN setup that results in a 'Network request failure' promise rejection in the JS layer. If instead, I say completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace, nil); - essentially, we can't do Basic anymore - the 401 does correctly make its way up to the JS layer. A side effect is that we end up with 3 401 requests now:

  1. 401 without credentials
  2. 401 with the NSURLCredential
  3. 401 without credentials again - I assume this one comes from the fact we've said "we can't do Basic auth anymore" and the NSURLSession is trying to work out if there's another auth challenge method that could be used (such as NTLM).

This obviously isn't ideal (using the renamed header to pass info around is clunky and this renamed header will still be sent to the server, etc) but fixing this properly is beyond my rusty Objective-C skills/the time I have available currently.

There's one extra wrinkle with this solution;

in the 0.70.2 PoC, it's working fine with correct credentials that result in a 200. You get the initial 401 with no credentials, then the 200 with credentials, and this 200 response is returned up to the JS layer.

However, it's not working correctly with our real-world 0.66.3 app. Upon a successful 200 response, it's the original 401 without credentials that ends up in the JS layer.

I'm guessing this is either a race condition (PoC = light and fast, our app = heavier and slower?) or something that's changed between 0.66.3 and 0.70.2 which has changed behaviour. Upgrading our app to 0.70.2 is something I want to do but is not simple due to various native dependency conflicts/upgrades I need to sort out.

I had a quick look at what had changed in Libraries/Network between 0.66.3 and 0.70.2 but the only commit that looked potentially relevant was this: 5ed6ac1 I backported the change to 0.66.3 but it still didn't resolve the 200 issue in our app. So it's either a race condition or a change somewhere else in RN which has altered behaviour between those versions.

Version

0.70.2 & 0.66.3

Output of npx react-native info

System:
OS: macOS 12.6
CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
Memory: 205.13 MB / 32.00 GB
Shell: 5.9 - /usr/local/bin/zsh
Binaries:
Node: 18.10.0 - /usr/local/bin/node
Yarn: 1.22.19 - /usr/local/bin/yarn
npm: 8.19.2 - /usr/local/bin/npm
Watchman: 2022.10.03.00 - /usr/local/bin/watchman
Managers:
CocoaPods: 1.11.3 - /usr/local/bin/pod
SDKs:
iOS SDK:
Platforms: DriverKit 21.4, iOS 16.0, macOS 12.3, tvOS 16.0, watchOS 9.0
Android SDK:
API Levels: 23, 25, 26, 27, 28, 29, 30, 31, 32
Build Tools: 27.0.3, 28.0.3, 29.0.2, 29.0.3, 30.0.1, 30.0.2, 30.0.3, 31.0.0, 31.0.0
System Images: android-24 | Google APIs Intel x86 Atom, android-24 | Google Play Intel x86 Atom, android-25 | Google APIs Intel x86 Atom, android-25 | Google Play Intel x86 Atom, android-26 | Google Play Intel x86 Atom, android-27 | Google Play Intel x86 Atom, android-28 | Google APIs Intel x86 Atom, android-28 | Google Play Intel x86 Atom, android-29 | Google APIs Intel x86 Atom, android-30 | Google APIs Intel x86 Atom, android-30 | Google Play Intel x86 Atom, android-31 | Google APIs Intel x86 Atom_64, android-31 | Google Play Intel x86 Atom_64, android-32 | Google Play Intel x86 Atom_64, android-33 | Google Play Intel x86 Atom_64, android-S | Google Play Intel x86 Atom_64, android-Tiramisu | Google Play Intel x86 Atom_64
Android NDK: Not Found
IDEs:
Android Studio: Dolphin 2021.3.1 Dolphin 2021.3.1
Xcode: 14.0.1/14A400 - /usr/bin/xcodebuild
Languages:
Java: 11.0.15 - /Users/liam.jones/.sdkman/candidates/java/11.0.15-tem/bin/javac
npmPackages:
@react-native-community/cli: Not Found
react: 18.1.0 => 18.1.0
react-native: 0.70.2 => 0.70.2
react-native-macos: Not Found
npmGlobalPackages:
react-native: Not Found

Steps to reproduce

  1. Issue a fetch request with an Authorization: Basic ... header to a server endpoint which will return with a 401 and WWW-Authenticate: Basic header.
  2. Observe network logs in some way, see 2 401s with the credentials were sent to the server rather than 1

This only happens on iOS, not Android.

Example code:

  fetch('https://mockbin.org/bin/3275aeb4-a08d-43cd-9ed1-7c2410277124', {
    headers: {
      Authorization: `Basic ${new Buffer('bad@example.org:password').toString('base64')}`,
    },
  })

Snack, code example, screenshot, or link to a repository

https://github.com/liamjones/RNDuplicateRequestsBug

yarn, yarn start, yarn ios and tap the buttons in the UI. The 401 button will result in 2 network requests. Tapping the 200 button will result in 1 request. The Mockbin /view URLs in the App.tsx can be used to see the requests (or you can reconfigure the URLs to point somewhere else if you don't want your request IP on a public page).

If changing the URLs; the important thing is that the 401 URL returns a 401 with a WWW-Authenticate: Basic with each request (to simulate the first request's credentials being rejected).

Running yarn patch-package followed by yarn ios again will apply a naive patch to RN which stops the duplicate requests with credentials against the 401 endpoint (but does result in an extra request to the endpoint without the credentials) - explanation of the patch is in the description above.

@react-native-bot react-native-bot added Platform: iOS iOS applications. 🌐Networking Related to a networking API. labels Oct 6, 2022
@ddikodroid
Copy link
Contributor

I encountered AxiosError 401 on iOS 16 when trying to login. Strange thing is I tried on iOS 15.5 and Android, everything is working fine.

Do you think it's related to your issue?

@liamjones
Copy link
Contributor Author

Do you think it's related to your issue?

@ddikodroid I don't use axios currently but it's probably not related. Axios is in the JS layer and RN correctly presents a single 401 or 200 to the JS layer depending on if login was successful or not. This issue just means that, when credentials are incorrect, two 401s happen in the native layer on iOS before the single result is returned to the JS layer.

@github-actions
Copy link

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label May 12, 2023
@liamjones
Copy link
Contributor Author

This is still how it works in RN currently.

@cortinico cortinico added Never gets stale Prevent those issues and PRs from getting stale and removed Stale There has been a lack of activity on this issue and it may be closed soon. labels May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Triage 🔍 🌐Networking Related to a networking API. Never gets stale Prevent those issues and PRs from getting stale Platform: iOS iOS applications.
Projects
None yet
Development

No branches or pull requests

4 participants