New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 808018 link

Starred by 10 users

Issue metadata

Status: Fixed
Owner:
Leaves the project on 2018/03/02
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Regression

Blocked on:
issue 808995



Sign in to add a comment

XMLHttpRequest.open sets user and password for to null

Reported by kuettner...@gmail.com, Feb 1 2018

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.119 Safari/537.36

Steps to reproduce the problem:
0. Set-up web server with basic authentication
1. Call xhr.open('GET', 'foo', true, null, null)
2. xhr.send()
3. Observe 401 in console (http://null:null@localhost:8080/foo)
4. Refresh page and you have to enter user/pw again despite still being in the same session

What is the expected behavior?
According to https://xhr.spec.whatwg.org/#the-open()-method user and password set to null shall be ignored and the cached session information should be used.

What went wrong?
Sending a XHR with user and password set to null resets the cached session authentication and replaces it with null:null.
Can be reproduced in Chrome 64 with the attached file and a simple web server with basic authentication.

Did this work before? Yes Chrome 63

Chrome version: 64.0.3282.119  Channel: stable
OS Version: Ubuntu 18.10
Flash Version: 

Also tested on the latest build of Chromium (66.0.3337.0) as of today an can be reproduced as well.
 
index.html
228 bytes View Download

Comment 1 by kochi@chromium.org, Feb 1 2018

Components: -Blink Blink>Network>XHR
Labels: Needs-Bisect Needs-Triage-M64
Cc: vamshi.k...@techmahindra.com
Labels: Triaged-ET Needs-Feedback
Checked the issue on reported chrome version 64.0.3282.119 and on the latest canary 66.0.3336.0 using Ubuntu 14.04 with the below mentioned steps.
1. Launched Chrome and terminal
2. In terminal gave the following command and started a web server python -m SimpleHTTPServer 8000
3. We have run http://localhost:8000/index.html in chrome window (Given index.html file saved in documents)
4. Opened Dev tools and we have run the following commands given in comment#0
         *xhr.open('GET', 'foo', true, null, null)
         *xhr.send()
We observed Error 404 in console, Attaching the screen cast of the same.

@Reporter: Could you please check the screen cast and let us know if we have missed any steps while reproducing the issue. Any further inputs from your end may help us to triage the issue in a better way.

Thanks! 
808018.mp4
2.6 MB View Download
You set up a simple web server without(!) basic authentication. You missed step 0 ;)
The problem only occurs if you actually have to provide username/password.

For my setup I used this python module https://github.com/tianhuil/SimpleHTTPAuthServer
and started it with 
python -m SimpleHTTPAuthServer 8000 user:pass

I hope this clarifies things.

Thanks for looking into it!
Project Member

Comment 5 by sheriffbot@chromium.org, Feb 2 2018

Labels: -Needs-Feedback
Thank you for providing more feedback. Adding requester "vamshi.kommuri@techmahindra.com" to the cc list and removing "Needs-Feedback" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 6 by ricea@chromium.org, Feb 2 2018

Status: Available (was: Unconfirmed)
Reproduced. I also verified that null:null is sent to the remote server as the username and password.

I verified that Firefox does not have the issue.


Comment 7 by ricea@chromium.org, Feb 2 2018

Components: Blink>Bindings
Labels: -Needs-Bisect
Owner: tyoshino@chromium.org
Status: Assigned (was: Available)
I've confirmed it was working in 4edb29b3035c2fd521916575e8bc4a9b667f2d65, r520039 and broken in the next CL, 8e8e51585e58e81b4149dc1fc4479f039d4cc5d5, r520040.

Description of the first broken CL is

"""
Update remained DOMString types with ByteString or USVString in the XMLHttpRequest
    
    r517781 replaced DOMString with them partially. So this CL update remained things
    with ByteString or USVString based on the latest specification.
    
    Spec: https://xhr.spec.whatwg.org/#the-send()-method
          https://github.com/whatwg/xhr/commit/cea2b9e1187921453434bc245208660edefa4c58
          https://github.com/whatwg/xhr/commit/cbbc56ab2dc5049eb59c69f760038d62b3439cca
"""

Assigned to the reviewer, tyoshino@chromium.org, as email to the author, gyuyoung.kim@lge.com is bouncing.

It looks like the underlying issue might be with bindings as "DOMString? user" worked but "USVString? user" does not. Adding the Blink>Bindings component.accordingly.
Cc: gyuyoung...@chromium.org
Trying to CC Gyuyoung's chromium.org email address.
Cc: ricea@chromium.org jbroman@chromium.org yukishiino@chromium.org haraken@chromium.org
OK, this is really a bindings issue that happens because we haven't fully converted the handling of the WebIDL string types to NativeValueTraits:
* "DOMString? user" invokes V8StringResource<kTreatNullAndUndefinedAsNullString>::Prepare() to convert |user| to a C++ type. The implementation considers a null v8::Local<v8::Value> as invalid and falls back to returning an empty WTF::String, which works as expected.
* "USVString? user" invokes NativeValueTraits<IDLUSVString>::NativeValue(), which just calls ToUSVString(). ToUSVString() does not special-case null and undefined, and essentially converts null into a string (the literal, 4-byte string "null").

I can look into it next week (I'm also CC'ing a few other bindings folks), but might want to consider reverting the original CL in the meantime (especially in M64 and M65) while this is properly dealt with (note this would very likely need involvement from a Googler).
Cc: yhirano@chromium.org
Labels: -Pri-2 OS-Android OS-Chrome OS-Fuchsia OS-Mac OS-Windows Pri-1
I'll also take a look next week.

For the time being, I've made a revert patch at: https://chromium-review.googlesource.com/c/chromium/src/+/899382 .

XHR experts, could you tell us how much severe this issue is?  M64 has already been released as stable, but we would need post-stable merge for M64.

