-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
feat: add webUtils module with getPathForFile method #38776
Conversation
9b10514
to
7ea03b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another todo: note deprecation of .path
in docs/breaking-changes.md
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API LGTM
The API name is fine to me, but I'm also good with other alternatives.
I agree that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 Any permission model we put in place would be self-governing because it would be the renderer guarding itself (which doesn't make sense) |
@MarshallOfSound does it make sense to expose this in sandboxed renderers as well? |
Per my comment above
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 👍 |
@MarshallOfSound what I mean is that the PR is not doing that, it's not listed in |
@MarshallOfSound in case it's added there, it should be listed in |
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 🤔 |
7ea03b7
to
b59cc61
Compare
Thanks for the reviews folks @dsanders11 @ckerr @itsananderson @zcbenz @miniak I've renamed the module to the suggested |
API LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes so that this doesn't get merged without tests.
a94e2c6
to
e063b12
Compare
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.
4e87064
to
6ff3dde
Compare
… replace-file-path
Both test failures are known flakes, merging |
Release Notes Persisted
|
* 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>
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:
blinkUtils
namespacegetPathForFile
method nameNotes: Added new
webUtils.getPathForFile
method to replaceFile.path
augmentation