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

Proposal: downloads.getFileHash() #587

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bershanskiy
Copy link
Member

This formalizes #401 into a concrete proposal.

proposals/downloads_get_file_hash.md Outdated Show resolved Hide resolved
proposals/downloads_get_file_hash.md Show resolved Hide resolved
proposals/downloads_get_file_hash.md Show resolved Hide resolved

| Permission Added | Suggested Warning |
| ------------------------- | ----------------- |
| `"downloads.getFileHash"` | `"View cryptographic digests of downloads"` |
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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?

@bershanskiy bershanskiy force-pushed the proposals-downloads-get-file-hash branch 2 times, most recently from cd709b6 to ace49a9 Compare April 18, 2024 08:59
@bershanskiy bershanskiy requested a review from Rob--W April 19, 2024 06:02
@bershanskiy bershanskiy self-assigned this Apr 19, 2024
Copy link
Member

@Rob--W Rob--W left a 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.

``` idl
namespace downloads {
enum FILE_HASH_ALGO {
"SHA-256"
Copy link
Member

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 -?

Copy link
Member Author

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"
    }
}

Copy link
Member

@oliverdunk oliverdunk left a 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.

proposals/downloads_get_file_hash.md Outdated Show resolved Hide resolved
proposals/downloads_get_file_hash.md Outdated Show resolved Hide resolved
@bershanskiy bershanskiy force-pushed the proposals-downloads-get-file-hash branch 2 times, most recently from 9f55418 to 2cfdc1b Compare April 25, 2024 13:35
| `"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
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

@bershanskiy bershanskiy force-pushed the proposals-downloads-get-file-hash branch 2 times, most recently from 57c0ca9 to 591f58d Compare April 26, 2024 19:18
@bershanskiy
Copy link
Member Author

I rebased the PR here and incorporated the feedback here.

proposals/downloads_get_file_hash.md Outdated Show resolved Hide resolved
proposals/downloads_get_file_hash.md Outdated Show resolved Hide resolved
proposals/downloads_get_file_hash.md Outdated Show resolved Hide resolved
@bershanskiy bershanskiy force-pushed the proposals-downloads-get-file-hash branch from 591f58d to ddf0ca8 Compare May 9, 2024 17:01
@Rob--W
Copy link
Member

Rob--W commented May 9, 2024

@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).

@oliverdunk
Copy link
Member

@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?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants