Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2321823002:
Exclude exe files while unzipping CRXes (Closed)
DescriptionExclude exe files while unzipping CRXes
BUG=468355
Committed: https://crrev.com/b040d3a050ac90bdae599ced55b2f8f7ac84bc30
Cr-Commit-Position: refs/heads/master@{#417494}
Patch Set 1 #Patch Set 2 : Fix compile on Windows #Patch Set 3 : Add browser tests #Patch Set 4 : Fix build #
Total comments: 10
Patch Set 5 : Devlin comments #
Total comments: 2
Patch Set 6 : Ignore case #
Messages
Total messages: 45 (24 generated)
The CQ bit was checked by meacer@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by meacer@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 checked by meacer@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: Exceeded global retry quota
The CQ bit was checked by meacer@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...
meacer@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin, can you PTAL? Thanks.
Devlin, can you PTAL? Thanks.
https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... File extensions/utility/extension_extractor_filter.cc (right): https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... extensions/utility/extension_extractor_filter.cc:14: return file_path.Extension() != FILE_PATH_LITERAL(".exe"); Should this be FinalExtension()? https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... File extensions/utility/extension_extractor_filter.h (right): https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... extensions/utility/extension_extractor_filter.h:19: : public base::RefCounted<ExtensionExtractorFilter> { Given this class doesn't need any state, I'd much rather not introduce another refcounted class. Let's maybe just have a function. https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... File extensions/utility/extension_extractor_filter_unittest.cc (right): https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... extensions/utility/extension_extractor_filter_unittest.cc:20: class ExtensionExtractorFilterTest : public PlatformTest { Do we need this instead of just testing::Test? https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/util... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/util... extensions/utility/utility_handler.cc:89: if (!zip::UnzipWithFilterCallback(zip_path, dir, filter_cb)) { This will silently ignore all .exe files. That's probably good in the case of normal installations, but I wonder if we can add an install warning for unpacked extensions and/or when packing an extension to warn about this. I'm fine with doing that in a separate patch, but can we add a TODO? https://codereview.chromium.org/2321823002/diff/60001/third_party/zlib/google... File third_party/zlib/google/zip.h (right): https://codereview.chromium.org/2321823002/diff/60001/third_party/zlib/google... third_party/zlib/google/zip.h:46: typedef base::Callback<bool(const base::FilePath&)> FilterCallback; prefer using syntax
The CQ bit was checked by meacer@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...
https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... File extensions/utility/extension_extractor_filter.cc (right): https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... extensions/utility/extension_extractor_filter.cc:14: return file_path.Extension() != FILE_PATH_LITERAL(".exe"); On 2016/09/08 21:10:16, Devlin wrote: > Should this be FinalExtension()? Extension() and FinalExtension() seems to differ only if the first extension is one of "gz", "z", "bz2" and "bz", but changed it to FinalExtension to be safe. https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... File extensions/utility/extension_extractor_filter.h (right): https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... extensions/utility/extension_extractor_filter.h:19: : public base::RefCounted<ExtensionExtractorFilter> { On 2016/09/08 21:10:16, Devlin wrote: > Given this class doesn't need any state, I'd much rather not introduce another > refcounted class. Let's maybe just have a function. That's fair, I copied this over from ExtensionCreatorFilter. Moved it under unpacker.cc as a static function. https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... File extensions/utility/extension_extractor_filter_unittest.cc (right): https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/exte... extensions/utility/extension_extractor_filter_unittest.cc:20: class ExtensionExtractorFilterTest : public PlatformTest { On 2016/09/08 21:10:16, Devlin wrote: > Do we need this instead of just testing::Test? Moved this to unpacker_unittest.cc https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/util... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2321823002/diff/60001/extensions/utility/util... extensions/utility/utility_handler.cc:89: if (!zip::UnzipWithFilterCallback(zip_path, dir, filter_cb)) { On 2016/09/08 21:10:16, Devlin wrote: > This will silently ignore all .exe files. That's probably good in the case of > normal installations, but I wonder if we can add an install warning for unpacked > extensions and/or when packing an extension to warn about this. > > I'm fine with doing that in a separate patch, but can we add a TODO? Done. https://codereview.chromium.org/2321823002/diff/60001/third_party/zlib/google... File third_party/zlib/google/zip.h (right): https://codereview.chromium.org/2321823002/diff/60001/third_party/zlib/google... third_party/zlib/google/zip.h:46: typedef base::Callback<bool(const base::FilePath&)> FilterCallback; On 2016/09/08 21:10:16, Devlin wrote: > prefer using syntax Actually, this is typedeffed above so removed the second definition.
I like it, lgtm!
meacer@chromium.org changed reviewers: + hshi@chromium.org
hshi: Can you PTAL at third_party/zlib/google/*?
hshi@google.com changed reviewers: + hshi@google.com
Mustafa: I don't have access to bug 468355 (I assume it is a security incident and the bug is locked down)? Is this specifically for Windows platform? Looks good but I wonder if you need to take into consideration that the file system may not be case sensitive. For example would ".EXE" files be excluded as well?
https://codereview.chromium.org/2321823002/diff/80001/extensions/utility/unpa... File extensions/utility/unpacker.cc (right): https://codereview.chromium.org/2321823002/diff/80001/extensions/utility/unpa... extensions/utility/unpacker.cc:120: return file_path.FinalExtension() != FILE_PATH_LITERAL(".exe"); See filepath.h: // Returns true if the file path matches the specified extension. The test is // case insensitive. Don't forget the leading period if appropriate. bool MatchesExtension(StringPieceType extension) const;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> Mustafa: I don't have access to bug 468355 (I assume it is a security incident and the bug is locked down)? Is this specifically for Windows platform? Apologies, I've CC'ed you on the bug. AFAICT it's not specific to Windows, but we are starting with that for now. https://codereview.chromium.org/2321823002/diff/80001/extensions/utility/unpa... File extensions/utility/unpacker.cc (right): https://codereview.chromium.org/2321823002/diff/80001/extensions/utility/unpa... extensions/utility/unpacker.cc:120: return file_path.FinalExtension() != FILE_PATH_LITERAL(".exe"); On 2016/09/08 22:54:43, hshi wrote: > See filepath.h: > > // Returns true if the file path matches the specified extension. The test is > // case insensitive. Don't forget the leading period if appropriate. > bool MatchesExtension(StringPieceType extension) const; Oh, great catch! Done, and I updated the crx with test1.EXE and test2.exe files.
lgtm
lgtm
Thanks!
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2321823002/#ps100001 (title: "Ignore case")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by meacer@chromium.org
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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Exclude exe files while unzipping CRXes BUG=468355 ========== to ========== Exclude exe files while unzipping CRXes BUG=468355 Committed: https://crrev.com/b040d3a050ac90bdae599ced55b2f8f7ac84bc30 Cr-Commit-Position: refs/heads/master@{#417494} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b040d3a050ac90bdae599ced55b2f8f7ac84bc30 Cr-Commit-Position: refs/heads/master@{#417494}
Message was sent while issue was closed.
On 2016/09/09 03:22:28, commit-bot: I haz the power wrote: > Patchset 6 (id:??) landed as > https://crrev.com/b040d3a050ac90bdae599ced55b2f8f7ac84bc30 > Cr-Commit-Position: refs/heads/master@{#417494} Does it make sense to change those DLOG(WARNING) to DVLOG(2)? Developers run debug builds all the time and I am getting a lot of logs from zip.cc, e.g. https://cs.chromium.org/chromium/src/third_party/zlib/google/zip.cc?rcl=0&l=126
Message was sent while issue was closed.
On 2016/10/21 00:41:51, xhwang wrote: > On 2016/09/09 03:22:28, commit-bot: I haz the power wrote: > > Patchset 6 (id:??) landed as > > https://crrev.com/b040d3a050ac90bdae599ced55b2f8f7ac84bc30 > > Cr-Commit-Position: refs/heads/master@{#417494} > > Does it make sense to change those DLOG(WARNING) to DVLOG(2)? Developers run > debug builds all the time and I am getting a lot of logs from zip.cc, e.g. > > https://cs.chromium.org/chromium/src/third_party/zlib/google/zip.cc?rcl=0&l=126 When the log was added, it was because we only skipped files that shouldn't be there at all (e.g. exes), and I'd be fine with having logspam for that, since it would indicate something is wrong. But now we do two passes here (the first just to get the manifest), so all the other files are skipped there. meacer@, can we only emit the log when we skip a dangerous file, rather than in the first pass?
Message was sent while issue was closed.
On 2016/10/21 15:07:30, Devlin wrote: > On 2016/10/21 00:41:51, xhwang wrote: > > On 2016/09/09 03:22:28, commit-bot: I haz the power wrote: > > > Patchset 6 (id:??) landed as > > > https://crrev.com/b040d3a050ac90bdae599ced55b2f8f7ac84bc30 > > > Cr-Commit-Position: refs/heads/master@{#417494} > > > > Does it make sense to change those DLOG(WARNING) to DVLOG(2)? Developers run > > debug builds all the time and I am getting a lot of logs from zip.cc, e.g. > > > > > https://cs.chromium.org/chromium/src/third_party/zlib/google/zip.cc?rcl=0&l=126 > > When the log was added, it was because we only skipped files that shouldn't be > there at all (e.g. exes), and I'd be fine with having logspam for that, since it > would indicate something is wrong. But now we do two passes here (the first > just to get the manifest), so all the other files are skipped there. meacer@, > can we only emit the log when we skip a dangerous file, rather than in the first > pass? We discussed adding an install warning for skipped files before: https://crbug.com/645263 I think if we implement that, we can remove this DLOG altogether or make it a DVLOG? Since the callers already know what files should be ignored, there would be little reason to keep the dlog except for extension developers who will be informed by the install warning. WDYT?
Message was sent while issue was closed.
On 2016/10/21 22:30:48, Mustafa Emre Acer wrote: > On 2016/10/21 15:07:30, Devlin wrote: > > On 2016/10/21 00:41:51, xhwang wrote: > > > On 2016/09/09 03:22:28, commit-bot: I haz the power wrote: > > > > Patchset 6 (id:??) landed as > > > > https://crrev.com/b040d3a050ac90bdae599ced55b2f8f7ac84bc30 > > > > Cr-Commit-Position: refs/heads/master@{#417494} > > > > > > Does it make sense to change those DLOG(WARNING) to DVLOG(2)? Developers run > > > debug builds all the time and I am getting a lot of logs from zip.cc, e.g. > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/zlib/google/zip.cc?rcl=0&l=126 > > > > When the log was added, it was because we only skipped files that shouldn't be > > there at all (e.g. exes), and I'd be fine with having logspam for that, since > it > > would indicate something is wrong. But now we do two passes here (the first > > just to get the manifest), so all the other files are skipped there. meacer@, > > can we only emit the log when we skip a dangerous file, rather than in the > first > > pass? > > We discussed adding an install warning for skipped files before: > https://crbug.com/645263 > > I think if we implement that, we can remove this DLOG altogether or make it a > DVLOG? > Since the callers already know what files should be ignored, there would be > little > reason to keep the dlog except for extension developers who will be informed by > the > install warning. WDYT? I agree; if we implement the install warnings, we don't need these logs at all. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
