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

Remove support for native file system API #11407

Closed
pes10k opened this issue Aug 24, 2020 · 33 comments · Fixed by brave/brave-core#6933
Closed

Remove support for native file system API #11407

pes10k opened this issue Aug 24, 2020 · 33 comments · Fixed by brave/brave-core#6933
Assignees
Labels
OS/Android Fixes related to Android browser functionality OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. privacy/chromium-redqueen Work to remove privacy-harming "features" added in Chromium. privacy QA/No release-notes/exclude security

Comments

@pes10k
Copy link
Contributor

pes10k commented Aug 24, 2020

Chromium is shipping a native file system API. The privacy / security model is extemely not fleshed out, but its shipping enabled in chromium. We should disable

cc @jumde

Spec: https://github.com/WICG/native-file-system/blob/master/EXPLAINER.md

@pes10k pes10k added security privacy OS/Android Fixes related to Android browser functionality OS/Desktop labels Aug 24, 2020
@jumde jumde self-assigned this Aug 24, 2020
@pes10k pes10k added priority/P2 A bad problem. We might uplift this to the next planned release. privacy/chromium-redqueen Work to remove privacy-harming "features" added in Chromium. labels Sep 1, 2020
@pes10k pes10k assigned pes10k and mkarolin and unassigned pes10k and jumde Sep 15, 2020
@rebron rebron added this to P1 & P2 Backlog in General Oct 14, 2020
mkarolin added a commit to brave/brave-core that referenced this issue Nov 25, 2020
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
mkarolin added a commit to brave/brave-core that referenced this issue Nov 25, 2020
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
mkarolin added a commit to brave/brave-core that referenced this issue Nov 30, 2020
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
mkarolin added a commit to brave/brave-core that referenced this issue Dec 12, 2020
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
mkarolin added a commit to brave/brave-core that referenced this issue Dec 12, 2020
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
mkarolin added a commit to brave/brave-core that referenced this issue Dec 12, 2020
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
mkarolin added a commit to brave/brave-core that referenced this issue Dec 14, 2020
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
mkarolin added a commit to brave/brave-core that referenced this issue Jan 7, 2021
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
mkarolin added a commit to brave/brave-core that referenced this issue Jan 8, 2021
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
mkarolin added a commit to brave/brave-core that referenced this issue Jan 8, 2021
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
@rebron rebron moved this from P1 & P2 Backlog to Pending review in General Jan 21, 2021
mkarolin added a commit to brave/brave-core that referenced this issue Jan 25, 2021
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
mkarolin added a commit to brave/brave-core that referenced this issue Jan 27, 2021
Fixes brave/brave-browser#11133
Fixes brave/brave-browser#11407
Fixes brave/brave-browser#11546
Fixes brave/brave-browser#11547

- Disables features via Chromium features:
  * Direct Sockets
  * Lang Client Hint Header
  * Signed Exchange Prefetch Cache For Navigations
  * Subresource Web Bundles

- Disabled the following blink features in
  BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization
  since they don't have Chromium features:
  * Digital Goods
  * Native File System (File System Access)
- Added browser tests for these features

- Modified redirect-cc.py to allow overriding files under out/XXX/gen

- Added an override for blink::origin_trials::IsTrialValid and overrides
  for blink::OriginTrialContext methods AddFeature and AddForceEnabledTrials
  for trials:
  * NativeFileSystem2
  * SignedExchangeSubresourcePrefetch
  * SubresourceWebBundles
- Added browser tests for trials disablement.
General automation moved this from Pending review to Completed Jan 28, 2021
@mkarolin mkarolin added this to the 1.21.x - Nightly milestone Jan 28, 2021
@mkarolin mkarolin added the QA/No label Jan 28, 2021
@rebron rebron removed this from Completed in General Mar 6, 2021
@dwelle
Copy link

dwelle commented Mar 26, 2021

The concern on our end (or, at least on my end, not giving a "official Brave position" or anything) is that the kinds of "blurring between websites and apps" that PWA do is dangerous to users

@pes10k can you please provide reasons as for why this API is considered a security risk? A vague statement like the above doesn't help.

If you have specific and actionable security concerns, I'd advise to voice them in the spec repo so they can be addressed.

