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

Moving local files with the File System Access API #805

Closed
1 task done
a-sully opened this issue Jan 17, 2023 · 17 comments
Closed
1 task done

Moving local files with the File System Access API #805

a-sully opened this issue Jan 17, 2023 · 17 comments
Assignees
Labels
Missing: Multi-stakeholder support Lack of multi-stakeholder support privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. Progress: review complete Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes Topic: Filesystem

Comments

@a-sully
Copy link

a-sully commented Jan 17, 2023

Wotcher TAG!

I'm requesting a TAG review of FileSystemFileHandle.move() for local files.

When launching SyncAccessHandles, we launched FileSystemFileHandle.move() for files within the Origin Private File System (OPFS). Moving of files outside of the OPFS and moving directories at all are not yet supported.

We're proposing to allow the FileSystemFileHandle.move() method to move files that do not live in the Origin Private File System, i.e. user-visible files on the device.

Further details:

  • I have reviewed the TAG's Web Platform Design Principles
  • The group where the work on this specification is currently being done: whatwg/fs and WICG/file-system-access. Initial work was started WICG, but many relevant parts to this proposal have been moved to whatwg. This specific feature involves local (non-OPFS) files, which are not specified in whatwg.
  • The group where standardization of this work is intended to be done: whatwg/fs
  • Major unresolved issues with or opposition to this specification:
  • This work is being funded by: Google

We'd prefer the TAG provide feedback as:

🐛 open issues in our GitHub repo for each point of feedback

@a-sully a-sully changed the title Moving non-OPFS files for the File System Access API Moving local files with the File System Access API Jan 17, 2023
@torgo torgo self-assigned this Jan 30, 2023
@torgo torgo added Topic: Filesystem privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. and removed Progress: untriaged labels Jan 30, 2023
@torgo torgo added this to the 2023-02-13-week milestone Feb 12, 2023
@torgo
Copy link
Member

torgo commented Mar 14, 2023

Noting: mozilla/standards-positions#732

@torgo
Copy link
Member

torgo commented Mar 14, 2023

Also: WebKit/standards-positions#28

@torgo
Copy link
Member

torgo commented Mar 14, 2023

Hi @a-sully – are there versions of the spec and explainer markdowns that can be directly linked to, rather than un-merged PRs? That would greatly help the review.

@torgo
Copy link
Member

torgo commented Mar 14, 2023

Hi @a-sully - thanks for this. We're reviewing today and noting that this is built on top of File System Access which we previously reviewed positively. Can we set up a session where y'all join us to discuss the security issues, multi-stakeholder interest, abuse cases, and potential mitigations?

@plinss
Copy link
Member

plinss commented Mar 15, 2023

I'm not sure if this is already covered, but there needs to be a limitation on how often this method can be called. Likely restricted to a single call per user activation.

Otherwise, a malicious site can use the fact that existing files can't be overwritten to probe for the existence of other files that the user has not granted access to.

@a-sully
Copy link
Author

a-sully commented Mar 16, 2023

Hi @a-sully – are there versions of the spec and explainer markdowns that can be directly linked to, rather than un-merged PRs? That would greatly help the review.

No, not at the moment. There had been some earlier discussions as to where these types of explainers should go but that thread was never resolved... I can attempt to merge them into whatwg/fs and cite this as justification :)

Hi @a-sully - thanks for this. We're reviewing today and noting that this is built on top of File System Access which we previously reviewed positively. Can we set up a session where y'all join us to discuss the security issues, multi-stakeholder interest, abuse cases, and potential mitigations?

Absolutely! (and sorry, I realized I had accidentally been filtering emails related to this repo. I should be more responsive going forward)

I'm not sure if this is already covered, but there needs to be a limitation on how often this method can be called. Likely restricted to a single call per user activation.

Otherwise, a malicious site can use the fact that existing files can't be overwritten to probe for the existence of other files that the user has not granted access to.

Correct! This is discussed in the explainer; both on requiring user activation if you don't have permission and overwriting existing files.

We expect this to be sufficient, but can add usage limitations later on if needed

@a-sully
Copy link
Author

a-sully commented Jun 30, 2023

Hi @torgo,

The move() spec PR has been blocked on several infrastructural changes and at this point is a bit out of date. I expect whatwg/fs#131 is the last remaining piece we need to finally be able to specify it properly, at which point I'll go back and update the spec PR

@a-sully
Copy link
Author

a-sully commented Jun 30, 2023

In the explainer it says "User agents are recommended to perform security checks on files moved within the local file system" but that isn't in the linked PR. And one issue we're concerned about is the strength of this "recommendation" and whether it's appropriate to the power of this API - especially when it comes to security.

Regarding this point specifically - we recent added to the spec the ability to distinguish between files on the local file system vs. files in a Bucket File System (f.k.a. OPFS). This now allows us to specify within the move() algorithm something like:

  1. If [=this=] [=FileSystemHandle/is in a bucket file system=], run [=implementation-defined=] malware checks

@torgo
Copy link
Member

torgo commented Jul 11, 2023