I forgot to add BUG= line in the patch.
The revert patch has got landed: https://chromium-review.googlesource.com/c/chromium/src/+/899382

@raphael, do you have a plan to fix the null checking problem of ToUSVString() this week? (Thank you for checking this issue!)
The original change could be re-landed once the issue in the IDL bindings has been fixed.

We should merge the revert to merge this at least to M65, and M64 if possible.

There could be libraries that specifies null to the username and password argument as a default. It's bad that auth by cached credentials is broken when such a library is in use.

Needs to bake the revert in canary first.
Gyuyoung: I do. Actually, it looks like both yukishiino@ and I do so it will be fixed in a way or another :-)
Rakuko: great! Let me try to re-land after you guys fix it.
Rakuco, may I ask you to support the USVString version?
Sure, I'm playing with a few different alternatives and will send a CL soon.
Cc: tyoshino@chromium.org
Owner: raphael....@intel.com
Status: Started (was: Assigned)
Cc: -tyoshino@chromium.org raphael....@intel.com
Owner: tyoshino@chromium.org
This issue should be used for addressing the regression of XHR.open (user/password args).  Rakuco (and Gyuyoung), could you file a new issue and use it (or reuse the original issue of changing DOMString to USVString)?

Blockedon: 808995
I've filed  bug 808995 .
Labels: Merge-Request-65
r534319 by yukishiino@ is included from branch 3341. The latest canary is at 3342. I don't see any new crash that looks caused by the revert.

REGEXP_CONTAINS(product.name, '^Chrome')
  AND REGEXP_CONTAINS(product.version, '^(66)\\.')
  AND crash.reason != 'EXCEPTION_BREAKPOINT'
  AND crash.reason != 'Out of Memory'
  AND expanded_custom_data.ChromeCrashProto.malware_verdict = false
  AND EXISTS (SELECT 1 FROM UNNEST(CrashedStackTrace.StackFrame)
    WHERE REGEXP_CONTAINS(FunctionName, '^blink::XMLHttpRequestV8Internal'))

Baked. Requesting merge.

 Bug 808995  has been fixed. Nullable ByteStrings and USVStrings are now handled correctly, so the original XHR IDL change can be re-landed.
Project Member

Comment 25 by sheriffbot@chromium.org, Feb 8 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: M65 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

r534319 is a revert. How safe it will be to merge this revert to M65? 
This is M64 regression, can't this wait until M66?
govind@, could you please elaborate your concern? Yes, it's a revert. Just changing the conversion logic between a string in JavaScript and a string in Blink for some corner cases. I think it wouldn't cause any big compatibility issue.

This bugs has broken some apps with XMLHttpRequest and the HTTP authentication. I'm receiving reports from them. At least I'd like to reduce the outage by merging it to beta if possible.
Hmm, looks if the web apps can be updated, replacing null with undefined would work as a workaround.

Those who suffered from the issue, could you please try that?
Replacing null with undefined as a default value works fine. 

I encountered null as a default in the popular d3-js library.

Merging this fix in the soonest possible release would be desirable.
Thanks for the input. I understand that it would be burden for developers.
Labels: -Merge-Review-65 M-65 Merge-Approved-65
Approving merge to M65 branch 3325 based on comments #27, #29 and #30. Please merge ASAP. Thank you.
Labels: -Merge-Approved-65 merge-merged-3325
I've merged as:
https://chromium-review.googlesource.com/c/chromium/src/+/912068

I'm sorry that I forgot to add BUG= line again, so the merge CL doesn't show up here automatically.

No worries and thank you.
Labels: Merge-Request-64
Web sites that are using XHR.open as part of authentification have started failing, and it's pretty severe, I think.  Users and web developers cannot wait for M65 becoming stable in one month.  I think that we should consider merging the fix into M64, too.
Cc: cma...@chromium.org abdulsyed@chromium.org
+abdulsyed@ & cmasso@ for M64 merge review.
Can you please comment on how widespread this is? How safe is this fix, and are there chances of introducing any new regressions?
In addition, has this been verified in Canary that the revert has fixed the issue?
I've discussed the plan with abdulsyed@. It's still targetted for M64 merge but needs more investigation.

Data points:
- HTTP auth is widely used and this issue breaks common use cases
- there's a workaround though it's not trivial to patch libraries under use
- the fix wouldn't cause any stability issue as all of DOMString, USVString and ByteString are widely used IDL types
  - the direction from JS to Blink is well tested as it's exposed to various input in the wild
  - the reverse direction is the same. not directly from JS but various inputs are given to the conversion and there's no requirement specified on the input
As an outsider to the release process, I thought merging https://chromium-review.googlesource.com/c/chromium/src/+/899382 into M64 would be pretty safe, in the sense that it'd basically revert to the behavior exposed up until M63.
Let's review this merge again after M65 Beta this week.
tyoshino@ - How does this change look in M65 Beta?
Cc: susanjun...@techmahindra.com
 Issue 808796  has been merged into this issue.
Before we consider the merge to M64, we need to ensure this is verified in Beta. We are planning to cut the RC soon in next hour, can someone please check and confirm? If not, we will have to target this for M65. 
I've confirmed that the issue is fixed in M65.
As discussed over chat with tyoshino@, there is a clear workaround and agreed that we can aim to fix this in M65 instead (which is only less than 2 weeks away). 
In this case, can we close this issue?
Status: Fixed (was: Started)
Yes. Marking as fixed.
Labels: -Merge-Request-64 Merge-Rejected-64
Rejecting merge to M64 based on comment #45.

Comment 49 by ricea@chromium.org, Mar 16 2018

Cc: vamshi.kommuri@chromium.org
 Issue 818115  has been merged into this issue.

Sign in to add a comment