@pes10k
Copy link
Contributor Author

pes10k commented Mar 26, 2021

@dwelle the concern is just that this is an extremely powerful but risky feature, and so a potential footgun. High risk features carry the cost of requiring users to be further responsible for their own privacy, and so we're cautious about including or enabling such features. For many, the constrained, sandboxed nature of the web is part of the appeal.

Brave doesn't have a strict policy about this, we do our best to make a case by case determination when weighing the benefits and costs of enabling.

@tomayac
Copy link

tomayac commented Mar 29, 2021

I just went to https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/webkitdirectory#result, which has the following innocent-looking code on it:

<input type="file" id="filepicker" name="fileList" webkitdirectory multiple />

Brave (and all other browsers really) will happily let the website know about every. single. file. I have ever downloaded since purging my Downloads folder (which, I reckon, it's time to do again):

Screen Shot 2021-03-29 at 13 56 25

The File System Access API has some built-in constraints that make this sort of attacks less likely.

@pes10k
Copy link
Contributor Author

pes10k commented Mar 29, 2021

@tomayac That seems like a nasty aspect of the HTML spec, agreed, but I don't follow the connection between the above and the Native File System API. They pose different risks; being able to overwrite or modify the users existing files seems like an important new privacy harm

@tomayac
Copy link

tomayac commented Mar 29, 2021

@tomayac That seems like a nasty aspect of the HTML spec, agreed

It's definitely an API design that shows its age, we both agree.

I don't follow the connection between the above and the Native File System API.

The File System Access API (it was renamed) prevents users from from uploading their whole Downloads folder (screenshot via using this demo):

Screen Shot 2021-03-29 at 16 50 41

They pose different risks; being able to overwrite or modify the users existing files seems like an important new privacy harm!

That's absolutely right, saving files to arbitrary locations is a different threat, but the built-in constraints make it impossible that someone overwrites important system files. The browser will also prompt before (over-)writing an existing file in the user-accessible part of the file system.

@dwelle
Copy link

dwelle commented Mar 29, 2021

I would add that not only is it opt-in (under a confirmation dialog) as @tomayac has pointed out, but it's also not a privacy risk, it's a data-loss risk. One that is inherent to doing anything meaningful with the file system, and it doesn't matter whether that file system resides on user's own drive, or in the cloud.

If anything, by blocking your users to use local file system, you're pushing them to host their files remotely, which then does have a privacy risk.

For many, the constrained, sandboxed nature of the web is part of the appeal.

I've not heard a sentiment saying that being unable to allow web apps to be more integrated with the OS is useful. On the contrary, our users at @excalidraw want and benefit from such integration. If you're giving users such agency so as to have opinions on the sandboxed nature of the web, you should give them agency to make their own decisions on whether they want to grant FS access to apps they're using.

@pes10k
Copy link
Contributor Author

pes10k commented Mar 29, 2021

I hear what you all are saying, but allowing a page to maintain ongoing read and / or write handles to files on disk (even keeping in mind the files ruled out by the built-in constraints) is a category of privacy and data-loss risk that the platform currently doesn't allow.

For example, currently, if I share a bunch of files with a site doing what @tomayac suggests in #11407 (comment), the site isn't going to learn about changes i make to those files until I go through another browser prompt; i can edit / modify / etc them as I please knowing the site wont see those changes.

Under this proposal, a site will (or at least, can) see every change I make until I revoke permissions (if im reading the spec right, this is true even if I come back to the site in the future). This is an obvious, large difference in privacy risk!

the built-in constraints make it impossible that someone overwrites important system files

The spec does not do this. The spec suggests to browser vendors that browser vendors determine which files are sensitive, and provides a list of possible suggestions. But the spec just doesn't itself define any files as off limits or sensitive.

This is a common, maddening anti-pattern in specs, where specs both want to reserve to vendors the ability to apply arbitrary policies, but also claim that they spec is private/secure/etc because of this undefined behavior.

@pes10k
Copy link
Contributor Author

pes10k commented Mar 29, 2021

Anyway, we may revisit our decision, but while the proposal is still being worked out, and for the time being, we think the feature is on the wrong side of the cost/benefit line for most of our users.

I can say what we are considering more seriously, but haven't made a decision on, is whether to ship the proposal default off, and allow folks to enable the proposal through an experts-only flag. No decisions have been made there yet, but we're considering that as a general approach to non-standard (yet?) features implemented upstream

@pes10k
Copy link
Contributor Author

pes10k commented Mar 30, 2021

Will also say, to take the temp down a bit from my previous message, that if Im misreading or misunderstanding the spec from the above discussion, I'd be very grateful for corrections! Because these not-yet-standardized features often change (both the text and in implementation), we tend to take a more conservative approach to these pre-standardized features.

And, last, just wanted to say that even if we don't currently have the outcome / decision ya'll prefer, I appreciate that thats frustrating, but that I appreciate everyone in the thread taking the time to voice their opinions and force us to revisit and reconsider our decisions.

@tomayac
Copy link

tomayac commented Apr 13, 2021

We're seeing steady growth of the feature in Chrome.

Applications from different genres start using the API:

For the permissions point: the spec leaves UAs a lot of choice, for example, UAs could implement a directory picker in such a way that the user would pick a directory, but then one-by-one green-light the actual files in a secondary, UA-specific picker. In the extreme case, UAs could even ask before every single read or write operation as to making it super transparent to the user when disk access happens.

@okdistribute
Copy link

okdistribute commented Apr 14, 2021

This is unfortunate as this causes many devs to tell their users to install Chrome which has even more privacy issues. We're planning a PWA now and I was hoping to be able to tell users to use Brave but it seems like we might have to encourage them to use Chrome instead for full functionality. A shame, really.

@pes10k
Copy link
Contributor Author

pes10k commented Apr 14, 2021

I understand your frustration but the ability to set permissions that allow for long lived disk access, even when revisiting a site is a serious privacy harm, and an enormous footgun we're not going to load and hand to our users.

I appreciate @tomayac point, that the spec is sufficiently vague in privacy protections that one could maybe implement it in such away that it was privacy protecting. But vagueness in privacy protections in a spec is not a feature, its a bug and a sign of a proposal that isn't solving the privacy risks it introduces.

I'll mention again what i said in #11407 (comment), that we're considering having these default off, and expert enable-able through flags, but we don't have any immediate plans to implement such a policy.

TL;DR; this is a spec with serious privacy risks, and protecting our users from those risks is higher priority right now than either 1) dedicating developer time to figure out how to implement this not-yet-standard in a privacy preserving way, or 2) supporting these use cases that we don't think are core to why folks want to use Brave.