Hi @a-sully - can you clarify what do you mean by "distinguish"? Do you mean that the spec itself would mandate a higher security level for anything to do with local file system files vs. "bucket file system" files? Is there additional normative spec text that could be therefore added to protect users from bad outcomes (i.e. necessary security guarantees)?

@a-sully
Copy link
Author

a-sully commented Jul 12, 2023

can you clarify what do you mean by "distinguish"

We now have the ability to say "this FileSystemHandle corresponds to a file in a bucket file system" (as opposed to the local file system). See https://fs.spec.whatwg.org/#filesystemhandle-is-in-a-bucket-file-system.

I expected the move() method to be specified similarly to the existing specification for running malware checks when closing a FileSystemWritableFileStream:

Run implementation-defined malware scans and safe browsing checks. If these checks fail, reject closeResult with an "AbortError" DOMException and abort these steps.

"Implementation defined" is broad enough that each implementation could choose to skip malware checks for files in the bucket file system - which I expect is the case currently across all browsers. We could be explicit that these checks must not run for files in the bucket file system (as I sketched out above). That would remove some theoretical flexibility on the part of the user agent, but in practice I don't expect that to be an issue (cc @jesup @szewai @annevk)

Meanwhile, there's the question @torgo posed about running security checks for local files ("necessary security guarantees"). From my perspective (please correct me if I'm misunderstanding), there are two pieces to this:

  1. Can we specify these security checks in more detail, say like downloads? (i.e. can we do better than just "implementation-defined"?)
  2. Should we specify that security checks must be run when saving or moving files on the local file system?

I don't think we want (2). See the non-normative language here: https://wicg.github.io/file-system-access/#security-malware

user agents are encouraged to verify the contents of files modified by this API via malware scans and safe browsing checks, unless some kind of external strong trust relation already exists

The user agent should have the flexibility to skip security checks in some scenarios (e.g. the user agent can determine what qualifies as a "strong trust relation", how it's established, etc)

Regarding (1), I'm open to discussing adding more detail (and since this also affects the existing specification of FileSystemWritableFileStream, we may want to move this discussion to a new issue on the FS spec). But without (2) we can't guarantee any security behaviors

@a-sully
Copy link
Author

a-sully commented Jul 17, 2023

I've just revamped whatwg/fs#10 to make use of the various infrastructural changes I mentioned above and to include the restrictions discussed in the explainer for non-BucketFileSystem moves. Feel free to leave comments here or on that PR

A few things to note:

@torgo torgo added the Missing: Multi-stakeholder support Lack of multi-stakeholder support label Jul 18, 2023
@torgo
Copy link
Member

torgo commented Jul 18, 2023

Hi @a-sully thanks for that and thanks for listening to our comments and concerns. Our main concern continues to be security-related issues related allowing access to the local filesystem. In the case where different UAs have made different security choices about this capability, does the API allow for graceful fallback when the browser in question doesn't support file moves in the local file system? Is that detectable? Have you thought about how this would behave differently with isolated web apps #842 ?

@torgo torgo added the Progress: propose closing we think it should be closed but are waiting on some feedback or consensus label Jul 18, 2023
@torgo torgo modified the milestones: 2023-07-24-week, 2023-08-14-week Aug 3, 2023
@a-sully
Copy link
Author

a-sully commented Aug 22, 2023

does the API allow for graceful fallback when the browser in question doesn't support file moves in the local file system

On browsers for which only the Bucket File System is supported, well, there is currently no way to get a FileSystemHandle corresponding to the local file system at all. But if a browser wanted to support the local file system in a limited capacity - say, by allowing read-only access to files which are dragged-and-dropped or selected via a file picker - then the requirement of "readwrite" permission to (at least) the source file would result in the move() method reliably throwing a NotAllowedError exception

Is that detectable?

Were other browsers to implement (even partial) support for the local file system, that could include upstreaming the FileSystemHandle.queryPermission() method (and possibly also the FileSystemHandle.requestPermission() method) from the WICG spec to the whatwg spec. The website could then check queryPermission({ mode : "readwrite" }) before bothering to try to move() the file

Have you thought about how this would behave differently with isolated web apps #842 ?

The permission requirements mentioned above will also apply to IWAs. If the user agent wanted to somehow privilege IWAs - say, by allowing only IWAs to call move() - that would be facilitated by granting the appropriate File System permissions to the IWA. The means of handing out File System permissions for the local file system - to IWAs or otherwise - will presumably remain implementation-defined

@torgo
Copy link
Member

torgo commented Aug 29, 2023

Hi folks - thanks for your responses and thanks for addressing our concerns. We're happy with the proposal as it stands, however I think the main concern we have now relates to multi-stakeholder support for the file system access API itself. We encourage you to work with other stakeholders / implementers to achieve a consensus-based approach to this capability.

@torgo torgo closed this as completed Aug 29, 2023
@torgo torgo removed this from the 2023-08-28-week milestone Aug 29, 2023
@torgo torgo added Progress: review complete Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes and removed Progress: propose closing we think it should be closed but are waiting on some feedback or consensus labels Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing: Multi-stakeholder support Lack of multi-stakeholder support privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. Progress: review complete Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes Topic: Filesystem
Projects
None yet
Development

No branches or pull requests

5 participants