Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1092113006:
Invalidate urls given in the content-location header in requests using
DescriptionInvalidate urls given in the content-location header in requests using
unsafe methods (POST, PUT etc)
BUG=477567
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addess Adam's comments. Add two new unit tests. #
Total comments: 13
Patch Set 3 : Nit fixes and test cleanup #Patch Set 4 : Rebase #Patch Set 5 : Don't invalidate if cache is disabled #
Total comments: 1
Messages
Total messages: 46 (12 generated)
haavardm@opera.com changed reviewers: + ricea@chromium.org
haavardm@opera.com changed reviewers: + mmenke@chromium.org
Adam: please review. mmemke: FYI
I would like to see two more tests: 1. A test to verify that Content-Location is not invalidated by a response to a GET request. 2. A test to verify that Content-Location is not invalidated if the hosts don't match. https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.cc:174: // Return true if |method| is considered safe, or false if |method| is unsafe or A reference to RFC7231 section 4.2.1 would be helpful to clarify what is meant by "safe" here. https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.cc:176: bool SafeHTTPMethod(const std::string& method) { Nitpick: capitalisation should be SafeHttpMethod. https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_unittes... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_unittes... net/http/http_cache_unittest.cc:3085: TEST(HttpCache, SimplePOST_Invalidate_content_location) { This file is full of strangely-named tests, especially considering https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_... Anyway, the first letter of each word should be capitalised. I suggest SimplePOST_InvalidateContentLocation as a compromise name. https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_unittes... net/http/http_cache_unittest.cc:3088: MockTransaction transaction1(kTypicalGET_Transaction); There is no need to create the transaction1 and req1 variables. You can just use RunTransactionTest(cache.http_cache(), kTypicalGET_Transaction). As kTypicalGET_Transaction is included in kBuiltinMockTransactions, it doesn't need to be registered explicitly. https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_unittes... net/http/http_cache_unittest.cc:3105: MockTransaction transaction2(kContentLocationPOST_Transaction); If you use ScopedMockTransaction, then you don't need the explicit calls to AddMockTransaction/RemoveMockTransaction. https://codereview.chromium.org/1092113006/diff/1/net/http/http_transaction_t... File net/http/http_transaction_test_util.h (right): https://codereview.chromium.org/1092113006/diff/1/net/http/http_transaction_t... net/http/http_transaction_test_util.h:78: extern const MockTransaction kContentLocationPOST_Transaction; Since this transaction is only used for one test, it should be defined inline in http_cache_unittest.cc rather than exported here.
You should probably have a cache expert (Like rvargas) also take a look.
mmenke@chromium.org changed reviewers: + gavinp@chromium.org
Looks like Ricardo's on leave. [+gavinp]: Mind taking a look at this?
Adressed Adam's comments and added two new tests. Adam PTAL gavinp, please take a look. https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.cc:174: // Return true if |method| is considered safe, or false if |method| is unsafe or On 2015/04/24 13:19:21, Adam Rice wrote: > A reference to RFC7231 section 4.2.1 would be helpful to clarify what is meant > by "safe" here. Done. https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.cc:176: bool SafeHTTPMethod(const std::string& method) { On 2015/04/24 13:19:21, Adam Rice wrote: > Nitpick: capitalisation should be SafeHttpMethod. Done. https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_unittes... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_unittes... net/http/http_cache_unittest.cc:3085: TEST(HttpCache, SimplePOST_Invalidate_content_location) { On 2015/04/24 13:19:21, Adam Rice wrote: > This file is full of strangely-named tests, especially considering > https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_... > > Anyway, the first letter of each word should be capitalised. I suggest > SimplePOST_InvalidateContentLocation as a compromise name. Yeah, I tend to follow the styles in the files I edit, but this seems wrong. You suggestion works as a compromise. Done. https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_unittes... net/http/http_cache_unittest.cc:3088: MockTransaction transaction1(kTypicalGET_Transaction); On 2015/04/24 13:19:21, Adam Rice wrote: > There is no need to create the transaction1 and req1 variables. You can just use > RunTransactionTest(cache.http_cache(), kTypicalGET_Transaction). As > kTypicalGET_Transaction is included in kBuiltinMockTransactions, it doesn't need > to be registered explicitly. Done. https://codereview.chromium.org/1092113006/diff/1/net/http/http_cache_unittes... net/http/http_cache_unittest.cc:3105: MockTransaction transaction2(kContentLocationPOST_Transaction); On 2015/04/24 13:19:21, Adam Rice wrote: > If you use ScopedMockTransaction, then you don't need the explicit calls to > AddMockTransaction/RemoveMockTransaction. Done. https://codereview.chromium.org/1092113006/diff/1/net/http/http_transaction_t... File net/http/http_transaction_test_util.h (right): https://codereview.chromium.org/1092113006/diff/1/net/http/http_transaction_t... net/http/http_transaction_test_util.h:78: extern const MockTransaction kContentLocationPOST_Transaction; On 2015/04/24 13:19:21, Adam Rice wrote: > Since this transaction is only used for one test, it should be defined inline in > http_cache_unittest.cc rather than exported here. Done.
https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1254: // given in the Content-Location header if it has the same origin as Nitpick: we only care whether the host is the same, not the rest of the origin. https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1255: // request_->url|. Nitpick: missing | before request_->url|. https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1256: if (!SafeHttpMethod(request_->method) && I think we also need to check whether the LOAD_DISABLE_CACHE flag is set, the same at at line 1270 below. https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:3121: AddMockTransaction(&transaction2); ScopedMockTransaction calls AddMockTransaction() in the constructor, there is no need to call it explicitly. Also in the other two tests. https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:3157: // Content-Location header.The hostname of kSimpleGET_Transaction does not Nitpick: space after ".". https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:3203: req2.upload_data_stream = &upload_data_stream; GET requests should not have an upload_data_stream. You can also remove the initialisation of the stream above.
https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1256: if (!SafeHttpMethod(request_->method) && On 2015/04/27 11:51:58, Adam Rice wrote: > I think we also need to check whether the LOAD_DISABLE_CACHE flag is set, the > same at at line 1270 below. This case is a bit different than line 1270 though, since "this url" invalidates a another url. So the question is if a request to url A with cache disabled should be allowed to invalidate url B in the cache. Does LOAD_DISABLE_CACHE mean that cache should not be touched at all?
Fixed all but the disable cache flag issue in latest CL, as it is still unclear what is the right decision there. PTAL https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1254: // given in the Content-Location header if it has the same origin as On 2015/04/27 11:51:58, Adam Rice wrote: > Nitpick: we only care whether the host is the same, not the rest of the origin. Done. https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1255: // request_->url|. On 2015/04/27 11:51:58, Adam Rice wrote: > Nitpick: missing | before request_->url|. Done. https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:3121: AddMockTransaction(&transaction2); On 2015/04/27 11:51:58, Adam Rice wrote: > ScopedMockTransaction calls AddMockTransaction() in the constructor, there is no > need to call it explicitly. Also in the other two tests. Done. https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:3157: // Content-Location header.The hostname of kSimpleGET_Transaction does not On 2015/04/27 11:51:58, Adam Rice wrote: > Nitpick: space after ".". Done. https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:3203: req2.upload_data_stream = &upload_data_stream; On 2015/04/27 11:51:58, Adam Rice wrote: > GET requests should not have an upload_data_stream. You can also remove the > initialisation of the stream above. Agh, copy/paste..
lgtm gavinp, please give your opinion on the correct behaviour of LOAD_DISABLE_CACHE here. https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1256: if (!SafeHttpMethod(request_->method) && On 2015/04/27 12:49:22, haavardm wrote: > On 2015/04/27 11:51:58, Adam Rice wrote: > > I think we also need to check whether the LOAD_DISABLE_CACHE flag is set, the > > same at at line 1270 below. > > This case is a bit different than line 1270 though, since "this url" invalidates > a another url. So the question is if a request to url A with cache disabled > should be allowed to invalidate url B in the cache. Does LOAD_DISABLE_CACHE mean > that cache should not be touched at all? My interpretation is that yes, LOAD_DISABLE_CACHE means the cache should not be touched at all. In some places it is used in privacy-sensitive contexts where leaving traces in the cache (even in the form of a removed entry) could lead to undesired information leakage.
On 2015/04/27 13:31:17, Adam Rice wrote: > lgtm > > gavinp, please give your opinion on the correct behaviour of LOAD_DISABLE_CACHE > here. > > https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... > net/http/http_cache_transaction.cc:1256: if (!SafeHttpMethod(request_->method) > && > On 2015/04/27 12:49:22, haavardm wrote: > > On 2015/04/27 11:51:58, Adam Rice wrote: > > > I think we also need to check whether the LOAD_DISABLE_CACHE flag is set, > the > > > same at at line 1270 below. > > > > This case is a bit different than line 1270 though, since "this url" > invalidates > > a another url. So the question is if a request to url A with cache disabled > > should be allowed to invalidate url B in the cache. Does LOAD_DISABLE_CACHE > mean > > that cache should not be touched at all? > > My interpretation is that yes, LOAD_DISABLE_CACHE means the cache should not be > touched at all. In some places it is used in privacy-sensitive contexts where > leaving traces in the cache (even in the form of a removed entry) could lead to > undesired information leakage. Sounds reasonable. I'll wait until gavinp has had his say.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
Ping. Gavinp, do you have any comments regarding how LOAD_DISABLE_CACHE should affect invalidation of the Content-Location url? On 2015/04/27 13:53:48, haavardm wrote: > On 2015/04/27 13:31:17, Adam Rice wrote: > > lgtm > > > > gavinp, please give your opinion on the correct behaviour of > LOAD_DISABLE_CACHE > > here. > > > > > https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... > > File net/http/http_cache_transaction.cc (right): > > > > > https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... > > net/http/http_cache_transaction.cc:1256: if (!SafeHttpMethod(request_->method) > > && > > On 2015/04/27 12:49:22, haavardm wrote: > > > On 2015/04/27 11:51:58, Adam Rice wrote: > > > > I think we also need to check whether the LOAD_DISABLE_CACHE flag is set, > > the > > > > same at at line 1270 below. > > > > > > This case is a bit different than line 1270 though, since "this url" > > invalidates > > > a another url. So the question is if a request to url A with cache disabled > > > should be allowed to invalidate url B in the cache. Does LOAD_DISABLE_CACHE > > mean > > > that cache should not be touched at all? > > > > My interpretation is that yes, LOAD_DISABLE_CACHE means the cache should not > be > > touched at all. In some places it is used in privacy-sensitive contexts where > > leaving traces in the cache (even in the form of a removed entry) could lead > to > > undesired information leakage. > > Sounds reasonable. I'll wait until gavinp has had his say.
Due do missing response, and after thinking it over, I removed invaliding the url from the content-location header if cache is disabled. On 2015/05/05 14:45:26, haavardm wrote: > Ping. > > Gavinp, do you have any comments regarding how LOAD_DISABLE_CACHE should affect > invalidation of the Content-Location url? > > On 2015/04/27 13:53:48, haavardm wrote: > > On 2015/04/27 13:31:17, Adam Rice wrote: > > > lgtm > > > > > > gavinp, please give your opinion on the correct behaviour of > > LOAD_DISABLE_CACHE > > > here. > > > > > > > > > https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... > > > File net/http/http_cache_transaction.cc (right): > > > > > > > > > https://codereview.chromium.org/1092113006/diff/20001/net/http/http_cache_tra... > > > net/http/http_cache_transaction.cc:1256: if > (!SafeHttpMethod(request_->method) > > > && > > > On 2015/04/27 12:49:22, haavardm wrote: > > > > On 2015/04/27 11:51:58, Adam Rice wrote: > > > > > I think we also need to check whether the LOAD_DISABLE_CACHE flag is > set, > > > the > > > > > same at at line 1270 below. > > > > > > > > This case is a bit different than line 1270 though, since "this url" > > > invalidates > > > > a another url. So the question is if a request to url A with cache > disabled > > > > should be allowed to invalidate url B in the cache. Does > LOAD_DISABLE_CACHE > > > mean > > > > that cache should not be touched at all? > > > > > > My interpretation is that yes, LOAD_DISABLE_CACHE means the cache should not > > be > > > touched at all. In some places it is used in privacy-sensitive contexts > where > > > leaving traces in the cache (even in the form of a removed entry) could lead > > to > > > undesired information leakage. > > > > Sounds reasonable. I'll wait until gavinp has had his say.
The CQ bit was checked by haavardm@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from ricea@chromium.org Link to the patchset: https://codereview.chromium.org/1092113006/#ps80001 (title: "Don't invalidate if cache is disabled")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092113006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
haavardm@opera.com changed reviewers: + mmenke@chromium.org
On 2015/05/15 12:21:44, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) The build failed due to missing LGTM (unsure why since adam approved): ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: net/http/http_cache_transaction.cc net/http/http_cache_unittest.cc Adam, gavinp or mmemke, can any of give a valid LGTM?
On 2015/05/29 08:34:16, haavardm wrote: > On 2015/05/15 12:21:44, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > The build failed due to missing LGTM (unsure why since adam approved): > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for these files: > net/http/http_cache_transaction.cc > net/http/http_cache_unittest.cc > > Adam, gavinp or mmemke, can any of give a valid LGTM? I'm insufficiently familiar with the cache to review this. But...What about the case that the content-location header has a hash / reference? Shouldn't that be stripped off? Duplicating redirect logic that's in the URLRequestJob here as well seems prone to break.
On 2015/05/29 12:45:22, mmenke wrote: > I'm insufficiently familiar with the cache to review this. But...What about the > case that the content-location header has a hash / reference? Shouldn't that be > stripped off? Duplicating redirect logic that's in the URLRequestJob here as > well seems prone to break. The URL hash is stripped off inside DoomMainEntryForUrl(), so there's no problem there. I can't find any duplication of code from URLRequestJob--in fact, I can't find any code that does this for the "Location" header at all, which worries me.
On 2015/05/29 13:43:03, Adam Rice wrote: > On 2015/05/29 12:45:22, mmenke wrote: > > I'm insufficiently familiar with the cache to review this. But...What about > the > > case that the content-location header has a hash / reference? Shouldn't that > be > > stripped off? Duplicating redirect logic that's in the URLRequestJob here as > > well seems prone to break. > > The URL hash is stripped off inside DoomMainEntryForUrl(), so there's no problem > there. > > I can't find any duplication of code from URLRequestJob--in fact, I can't find > any code that does this for the "Location" header at all, which worries me. Looks like it's HttpUtil::PathForRequest() that does that (Called through an HttpUtil method in https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_basi...). I had thought we didn't pass fragments below the URLRequestJob layer, but looks like I was wrong. Sadly, there's also GURL::PathForRequest(), which I think does the same thing
On 2015/05/29 14:19:07, mmenke wrote: > On 2015/05/29 13:43:03, Adam Rice wrote: > > On 2015/05/29 12:45:22, mmenke wrote: > > > I'm insufficiently familiar with the cache to review this. But...What about > > the > > > case that the content-location header has a hash / reference? Shouldn't > that > > be > > > stripped off? Duplicating redirect logic that's in the URLRequestJob here > as > > > well seems prone to break. > > > > The URL hash is stripped off inside DoomMainEntryForUrl(), so there's no > problem > > there. > > > > I can't find any duplication of code from URLRequestJob--in fact, I can't find > > any code that does this for the "Location" header at all, which worries me. > > Looks like it's HttpUtil::PathForRequest() that does that (Called through an > HttpUtil method in > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_basi...). > I had thought we didn't pass fragments below the URLRequestJob layer, but looks > like I was wrong. > > Sadly, there's also GURL::PathForRequest(), which I think does the same thing If I get this right, this is a bit on the side of this CL though? If neither of you feel comfortable reviewing this change, and since Ricardo is on leave and gavinp is not responding, do you have suggestion for who I can add as reviewer?
On 2015/05/29 14:19:07, mmenke wrote: > On 2015/05/29 13:43:03, Adam Rice wrote: > > On 2015/05/29 12:45:22, mmenke wrote: > > > I'm insufficiently familiar with the cache to review this. But...What about > > the > > > case that the content-location header has a hash / reference? Shouldn't > that > > be > > > stripped off? Duplicating redirect logic that's in the URLRequestJob here > as > > > well seems prone to break. > > > > The URL hash is stripped off inside DoomMainEntryForUrl(), so there's no > problem > > there. > > > > I can't find any duplication of code from URLRequestJob--in fact, I can't find > > any code that does this for the "Location" header at all, which worries me. > > Looks like it's HttpUtil::PathForRequest() that does that (Called through an > HttpUtil method in > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_basi...). > I had thought we didn't pass fragments below the URLRequestJob layer, but looks > like I was wrong. > > Sadly, there's also GURL::PathForRequest(), which I think does the same thing If I get this right, this is a bit on the side of this CL though? If neither of you feel comfortable reviewing this change, and since Ricardo is on leave and gavinp is not responding, do you have suggestion for who I can add as reviewer?
On 2015/05/29 15:12:36, haavardm wrote: > On 2015/05/29 14:19:07, mmenke wrote: > > On 2015/05/29 13:43:03, Adam Rice wrote: > > > On 2015/05/29 12:45:22, mmenke wrote: > > > > I'm insufficiently familiar with the cache to review this. But...What > about > > > the > > > > case that the content-location header has a hash / reference? Shouldn't > > that > > > be > > > > stripped off? Duplicating redirect logic that's in the URLRequestJob here > > as > > > > well seems prone to break. > > > > > > The URL hash is stripped off inside DoomMainEntryForUrl(), so there's no > > problem > > > there. > > > > > > I can't find any duplication of code from URLRequestJob--in fact, I can't > find > > > any code that does this for the "Location" header at all, which worries me. > > > > Looks like it's HttpUtil::PathForRequest() that does that (Called through an > > HttpUtil method in > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_basi...). > > I had thought we didn't pass fragments below the URLRequestJob layer, but > looks > > like I was wrong. > > > > Sadly, there's also GURL::PathForRequest(), which I think does the same thing > > If I get this right, this is a bit on the side of this CL though? If neither of > you feel comfortable reviewing this change, and since Ricardo is on leave and > gavinp is not responding, > do you have suggestion for who I can add as reviewer? I'm happy with the change. The only problem I see is that maybe it should cover the "Location" header as well, but that would require significantly more research (ie. what exactly is the current behaviour? what do other browsers do?) and care (eg. putting the behaviour change behind a flag and doing an experiment). The problem is that I'm not an OWNER of this code.
haavardm@opera.com changed reviewers: + eroman@chromium.org
On 2015/05/29 15:23:16, Adam Rice wrote: > On 2015/05/29 15:12:36, haavardm wrote: > > On 2015/05/29 14:19:07, mmenke wrote: > > > On 2015/05/29 13:43:03, Adam Rice wrote: > > > > On 2015/05/29 12:45:22, mmenke wrote: > > > > > I'm insufficiently familiar with the cache to review this. But...What > > about > > > > the > > > > > case that the content-location header has a hash / reference? Shouldn't > > > that > > > > be > > > > > stripped off? Duplicating redirect logic that's in the URLRequestJob > here > > > as > > > > > well seems prone to break. > > > > > > > > The URL hash is stripped off inside DoomMainEntryForUrl(), so there's no > > > problem > > > > there. > > > > > > > > I can't find any duplication of code from URLRequestJob--in fact, I can't > > find > > > > any code that does this for the "Location" header at all, which worries > me. > > > > > > Looks like it's HttpUtil::PathForRequest() that does that (Called through an > > > HttpUtil method in > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_basi...). > > > I had thought we didn't pass fragments below the URLRequestJob layer, but > > looks > > > like I was wrong. > > > > > > Sadly, there's also GURL::PathForRequest(), which I think does the same > thing > > > > If I get this right, this is a bit on the side of this CL though? If neither > of > > you feel comfortable reviewing this change, and since Ricardo is on leave and > > gavinp is not responding, > > do you have suggestion for who I can add as reviewer? > > I'm happy with the change. . > The problem is that I'm not an OWNER of this code. Ah, right. I added eroman as reviewer. Eroman, can you take a look?
eroman@chromium.org changed reviewers: + rsleevi@chromium.org
+rsleevi because we were talking about origin comparison in //net code just today, and this is another example of it. I am not familiar with rfc7234 but this behavior strikes me as wrong and incongruent with web platform's concept of origin. In particular section 4.4 says to do a host check, but without any mention of the URL's scheme [1] This would mean, both in the spec and this implementation, that a non-secure HTTP response could select which https:// URLs it wants to invalidate from the cache, unless I am missing something. [1] https://tools.ietf.org/html/rfc7234#section-4.4 However, a cache MUST NOT invalidate a URI from a Location or Content-Location response header field if the host part of that URI differs from the host part in the effective request URI (Section 5.5 of [RFC7230]). This helps prevent denial-of-service attacks.
On 2015/05/29 21:28:08, eroman wrote: > +rsleevi because we were talking about origin comparison in //net code just > today, and this is another example of it. > > I am not familiar with rfc7234 but this behavior strikes me as wrong and > incongruent with web platform's concept of origin. In particular section 4.4 > says to do a host check, but without any mention of the URL's scheme [1] > > This would mean, both in the spec and this implementation, that a non-secure > HTTP response could select which https:// URLs it wants to invalidate from the > cache, unless I am missing something. > > [1] https://tools.ietf.org/html/rfc7234#section-4.4 > > However, a cache MUST NOT invalidate a URI from a Location or > Content-Location response header field if the host part of that URI > differs from the host part in the effective request URI (Section 5.5 > of [RFC7230]). This helps prevent denial-of-service attacks. Thanks for the head's up, Eric. I agree with Eric's reading of the spec and that is concerning. [Note for anyone who might file an errata - I don't think referencing RFC 6454 is particularly helpful for that; it's already been supplanted / violated / extended by https://url.spec.whatwg.org/#origin ]
Yeah, I'm gonna be a pain and slap a "not LGTM" on this until we can resolve the issue, just so it doesn't accidentally get CQ'd Adding Mike West for "all things origins", and to make sure we're spreading knowledge around. I think someone (haavard?) should raise an errata against 7234, or dig in to some historical context to see if this was discussed during the httpbis work. It strikes me as wrong to have a cross-origin covert communication channel in httpbis, which is what this effectively is. I think the correct thing should be to consider the full origin. That said, I'm not sure if we should consider the 6454 sense of origin, the URL spec sense of origin, or simply the (scheme, host, port) tuple sense of origin. My inclination is "scheme, host, port" is the correct answer here, since I'm not sure where or how we'd use HTTP framing on a hostless/portless transport (and, indeed, http and https schemes follow the generic URI syntax, so we should never ever run into the null/unique origin scenario) https://codereview.chromium.org/1092113006/diff/80001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1092113006/diff/80001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1209: cache_->DoomMainEntryForUrl(absolute_location_url); To add to Eric's concerns about scheme, this also allows dooming for different ports, which also doesn't seem correct.
On 2015/05/29 22:04:52, Ryan Sleevi wrote: > Yeah, I'm gonna be a pain and slap a "not LGTM" on this until we can resolve the > issue, just so it doesn't accidentally get CQ'd > No worries, I fully agree. Taking a second look, it seems obvious it needs a proper origin check. > Adding Mike West for "all things origins", and to make sure we're spreading > knowledge around. > > I think someone (haavard?) should raise an errata against 7234, or dig in to > some historical context to see if this was discussed during the httpbis work. It I did search through the ietf email lists, and found nothing about this specifically. But I can't guarantee I haven't missed it. Admittedly I don't have much experience with IETF, and is actually a bit unsure how to file an errata. Is it simply done through http://www.rfc-editor.org/info/rfc7234 ? > strikes me as wrong to have a cross-origin covert communication channel in > httpbis, which is what this effectively is. I think the correct thing should be > to consider the full origin. Agree. It's not completely cross origin due to the hostname check, but a MITM issuing a post and rewriting the http response, or a different server behind the same hostname can easily invalidate a cached https resource the way it is now. Doesn't seem like the worst of security issues, but that has been the last famous words many times before. > That said, I'm not sure if we should consider the 6454 sense of origin, the URL > spec sense of origin, or simply the (scheme, host, port) tuple sense of origin. Aren't all these based on the (scheme, host port) tuple, or is there some details about top level/subdomain I'm missing? > My inclination is "scheme, host, port" is the correct answer here, since I'm not > sure where or how we'd use HTTP framing on a hostless/portless transport (and, > indeed, http and https schemes follow the generic URI syntax, so we should never > ever run into the null/unique origin scenario) > > https://codereview.chromium.org/1092113006/diff/80001/net/http/http_cache_tra... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/1092113006/diff/80001/net/http/http_cache_tra... > net/http/http_cache_transaction.cc:1209: > cache_->DoomMainEntryForUrl(absolute_location_url); > To add to Eric's concerns about scheme, this also allows dooming for different > ports, which also doesn't seem correct.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
On 2015/06/02 at 14:38:31, haavardm wrote: > > That said, I'm not sure if we should consider the 6454 sense of origin, the URL > > spec sense of origin, or simply the (scheme, host, port) tuple sense of origin. > > Aren't all these based on the (scheme, host port) tuple, or is there some details > about top level/subdomain I'm missing? I think Ryan is referring to some of the more esoteric properties of origins, namely that some (scheme, host, port) pairs are actually "unique" and don't match anything, including themselves (think <iframe sandbox>, for instance). I'd agree that here we're likely most concerned about the serialization of the URL's origin, rather than the security origin of any particular document.
On 2015/06/02 15:04:17, Mike West wrote: > On 2015/06/02 at 14:38:31, haavardm wrote: > > > That said, I'm not sure if we should consider the 6454 sense of origin, the > URL > > > spec sense of origin, or simply the (scheme, host, port) tuple sense of > origin. > > > > Aren't all these based on the (scheme, host port) tuple, or is there some > details > > about top level/subdomain I'm missing? > > I think Ryan is referring to some of the more esoteric properties of origins, > namely that some (scheme, host, port) pairs are actually "unique" and don't > match anything, including themselves (think <iframe sandbox>, for instance). > > I'd agree that here we're likely most concerned about the serialization of the > URL's origin, rather than the security origin of any particular document. How does this interact with Service Workers, for example? Let's say Site A installs a SW. They intercept the Fetch request for Site B, and respond with a new Response object. Will the SW flow through this path? Because SW are handled presently as URLRequestJobs, rather than being at the Resource Loader, I fear they might. This would give SW A a chance to interfere with Site B beyond the scope of SW A.
I had a look through the httpbis issues and mailing list, and couldn't find discussion of this specific issue (although we brushed against it a few times). The text came from 2616. You could certainly try filing an errata, but I suspect it wouldn't get approved, as this is a technical change. It might be better to open an issue here: https://github.com/httpwg/http11bis/issues (We'll be taking the documents to full standard in the not-to-distant future, and it'd get resolved then).
On 2015/06/02 16:21:23, Ryan Sleevi wrote: > On 2015/06/02 15:04:17, Mike West wrote: > > On 2015/06/02 at 14:38:31, haavardm wrote: > > > > That said, I'm not sure if we should consider the 6454 sense of origin, > the > > URL > > > > spec sense of origin, or simply the (scheme, host, port) tuple sense of > > origin. > > > > > > Aren't all these based on the (scheme, host port) tuple, or is there some > > details > > > about top level/subdomain I'm missing? > > > > I think Ryan is referring to some of the more esoteric properties of origins, > > namely that some (scheme, host, port) pairs are actually "unique" and don't > > match anything, including themselves (think <iframe sandbox>, for instance). > > > > I'd agree that here we're likely most concerned about the serialization of the > > URL's origin, rather than the security origin of any particular document. > > How does this interact with Service Workers, for example? > > Let's say Site A installs a SW. They intercept the Fetch request for Site B, and > respond with a new Response object. > > Will the SW flow through this path? Because SW are handled presently as > URLRequestJobs, rather than being at the Resource Loader, I fear they might. > This would give SW A a chance to interfere with Site B beyond the scope of SW A. Interesting. In general, it does problematic though, if site A is allowed to create responses for server B and then have that handled by the low level network code. Wouldn't this also allow a SW on site A setting cookies for Site B? To me it would make sense if the SW interception happened on a level above that.
On 2015/06/03 14:41:50, haavardm wrote: > On 2015/06/02 16:21:23, Ryan Sleevi wrote: > > On 2015/06/02 15:04:17, Mike West wrote: > > > On 2015/06/02 at 14:38:31, haavardm wrote: > > > > > That said, I'm not sure if we should consider the 6454 sense of origin, > > the > > > URL > > > > > spec sense of origin, or simply the (scheme, host, port) tuple sense of > > > origin. > > > > > > > > Aren't all these based on the (scheme, host port) tuple, or is there some > > > details > > > > about top level/subdomain I'm missing? > > > > > > I think Ryan is referring to some of the more esoteric properties of > origins, > > > namely that some (scheme, host, port) pairs are actually "unique" and don't > > > match anything, including themselves (think <iframe sandbox>, for instance). > > > > > > I'd agree that here we're likely most concerned about the serialization of > the > > > URL's origin, rather than the security origin of any particular document. > > > > How does this interact with Service Workers, for example? > > > > Let's say Site A installs a SW. They intercept the Fetch request for Site B, > and > > respond with a new Response object. > > > > Will the SW flow through this path? Because SW are handled presently as > > URLRequestJobs, rather than being at the Resource Loader, I fear they might. > > This would give SW A a chance to interfere with Site B beyond the scope of SW > A. > > Interesting. In general, it does problematic though, if site A is allowed to > create responses for server B and then have that handled by the low level > network code. Wouldn't this also allow a SW on site A setting cookies for Site > B? To me it would make sense if the SW interception happened on a level above > that. I believe the layer at which SW intercepts would neither affect this code nor cookies. (But I completely agree that they should not be at the URLRequestJob layer. There's a net-dev thread that I need to find time to respond to after I get a chance to sit down and put together a more concrete proposal to sell them on.)
On 2015/06/03 20:53:12, David Benjamin wrote: > I believe the layer at which SW intercepts would neither affect this code nor > cookies. (But I completely agree that they should not be at the URLRequestJob > layer. There's a net-dev thread that I need to find time to respond to after I > get a chance to sit down and put together a more concrete proposal to sell them > on.) Thanks. So we need to decide whether to not do this at all (and file an issue, as mnot suggested; he raises a good point with the errata process) or to violate the spec and do a full origin check. My inclination is "Don't do this" (well, it's that for a lot of things), but the reasoning is that I'm not even sure how logic like this might play (or should play) with things like iframe sandboxing or filesystem or any of the weird ways that can cause a request in which the URL is X, but that it shouldn't have the 'normal' behaviour Y. Maybe I'm entirely misguided (and we can use this bug to educate me and my irrational fears), but the attention to security by the spec seems lacking, and thus I'm less-inclined to believe this is good/right/desirable behaviour. Even if it's quite appealing on the face and understandable why useful. Question: What do other browsers (Edge, Firefox, Safari) do? What did Opera do?
On 2015/06/03 20:56:49, Ryan Sleevi wrote: > On 2015/06/03 20:53:12, David Benjamin wrote: > > I believe the layer at which SW intercepts would neither affect this code nor > > cookies. (But I completely agree that they should not be at the URLRequestJob > > layer. There's a net-dev thread that I need to find time to respond to after I > > get a chance to sit down and put together a more concrete proposal to sell > them > > on.) > > Thanks. > > So we need to decide whether to not do this at all (and file an issue, as mnot > suggested; he raises a good point with the errata process) or to violate the > spec and do a full origin check. > > My inclination is "Don't do this" (well, it's that for a lot of things), but the > reasoning is that I'm not even sure how logic like this might play (or should > play) with things like iframe sandboxing or filesystem or any of the weird ways > that can cause a request in which the URL is X, but that it shouldn't have the > 'normal' behaviour Y. Maybe I'm entirely misguided (and we can use this bug to > educate me and my irrational fears), but the attention to security by the spec > seems lacking, and thus I'm less-inclined to believe this is > good/right/desirable behaviour. Even if it's quite appealing on the face and > understandable why useful. Thinking about it, this might cause trouble for offline technologies (such as appcache or service workers when used to provide offline capabilities), if one url is able to invalidate another url that is supposed to be stored offline. The cache seems to have a special handling for application cache, but I'm unsure about WS. I'm fine with not implementing this until it has been resolved in the spec, and file an spec issue instead. I don't think this has wide spread support among other browsers. Opera's presto engine did not support it. > > Question: What do other browsers (Edge, Firefox, Safari) do? What did Opera do?
ricea@chromium.org changed reviewers: - ricea@chromium.org
is this coming alive again? |
