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

Initial take on Security/Privacy self-review #150

Merged
merged 7 commits into from
May 14, 2020
Merged

Initial take on Security/Privacy self-review #150

merged 7 commits into from
May 14, 2020

Conversation

inexorabletash
Copy link
Member

Closes #149

I went through the S&P self review questionnaire and did my best to answer the questions.

@inexorabletash
Copy link
Member Author

inexorabletash commented Apr 29, 2020

Note that I was looking at the non-level2 spec. It may be necessary to revisit...

Yeah, the canShare() and use of TypeError for bad types reveals more information. Also, use of DataError was replaced by AbortError. I'll update.

@inexorabletash
Copy link
Member Author

Updated for level-2. I think we should probably add some things into the spec's privacy considerations section about how canShare() could be used for fingerprinting, and how user agents should mitigate that.

@ewilligers
Copy link
Collaborator

Many thanks for filling out the survey.

canShare() does not reveal if there are apps installed that support given file types, or even if shares of a file type would be blocked for security reasons. It is more of a feature detection API, for example it reveals if the user agent supports sharing files. It should less useful for fingerprinting than CSS.supports().

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional comments...

docs/security-privacy-self-review.md Outdated Show resolved Hide resolved
docs/security-privacy-self-review.md Outdated Show resolved Hide resolved
docs/security-privacy-self-review.md Outdated Show resolved Hide resolved
docs/security-privacy-self-review.md Outdated Show resolved Hide resolved
docs/security-privacy-self-review.md Outdated Show resolved Hide resolved
docs/security-privacy-self-review.md Show resolved Hide resolved
@marcoscaceres
Copy link
Member

Again noting that we should make a move to get rid of level 2 as per #107 (and have a single spec). We can avoid the whole l1/l2 confusion.

@mgiuca
Copy link
Collaborator

mgiuca commented Apr 30, 2020

Again noting that we should make a move to get rid of level 2 as per #107 (and have a single spec). We can avoid the whole l1/l2 confusion.

That's only feasible if we promote all of the L2 features into the spec. I don't want them taken out of the L2 spec and moved into PRs that linger for a long time while they are debated.

@marcoscaceres
Copy link
Member

I don't want them taken out of the L2 spec and moved into PRs that linger for a long time while they are debated.

Understood. But we should then make it more clear in the L2 document that what is only supported by a single engine (related WICG/admin#102 -- cc @hober, as this has come up again, and we are trying to figure out a solution).

@marcoscaceres
Copy link
Member

Should we move this into the spec itself?

inexorabletash and others added 3 commits April 30, 2020 10:31
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
@inexorabletash
Copy link
Member Author

Many thanks for filling out the survey.

No problem! It's a chore... you should see the I18N one though. Yikes.

canShare() does not reveal if there are apps installed that support given file types, or even if shares of a file type would be blocked for security reasons. It is more of a feature detection API, for example it reveals if the user agent supports sharing files.

This is pretty subtle and we should call it out explicitly both in the normative text near the definition, and in the Security and Privacy section. Implementations should be warned against making the test more precise for these reasons, and developers (if you have dev-focused text like domintro blocks) should be told that it's not that granular of a test.

@inexorabletash
Copy link
Member Author

Should we move this into the spec itself?

My guidance would be to (1) keep this as a separate doc in the repo, for quick reference during horizontal review, but (2) look for any juicy bits that aren't covered in the spec's existing S&P section and copy them in.

The questionnaire responses end up being repetitive so I don't think just copy/pasting the whole thing into the spec will result in something that's very readable.

@marcoscaceres
Copy link
Member

@inexorabletash sounds like a plan!

Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
@inexorabletash
Copy link
Member Author

So this doc now covers level-2, which is mostly a superset of level-1 (there's a difference in error handling w/r/t DataError in L1 vs. AbortError in L2).

Do we think that matters for the purposes of #149 ? Should there be two questionnaires? Should there be one but call out level-specific items? Apologies for barging in trying to help but not being cognizant of the nuances around the spec levels before starting...

@inexorabletash
Copy link
Member Author

I can't merge in this repo, so if everyone is happy with this can someone hit the button?

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.

Complete privacy/security self-review
4 participants