As always, decision is subject to change at any point, but #11407 (comment) is the current position. If sites need to use privacy-risking features, thats (at least for now) what other browsers are for ;)

@okdistribute
Copy link

okdistribute commented Apr 14, 2021

OK! Well we are looking forward to the day we can have more full-featured local-first applications in Brave! I will be following this thread in case things change and we can start recommending brave instead of chrome for this work.

@samueljim
Copy link

@pes10k
I was under the impression that the file system API only has access to the file system while the domain is open.
In Chrome, you need to give the site access to the file system every time you open the webapp. The file system access is also sandboxed to the folder that the user requests.

This might be different for PWA apps.

@pes10k
Copy link
Contributor Author

pes10k commented Apr 15, 2021

@samueljim, I think I explained my concerns poorly. On my initial read through the spec, I have two concerns:

  1. that while the page has a FileSystemHandle, the page can continue watching and reading updates to that file; this seems to be an intended use for the API. This is not my main concern, but its a concern none the less, as this is a completely different way than how pages can deal with files currently (again, by design). Basically, right now there is a high level of intentionality in telling a page "please read or upload the contents of this file", every time. This changes that model significantly, in a way that i expect most non-expert users will not understand, and create two very similar ways of doing similar things with very different privacy implications in the browser. The risk for misuse and error is high to my mind.

  2. More significantly, if I am reading the spec right, pages have access to the file handles they had the last time the page was open, and w/o having to re-request permission from the user (since the permission lives across page visits). This seems possible through the API either by serializing, storing, and then later unserializing the handle, and also possibly just by the general behavior of getFile (its not 100% clear to me whether the spec intends the permission to be tied to the JS object, or to the underlying file. If the former, this is surprising to me, but would reduce some at least some of my concerns).

