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.
,
Feb 1 2018
,
Feb 2 2018
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!
,
Feb 2 2018
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!
,
Feb 2 2018
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
,
Feb 2 2018
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.
,
Feb 2 2018
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.
,
Feb 2 2018
,
Feb 2 2018
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).
,
Feb 2 2018
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.
,
Feb 5 2018
I forgot to add BUG= line in the patch. The revert patch has got landed: https://chromium-review.googlesource.com/c/chromium/src/+/899382
,
Feb 5 2018
@raphael, do you have a plan to fix the null checking problem of ToUSVString() this week? (Thank you for checking this issue!)
,
Feb 5 2018
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.
,
Feb 5 2018
Needs to bake the revert in canary first.
,
Feb 5 2018
Gyuyoung: I do. Actually, it looks like both yukishiino@ and I do so it will be fixed in a way or another :-)
,
Feb 5 2018
Rakuko: great! Let me try to re-land after you guys fix it.
,
Feb 5 2018
Rakuco, may I ask you to support the USVString version?
,
Feb 5 2018
Sure, I'm playing with a few different alternatives and will send a CL soon.
,
Feb 5 2018
,
Feb 5 2018
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)?
,
Feb 5 2018
,
Feb 5 2018
I've filed bug 808995 .
,
Feb 8 2018
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.
,
Feb 8 2018
Bug 808995 has been fixed. Nullable ByteStrings and USVStrings are now handled correctly, so the original XHR IDL change can be re-landed.
,
Feb 8 2018
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
,
Feb 8 2018
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?
,
Feb 9 2018
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.
,
Feb 9 2018
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?
,
Feb 9 2018
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.
,
Feb 9 2018
Thanks for the input. I understand that it would be burden for developers.
,
Feb 9 2018
Approving merge to M65 branch 3325 based on comments #27, #29 and #30. Please merge ASAP. Thank you.
,
Feb 9 2018
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.
,
Feb 9 2018
No worries and thank you.
,
Feb 9 2018
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.
,
Feb 9 2018
+abdulsyed@ & cmasso@ for M64 merge review.
,
Feb 12 2018
Can you please comment on how widespread this is? How safe is this fix, and are there chances of introducing any new regressions?
,
Feb 13 2018
In addition, has this been verified in Canary that the revert has fixed the issue?
,
Feb 13 2018
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
,
Feb 13 2018
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.
,
Feb 13 2018
Let's review this merge again after M65 Beta this week.
,
Feb 19 2018
tyoshino@ - How does this change look in M65 Beta?
,
Feb 20 2018
,
Feb 22 2018
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.
,
Feb 22 2018
I've confirmed that the issue is fixed in M65.
,
Feb 22 2018
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).
,
Feb 23 2018
In this case, can we close this issue?
,
Feb 26 2018
Yes. Marking as fixed.
,
Mar 5 2018
Rejecting merge to M64 based on comment #45.
,
Mar 16 2018
|
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by kochi@chromium.org
, Feb 1 2018