Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
544483003:
Add tests for File System URLs with the file:// origin. (Closed)
|
Created:
6 years, 3 months ago by hirono Modified:
6 years, 3 months ago Reviewers:
mtomasz CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, yoshiki+watch_chromium.org, rginda+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd tests for File System URLs with the file:// origin.
The tests ensures extensions do not access filesystem:file:///* URL.
BUG=367027
TEST=the new tests
Committed: https://crrev.com/8a021cc9e570d502a7ce46e5d108776065c910f3
Cr-Commit-Position: refs/heads/master@{#293821}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 6
Patch Set 4 : Fixed #
Total comments: 7
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 18 (4 generated)
hirono@chromium.org changed reviewers: + mtomasz@chromium.org
@mtomasz - PTAL the CL? @tzik - CC Thank you!
https://codereview.chromium.org/544483003/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js (right): https://codereview.chromium.org/544483003/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:14: chrome.test.fail(error.stack || error); chrome.test.fail already prints the stack trace. Isn't returning it as the error message duplicating it? https://codereview.chromium.org/544483003/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:110: 'filesystem:file:///external/drive/root/test_dir/test_file.xul')) I think we should confirm the error code. Eg. it shouldn't be 404, but rather a security error. https://codereview.chromium.org/544483003/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/manifest.json (right): https://codereview.chromium.org/544483003/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/manifest.json:4: "version": "0.0", nit: Why 0.1 -> 0.0? Are other tests also using 0.0?
https://codereview.chromium.org/544483003/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js (right): https://codereview.chromium.org/544483003/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:14: chrome.test.fail(error.stack || error); On 2014/09/04 04:34:10, mtomasz wrote: > chrome.test.fail already prints the stack trace. Isn't returning it as the error > message duplicating it? chrome.test.fail prints the current stack. If the exception is thrown in the Promise, the error has a different stack trace. https://codereview.chromium.org/544483003/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:110: 'filesystem:file:///external/drive/root/test_dir/test_file.xul')) On 2014/09/04 04:34:11, mtomasz wrote: > I think we should confirm the error code. Eg. it shouldn't be 404, but rather a > security error. Done. https://codereview.chromium.org/544483003/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/manifest.json (right): https://codereview.chromium.org/544483003/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/manifest.json:4: "version": "0.0", On 2014/09/04 04:34:11, mtomasz wrote: > nit: Why 0.1 -> 0.0? Are other tests also using 0.0? Turned it back to 0.1.
https://codereview.chromium.org/544483003/diff/60001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js (right): https://codereview.chromium.org/544483003/diff/60001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:117: then(function(error) { nit: Does it work? Shouldn't it be catch() instead of then() here? https://codereview.chromium.org/544483003/diff/60001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:118: chrome.test.assertEq('SECURITY_ERROR', error); nit: If another error happened, then we would compare 'SECURITY_ERROR' vs. an Event object. Is it OK?
https://codereview.chromium.org/544483003/diff/60001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js (right): https://codereview.chromium.org/544483003/diff/60001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:117: then(function(error) { On 2014/09/04 05:39:29, mtomasz wrote: > nit: Does it work? Shouldn't it be catch() instead of then() here? This is handled by expectRejection function. The function returns error if the passed promise succeeds unintentionally, and returns a fulfilled error since the error is intentional here. https://codereview.chromium.org/544483003/diff/60001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:118: chrome.test.assertEq('SECURITY_ERROR', error); On 2014/09/04 05:39:29, mtomasz wrote: > nit: If another error happened, then we would compare 'SECURITY_ERROR' vs. an > Event object. Is it OK? Double checked. Yes, chrome.test.assertEq raises test failure for the case.
Sorry for late, I missed this one. https://codereview.chromium.org/544483003/diff/60001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js (right): https://codereview.chromium.org/544483003/diff/60001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:117: then(function(error) { On 2014/09/04 06:15:33, hirono wrote: > On 2014/09/04 05:39:29, mtomasz wrote: > > nit: Does it work? Shouldn't it be catch() instead of then() here? > > This is handled by expectRejection function. The function returns error if the > passed promise succeeds unintentionally, and returns a fulfilled error since > the error is intentional here. Ah, my bad. I thought then() is called on sendXHR, but there was a ")" I didn't notice :). https://codereview.chromium.org/544483003/diff/60001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:118: chrome.test.assertEq('SECURITY_ERROR', error); On 2014/09/04 06:15:33, hirono wrote: > On 2014/09/04 05:39:29, mtomasz wrote: > > nit: If another error happened, then we would compare 'SECURITY_ERROR' vs. an > > Event object. Is it OK? > > Double checked. > Yes, chrome.test.assertEq raises test failure for the case. My concern is that we're passing Event as variable called "error". That works, but is confusing. Maybe we should just return the error code? Eg. xhr.onerror = function(event) { reject('Status: ' + xhr.status) } Then we'd compare eg. 'SECURITY_ERROR' vs. 'Status: 404', which I think makes more sense, and the test log would be more helpful. WDYT?
https://codereview.chromium.org/544483003/diff/60001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js (right): https://codereview.chromium.org/544483003/diff/60001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:118: chrome.test.assertEq('SECURITY_ERROR', error); On 2014/09/05 05:20:12, mtomasz wrote: > On 2014/09/04 06:15:33, hirono wrote: > > On 2014/09/04 05:39:29, mtomasz wrote: > > > nit: If another error happened, then we would compare 'SECURITY_ERROR' vs. > an > > > Event object. Is it OK? > > > > Double checked. > > Yes, chrome.test.assertEq raises test failure for the case. > > My concern is that we're passing Event as variable called "error". That works, > but is confusing. > > Maybe we should just return the error code? > > Eg. > > xhr.onerror = function(event) { > reject('Status: ' + xhr.status) > } > > Then we'd compare eg. 'SECURITY_ERROR' vs. 'Status: 404', which I think makes > more sense, and the test log would be more helpful. WDYT? I just noticed if we specify "file:///*" as a permission in the manifest.json, the xhr.send does not throw DOMException, and reports an error by using the onerror handler. The status code is 0. But if we specify the URL referring an in-existent file, it also returns 0 status code. We cannot discriminate both cases.
lgtm! https://codereview.chromium.org/544483003/diff/80001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js (right): https://codereview.chromium.org/544483003/diff/80001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:65: reject('ERROR_STATUS:' + xhr.status); nit: We usually put a space after :.
Thank you! https://codereview.chromium.org/544483003/diff/80001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js (right): https://codereview.chromium.org/544483003/diff/80001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/file_browser/filesystem_file_origin_url/background.js:65: reject('ERROR_STATUS:' + xhr.status); On 2014/09/05 09:30:29, mtomasz wrote: > nit: We usually put a space after :. Done.
The CQ bit was checked by hirono@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/544483003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by hirono@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/544483003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as 480dfb3d4f372dcba11fa7b51beeda825a241dfa
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8a021cc9e570d502a7ce46e5d108776065c910f3 Cr-Commit-Position: refs/heads/master@{#293821} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
