Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11570081:
Support file system access in DevTools with isolated file system. (Closed)
|
Created:
7 years, 12 months ago by vsevik Modified:
7 years, 11 months ago CC:
chromium-reviews, jam, yurys, joi+watch-content_chromium.org, pam+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport file system access in DevTools with isolated file system.
BUG=167511
TBR=jam@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177130
Patch Set 1 #
Total comments: 8
Patch Set 2 : Pavel's comments addressed #
Total comments: 4
Patch Set 3 : Inlined methods #
Total comments: 15
Patch Set 4 : Renamed methods and added CHECK() #
Total comments: 19
Patch Set 5 : Addressed Kinuko's comments #
Total comments: 12
Patch Set 6 : Added ALLOW_THIS_IN_INITIALIZER_LIST, fixed nits. #
Total comments: 14
Patch Set 7 : Added fs name / root url generation in browser process as recommended by kinuko@ and addressed tsep… #Messages
Total messages: 34 (0 generated)
Hi Pavel, could you please review this DevTools-wise? Hi Kinuko, I understand you are working on File System API and you have implemented isolated context I am using here. Could you please review this?
Please note that all file system related stuff happens in devtools_file_system_helper. I'll upload the webkit counterpart shortly.
https://codereview.chromium.org/11570081/diff/1/chrome/app/generated_resource... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11570081/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:4180: + <message name="IDS_DEV_TOOLS_FILE_SYSTEM_SECURITY_FILE_NOT_EXISTS_MESSAGE" desc="Message that is shown in DevTools front-end when user selects file system folder without security file while setting up DevTools file system mappings."> _MAGIC_FILE_ ? Nobody knows what front-end means. https://codereview.chromium.org/11570081/diff/1/chrome/browser/devtools/devto... File chrome/browser/devtools/devtools_file_system_helper.cc (right): https://codereview.chromium.org/11570081/diff/1/chrome/browser/devtools/devto... chrome/browser/devtools/devtools_file_system_helper.cc:35: class DevToolsFileSystemHelper::SelectFolderDialog This class is almost exactly the same as the one in devtools_file_helper. Can we unify the select dialogs? https://codereview.chromium.org/11570081/diff/1/chrome/browser/devtools/devto... File chrome/browser/devtools/devtools_file_system_helper.h (right): https://codereview.chromium.org/11570081/diff/1/chrome/browser/devtools/devto... chrome/browser/devtools/devtools_file_system_helper.h:21: class DevToolsFileSystemHelper I think devtools_file_helper is sufficient and you should merge this one into it. Eventually, we will want everyone to use functionality of this one. https://codereview.chromium.org/11570081/diff/1/chrome/browser/devtools/devto... chrome/browser/devtools/devtools_file_system_helper.h:50: void SelectFolderAndGrantPermission(); It seems like these will produce corresponding delegate responses. Consider using base::Callback as a second callback parameter if so. Btw, all these names sound confusing. What "permission"? https://codereview.chromium.org/11570081/diff/1/chrome/browser/devtools/devto... File chrome/browser/devtools/devtools_window.cc (right): https://codereview.chromium.org/11570081/diff/1/chrome/browser/devtools/devto... chrome/browser/devtools/devtools_window.cc:99: prefs->RegisterDictionaryPref(prefs::kDevToolsSavedFileSystemPathes, Paths https://codereview.chromium.org/11570081/diff/1/chrome/browser/devtools/devto... chrome/browser/devtools/devtools_window.cc:838: DictionaryValue* CreateFileSystemValue( You should also make this function static instead of putting into an anonymous namespace. https://codereview.chromium.org/11570081/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/11570081/diff/1/chrome/common/pref_names.cc#n... chrome/common/pref_names.cc:1637: "devtools.saved_file_system_pathes"; paths https://codereview.chromium.org/11570081/diff/1/content/common/devtools_messa... File content/common/devtools_messages.h (right): https://codereview.chromium.org/11570081/diff/1/content/common/devtools_messa... content/common/devtools_messages.h:142: IPC_MESSAGE_ROUTED0(DevToolsHostMsg_RequestFileSystemPermissions) Again, these names do not sound clear.
Please take another look > https://codereview.chromium.org/11570081/diff/1/chrome/app/generated_resource... > chrome/app/generated_resources.grd:4180: + <message > name="IDS_DEV_TOOLS_FILE_SYSTEM_SECURITY_FILE_NOT_EXISTS_MESSAGE" desc="Message > that is shown in DevTools front-end when user selects file system folder without > security file while setting up DevTools file system mappings."> > _MAGIC_FILE_ ? Renamed to IDS_DEV_TOOLS_MAGIC_FILE_NOT_EXISTS_MESSAGE > Nobody knows what front-end means. Updated description. > https://codereview.chromium.org/11570081/diff/1/chrome/browser/devtools/devto... > chrome/browser/devtools/devtools_file_system_helper.cc:35: class > DevToolsFileSystemHelper::SelectFolderDialog > This class is almost exactly the same as the one in devtools_file_helper. Can we > unify the select dialogs? Reused this dialog. > https://codereview.chromium.org/11570081/diff/1/chrome/browser/devtools/devto... > chrome/browser/devtools/devtools_file_system_helper.h:21: class > DevToolsFileSystemHelper > I think devtools_file_helper is sufficient and you should merge this one into > it. Eventually, we will want everyone to use functionality of this one. Moved everything to DevToolsFileHelper > https://codereview.chromium.org/11570081/diff/1/chrome/browser/devtools/devto... > chrome/browser/devtools/devtools_file_system_helper.h:50: void > SelectFolderAndGrantPermission(); > It seems like these will produce corresponding delegate responses. Consider > using base::Callback as a second callback parameter if so. Implemented everything with callbacks this time. > Btw, all these names sound confusing. What "permission"? Simplified the names.
tsepez@, we would like to support file system access from DevTools. Could you please review this patch security-wise? cevans@, FYI
https://codereview.chromium.org/11570081/diff/9001/chrome/browser/devtools/de... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/9001/chrome/browser/devtools/de... chrome/browser/devtools/devtools_file_helper.cc:152: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); inline https://codereview.chromium.org/11570081/diff/9001/chrome/browser/devtools/de... chrome/browser/devtools/devtools_file_helper.cc:193: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); inline https://codereview.chromium.org/11570081/diff/9001/chrome/browser/devtools/de... chrome/browser/devtools/devtools_file_helper.cc:206: void CheckFolder(const FilePath& path, const CheckFolderCallback& callback) { Please inline it. https://codereview.chromium.org/11570081/diff/9001/chrome/browser/devtools/de... chrome/browser/devtools/devtools_file_helper.cc:325: CheckFolder(path, Bind(&DevToolsFileHelper::SelectedFilesystemChecked, CheckFolders(std::vector(1, path))
I'm still out for a few more days, adding jschuh@
> inline Done. > CheckFolders(std::vector(1, path)) Done.
https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:132: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); CHECK(content::GetContentClient()->HasWebUIScheme(web_contents->GetURL()) https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:151: typedef Callback<void(const std::vector<FilePath>&)> CheckFoldersCallback; Check -> Validate https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:189: : web_contents_(web_contents), profile_(profile), weak_factory_(this) { stack there. https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:285: void DevToolsFileHelper::FolderSelected(const AddFilesystemCallback& callback, InnerAddFilesystem https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:289: &DevToolsFileHelper::SelectedFilesystemChecked, AddValidatedFilesystem https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:343: &DevToolsFileHelper::RegisterPermittedFilesystems, RestoreFilesystems https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:352: void DevToolsFileHelper::RegisterPermittedFilesystems( RestoreValidatedFilesystems https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_window.cc (right): https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_window.cc:851: file_helper_->RequestFilesystems( RestoreFilesystems new DevToolsFileHelper(web_contents)->RestoreFilesystems();
https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:151: typedef Callback<void(const std::vector<FilePath>&)> CheckFoldersCallback; On 2012/12/29 10:23:02, pfeldman wrote: > Check -> Validate Done. https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:189: : web_contents_(web_contents), profile_(profile), weak_factory_(this) { On 2012/12/29 10:23:02, pfeldman wrote: > stack there. Done. https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:285: void DevToolsFileHelper::FolderSelected(const AddFilesystemCallback& callback, On 2012/12/29 10:23:02, pfeldman wrote: > InnerAddFilesystem Done. https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:289: &DevToolsFileHelper::SelectedFilesystemChecked, On 2012/12/29 10:23:02, pfeldman wrote: > AddValidatedFilesystem Done. https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:343: &DevToolsFileHelper::RegisterPermittedFilesystems, On 2012/12/29 10:23:02, pfeldman wrote: > RestoreFilesystems Done. https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:352: void DevToolsFileHelper::RegisterPermittedFilesystems( On 2012/12/29 10:23:02, pfeldman wrote: > RestoreValidatedFilesystems Done. https://codereview.chromium.org/11570081/diff/16001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:352: void DevToolsFileHelper::RegisterPermittedFilesystems( On 2012/12/29 10:23:02, pfeldman wrote: > RestoreValidatedFilesystems Done.
DevTools lgtm. Please get security clearance on the IPC messages. Lets also ask Yury to take another look.
Yury, could you please take a look on this?
https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:56: const char kMagicFileName[] = ".allow-devtools-edit"; const FilePath::CharType kMagicFileName[] https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:168: FilePath security_file_path = it->Append(FILE_PATH_LITERAL(kMagicFileName)); This wouldn't work on windows (the macro works only for literal). You can use FilePath::CharType (in line 56) and remove FILE_PATH_LITERAL. https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:174: Bind(&RunFoldersValidationCallback, callback, permitted_paths)); Unless you really want to check the callback runs on UI thread (it's posting on UI so it must do) I think you can just do: PostTask(UI, FROM_HERE, Bind(callback, permitted_paths)); https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:279: void DevToolsFileHelper::AddFilesystem(const AddFilesystemCallback& callback) { nit: In most other places we use 'FileSystem' (capital 's') notion in fileapi code for filesystem. If devtool side has no strong preference mind following the convention? https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:286: callback)); Bind(callback, "", Filesystem()) might be simpler? https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:311: UTF8ToUTF16(kMagicFileName)); FilePathStringToWebString(kMagicFileName) https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:368: ®istered_name); nit: indent https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:384: filesystems_paths_value->RemoveWithoutPathExpansion(filesystem_path, NULL); These file paths look to be saved/removed per profile and revoked at once on all renderers when RemoveFileSystem() is called (while filesystem permission is given per renderer when AddFileSystem() is called), and I assume it's exactly what's intended. https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.h (right): https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.h:80: // Removes isolated file system for given \filesystem_path|. nit: \filesystem_path| -> |filesystem_path| ?
https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:56: const char kMagicFileName[] = ".allow-devtools-edit"; On 2013/01/04 14:20:55, kinuko wrote: > const FilePath::CharType kMagicFileName[] Done. https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:168: FilePath security_file_path = it->Append(FILE_PATH_LITERAL(kMagicFileName)); On 2013/01/04 14:20:55, kinuko wrote: > This wouldn't work on windows (the macro works only for literal). You can use > FilePath::CharType (in line 56) and remove FILE_PATH_LITERAL. Done, moved FILE_PATH_LITERAL to line 56. https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:174: Bind(&RunFoldersValidationCallback, callback, permitted_paths)); On 2013/01/04 14:20:55, kinuko wrote: > Unless you really want to check the callback runs on UI thread (it's posting on > UI so it must do) I think you can just do: > > PostTask(UI, FROM_HERE, Bind(callback, permitted_paths)); Done. https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:279: void DevToolsFileHelper::AddFilesystem(const AddFilesystemCallback& callback) { On 2013/01/04 14:20:55, kinuko wrote: > nit: In most other places we use 'FileSystem' (capital 's') notion in fileapi > code for filesystem. If devtool side has no strong preference mind following the > convention? Done. https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:286: callback)); On 2013/01/04 14:20:55, kinuko wrote: > Bind(callback, "", Filesystem()) might be simpler? Done. https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:311: UTF8ToUTF16(kMagicFileName)); On 2013/01/04 14:20:55, kinuko wrote: > FilePathStringToWebString(kMagicFileName) I don't need WebString at this point as GetStringFUTF8 accepts UTF16 as a string parameter. So I am doing CharType[] -> FilePath -> UTF8 -> UTF16 conversion now. https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:368: ®istered_name); On 2013/01/04 14:20:55, kinuko wrote: > nit: indent Done. https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:368: ®istered_name); On 2013/01/04 14:20:55, kinuko wrote: > nit: indent Done. https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:384: filesystems_paths_value->RemoveWithoutPathExpansion(filesystem_path, NULL); This is correct and intended behavior. https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.h (right): https://codereview.chromium.org/11570081/diff/21001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.h:80: // Removes isolated file system for given \filesystem_path|. On 2013/01/04 14:20:55, kinuko wrote: > nit: \filesystem_path| -> |filesystem_path| ? Done.
Kinuko, could you please take another look?
https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:164: permitted_paths.push_back(*it); I'm afraid I don't fully understand this 'kMagicFileName' logic. So if a user creates the '.allow-devtools-edit' file the directory will become writable from Inspector? https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:187: weak_factory_(this) { I think you need to use ALLOW_THIS_IN_INITIALIZER_LIST(this) to avoid warnings on some platforms (e.g. weak_factory_(ALLOW_...(this)) ) https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:311: ®istered_name); nit: indent is off by 1 char? (line 310-311) https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:355: ®istered_name); nit: indent? (line 354-355)
https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:164: permitted_paths.push_back(*it); On 2013/01/09 15:17:50, kinuko wrote: > I'm afraid I don't fully understand this 'kMagicFileName' logic. So if a user > creates the '.allow-devtools-edit' file the directory will become writable from > Inspector? The directory will become writable only if the corresponding file system was added through addFileSystem method, that is once user selects this directory in file selection dialog, provided that magic file exists in this directory.
Please take another look https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:187: weak_factory_(this) { On 2013/01/09 15:17:50, kinuko wrote: > I think you need to use ALLOW_THIS_IN_INITIALIZER_LIST(this) to avoid warnings > on some platforms (e.g. weak_factory_(ALLOW_...(this)) ) > Done. https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:311: ®istered_name); On 2013/01/09 15:17:50, kinuko wrote: > nit: indent is off by 1 char? (line 310-311) Done. https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:355: ®istered_name); On 2013/01/09 15:17:50, kinuko wrote: > nit: indent? (line 354-355) Done.
I have a few more questions. Thanks for your patience, https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:164: permitted_paths.push_back(*it); On 2013/01/09 17:13:17, vsevik wrote: > On 2013/01/09 15:17:50, kinuko wrote: > > I'm afraid I don't fully understand this 'kMagicFileName' logic. So if a user > > creates the '.allow-devtools-edit' file the directory will become writable > from > > Inspector? > > The directory will become writable only if the corresponding file system was > added through addFileSystem method, that is once user selects this directory in > file selection dialog, provided that magic file exists in this directory. Who creates the magic file? Sorry if I'm asking something obvious. https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_window.cc (right): https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_window.cc:845: file_system_value->SetString("fileSystemPath", file_system.file_system_path); A follow-up comment to https://bugs.webkit.org/show_bug.cgi?id=105727#c14: if you want to follow the way you can use following helper methods to get necessary parameters (filesystem name and root URL): fileSystemName = fileapi::GetIsolatedFileSystemName(origin, file_system_id); rootUrl = fileapi::GetIsolatedFileSystemRootURIString(origin, file_system_id, registered_name); Then in WebKit code you can create isolated DOMFileSystem by: DOMFileSystem::create(context, FileSystemTypeIsolated, fileSystemName, rootUrl); https://codereview.chromium.org/11570081/diff/30001/content/renderer/devtools... File content/renderer/devtools/devtools_client.cc (right): https://codereview.chromium.org/11570081/diff/30001/content/renderer/devtools... content/renderer/devtools/devtools_client.cc:105: nit: extra empty line https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:366: isolated_context()->RevokeFileSystemByPath(path); Why do we return file_system_path to the inspector frontend and use the path to manage/remove filesystems instead of filesystem ID? (I'm asking this because in usual WebKit API we generally avoid giving back raw file paths to the renderer)
https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/30001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:164: permitted_paths.push_back(*it); On 2013/01/10 07:33:58, kinuko wrote: > On 2013/01/09 17:13:17, vsevik wrote: > > On 2013/01/09 15:17:50, kinuko wrote: > > > I'm afraid I don't fully understand this 'kMagicFileName' logic. So if a > user > > > creates the '.allow-devtools-edit' file the directory will become writable > > from > > > Inspector? > > > > The directory will become writable only if the corresponding file system was > > added through addFileSystem method, that is once user selects this directory > in > > file selection dialog, provided that magic file exists in this directory. > > Who creates the magic file? Sorry if I'm asking something obvious. User should create a magic file in order to add a folder to DevTools. This way we double check that it is safe enough to grant permissions for this folder: we know that user has selected this folder in the dialog and created a magic file separately. https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:366: isolated_context()->RevokeFileSystemByPath(path); On 2013/01/10 07:33:58, kinuko wrote: > Why do we return file_system_path to the inspector frontend and use the path to > manage/remove filesystems instead of filesystem ID? (I'm asking this because in > usual WebKit API we generally avoid giving back raw file paths to the renderer) I understand your concern. We need that because we would like to allow user to setup prefix-based mappings between network urls and file system, e.g. /var/www/myapp <-> http://localhost/myapp, so we need full raw file system paths. Since we already expect that renderer has raw file system path entered by user I don't think we need to hide it from renderer here.
fileapi related code lgtm https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:366: isolated_context()->RevokeFileSystemByPath(path); On 2013/01/10 08:14:21, vsevik wrote: > On 2013/01/10 07:33:58, kinuko wrote: > > Why do we return file_system_path to the inspector frontend and use the path > to > > manage/remove filesystems instead of filesystem ID? (I'm asking this because > in > > usual WebKit API we generally avoid giving back raw file paths to the > renderer) > > I understand your concern. We need that because we would like to allow user to > setup prefix-based mappings between network urls and file system, e.g. > /var/www/myapp <-> http://localhost/myapp, so we need full raw file system > paths. > Since we already expect that renderer has raw file system path entered by user I > don't think we need to hide it from renderer here. I see, thanks for your explanation! I think I'm clearer now. https://codereview.chromium.org/11570081/diff/34001/content/renderer/devtools... File content/renderer/devtools/devtools_client.cc (right): https://codereview.chromium.org/11570081/diff/34001/content/renderer/devtools... content/renderer/devtools/devtools_client.cc:105: nit: extra empty line
Could someone from security team please take a look on this patch?
On 2013/01/11 17:32:29, vsevik wrote: > Could someone from security team please take a look on this patch? Tom, you seem a better candidate to review this one, so I'm removing myself.
Q. On startup, do we automatically grant permission to the filesystems stored in the preference that contain the magic file? I presume the peference is initially empty, and the only way to add to it is through the dialog box. https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:56: const FilePath::CharType kMagicFileName[] = How does this file get created? Manually, outside of chrome, as an extra sanity check before using a directory? What about other places where there is some user control over the filename, like in a downloads directory -- what happens if a page downloads a .allow-devtools-edit for example. https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:353: std::string file_system_id = RegisterFileSystem(web_contents_, Why is OK to skip the magic file check here? https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_window.cc (right): https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_window.cc:101: PrefServiceSyncable::UNSYNCABLE_PREF); Good. Syncing across systems can't be allowed. https://codereview.chromium.org/11570081/diff/34001/content/common/devtools_m... File content/common/devtools_messages.h (right): https://codereview.chromium.org/11570081/diff/34001/content/common/devtools_m... content/common/devtools_messages.h:142: IPC_MESSAGE_ROUTED0(DevToolsHostMsg_RequestFileSystems) nit: I might call this _ListFileSystems since request implies to me some sort of active action. https://codereview.chromium.org/11570081/diff/34001/content/public/browser/de... File content/public/browser/devtools_frontend_host_delegate.h (right): https://codereview.chromium.org/11570081/diff/34001/content/public/browser/de... content/public/browser/devtools_frontend_host_delegate.h:45: // Requests the list of file systems added for devtools previously. nit: Requests the list of filesystems previously added for devtools. https://codereview.chromium.org/11570081/diff/34001/content/public/browser/de... content/public/browser/devtools_frontend_host_delegate.h:48: // Shows a dialog to select a folder to add an isolated file system for. nit: Shows a dialog to select a folder to which an isolated filesystem is added. https://codereview.chromium.org/11570081/diff/34001/content/public/browser/de... content/public/browser/devtools_frontend_host_delegate.h:51: // Removes previously added devtools file system by given |file_system_path|. nit: Removes a previously added devtools filesystem given by |file_system_path|. https://codereview.chromium.org/11570081/diff/34001/content/public/browser/de... content/public/browser/devtools_frontend_host_delegate.h:52: virtual void RemoveFileSystem(const std::string& file_system_path) = 0; nit: Removes a previously added devtools filesystem given by |file_system_path|.
https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... File chrome/browser/devtools/devtools_file_helper.cc (right): https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... chrome/browser/devtools/devtools_file_helper.cc:131: std::string RegisterFileSystem(WebContents* web_contents, Maybe put the magic file check here so there can't be a path that registers a filesystem without checking for it. https://codereview.chromium.org/11570081/diff/34001/content/common/devtools_m... File content/common/devtools_messages.h (right): https://codereview.chromium.org/11570081/diff/34001/content/common/devtools_m... content/common/devtools_messages.h:142: IPC_MESSAGE_ROUTED0(DevToolsHostMsg_RequestFileSystems) On 2013/01/11 18:34:17, Tom Sepez wrote: > nit: I might call this _ListFileSystems since request implies to me some sort > of active action. Nevermind. It looks like this is doing more than just making a list.
Tom, could you please take another look? > Q. On startup, do we automatically grant permission to the filesystems stored in > the preference that contain the magic file? I presume the peference is initially > empty, and the only way to add to it is through the dialog box. Yes, the preference is initially empty and the only way to add to it is through the dialog box. The preference is persisted accross Browser / DevTools restarts. Permissions are automatically granted on DevTools startup for previously added folders (but only if they still contain magic file). > https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... > File chrome/browser/devtools/devtools_file_helper.cc (right): > https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... > chrome/browser/devtools/devtools_file_helper.cc:56: const FilePath::CharType > kMagicFileName[] = > How does this file get created? Manually, outside of chrome, as an extra sanity > check before using a directory? What about other places where there is some > user control over the filename, like in a downloads directory -- what happens if > a page downloads a .allow-devtools-edit for example. Yes, it is supposed to be manually created outside of chrome as an extra sanity check. If a page downloads a .allow-devtools-edit the user should still select a folder in the dialog. Do you think it is safe enough? > https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... > chrome/browser/devtools/devtools_file_helper.cc:353: std::string file_system_id > = RegisterFileSystem(web_contents_, > Why is OK to skip the magic file check here? It wouldn't be OK but this this check is actually done in RequestFileSystems() and RestoreValidatedFileSystems() is called back after the check. > nit: Requests the list of filesystems previously added for devtools. > nit: Shows a dialog to select a folder to which an isolated filesystem is added. > nit: Removes a previously added devtools filesystem given by |file_system_path|. Done. > https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... > File chrome/browser/devtools/devtools_file_helper.cc (right): > https://codereview.chromium.org/11570081/diff/34001/chrome/browser/devtools/d... > chrome/browser/devtools/devtools_file_helper.cc:131: std::string > RegisterFileSystem(WebContents* web_contents, > Maybe put the magic file check here so there can't be a path that registers a > filesystem without checking for it. This check should be done on IO thread so it is not possible to simply move it here. I would also prefer RegisterFileSystem to keep called from DevToolsFileHelper instance methods so that we could be sure that this instance still exists.
LGTM.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vsevik@chromium.org/11570081/46001
Presubmit check for 11570081-46001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content/public/browser
Presubmit checks took 2.2s to calculate.
jam@, I need an OWNER review for content/public/browser, could you please have a look on this patch?
On 2013/01/16 07:11:10, vsevik wrote: > jam@, I need an OWNER review for content/public/browser, > could you please have a look on this patch? Pavel suggested that I add you as TBR instead for rubber-stamping devtools_frontend_host_delegate.h changes.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vsevik@chromium.org/11570081/46001
Message was sent while issue was closed.
Change committed as 177130 |
