-
Notifications
You must be signed in to change notification settings - Fork 56
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
Proposal: downloads.getFileHash() #587
base: main
Are you sure you want to change the base?
Proposal: downloads.getFileHash() #587
Conversation
proposals/downloads_get_file_hash.md
Outdated
|
||
| Permission Added | Suggested Warning | | ||
| ------------------------- | ----------------- | | ||
| `"downloads.getFileHash"` | `"View cryptographic digests of downloads"` | |
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.
Open questions:
- do we want a permission warning? If not, no more questions. If yes, continue.
- is this the permission warning that we'd like to settle on? It is quite technical and I am not sure if the average user would know what they are consenting to.
- do we want this API permission for any API?
- do we want to enable extensions with host permissions to finalUrl to get the hash without additional permission warnings?
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.
These are great questions, let's discuss them separately:
- do we want a permission warning?
Yes, I believe every permission should have a corresponding permission "message", even if it not a "warning". I have an ambition to (eventually) make every permission optional and adding a toggle in browser-provided UI.
- is this the permission warning that we'd like to settle on?
Better proposals are welcome. I would like to add a "learn more" UI of some form eventually.
- do we want this API permission for any API?
- do we want to enable extensions with host permissions to
finalUrl
to get the hash without additional permission warnings?
Yes, but I intentionally left this permission relaxation out of this proposal and first implementation in Chromium. I would like to eventually move to host-based permission model. Specifically, if extension has a host permission for a site, it should be granted "tabs"
, "scripting"
, "downloads"
, "favicon"
, and many other permissions for that site specifically.
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.
Per WECG meeting minutes, we removed the permission warning and added requirement for host access to DownloadItem.finalURL
. Should we mark this as resolved?
cd709b6
to
ace49a9
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.
Looks good from my perspective. Before merging this PR also needs approval from at least Chrome and Safari.
proposals/downloads_get_file_hash.md
Outdated
``` idl | ||
namespace downloads { | ||
enum FILE_HASH_ALGO { | ||
"SHA-256" |
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.
Not blocking, but I wonder how this is exposed as the constant, since there is no obvious approach as seen at #563
- lowercase or upper case?
- what happens with the
-
?
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.
This is a good point and this proposal lacks clarity about enums. Here we actually have two hings to consider: the enum key and the enum value literal. This document currently does not specify the enum key explicitly. I copied enum value literal "SHA-256"
from Web Cryptography API §30.2 Registration.
What do you think about this?
namespace downloads {
enum FileHashAlgorithm {
SHA256 = "SHA-256"
}
}
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.
Thanks Anton! This generally looks good to me, but I want to run this by folk internally before approving to make sure this matches what we expected.
9f55418
to
2cfdc1b
Compare
proposals/downloads_get_file_hash.md
Outdated
| `"downloads.getFileHash"` | No warning | | ||
|
||
The new `"downloads.getFileHash"` permission is optional and is honored only if | ||
extension has `"downloads"` permission as well, and the extension has host |
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.
@oliverdunk The discussion reminds me that not all URLs have an origin. E.g. data:-URLs. What is your recommendation for these cases?
In the case of data:-URLs specifically, since the data is already fully revealing its content, I don't think that any origin permissions are needed there.
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.
Good point! It looks like we do expose the full URL already there so I don't have any strong feelings. One thing that came up in discussion about this proposal is that we want to run it by the downloads API owners (which is not the general extensions team) and privacy / security review. I'm going to drive that but suspect this may be something where they will have more thoughts.
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.
@oliverdunk Did you have a chance to discuss this internally?
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.
I confirmed with the downloads API owners last week that they are comfortable with this change. I haven't yet been able to go through privacy/security review though, which I'll do as soon as possible.
57c0ca9
to
591f58d
Compare
591f58d
to
ddf0ca8
Compare
@oliverdunk Chromium is listed as a supporting browser. Could you sign off on this PR to confirm that the proposal is in a good shape and that you agree to sponsor this proposal? After approval this can be merged (no need to squash because there is only one commit). |
I'd like to get through security and privacy review internally before approving this - I've started that process but haven't heard back yet. I'll update this PR as soon as I do. |
This formalizes #401 into a concrete proposal.