Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2455663004:
Add test to ensure that URLs that redirect inside the PDF plugin fail to load (Closed)
DescriptionAdd test to ensure that URLs that redirect inside the PDF plugin fail to load
A URL that is passed to the PDF plugin should already have its redirects
resolved. If it doesn't, then the PDF that gets loaded may not have the same
origin as the one the PDF is assumed to be in. If that happens, it can cause
same origin policy violations. So redirects are disabled for requests made from
the plugin.
Redirects were disabled here: https://codereview.chromium.org/2409423004/.
There is one other place where we make a request in the plugin which has
disabled in this CL (the URLs that get loaded here are chrome internal
URLs so they should never trigger redirects anyway but this is done for
safety). This CL also adds some plumbing to ensure the redirect requests
trigger a document load failure message to be sent to JS so that we can
properly detect the load failure in tests.
BUG=653749
Committed: https://crrev.com/a8563ff4076d3ee37db3aaa85cf5226a8922e30f
Cr-Commit-Position: refs/heads/master@{#432293}
Patch Set 1 #Patch Set 2 #Patch Set 3 #
Total comments: 6
Patch Set 4 #Patch Set 5 #
Total comments: 11
Patch Set 6 #
Total comments: 2
Patch Set 7 : . #
Messages
Total messages: 49 (28 generated)
Description was changed from ========== [NOT READY] PDF Redirect Test BUG= ========== to ========== [NOT READY] PDF Redirect Test BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [NOT READY] PDF Redirect Test BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add test to ensure that URLs that redirect inside the PDF plugin fail to load A URL that is passed to the PDF plugin should already have its redirects resolved. If it doesn't, then the PDF that gets loaded may not have the same origin as the one the PDF is assumed to be in. If that happens, it can cause same origin policy violations. So redirects are disabled for requests made from the plugin. Redirects were disabled here: https://codereview.chromium.org/2409423004/. There is one other place where we make a request in the plugin which has disabled in this CL (the URLs that get loaded here are chrome internal URLs so they should never trigger redirects anyway but this is done for safety). This CL also adds some plumbing to ensure the redirect requests trigger a document load failure message to be sent to JS so that we can properly detect the load failure in tests. BUG=653749 ==========
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add test to ensure that URLs that redirect inside the PDF plugin fail to load A URL that is passed to the PDF plugin should already have its redirects resolved. If it doesn't, then the PDF that gets loaded may not have the same origin as the one the PDF is assumed to be in. If that happens, it can cause same origin policy violations. So redirects are disabled for requests made from the plugin. Redirects were disabled here: https://codereview.chromium.org/2409423004/. There is one other place where we make a request in the plugin which has disabled in this CL (the URLs that get loaded here are chrome internal URLs so they should never trigger redirects anyway but this is done for safety). This CL also adds some plumbing to ensure the redirect requests trigger a document load failure message to be sent to JS so that we can properly detect the load failure in tests. BUG=653749 ========== to ========== Add test to ensure that URLs that redirect inside the PDF plugin fail to load A URL that is passed to the PDF plugin should already have its redirects resolved. If it doesn't, then the PDF that gets loaded may not have the same origin as the one the PDF is assumed to be in. If that happens, it can cause same origin policy violations. So redirects are disabled for requests made from the plugin. Redirects were disabled here: https://codereview.chromium.org/2409423004/. There is one other place where we make a request in the plugin which has disabled in this CL (the URLs that get loaded here are chrome internal URLs so they should never trigger redirects anyway but this is done for safety). This CL also adds some plumbing to ensure the redirect requests trigger a document load failure message to be sent to JS so that we can properly detect the load failure in tests. BUG=653749 ==========
raymes@chromium.org changed reviewers: + thestig@chromium.org
Let me know if you're busy and I can ask sammc to review. Thanks!
r429514 landed, so you need to merge and resolve conflicts.
https://codereview.chromium.org/2455663004/diff/40001/chrome/test/data/pdf/re... File chrome/test/data/pdf/redirects_fail_test.js (right): https://codereview.chromium.org/2455663004/diff/40001/chrome/test/data/pdf/re... chrome/test/data/pdf/redirects_fail_test.js:10: console.error(url); Debug code? https://codereview.chromium.org/2455663004/diff/40001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2455663004/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:22: // Return true if the HTTP response of |loader| is a succesful one and loading typo https://codereview.chromium.org/2455663004/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:30: if ((http_code >= 400 && http_code < 500) || http_code == 301) Just: return (http_code < 400 && http_code != 301) || http_code >= 500; Or: keep the logic and change the method to ResponseStatusFailed().
The CQ bit was checked by raymes@chromium.org
The CQ bit was unchecked by raymes@chromium.org
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! ptal https://codereview.chromium.org/2455663004/diff/40001/chrome/test/data/pdf/re... File chrome/test/data/pdf/redirects_fail_test.js (right): https://codereview.chromium.org/2455663004/diff/40001/chrome/test/data/pdf/re... chrome/test/data/pdf/redirects_fail_test.js:10: console.error(url); On 2016/11/03 07:10:45, Lei Zhang wrote: > Debug code? Done. https://codereview.chromium.org/2455663004/diff/40001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/2455663004/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:22: // Return true if the HTTP response of |loader| is a succesful one and loading On 2016/11/03 07:10:45, Lei Zhang wrote: > typo Done. https://codereview.chromium.org/2455663004/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:30: if ((http_code >= 400 && http_code < 500) || http_code == 301) On 2016/11/03 07:10:45, Lei Zhang wrote: > Just: return (http_code < 400 && http_code != 301) || http_code >= 500; > Or: keep the logic and change the method to ResponseStatusFailed(). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thestig@chromium.org changed reviewers: + art-snake@yandex-team.ru, spelchat@chromium.org
Looks fine, but I'm not 100% certain on the pdfium_engine.cc change. https://codereview.chromium.org/2455663004/diff/80001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2455663004/diff/80001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:914: // Test that if the plugin tries to load a URL that redirects that it will fail ... _then_ it will fail? https://codereview.chromium.org/2455663004/diff/80001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:916: // the redirect, which can have security implications. crbug.com/653749 If you add https:// in the URL, then cs.chromium.org will auto-linkify, which is nice. https://codereview.chromium.org/2455663004/diff/80001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2455663004/diff/80001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:850: document_load_state_ = LOAD_STATE_LOADING; Side note: I wonder if this is actually needed to satisfy the DCHECK at the top of DocumentLoadFailed(). i.e. Is |document_load_state_| already in the loading state? https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (left): https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1109: OnDocumentComplete(); I defer to spelchat / art-snake on this.
https://codereview.chromium.org/2455663004/diff/80001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2455663004/diff/80001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:914: // Test that if the plugin tries to load a URL that redirects that it will fail On 2016/11/08 01:28:10, Lei Zhang wrote: > ... _then_ it will fail? Done. https://codereview.chromium.org/2455663004/diff/80001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:916: // the redirect, which can have security implications. crbug.com/653749 On 2016/11/08 01:28:10, Lei Zhang wrote: > If you add https:// in the URL, then http://cs.chromium.org will auto-linkify, which is > nice. Done. https://codereview.chromium.org/2455663004/diff/80001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2455663004/diff/80001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:850: document_load_state_ = LOAD_STATE_LOADING; On 2016/11/08 01:28:10, Lei Zhang wrote: > Side note: I wonder if this is actually needed to satisfy the DCHECK at the top > of DocumentLoadFailed(). i.e. Is |document_load_state_| already in the loading > state? Done. https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (left): https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1109: OnDocumentComplete(); On 2016/11/08 01:28:10, Lei Zhang wrote: > I defer to spelchat / art-snake on this. Sure. Without this the client was never notified that the document load failed. Also I'm unsure what the benefit is of calling OnDocumentComplete() is here but I might be missing something.
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Looks good. Will stamp (and CQ?) once you work out the PDFiumEngine::OnDocumentCanceled() bit. https://codereview.chromium.org/2455663004/diff/80001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2455663004/diff/80001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:850: document_load_state_ = LOAD_STATE_LOADING; On 2016/11/08 06:00:01, raymes wrote: > On 2016/11/08 01:28:10, Lei Zhang wrote: > > Side note: I wonder if this is actually needed to satisfy the DCHECK at the > top > > of DocumentLoadFailed(). i.e. Is |document_load_state_| already in the loading > > state? > > Done. Wasn't expecting this change, but if it works, then ok.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (left): https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1109: OnDocumentComplete(); On 2016/11/08 06:00:01, raymes wrote: > On 2016/11/08 01:28:10, Lei Zhang wrote: > > I defer to spelchat / art-snake on this. > > Sure. Without this the client was never notified that the document load failed. > Also I'm unsure what the benefit is of calling OnDocumentComplete() is here but > I might be missing something. Loading of the document may be canceled (by user or other) at moment, when pages needs for him already loaded. And if we call DocumentLoadFailed, he will se only error message after that. But i think, we should allow him to use already loaded pages in this case.
https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (left): https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1109: OnDocumentComplete(); On 2016/11/08 15:11:25, snake wrote: > On 2016/11/08 06:00:01, raymes wrote: > > On 2016/11/08 01:28:10, Lei Zhang wrote: > > > I defer to spelchat / art-snake on this. > > > > Sure. Without this the client was never notified that the document load > failed. > > Also I'm unsure what the benefit is of calling OnDocumentComplete() is here > but > > I might be missing something. > > Loading of the document may be canceled (by user or other) at moment, when pages > needs for him already loaded. And if we call DocumentLoadFailed, he will se only > error message after that. But i think, we should allow him to use already loaded > pages in this case. Yeah thinking about it, maybe the noisy failure (DocumentLoadFailed) is better. It's unfortunate that the user could potentially be able to view a few pages if we didn't display an error, but it seems preferable to showing a partially loaded document without mentioning that loading the document failed (and refreshing the page would potentially fix the issue). FWIW I don't think the user can cancel loading the document, short of pulling out the plug (there's no button to stop loading the page for PDFs AFAIK), so if OnDocumentCanceled is called, the user isn't likely to know the page load was canceled.
On 2016/11/08 17:53:33, spelchat wrote: > https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... > File pdf/pdfium/pdfium_engine.cc (left): > > https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... > pdf/pdfium/pdfium_engine.cc:1109: OnDocumentComplete(); > On 2016/11/08 15:11:25, snake wrote: > > On 2016/11/08 06:00:01, raymes wrote: > > > On 2016/11/08 01:28:10, Lei Zhang wrote: > > > > I defer to spelchat / art-snake on this. > > > > > > Sure. Without this the client was never notified that the document load > > failed. > > > Also I'm unsure what the benefit is of calling OnDocumentComplete() is here > > but > > > I might be missing something. > > > > Loading of the document may be canceled (by user or other) at moment, when > pages > > needs for him already loaded. And if we call DocumentLoadFailed, he will se > only > > error message after that. But i think, we should allow him to use already > loaded > > pages in this case. > > Yeah thinking about it, maybe the noisy failure (DocumentLoadFailed) is better. > It's unfortunate that the user could potentially be able to view a few pages if > we didn't display an error, but it seems preferable to showing a partially > loaded document without mentioning that loading the document failed (and > refreshing the page would potentially fix the issue). > > FWIW I don't think the user can cancel loading the document, short of pulling > out the plug (there's no button to stop loading the page for PDFs AFAIK), so if > OnDocumentCanceled is called, the user isn't likely to know the page load was > canceled. The case we're handling here is only if there is a HTTP response for a part of the document which returns 401 or some other unexpected HTTP error code. Generally, if that's going to happen it's going to happen on the first request for the document and not half way through a series of range requests, in which case there would be nothing to display anyway. So I think it's fine just to show an error here and not try to display anything. spelchat: if you're ok with this approach, please let me know :)
https://codereview.chromium.org/2455663004/diff/100001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2455663004/diff/100001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1109: client_->DocumentLoadFailed(); May be more better?: if (visible_pages_.empty()) { client_->DocumentLoadFailed(); } else { OnDocumentComplete(); }
On 2016/11/09 03:33:43, raymes wrote: > On 2016/11/08 17:53:33, spelchat wrote: > > > https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... > > File pdf/pdfium/pdfium_engine.cc (left): > > > > > https://codereview.chromium.org/2455663004/diff/80001/pdf/pdfium/pdfium_engin... > > pdf/pdfium/pdfium_engine.cc:1109: OnDocumentComplete(); > > On 2016/11/08 15:11:25, snake wrote: > > > On 2016/11/08 06:00:01, raymes wrote: > > > > On 2016/11/08 01:28:10, Lei Zhang wrote: > > > > > I defer to spelchat / art-snake on this. > > > > > > > > Sure. Without this the client was never notified that the document load > > > failed. > > > > Also I'm unsure what the benefit is of calling OnDocumentComplete() is > here > > > but > > > > I might be missing something. > > > > > > Loading of the document may be canceled (by user or other) at moment, when > > pages > > > needs for him already loaded. And if we call DocumentLoadFailed, he will se > > only > > > error message after that. But i think, we should allow him to use already > > loaded > > > pages in this case. > > > > Yeah thinking about it, maybe the noisy failure (DocumentLoadFailed) is > better. > > It's unfortunate that the user could potentially be able to view a few pages > if > > we didn't display an error, but it seems preferable to showing a partially > > loaded document without mentioning that loading the document failed (and > > refreshing the page would potentially fix the issue). > > > > FWIW I don't think the user can cancel loading the document, short of pulling > > out the plug (there's no button to stop loading the page for PDFs AFAIK), so > if > > OnDocumentCanceled is called, the user isn't likely to know the page load was > > canceled. > > The case we're handling here is only if there is a HTTP response for a part of > the document which returns 401 or some other unexpected HTTP error code. > Generally, if that's going to happen it's going to happen on the first request > for the document and not half way through a series of range requests, in which > case there would be nothing to display anyway. So I think it's fine just to show > an error here and not try to display anything. > > spelchat: if you're ok with this approach, please let me know :) SGTM
https://codereview.chromium.org/2455663004/diff/100001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2455663004/diff/100001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1109: client_->DocumentLoadFailed(); On 2016/11/09 14:27:28, snake wrote: > May be more better?: > if (visible_pages_.empty()) { > client_->DocumentLoadFailed(); > } else { > OnDocumentComplete(); > } Done.
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
spelchat/snake PTAL. If it lg to you then I'll land (since I'm an owner and thestig is not reviewing and he has already commented that it seems ok to him). Thanks!
On 2016/11/15 03:13:02, raymes wrote: > spelchat/snake PTAL. If it lg to you then I'll land (since I'm an owner and > thestig is not reviewing and he has already commented that it seems ok to him). > > Thanks! lg, Thanks.
lgtm
lgtm lgtm
The CQ bit was checked by raymes@chromium.org
On 2016/11/15 19:35:20, spelchat wrote: > lgtm > > lgtm Thanks all!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add test to ensure that URLs that redirect inside the PDF plugin fail to load A URL that is passed to the PDF plugin should already have its redirects resolved. If it doesn't, then the PDF that gets loaded may not have the same origin as the one the PDF is assumed to be in. If that happens, it can cause same origin policy violations. So redirects are disabled for requests made from the plugin. Redirects were disabled here: https://codereview.chromium.org/2409423004/. There is one other place where we make a request in the plugin which has disabled in this CL (the URLs that get loaded here are chrome internal URLs so they should never trigger redirects anyway but this is done for safety). This CL also adds some plumbing to ensure the redirect requests trigger a document load failure message to be sent to JS so that we can properly detect the load failure in tests. BUG=653749 ========== to ========== Add test to ensure that URLs that redirect inside the PDF plugin fail to load A URL that is passed to the PDF plugin should already have its redirects resolved. If it doesn't, then the PDF that gets loaded may not have the same origin as the one the PDF is assumed to be in. If that happens, it can cause same origin policy violations. So redirects are disabled for requests made from the plugin. Redirects were disabled here: https://codereview.chromium.org/2409423004/. There is one other place where we make a request in the plugin which has disabled in this CL (the URLs that get loaded here are chrome internal URLs so they should never trigger redirects anyway but this is done for safety). This CL also adds some plumbing to ensure the redirect requests trigger a document load failure message to be sent to JS so that we can properly detect the load failure in tests. BUG=653749 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add test to ensure that URLs that redirect inside the PDF plugin fail to load A URL that is passed to the PDF plugin should already have its redirects resolved. If it doesn't, then the PDF that gets loaded may not have the same origin as the one the PDF is assumed to be in. If that happens, it can cause same origin policy violations. So redirects are disabled for requests made from the plugin. Redirects were disabled here: https://codereview.chromium.org/2409423004/. There is one other place where we make a request in the plugin which has disabled in this CL (the URLs that get loaded here are chrome internal URLs so they should never trigger redirects anyway but this is done for safety). This CL also adds some plumbing to ensure the redirect requests trigger a document load failure message to be sent to JS so that we can properly detect the load failure in tests. BUG=653749 ========== to ========== Add test to ensure that URLs that redirect inside the PDF plugin fail to load A URL that is passed to the PDF plugin should already have its redirects resolved. If it doesn't, then the PDF that gets loaded may not have the same origin as the one the PDF is assumed to be in. If that happens, it can cause same origin policy violations. So redirects are disabled for requests made from the plugin. Redirects were disabled here: https://codereview.chromium.org/2409423004/. There is one other place where we make a request in the plugin which has disabled in this CL (the URLs that get loaded here are chrome internal URLs so they should never trigger redirects anyway but this is done for safety). This CL also adds some plumbing to ensure the redirect requests trigger a document load failure message to be sent to JS so that we can properly detect the load failure in tests. BUG=653749 Committed: https://crrev.com/a8563ff4076d3ee37db3aaa85cf5226a8922e30f Cr-Commit-Position: refs/heads/master@{#432293} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a8563ff4076d3ee37db3aaa85cf5226a8922e30f Cr-Commit-Position: refs/heads/master@{#432293} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