And then, finally, I'm very nervous about APIs that are not (yet?) standards (and so may change out from underneath us at any time) that do very privacy sensitive things. Thats not a controlling concern, but its a reason we're extremely wary about enabling non-standard, under development features that are privacy risky.

@tomayac
Copy link

tomayac commented Apr 15, 2021

  1. More significantly, if I am reading the spec right, pages have access to the file handles they had the last time the page was open, and w/o having to re-request permission from the user (since the permission lives across page visits). This seems possible through the API either by serializing, storing, and then later unserializing the handle, and also possibly just by the general behavior of getFile (its not 100% clear to me whether the spec intends the permission to be tied to the JS object, or to the underlying file. If the former, this is surprising to me, but would reduce some at least some of my concerns).

Maybe @mkruisselbrink as the spec author can speak to this.

@HKalbasi
Copy link

Valid concerns on non-standard api. I hope it will soon become standard with privacy/security issues solved. Until that day, I don't see anything wrong with flag. We shouldn't choose what is correct, people should. Concerns can be explained in flags page.

@Dmitra
Copy link

Dmitra commented Apr 24, 2021

I might be missing smth, but what flag can I use to enable this feature?

@ndarilek
Copy link

Wondering if there's a flag as well? I remember this being supported previously, so was surprised when it didn't work. Tried enabling a few promising-looking flags, but none seemed to work.

Thanks.

@kepta
Copy link

kepta commented May 30, 2021

More significantly, if I am reading the spec right, pages have access to the file handles they had the last time the page was open, and w/o having to re-request permission from the user (since the permission lives across page visits).

@pes10k this is incorrect, pages can store a file handle but if a page is closed (including all the tabs of that subdomain), browser prevents access to the handle and shows a user a prompt to regrant the permission.

Here is an example from my note taking app https://bangle.io , which shows explicitly the prompt message for the first time a user selects a directory:
image

And once you close the website and visit again, the browser shows this prompt:
image

This feature is not a dangerous feature and should not be dismissed just because the highlight of this feature is the ability to read users files. As I mentioned above there are proper safe guards in the spec which give the power to the user to decide whether they trust the website or not and also prevent accessing sensitive directories.

I kindly request you to rethink this, as it a monumental step in giving power to the web apps and enabling things that weren't possible before. With this said, I am curious to hear what your thought process is like for enabling this feature for brave users.

@pes10k
Copy link
Contributor Author

pes10k commented Jun 1, 2021

Thank you for the notes @kepta !

Folks can find more details for how we're considering dealing with non-standardized, privacy-risking, still changing Chrome features here: #9586 (comment).

But the TL;DR; here is:

  1. the spec is still changing (even the name!), so we're cautious about enabling non-standard features
  2. the idea of "the page can read and write to this file, even after i close the window" is a new, unique, privacy boundary on Web. I appreciate that you all think your users can handle it just fine, and thats great and cool! But Brave needs to make choices that reflect millions of users, including those that encounter malicious uses too. Consider, for example, the risk of a site serializing a file handle into storage, and then a 3p script reading the handle out of storage and accessing disk, just to name one unique risk this feature.
  3. as mentioned in [Desktop] Web bluetooth fails on Linux #9586 (comment), we're considering moving from our current policy towards non-standard, privacy-risky features (disable w/ no option to enable) to a new policy of "disable by default, but allow advanced users to flip a flag," but we dont have anything further to announce at the moment
@ibsusu
Copy link

ibsusu commented Jun 15, 2021

@pes10k, I'm just a nobody but I really think that while you and the rest of your team handle Brave's adoption of Chrome features amazingly well, disabling the file system access api is overly protective. The guards in place are enough and the power and consistency that it affords developers as a superior alternative to indexedDB and localStorage are easily worth it, even on the balance of 'millions of users'.

Your second point is really weak and the first and third are just the reasonable amount of bias I expect anyone to have with the Chrome team.

You can serialize a handle into storage but how is the 3rd party script going to read it? IndexedDB? LocalStorage? If you're really worried about other attack avenues just lock it down to one directory that you specify.

