Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2234543002:
Allow loading MHTML archive from http/https schemes (Closed)
DescriptionAllow loading MHTML archive from http/https schemes
With full sandboxing protection, loading MHTML archive from remote
scheme is not a risk any more.
BUG=636072
TEST=new test added
Committed: https://crrev.com/302b546d43a6c5e2ad57282441d26e52dc88dad5
Cr-Commit-Position: refs/heads/master@{#411206}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update comments #
Total comments: 4
Patch Set 3 : Update comments per feedback #
Messages
Total messages: 29 (16 generated)
The CQ bit was checked by jianli@chromium.org to run a CQ dry run
jianli@chromium.org changed reviewers: + dimich@chromium.org, nasko@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2234543002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right): https://codereview.chromium.org/2234543002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:73: // The latter is now allowed due to full sandboxing enforcement on MHTML pages. lets add comment on why not to just remove the check at all. https://codereview.chromium.org/2234543002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/MHTMLTest.cpp (right): https://codereview.chromium.org/2234543002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/MHTMLTest.cpp:255: // Checks that the domain is set to the actual MHTML file, not the URL it was I can't parse this comment, how can domain be set to a file? From what I see, the test is testing that when the multipart/related is loaded into main frame, that frame is sandboxed properly.
https://codereview.chromium.org/2234543002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right): https://codereview.chromium.org/2234543002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:73: // The latter is now allowed due to full sandboxing enforcement on MHTML pages. On 2016/08/10 00:25:04, Dmitry Titov wrote: > lets add comment on why not to just remove the check at all. Done. https://codereview.chromium.org/2234543002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/MHTMLTest.cpp (right): https://codereview.chromium.org/2234543002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/MHTMLTest.cpp:255: // Checks that the domain is set to the actual MHTML file, not the URL it was On 2016/08/10 00:25:04, Dmitry Titov wrote: > I can't parse this comment, how can domain be set to a file? > From what I see, the test is testing that when the multipart/related is loaded > into main frame, that frame is sandboxed properly. Forgot to update the comment. Updated.
lgtm
The CQ bit was checked by jianli@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
nasko@chromium.org changed reviewers: + lukasza@chromium.org
Adding lukasza@, who is the MHTML expert. Happy to stamp it once he is happy with it. https://codereview.chromium.org/2234543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right): https://codereview.chromium.org/2234543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:72: // MHTML pages could only be loaded from local URLs and http/https URLs. If it is "could" (implying past tense), then http/https should not be in that sentence, right? This CL is now allowing those two, which was not the case in the past. Otherwise s/could/can/. https://codereview.chromium.org/2234543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:74: // Other schemes might be allowed upon more security analysis. I don't think this sentence is needed. HTTP is pretty bad already, so it can hardly be worse than that : ).
The CQ bit was checked by jianli@chromium.org to run a CQ dry run
https://codereview.chromium.org/2234543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right): https://codereview.chromium.org/2234543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:72: // MHTML pages could only be loaded from local URLs and http/https URLs. On 2016/08/10 20:36:54, nasko (slow) wrote: > If it is "could" (implying past tense), then http/https should not be in that > sentence, right? This CL is now allowing those two, which was not the case in > the past. Otherwise s/could/can/. Done. https://codereview.chromium.org/2234543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:74: // Other schemes might be allowed upon more security analysis. On 2016/08/10 20:36:54, nasko (slow) wrote: > I don't think this sentence is needed. HTTP is pretty bad already, so it can > hardly be worse than that : ). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/10 20:36:55, nasko (slow) wrote: > Adding lukasza@, who is the MHTML expert. Happy to stamp it once he is happy > with it. > LGTM - thanks! In particular, I am satisfied with the explanation of security considerations given in https://docs.google.com/document/d/17MGCpVsoUv5fTIJ6cHDEksKHiq-48-BOLk5xjf1Ny...
I'm not an owner in Blink, but from security perspective, LGTM.
jianli@chromium.org changed reviewers: + esprehn@chromium.org
esprehn, could you review and approve platform changes as owner? thanks.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2234543002/#ps40001 (title: "Update comments per feedback")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Allow loading MHTML archive from http/https schemes With full sandboxing protection, loading MHTML archive from remote scheme is not a risk any more. BUG=636072 TEST=new test added ========== to ========== Allow loading MHTML archive from http/https schemes With full sandboxing protection, loading MHTML archive from remote scheme is not a risk any more. BUG=636072 TEST=new test added Committed: https://crrev.com/302b546d43a6c5e2ad57282441d26e52dc88dad5 Cr-Commit-Position: refs/heads/master@{#411206} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/302b546d43a6c5e2ad57282441d26e52dc88dad5 Cr-Commit-Position: refs/heads/master@{#411206} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
