1274959 - Do we want to support link files in HTMLInputElement - filePicker/directory upload API/blink fileSystem API?

Status: RESOLVED FIXED

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

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I believe we do want to support them. We support them currently when picking files and IIRC other browsers support them too (at least on linux).

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I don't recall where and why we added the check to not support links in new file picker stuff, and that check has been then copied elsewhere.

Comment 3

[Jonathan Watt [:jwatt]]

You mean the filtering at:

https://mxr.mozilla.org/mozilla-central/source/dom/filesystem/GetDirectoryListingTask.cpp?rev=16663eb3dcfa#369

That's been there since I added the GetDirectoryListingTask task. See the part 2 patch in bug 1177688. There's no reason to exclude links other than the fact that I decided to start conservative and relax restrictions later. If we support links we would need to decide what we do when the links point outside the directory picked by the user, and we should definitely document our rational in code comments.

Comment 4

[Andrea Marchesini [:baku]]

The reason why I excluded them, is because with getFiles, recursively, we must be smart enough to avoid loops. I'll take this bug.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 1 - Directory.getFilesAndDirectories

I would like to write a test but it seems that our nsIFile doesn't support the creation of symlinks.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 2 - Unify GetFilesHelper and GetFilesTaskParent

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 3 - no loops with symlink in Directory.getFiles()

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8773719 [details] [diff] [review]
part 2 - Unify GetFilesHelper and GetFilesTaskParent

> ///////////////////////////////////////////////////////////////////////////////
> // GetFilesHelper Base class
> 
>+
> already_AddRefed<GetFilesHelper>
extra newline

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8773734 [details] [diff] [review]
part 3 - no loops with symlink in Directory.getFiles()

>+  if (!isLink) {
>+    nsAutoString path16;
>+    rv = aDir->GetPath(path16);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return rv;
>+    }
>+
>+    path = NS_ConvertUTF16toUTF8(path16);
>+  } else {
>+    rv = aDir->GetNativeTarget(path);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return rv;
>+    }
>+  }
>+
>+  if (NS_WARN_IF(!mExploredSymLinks.AppendElement(path, fallible))) {
So we append all directories to mExploredSymLinks, not only links. So change the name
mExploredSymLinks to mmExploredDirectories or some such?


>+  for (uint32_t i = 0; i < mExploredSymLinks.Length(); ++i) {
>+    if (mExploredSymLinks[i].Equals(targetPath)) {
>+      return false;
>+    }
>+  }
This is probably fine, though hashtable might be quite nice too. Up to you.
Hashtable would probably make this a bit simpler.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1e16d3bfb2
Support symlinks in Directory API - part 1 - Directory.getFilesAndDirectories, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2462f5e5e38
Support symlinks in Directory API - part 2 - Unify GetFilesHelper and GetFilesTaskParent, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e28c3a696524
Support symlinks in Directory API - part 3 - no loops with symlink in Directory.getFiles(), r=smaug

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f1e16d3bfb2
https://hg.mozilla.org/mozilla-central/rev/c2462f5e5e38
https://hg.mozilla.org/mozilla-central/rev/e28c3a696524