Closed Bug 1209924 Opened 9 years ago Closed 8 years ago

Implement a general filtering mechanism for filtering of Directory::GetFilesAndDirectories, and create a filter to filter out sensitive files/directories

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

As part of exploring the effort that would be involved for some of the ideas in bug 907707 (security restrictions for directory picking and directory drag-and-drop) I had a pass at implementing filtering of the results from Directory::GetFilesAndDirectories. This may not go anywhere, but I'll attach my POC code.
This does the filtering on the content process in Directory::GetFilesAndDirectories itself, which isn't ideal. In this case we can't pass through the dom::Promise from GetDirectoryListingTask to use as the return value for Directory::GetFilesAndDirectories, so I've inserted an intermediary MozPromise.

Better would be to pass filtering information up to GetDirectoryListingTask and have that information passed across to the Chrome process to do the filtering there. That may be trickier, but it would be more efficient and the more practical way to filter based on information that we don't [currently] have in the content process, such as whether a file/directory is marked as hidden or not.
Attached patch patchSplinter Review
This is a somewhat cleaned up version of the 'POC code for chrome process side filtering' patch.

The filtering of sensitive files/directories can be trivially expanded by modifying the 'filterOutSensitive' block in GetDirectoryListingTask::Work.
Attachment #8667943 - Attachment is obsolete: true
Attachment #8667944 - Attachment is obsolete: true
Attachment #8686038 - Flags: review?(amarchesini)
Summary: Support filtering of Directory::GetFilesAndDirectories → Implement a general filtering mechanism for filtering of Directory::GetFilesAndDirectories, and create a filter to filter out sensitive files/directories
Flags: needinfo?(dveditz)
Richard, Daniel, this patch creates the necessary framework to implement filtering of the results of directory picking, but currently I'm only filtering out files/directories that are hidden or that begin with ".":

https://bugzilla.mozilla.org/attachment.cgi?id=8686038&action=diff#a/dom/filesystem/GetDirectoryListingTask.cpp_sec5

Can you decide exactly what else you want to filter out and then we can modify the 'filterOutSensitive' block in GetDirectoryListingTask::Work as necessary.
Flags: needinfo?(rlb)
Blocks: 907707
Comment on attachment 8686038 [details] [diff] [review]
patch

Review of attachment 8686038 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/DataTransfer.cpp
@@ +910,5 @@
>          nsDependentSubstring dirname = Substring(path, 0, leafSeparatorIndex);
>          nsDependentSubstring basename = Substring(path, leafSeparatorIndex);
>          fs = MakeOrReuseFileSystem(dirname, fs, window);
> +        RefPtr<Directory> directory = new Directory(fs, basename);
> +        directory->SetContentFilters(NS_LITERAL_STRING("filter-out-sensitive"));

What about if you set a default contentFilter in the CTOR ?
Maybe optional.

::: dom/filesystem/GetDirectoryListingTask.cpp
@@ +165,5 @@
> +    HTMLSplitOnSpacesTokenizer tokenizer(mFilters, ';');
> +    nsAutoString token;
> +    while (tokenizer.hasMoreTokens()) {
> +      token = tokenizer.nextToken();
> +      if (token == NS_LITERAL_STRING("filter-out-sensitive")) {

EqualLiteral("filter-out-sensitive")

@@ +168,5 @@
> +      token = tokenizer.nextToken();
> +      if (token == NS_LITERAL_STRING("filter-out-sensitive")) {
> +        filterOutSensitive = true;
> +      } else {
> +        MOZ_ASSERT(false, "Unrecognized filter");

MOZ_CRASH ?

@@ +261,5 @@
>        }
>  #endif
> +      RefPtr<Directory> directory = new Directory(mFileSystem, path);
> +      // Propogate mFilter onto sub-Directory object:
> +      directory->SetContentFilters(mFilters);

Also here you can pass mFilters in the CTOR.

::: dom/html/HTMLInputElement.cpp
@@ +4937,5 @@
> +      RefPtr<Directory> directory = new Directory(fs, dompath);
> +      // In future we could refactor SetFilePickerFiltersFromAccept to return a
> +      // semicolon separated list of file extensions and include that in the
> +      // filter string passed here.
> +      directory->SetContentFilters(NS_LITERAL_STRING("filter-out-sensitive"));

ditto.
Attachment #8686038 - Flags: review?(amarchesini) → review+
Regarding setting a filter in the ctor, I've added a to-do item for me to follow-up on once I'm back from PTO. I addressed the EqualLiteral and MOZ_CRASH comments for landing.
https://hg.mozilla.org/mozilla-central/rev/e603b8428731
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(In reply to Jonathan Watt [:jwatt] from comment #4)
> Richard, Daniel, this patch creates the necessary framework to implement
> filtering of the results of directory picking, but currently I'm only
> filtering out files/directories that are hidden or that begin with ".":

Nothing else comes to my mind.
 
> https://bugzilla.mozilla.org/attachment.cgi?id=8686038&action=diff#a/dom/
> filesystem/GetDirectoryListingTask.cpp_sec5

I would have preferred that this was done with a little less flexibility in whether the filters are applied.  I filed Bug 1235229 as a follow-up to discuss locking this down some more.
Flags: needinfo?(rlb)
Blocks: 1266531
Please reconsider the filtering of dotfiles which is currently implemented. I was quite surprised that Firefox thinks that a .git folder is sensitive information when uploading a project to my own server. Please don't draw conclusions based on just the filename.

While some dotfiles can contain sensitive data, most of them are of benign nature. If I upload a directory it's my own responsibilty to know what's in there!

Also see my comment over at https://bugzilla.mozilla.org/show_bug.cgi?id=907707#c32
Component: DOM → DOM: Core & HTML
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: