feat: add webUtils module with getPathForFile method #38776

MarshallOfSound · 2023-06-13T22:48:02Z

This is designed to replace the File.path augmentation we currently have in place to allow apps to get the filesystem path for a file that blink has a representation of.

File.path is non-standard and messes with certain websites, using a method like this is effectively 0-cost and removes one of the final deviations we have with web standards.

TODO:

Bikeshed of blinkUtils namespace
Bikeshed over getPathForFile method name
Tests

Notes: Added new webUtils.getPathForFile method to replace File.path augmentation

docs/api/blink-utils.md

ckerr

What's here so far LGTM.

Agree on the TODO for adding tests. I have no opinion on the naming but surely the API WG will want to bikeshed 😸

dsanders11

Another todo: note deprecation of .path in docs/breaking-changes.md

shell/renderer/api/electron_api_blink_utils.cc

dsanders11 · 2023-06-14T21:04:40Z

Bikeshed of blinkUtils namespace

I'm not in API WG but my 2 cents on the namespace would be not including blink since that feels a bit too internal/unnecessary detail for end API users.

Possible alternatives:

webUtils
rendererUtils

zcbenz

API LGTM

The API name is fine to me, but I'm also good with other alternatives.

shell/renderer/api/electron_api_blink_utils.cc

jkleinsc · 2023-06-15T14:45:55Z

Bikeshed of blinkUtils namespace

I'm not in API WG but my 2 cents on the namespace would be not including blink since that feels a bit too internal/unnecessary detail for end API users.

Possible alternatives:

webUtils

rendererUtils

I agree that blinkUtils is a bit esoteric. I think rendererUtils makes sense.

itsananderson

The old File.path augmentation always struck me as a weird privacy gap because AFAIK there's no way to prevent an "untrusted" renderer from accessing the full path of an uploaded file. If we're going to deprecate the old API, would it be worth considering the privacy implications of the new API, and perhaps having a way to limit its availability? Having a way to enable it in "preload" but disable it in the main world might be useful, for example.

docs/api/file-object.md

MarshallOfSound · 2023-06-23T08:54:09Z

If we're going to deprecate the old API, would it be worth considering the privacy implications of the new API, and perhaps having a way to limit its availability? Having a way to enable it in "preload" but disable it in the main world might be useful, for example.

At it's core the file path is available to the renderer process (this just returns a value that blink already had). Following our standard security assumptions this doesn't change the availability of that information to the process. All it does is improve web compatibility with things that don't expect File.path to be present and prevent zero-exploit exposure of that path.

