Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1506803002:
[XHR] Replace isSetCookieHeader() with isForbiddenResponseHeaderName() (Closed)
|
Created:
5 years ago by tyoshino (SeeGerritForStatus) Modified:
5 years ago Reviewers:
yhirano CC:
chromium-reviews, blink-reviews, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[XHR] Replace isSetCookieHeader() with isForbiddenResponseHeaderName()
BUG=none
R=yhirano
Committed: https://crrev.com/9e1dad644c48db574407ccee4e95b8b10f5174ee
Cr-Commit-Position: refs/heads/master@{#364665}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed #3 #Patch Set 3 : Rebase #Messages
Total messages: 14 (6 generated)
tyoshino@chromium.org changed reviewers: + yhirano@chromium.org
https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right): https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1233: // 3) Firefox has implemented this policy. Can you update the comments? https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1234: if (FetchUtils::isForbiddenResponseHeaderName(it->key) && !securityOrigin()->canLoadLocalResources()) [optional] It would be good to use this function in modules/fetch/FetchResponseData.cpp as well.
https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right): https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1233: // 3) Firefox has implemented this policy. On 2015/12/08 07:23:51, yhirano wrote: > Can you update the comments? Done. https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1234: if (FetchUtils::isForbiddenResponseHeaderName(it->key) && !securityOrigin()->canLoadLocalResources()) On 2015/12/08 07:23:51, yhirano wrote: > [optional] It would be good to use this function in > modules/fetch/FetchResponseData.cpp as well. Seems the check in FetchResponseData is unnecessary. Check for forbidden response-header name is done when creating a Response object. If it's really needed, createCORSFilteredResponse() should also have it.
On 2015/12/08 13:21:38, tyoshino wrote: > https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right): > > https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1233: // 3) > Firefox has implemented this policy. > On 2015/12/08 07:23:51, yhirano wrote: > > Can you update the comments? > > Done. > > https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1234: if > (FetchUtils::isForbiddenResponseHeaderName(it->key) && > !securityOrigin()->canLoadLocalResources()) > On 2015/12/08 07:23:51, yhirano wrote: > > [optional] It would be good to use this function in > > modules/fetch/FetchResponseData.cpp as well. > > Seems the check in FetchResponseData is unnecessary. Check for forbidden > response-header name is done when creating a Response object. > > If it's really needed, createCORSFilteredResponse() should also have it. Hmm, OK. I think it's really needed, but it seems fixing separately is a good way. LGTM
Description was changed from ========== [XHR] Replace isSetCookieHeader() with isForbiddenResponseHeaderName() BUG=none ========== to ========== [XHR] Replace isSetCookieHeader() with isForbiddenResponseHeaderName() BUG=none R=yhirano ==========
The CQ bit was checked by tyoshino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1506803002/#ps40001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1506803002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1506803002/40001
Message was sent while issue was closed.
Description was changed from ========== [XHR] Replace isSetCookieHeader() with isForbiddenResponseHeaderName() BUG=none R=yhirano ========== to ========== [XHR] Replace isSetCookieHeader() with isForbiddenResponseHeaderName() BUG=none R=yhirano ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [XHR] Replace isSetCookieHeader() with isForbiddenResponseHeaderName() BUG=none R=yhirano ========== to ========== [XHR] Replace isSetCookieHeader() with isForbiddenResponseHeaderName() BUG=none R=yhirano Committed: https://crrev.com/9e1dad644c48db574407ccee4e95b8b10f5174ee Cr-Commit-Position: refs/heads/master@{#364665} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9e1dad644c48db574407ccee4e95b8b10f5174ee Cr-Commit-Position: refs/heads/master@{#364665}
Message was sent while issue was closed.
On 2015/12/09 06:18:18, yhirano wrote: > On 2015/12/08 13:21:38, tyoshino wrote: > > > https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right): > > > > > https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1233: // > 3) > > Firefox has implemented this policy. > > On 2015/12/08 07:23:51, yhirano wrote: > > > Can you update the comments? > > > > Done. > > > > > https://codereview.chromium.org/1506803002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1234: if > > (FetchUtils::isForbiddenResponseHeaderName(it->key) && > > !securityOrigin()->canLoadLocalResources()) > > On 2015/12/08 07:23:51, yhirano wrote: > > > [optional] It would be good to use this function in > > > modules/fetch/FetchResponseData.cpp as well. > > > > Seems the check in FetchResponseData is unnecessary. Check for forbidden > > response-header name is done when creating a Response object. > > > > If it's really needed, createCORSFilteredResponse() should also have it. > > Hmm, OK. > I think it's really needed, but it seems fixing separately is a good way. > LGTM Created: https://codereview.chromium.org/1526903002/ |
