Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit allowed "accepted" extensions in File System Access API file pickers. #580

Closed
1 task done
mkruisselbrink opened this issue Dec 2, 2020 · 10 comments
Closed
1 task done
Assignees
Labels
Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review Review type: later review Venue: WICG

Comments

@mkruisselbrink
Copy link

HIQaH! QaH! TAG!

I'm requesting a TAG review of a small tweak to the File System Access API.

Initially the File System Access API (previously known as Native File System API) had no limitations on what strings were allowed to be used as accepted file extensions in the showOpenFilePicker and showSaveFilePicker methods.

Since the file picker (on most platforms) appends these extensions to the filename the user enters, this can result in filenames with characters we don’t want to allow/that are otherwise problematic. In particular we don't want to allow control characters or whitespace in suffixes, or filenames that end in a '.'. As such we add restrictions on what characters are allowed in accepts file extensions/suffixes, as well as limiting their length to 16.

Limiting extensions to only contain alphanumeric characters, + or . still allows all
extensions in the shared-mime-info database as well as nearly all extensions in Wikipedia's List of filename extensions.

Further details:

  • I have reviewed the TAG's API Design Principles
  • Relevant time constraints or deadlines: As this fixes potential security issues we will be shipping these changes as soon as possible. We will try to address any feedback that comes in afterwards.
  • The group where the work on this specification is currently being done: WICG
  • The group where standardization of this work is intended to be done (if current group is a community group or other incubation venue): WebAppsWG
  • Major unresolved issues with or opposition to this specification:
  • This work is being funded by: Google

You should also know that...

[please tell us anything you think is relevant to this review]

We'd prefer the TAG provide feedback as (please delete all but the desired option):

💬 leave review feedback as a comment in this issue and @-notify @mkruisselbrink

@annevk
Copy link
Member

annevk commented Dec 3, 2020

Should these restrictions be aligned with <input type=file accept>?

@mkruisselbrink
Copy link
Author

I don't see any reason why we couldn't align these restrictions, but it also matters less for <input type=file accept>. The security concerns are mostly about writing to files with dangerous extensions, so for security purposes I don't think we need to align them, but I don't see a reason why aligning would hurt either.

@cynthia
Copy link
Member

cynthia commented Jan 27, 2021

@kenchris and I looked at this today.

First of all, neither of us think we have ever seen a main.c++ file in the wild. Is that particular bit really needed, aside from this really weird case? (Unless this was a brown M&M, in which case well done!)

Otherwise, this looks like a sensible change.

@kenchris
Copy link

kenchris commented Jan 27, 2021

I think that restriction to ASCII + "." makes a lot of sense - thought you can actually use emoji and control chars and the like in file extensions on Windows

image

Does any OS actually associate .c++ with an IDE? I have never in my life as a C++ developer seen ".c++" extension and maybe we can avoid whitelisting "+"

@kenchris
Copy link

kenchris commented Jan 27, 2021

I don't think we should support "+" because then you could argue that we should also support "#" as some people and tools support for that C#

image

image

image

@kenchris
Copy link

There is of course also ".c--":

image

Maybe we should just add "+", "-" and "#". @cynthia found another example of # used in the wild

@kenchris
Copy link

kenchris commented Jan 27, 2021

And "$" and "!" :-)

image
image

Luckily I don't find anything else weird here:

https://en.wikipedia.org/wiki/List_of_filename_extensions_(A%E2%80%93E) (and other sections)

Almost forgot that ~ is also used in Linux for temporary files and also seen elsewhere:

image

So ["+", "-", "#", "$", "!", "~"] - I am fine with adding those, but just adding "+" and not the others seems strange

@plinss plinss added Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review and removed Progress: unreviewed labels Feb 14, 2021
@torgo
Copy link
Member

torgo commented Mar 23, 2021

Hi @mkruisselbrink any feedback you can share based on Ken's comments above?

@mkruisselbrink
Copy link
Author

Sorry for the slow reply. The current list of characters is indeed fairly arbitrary. I think I originally did include most of what you're asking about (going of the wikipedia list), but got some push back that without clear use cases it was better to be as minimal as possible (because who knows what characters might have special meaning on particular file systems/operating systems). I agree that + is a bit of an odd one out in that regard. I believe I mostly went by what was in the freedesktop.org shared-mime-info database, which does include .c++, but none of the other characters (so yes, most linux installations will treat .c++ files as the same mime type as .cpp and .cc, and thus have the same applications associated with them).

I'd be fine with adding the rest, although so far nobody has asked for them yet.

@kenchris
Copy link

Ok sounds good, we are good to close this then. Thanks for consulting the TAG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review Review type: later review Venue: WICG
Projects
None yet
Development

No branches or pull requests

6 participants