content icon indicating copy to clipboard operation
content copied to clipboard

Add a security section to APIs that require user activation

Open Elchi3 opened this issue 3 years ago • 14 comments

Description

Follow-up to https://github.com/mdn/content/pull/20358. It was proposed to add a note to all APIs that require user activation. I was considering to add a notecard banner to these pages but many pages already use too many banners and I don't want to add to that callout business on the pages [1]. So, I thought it might be wise to introduce a section called "Security" where we could mention all security related aspects of APIs. I did that in this PR. Let me know if that's okay.

[1] https://twitter.com/ddbeck/status/1509925868021420042

Motivation

I was motivated by this comment https://github.com/mdn/content/pull/20358#issuecomment-1238866543.

Related issues and pull requests

This also contributes to the request made at https://github.com/openwebdocs/project/issues/73.

Elchi3 avatar Sep 08 '22 15:09 Elchi3

Preview URLs

Flaws

Note! 22 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/API/PaymentRequest/show Title: PaymentRequest.show() Flaw count: 5

  • macros:
    • /en-US/docs/Web/API/PaymentDetailsModifier does not exist
    • /en-US/docs/Web/API/AddressErrors does not exist
    • /en-US/docs/Web/API/PaymentShippingOption does not exist
    • /en-US/docs/Web/API/PaymentRequest/retry does not exist
    • /en-US/docs/Web/API/PaymentRequest/complete does not exist

URL: /en-US/docs/Web/API/MediaDevices/getDisplayMedia Title: MediaDevices.getDisplayMedia() Flaw count: 2

  • broken_links:
    • Can't resolve /en-US/docs/Web/API/Media_Streams_API/Constraints
    • Can't resolve /en-US/docs/Web/API/Media_Streams_API

(this comment was updated 2022-09-21 12:23:05.461808)

github-actions[bot] avatar Sep 08 '22 15:09 github-actions[bot]

