New issue
Advanced search Search tips

Issue 881297 link

Starred by 1 user
Status: WontFix
Owner:
droger@chromium.org
Closed: Sep 6
Cc:
msarda@chromium.org
mmenke@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Audit uses of SimpleURLLoader in signin

Project Member Reported by droger@chromium.org, Sep 6 
As mentionned here:
https://bugs.chromium.org/p/chromium/issues/detail?id=876306#c12

SimpleURLLoader now considers HTTP response codes that is not 2XX to be a network error.

This was not taken into account when the migration to SimpleURLLoader was done (at least by reviewers).

I might affect backoff algorithms (we may now retry when we receive 401s for example, which is not expected), and also error handling (401s in particular may be treated like connection errors, which could lead to Chrome not noticing that credentials are expired).

There is no known actual example for such bugs, but the code should probably be audited.

Comment 1 by droger@chromium.org, Sep 6

Components: Internals>Network Services>SignIn

Comment 2 by droger@chromium.org, Sep 6 

Here is a concrete example:

https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.cc?rcl=d30bc83490e3fad014f50e37d650a67644fc7d93&l=987

Authentication errors (401) would be treated like connections errors now.

Comment 3 by droger@chromium.org, Sep 6 

GaiaAuthFetcher should be fine, because SetAllowHttpErrorResults(true) is called here:
https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.cc?rcl=3006696ff98d6a8fe7063de33d9e5799ef959e2d&l=292

Comment 4 by droger@chromium.org, Sep 6 

I checked all uses of SimpleURLLoader in google_apis, components/signin and chrome/browser/signin and most of them have SetAllowHttpErrorResults(true).

The only one that does not have it is ExternalCcResultFetcher.
This one treats non-2XX codes like network errors, so it does not need that bit to be set.
The code handling the network response is:
  if (source->NetError() != net::OK || !source->ResponseInfo() ||
      !source->ResponseInfo()->headers ||
      source->ResponseInfo()->headers->response_code() != net::HTTP_OK) {
    return;
  }

https://cs.chromium.org/chromium/src/components/signin/core/browser/gaia_cookie_manager_service.cc?rcl=104a2ebeb89e7e4b9ea2c34ae173261d1812072f&l=291

I wonder if we could remove the test for response_code(). It would only make a difference if we receive a 2XX response that is not 200. It's probably not worth the effort to dig into this though, since it's unlikely to really matter.

Comment 5 by droger@chromium.org, Sep 6

Status: WontFix (was: Assigned)
Ok, I guess we can close this now, false alarm!
►

Sign in to add a comment