Please prioritize researching the api more thoroughly because right now it looks like your conclusion isn't well supported. I don't know what else you have on your plate but this one shouldn't take long.

@kepta
Copy link

kepta commented Jun 15, 2021

To be blatantly honest, it looks like a decision was made first and then you all decided to look for ways to support that decision.

Consider, for example, the risk of a site serializing a file handle into storage, and then a 3p script reading the handle out of storage and accessing disk, just to name one unique risk this feature.

the spec is still changing (even the name!), so we're cautious about enabling non-standard features

The spec is pretty spec-d out, as far as reading and writing files is considered with millions of chrome devices out in the wild using it.

the idea of "the page can read and write to this file, even after i close the window" is a new, unique, privacy boundary on Web. Brave needs to make choices that reflect millions of users, including those that encounter malicious uses too.

There is a fine line between thinking that you are protecting your users and actually protecting your users. Limiting the advancement of the web ecosystem on the pretense of security, "oh that poor user doesn't know what they are doing" and "oh this sounds privacy invasive and is coming from google, lets block it without proper research".

This API by design is 'opt in' --- the user has to take an action (clicking 'yes' on a prompt) to even make a malicious attack feasible in the first place.

, we're considering moving from our current policy towards non-standard, privacy-risky features (disable w/ no option to enable) to a new policy of "disable by default,

I would really be interested in hearing how you think this is privacy risky.

@pes10k
Copy link
Contributor Author

pes10k commented Jun 16, 2021

I'm locking this conversation.

Brave issues are for keeping track of bugs and things Brave plans on implementing, not to debate Brave product decisions. I've already shared Brave's plans for features like this. As mentioned before, this plan does not currently have an implementation timeline, but when implemented would allow users to enable the feature through brave://flags.

Further, the conversation has drifted from the substance of the issue, to presuming motives or assuming ignorance, which is definitely not what these issues are for.

Ya'll, I appreciate we disagree. Thats okay and inevitable given the size of the Web. But, without any snark or sarcasm meant, it shouldn't be a surprise to anyone that Brave considers some things privacy risks that other browsers (including Chrome) don't consider privacy risks. A partial list of Brave's concerns (already discussed above) include:

  • that allowing sites to (for the life time of any tab for the site being open) read and write files to disk is a powerful and privacy-affecting capability with ramifications beyond what many Web users will appreciate (e.g., I expect that giving text-editor.com access to my "Documents" folder might seem like an obvious no-brainer to folks who want to use text-editor.com, but holy heck think of the implications, etc)
  • XSS and similar style attacks have new categories of ways to be harmful. This is the point made above about a 3p script (or any other script) being able to access the handles too (out of storage or anywhere else).
  • this is not a standard, or even standards track, increasing the chance that the API / privacy boundaries / etc could change out from beneath us. Non-standard features are more likely to change significantly between Chromium versions than features defined by Web standards. This isn't a fatal concern, but its why were extra cautious around features still in "incubation" (i.e., the "I" in WICG). Its also why its cynical and incorrect to dismiss the concern as "oh this… is coming from google, lets block it…",

Next, even though I expect they don't like or approve of the decision here, I'll just say again that I appreciate the testing and additional information provided by @kepta in #11407 (comment).

Finally, a correction:

You can serialize a handle into storage but how is the 3rd party script going to read it?

I think there is a misunderstanding here. 3p scripts can read the storage of the frame they're executing in. If a 1p script writes something into storage (say… a file handle), nothing prevents a 3p script (or any other script), executing in the same frame, from reading that value out of storage.

The "cross origin" limitation mentioned above prevents a script (3p or otherwise) from accessing storage for a eTLD+1 other than the eTLD+1 of the frame the script is being executed in. With storage, as with most other parts of the Web platform, access is scoped to the eTLD+1 of the frame code is executing in, not the eTLD+1 that the code came from.

@brave brave locked and limited conversation to collaborators Jun 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
OS/Android Fixes related to Android browser functionality OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. privacy/chromium-redqueen Work to remove privacy-harming "features" added in Chromium. privacy QA/No release-notes/exclude security