react / react/react-native

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

Aperta
#34,883 4 commenti 0 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

🌐Networking Needs: Triage :mag: Never gets stale Platform: iOS
Lingua principale
C++
Stelle
127k
Fork
25.3k
Merge medio
1g 23h
PR unite (30g)
4

Descrizione

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: https://github.com/facebook/react-native/issues/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): https://github.com/facebook/react-native/blob/b9c297537a5eda68621ba77ead272cfd8b4c6e2c/Libraries/Network/RCTNetworking.mm#L302

This NSURLRequest makes its way down to an NSURLSessionTask in RCTHTTPRequestHandler.mm and that task is eventually run: https://github.com/facebook/react-native/blob/b9c297537a5eda68621ba77ead272cfd8b4c6e2c/Libraries/Network/RCTHTTPRequestHandler.mm#L108

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: https://github.com/facebook/react-native/commit/5ed6ac1f25b0b793c74ffc23005a82530618eb55 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.

Guida per i contributori

Apri la guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Direzione di ricerca

Riproduci il problema con il progetto RNDuplicateRequestsBug collegato e ispeziona Libraries/Network/RCTNetworking.mm e RCTHTTPRequestHandler.mm, in particolare buildRequest e il percorso delle attività NSURLSession. Confronta il comportamento attuale con la documentazione di Foundation su authentication-challenge e con il PoC della versione 0.70.2. Il lavoro è completato quando un 401 Basic-auth non produce alcuna richiesta autenticata duplicata e il risultato corretto raggiunge il livello JS.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Valutazione

Stack tecnologico
ios, objective-c
Ambito
mobile-dev, networking
Tipo di issue
Bug
Difficoltà
5/5
Tempo stimato
Più di una settimana
Stato di attività
Ferma
Chiarezza
Abbastanza chiara
Idoneità per principianti
25/100

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.