This looks good! I have two comments.

  • Should we update the template? I think so, and mentioning the two types of user activation (there will likely be others, like the ones used for the Web Audio API)
  • Also, is there a way to know about this constraint from the WebIDL or w3c/webref? (I'm trying to think how we could not forget one case in the future)

teoli2003 avatar Sep 08 '22 16:09 teoli2003

What do you think about using a macro/two macros for this? Yes I know, macros, but:

  • it's boilerplate text. Why not define it in one place?
  • macros are easier to find and replace if we ever manage to move to a more sensible system

Presumably the future here is that "user activation required" and "permissions" are bit of metadata associated with the API, that the page builder reads and inserts whatever note stuff we think is best to convey that.

I was considering to add a notecard banner to these pages but many pages already use too many banners and I don't want to add to that callout business on the pages [1].

I mean I'm with you on this, totally, but that's just a choice about how to style it and where to put it, not about how we represent it internaly.

Edited to add: or, why not use metadata now, and do it right from the start?

wbamberg avatar Sep 08 '22 16:09 wbamberg

Presumably the future here is that "user activation required" and "permissions" are bit of metadata associated with the API, that the page builder reads and inserts whatever note stuff we think is best to convey that.

Yes, I thought the same. I think the amount of APIs that use this metadata is growing but is it still low overall. I think security related metadata is:

  1. secure context (https) required?
  2. permission required?
  3. user activation required?

I think we use a macro for 1. and nothing for 2. and 3. because macros seem old school and deprecated. I'm happy to use a macro if it is okay to create a new one. (is it?)

Edited to add: or, why not use metadata now, and do it right from the start?

Does "use metadata now" mean to add a front-matter key, or something else? What is our metadata strategy? (it feels this PR is stumbling into some larger questions, which are worth asking, but it also feels like I just entered a rabbit hole.)

Elchi3 avatar Sep 08 '22 16:09 Elchi3

@wbamberg Where do you think we should store this metadata? YAML, mdn/data, mdn/browser-compat-data ?

teoli2003 avatar Sep 08 '22 16:09 teoli2003

@wbamberg Where do you think we should store this metadata? YAML, mdn/data, mdn/browser-compat-data ?

I think we should store it in YAML front matter (if we can't fetch it out of webref). But I think we would need to have a think about what all the front matter we would expect to need is, and how to represent it (e.g. how do you represent things like "needs-user-activation" which is like a flag).

wbamberg avatar Sep 08 '22 16:09 wbamberg

Does "use metadata now" mean to add a front-matter key, or something else? What is our metadata strategy? (it feels this PR is stumbling into some larger questions, which are worth asking, but it also feels like I just entered a rabbit hole.)

Sorry.

I think it is easier to get from: "macro" to "data" than to get from "prose" to "data". So using a macro now seems like a way to be able to implement this feature today without having to ask these difficult questions today, and without having to deal with so much soup if we ever do want to tackle these questions. But since I don't have good answers about metadata, I'm also happy for this to get merged as it is.

I think security related metadata is:

  1. secure context (https) required?

  2. permission required?

  3. user activation required?

I think we use a macro for 1. and nothing for 2. and 3.

I wonder if we could extend https://github.com/mdn/yari/blob/main/kumascript/macros/secureContext_header.ejs with some arguments, and use it for the other things. Then at least there wouldn't be a new banner/macro.

wbamberg avatar Sep 08 '22 17:09 wbamberg

I wonder if we could extend https://github.com/mdn/yari/blob/main/kumascript/macros/secureContext_header.ejs with some arguments, and use it for the other things. Then at least there wouldn't be a new banner/macro.

FWIW I'd prefer to go straight to yaml. Make them any requirement default as false if not declared.

security_requirements:
  secure_context: true
  user_permission: true
  user_activation: true

I would render them in the securecontext macro, potentially later do based on page type.

Why? My guess is that this is just as easy as working out how to get the macro to take various optional arguments, and should be fairly easy to extend. You could even render it more or less based on the list: "Following security requirements apply:".

P.S. This does not necessarily preclude the macro being required to be at a particular spot such as a "Security" section, in the same way that we have "Compatibility" etc. But that's a separate discussion.

hamishwillee avatar Sep 08 '22 23:09 hamishwillee

I wonder if we could extend https://github.com/mdn/yari/blob/main/kumascript/macros/secureContext_header.ejs with some arguments, and use it for the other things. Then at least there wouldn't be a new banner/macro.

FWIW I'd prefer to go straight to yaml. Make them any requirement default as false if not declared.

security_requirements:
  secure_context: true
  user_permission: true
  user_activation: true

Small points, but we use dashes, not underscores, and if they are just flags it would be cleaner just to say: if they are present the requirement applies:

security-requirements:
  - secure-context
  - user-activation
  - user-permission

But also I think permissions are names, not just booleans, so maybe, er:

security-requirements:
  flags:
    - secure-context
    - user-activation
  user-permission: clipboard-read

I would render them in the securecontext macro, potentially later do based on page type.

Yeah, this ties into page types inasmuch as it is only valid for certain page types (not to CSS properties, for instance).

Why? My guess is that this is just as easy as working out how to get the macro to take various optional arguments, and should be fairly easy to extend. You could even render it more or less based on the list: "Following security requirements apply:".

P.S. This does not necessarily preclude the macro being required to be at a particular spot such as a "Security" section, in the same way that we have "Compatibility" etc. But that's a separate discussion.

Yes, agreed. Just like with BCD, unless you want a banner at the top of the page you need a macro to indicate the spot (until we can build pages from templates).

One thing: in BCD we have the "## Browser compatibility" heading in content, and the macro/yari only supply the table. That works OK because just about all pages have BCD, but not so many have something to say about these requirements.

So in this case do we also have "## Security requirements" in content, and have the macro just say "None." in all the cases where there is nothing to say? Seems ugly. Or does the macro render the heading as well if it has something to say, and not otherwise?

And do we have the macro in all pages, which seems weird when it almost always does nothing, or do we only have it when there is something to say, which means authors have to update the page in 2 places (the data and the macro call).

wbamberg avatar Sep 09 '22 00:09 wbamberg

Yeah, why not:

security-requirements:
  flags:
    - secure-context
    - user-activation
  user-permission: 
    - clipboard-read
    - whatever-thingy

So in this case do we also have "## Security requirements" in content, and have the macro just say "None." in all the cases where there is nothing to say? Seems ugly. Or does the macro render the heading as well if it has something to say, and not otherwise?

I would say we only have the section if it is needed. We could choose to just leave it up the top of the page or we could say "must live above the compatibility, if it is defined". But this is a distraction. If we can get the metadata and macro agreed, the rest will be relatively easy.

hamishwillee avatar Sep 09 '22 01:09 hamishwillee

(I think good data modeling takes some time and thoughts. I'm unsure if it is a good idea to add-on to this PR to do it, but here we are.)

So, user activation can't be a flag either, I think. Most features are gated by "transient" user activation, but there is also "sticky" user activation for a few. Also, I'm not sure I'd introduce a grouping called "flags". It seems like unnecessary nesting to me.

I'd propose it like so:

security-requirements:
 - secure-context: true
 - user-activation: transient
 - permissions: 
   - clipboard-read

So, how do we proceed here?

  1. Keep the prose changes to add a "Security" section to the pages
  2. Add YAML (if we agree on how it looks like exactly).
  3. Merge this PR
  4. Do a follow up project to do something sensible with the new YAML.

Yes?

Elchi3 avatar Sep 09 '22 07:09 Elchi3

I like the direction this is taking!

So, user activation can't be a flag either, I think. Most features are gated by "transient" user activation, but there is also "sticky" user activation for a few.

Also we can discover more cases in the future (peeking at Web Audio API where contexts are not usable unless there has been some user interaction, and I don't know if it fits the two cases).

Maybe we can even do the following:

security-requirements:
 - secure-context
 - user-activation: transient
 - permissions: 
   - clipboard-read

(Avoiding the true)

And I'm 👍 for a security section above spec or compat sections, when needed.

teoli2003 avatar Sep 09 '22 10:09 teoli2003

(I think good data modeling takes some time and thoughts. I'm unsure if it is a good idea to add-on to this PR to do it, but here we are.)

Yes, it does, and I'm also unsure about this, which is why I suggested using a macro as a stepping stone.

So, how do we proceed here?

1. Keep the prose changes to add a "Security" section to the pages

2. Add YAML (if we agree on how it looks like exactly).

3. Merge this PR

4. Do a follow up project to do something sensible with the new YAML.

Yes?

I guess the thing is, once we've merged this PR, it's not so easy to find the places that had this prose added so we can update them. Because it's just prose. If it's a macro, that's a lot easier.

wbamberg avatar Sep 09 '22 21:09 wbamberg

If we add YAML for security data, we should use it and, therefore, add a macro (like {{Compat}} and {{Specifications}}). Once we are advanced enough with templates and page types, we probably won't need it anymore, but I agree that a new macro here would:

  • Allow us to find & replace easily once the templating system is in place.
  • Allow easy maintenance as scattered prose is a nightmare to maintain.
  • Don't add more complexity than what we have with {{Compat}} and {{Specifications}}.
  • Is a good stepping-stone towards our utopia.

teoli2003 avatar Sep 10 '22 06:09 teoli2003

I guess the thing is, once we've merged this PR, it's not so easy to find the places that had this prose added so we can update them.

User activation gated features are now listed here: https://developer.mozilla.org/en-US/docs/Web/Security/User_activation

I looked into developing this PR further, like adding a macro and adding more front-matter, but it feels really half baked. It seems that macros+front-matter should then also be added to all pages that talk about the HTTPS requirement or about needed permissions. I'm not ready to go down this route in this PR. I think it would be fine to accept the prose changes this PR does, but if it isn't, I'm leaning towards closing here and make a plan somewhere to implement the Ideas that came up here properly.

Elchi3 avatar Sep 22 '22 16:09 Elchi3