Any permission model we put in place would be self-governing because it would be the renderer guarding itself (which doesn't make sense)

miniak · 2023-06-23T09:02:16Z

@MarshallOfSound does it make sense to expose this in sandboxed renderers as well?

MarshallOfSound · 2023-06-23T09:09:06Z

@MarshallOfSound does it make sense to expose this in sandboxed renderers as well?

Per my comment above

At it's core the file path is available to the renderer process (this just returns a value that blink already had).

This isn't exposing any new capabilities to the process, so I think exposing this information to the slightly-more-privileged isolated preload context is perfectly valid 👍

miniak · 2023-06-23T09:12:11Z

@MarshallOfSound what I mean is that the PR is not doing that, it's not listed in lib/sandboxed_renderer/api/module-list.ts. Even though the path won't be directly usable there due to the sandbox, it might still be useful?

miniak · 2023-06-23T09:13:36Z

@MarshallOfSound in case it's added there, it should be listed in docs/tutorial/sandbox.md as added by #38568

MarshallOfSound · 2023-06-23T09:15:09Z

Even though the path won't be directly usable there due to the sandbox, it might still be useful?

Yeah I think it will still be useful because you could take that path (albeit untrusted) and hand it off to the main process to do something with 🤔

MarshallOfSound · 2023-06-26T07:56:36Z

Thanks for the reviews folks @dsanders11 @ckerr @itsananderson @zcbenz @miniak

I've renamed the module to the suggested webUtils as it's a good home for interactions with "Web" APIs (File, Blob, Etc.). I think I've addressed all the other comments too 👍

miniak · 2023-06-26T12:37:52Z

API LGTM

jkleinsc

Requesting changes so that this doesn't get merged without tests.

This is designed to replace the File.path augmentation we currently have in place to allow apps to get the filesystem path for a file that blink has a representation of. File.path is non-standard and messes with certain websites, using a method like this is effectively 0-cost and removes one of the final deviations we have with web standards.

… replace-file-path

tests added

MarshallOfSound · 2023-11-20T23:59:31Z

Both test failures are known flakes, merging

release-clerk · 2023-11-20T23:59:39Z

Release Notes Persisted

Added new webUtils.getPathForFile method to replace File.path augmentation

* feat: add blinkUtils module with getPathForFile method This is designed to replace the File.path augmentation we currently have in place to allow apps to get the filesystem path for a file that blink has a representation of. File.path is non-standard and messes with certain websites, using a method like this is effectively 0-cost and removes one of the final deviations we have with web standards. * add error * refactor: update per PR feedback * chore: update patches * oops * chore: update patches * chore: update patches * feat: add blinkUtils module with getPathForFile method This is designed to replace the File.path augmentation we currently have in place to allow apps to get the filesystem path for a file that blink has a representation of. File.path is non-standard and messes with certain websites, using a method like this is effectively 0-cost and removes one of the final deviations we have with web standards. * add error * refactor: update per PR feedback * chore: update patches * oops * chore: update patches * chore: update patches * chore: update patches * fix: provide isolate to WebBlob::FromV8Value * chore: add tests * build: fix depshash mismatch on arm64 macOS --------- Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

MarshallOfSound requested a review from a team as a code owner June 13, 2023 22:48

electron-cation bot added the new-pr 🌱 PR opened recently label Jun 13, 2023

ckerr reviewed Jun 13, 2023

View reviewed changes

docs/api/blink-utils.md Show resolved Hide resolved

MarshallOfSound force-pushed the replace-file-path branch from 9b10514 to 7ea03b7 Compare June 13, 2023 23:46

MarshallOfSound added the no-backport label Jun 13, 2023

ckerr reviewed Jun 14, 2023

View reviewed changes

dsanders11 reviewed Jun 14, 2023

View reviewed changes

shell/renderer/api/electron_api_blink_utils.cc Outdated Show resolved Hide resolved

jkleinsc added the semver/minor backwards-compatible functionality label Jun 14, 2023

electron-cation bot added the api-review/requested 🗳 label Jun 14, 2023

zcbenz approved these changes Jun 14, 2023

View reviewed changes

shell/renderer/api/electron_api_blink_utils.cc Outdated Show resolved Hide resolved

MarshallOfSound mentioned this pull request Jun 16, 2023

[Bug]: Issues while attempting to upload files to a web app. #38745

Closed

3 tasks

itsananderson reviewed Jun 20, 2023

View reviewed changes

docs/api/file-object.md Outdated Show resolved Hide resolved

electron-cation bot removed the new-pr 🌱 PR opened recently label Jun 20, 2023

MarshallOfSound force-pushed the replace-file-path branch from 7ea03b7 to b59cc61 Compare June 23, 2023 09:19

miniak approved these changes Jun 26, 2023

View reviewed changes

electron-cation bot added api-review/approved ✅ and removed api-review/requested 🗳 labels Jun 26, 2023

jkleinsc changed the title ~~feat: add blinkUtils module with getPathForFile method~~ feat: add webUtils module with getPathForFile method Jun 26, 2023

jkleinsc previously requested changes Jun 26, 2023

View reviewed changes

electron-cation bot removed the api-review/approved ✅ label Jun 26, 2023

MarshallOfSound and others added 3 commits August 26, 2023 21:04

chore: update patches

8e7455b

oops

9e1e6cb

chore: update patches

e063b12

MarshallOfSound force-pushed the replace-file-path branch from a94e2c6 to e063b12 Compare August 28, 2023 18:46

MarshallOfSound and others added 9 commits September 1, 2023 00:25

Merge remote-tracking branch 'origin/main' into replace-file-path

c52aca0

chore: update patches

4e87064

add error

2032bf2

refactor: update per PR feedback

8100988

chore: update patches

30b7294

oops

75b2d89

chore: update patches

26790cf

chore: update patches

6ff3dde

MarshallOfSound force-pushed the replace-file-path branch from 4e87064 to 6ff3dde Compare September 6, 2023 18:27

MarshallOfSound added 7 commits September 6, 2023 16:31

Merge branch 'replace-file-path' of github.com:electron/electron into…

7d5cac1

… replace-file-path

Merge remote-tracking branch 'origin/main' into replace-file-path

0d85e7c

chore: update patches

cfd9ed5

fix: provide isolate to WebBlob::FromV8Value

55ccd5f

Merge remote-tracking branch 'origin/main' into replace-file-path

64c51ba

chore: add tests

0049ff8

build: fix depshash mismatch on arm64 macOS

7185791

electron-cation bot added api-review/approved ✅ and removed api-review/requested 🗳 labels Nov 20, 2023

MarshallOfSound merged commit d6bb9b4 into main Nov 20, 2023

MarshallOfSound deleted the replace-file-path branch November 20, 2023 23:59

nornagon mentioned this pull request May 6, 2024

feat: remove File.path #42053

Merged

5 tasks

feat: add webUtils module with getPathForFile method #38776

feat: add webUtils module with getPathForFile method #38776

Uh oh!

Conversation

MarshallOfSound commented Jun 13, 2023 • edited by jkleinsc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ckerr left a comment

Choose a reason for hiding this comment

Uh oh!

dsanders11 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dsanders11 commented Jun 14, 2023

Uh oh!

zcbenz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkleinsc commented Jun 15, 2023

Uh oh!

itsananderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MarshallOfSound commented Jun 23, 2023

Uh oh!

miniak commented Jun 23, 2023

Uh oh!

MarshallOfSound commented Jun 23, 2023

Uh oh!

miniak commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miniak commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarshallOfSound commented Jun 23, 2023

Uh oh!

MarshallOfSound commented Jun 26, 2023

Uh oh!

miniak commented Jun 26, 2023

Uh oh!

jkleinsc left a comment

Choose a reason for hiding this comment

Uh oh!

MarshallOfSound commented Nov 20, 2023

Uh oh!

release-clerk bot commented Nov 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

MarshallOfSound commented Jun 13, 2023 •

edited by jkleinsc

Loading

miniak commented Jun 23, 2023 •

edited

Loading

miniak commented Jun 23, 2023 •

edited

Loading