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

Feature Policy syntax: Structured Header? #376

Closed
clelland opened this issue May 11, 2020 · 8 comments · Fixed by #383
Closed

Feature Policy syntax: Structured Header? #376

clelland opened this issue May 11, 2020 · 8 comments · Fixed by #383

Comments

@clelland
Copy link
Collaborator

If we rename the header, then we have a chance to change the syntax, as existing sites would need to make changes anyway.

A concrete proposal here would be a dictionary, mapping features to lists of origins / keywords:

Something-Policy: feature=(self "https://example.com"),
                  another-feature=(),
                  grant-it-to-everyone=(*)

Similar to ES6 arrow function syntax, we could choose to drop the parentheses if there is exactly one item in the list, for developer convenience. (Essentially allowing a single Item to take the place of an Inner List, in SH terms)

If we do this, what should we do with the existing allow attribute? I'd prefer to keep the existing name, and it's not clear that SH-syntax works well in this case -- the double-quotes around origins could be problematic, and there's no clear backwards-compatibility story. The ability to just use <iframe allow="feature; another-feature; a-third-feature"> is a very useful (and widespread) shorthand, but there's no way to make that SH-compatible.

One option is to keep the existing syntax for the attribute, and define it as an alternate serialization of a policy declaration for use in HTML attributes, while the SH serialization is used for headers. I'm not sure if there's precedent for that.

It could also be that the benefits of having exactly one serialization outweigh all of my concerns, and we should just change everything all at once. I'm open to any and all suggestions here.

@annevk
Copy link
Member

annevk commented May 13, 2020

I was thinking that we keep the allow attribute as-is as it has shipped in at least two implementations (not sure about Safari). It's unfortunate that it has a unique parser, but at least that parser would be bound to the content process.

@clelland
Copy link
Collaborator Author

I think I'm happiest with that outcome, too. It's the least disruptive to developers.

Safari (WebKit at least, not sure about the shipping status) also supports the current allow syntax, so it's been implemented in three engines.

@arturjanc
Copy link

My opinion carries little weight here, but I'd be happy if the new header followed the Structured Header format.

No opinions re: the allow attribute, though it does seem like having a different parser there could be a bit counterintuitive. Would it make sense to consider a different attribute (permissions?) with a syntax that matches SH, or is that even less understandable?

@annevk
Copy link
Member

annevk commented May 19, 2020

It might be interesting to consider if there's a reasonable path to full migration, but as I understand it the allow attribute is already quite widely deployed. The next time we invent a complicated attribute for HTML we should strongly consider something like that though. I don't think it's acceptable to add another custom syntax at this point without a longer term plan.

cc @domenic

@johannhof
Copy link
Member

Yeah, looking at bug reports this seems to be used far too much to significantly change it now and a new attribute sounds even messier. I wouldn't say the syntax of allow is too bad to keep, even if it's a separate parser.

@clelland
Copy link
Collaborator Author

Agreed; I'd prefer to leave allow as-is.

Any thoughts on the header syntax? A straightforward dictionary mapping would be as proposed above:

Permissions-Policy: feature=(self "https://example.com"),
                    another-feature=(),
                    grant-it-to-everyone=(*)

I feel like allowing a single origin/keyword to exist outside of an inner list looks nicer, at the cost of a bit of parsing complexity (probably not a significant amount, given an existing structured header parser):

Permissions-Policy: feature=self,
                    another-feature=(),
                    grant-it-to-everyone=*,

An empty list in that case still needs the (), like an ES6 arrow function.

@annevk
Copy link
Member

annevk commented May 26, 2020

It wouldn't complicate the parser, right? It would complicate the logic on top of the parser result. Allowing that seems reasonable to me. (I don't think we should go as far as allowing the value to be omitted though as that seems like abuse of the boolean type.)

clelland added a commit that referenced this issue May 27, 2020
@clelland
Copy link
Collaborator Author

It wouldn't complicate the parser, right? It would complicate the logic on top of the parser result.

Yes, I meant the logic layered on top of the structured header parser.

(I don't think we should go as far as allowing the value to be omitted though as that seems like abuse of the boolean type.)

Agreed. We wouldn't say feature=?1, so abusing the shorthand for that seems wrong.

blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Jun 17, 2020
This CL splits the parsing logic and semantic logic in
|FeaturePolicyParser| so that parsing logic can be easily
substituted.

This is pre-work to implement permission policy parser which
uses structured header format. (w3c/webappsec-permissions-policy#376)

Change-Id: I116e69a562974d21e0d3a53b15cfbb3276b37d72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2236726
Commit-Queue: Charlie Hu <chenleihu@google.com>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779106}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Jun 29, 2020
Implement permissions policy parser based on this proposal:
w3c/webappsec-permissions-policy#376

Change-Id: I50e160afa0f2e3039b67388c71bf5879957ad784
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2240900
Commit-Queue: Charlie Hu <chenleihu@google.com>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#783581}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Jun 29, 2020
This reverts commit a96776d.

Reason for revert: Broke Linux bots:
https://ci.chromium.org/p/chromium/builders/ci/linux-blink-cors-rel/6631
https://ci.chromium.org/p/chromium/builders/ci/Cast%20Linux/95081

Original change's description:
> Implement permissions policy parser
> 
> Implement permissions policy parser based on this proposal:
> w3c/webappsec-permissions-policy#376
> 
> Change-Id: I50e160afa0f2e3039b67388c71bf5879957ad784
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2240900
> Commit-Queue: Charlie Hu <chenleihu@google.com>
> Reviewed-by: Ian Clelland <iclelland@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#783581}

TBR=iclelland@chromium.org,chenleihu@google.com

Change-Id: Iee0cbaa33aff49f2473c374d5c0af5d0e04e6985
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2273604
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#783606}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Jun 30, 2020
This reverts commit 9a47d28.

Original change's description:
> Revert "Implement permissions policy parser"
>
> This reverts commit a96776d.
>
> Reason for revert: Broke Linux bots:
> https://ci.chromium.org/p/chromium/builders/ci/linux-blink-cors-rel/6631
> https://ci.chromium.org/p/chromium/builders/ci/Cast%20Linux/95081
>
> Original change's description:
> > Implement permissions policy parser
> >
> > Implement permissions policy parser based on this proposal:
> > w3c/webappsec-permissions-policy#376
> >
> > Change-Id: I50e160afa0f2e3039b67388c71bf5879957ad784
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2240900
> > Commit-Queue: Charlie Hu <chenleihu@google.com>
> > Reviewed-by: Ian Clelland <iclelland@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#783581}
>
> TBR=iclelland@chromium.org,chenleihu@google.com
>
> Change-Id: Iee0cbaa33aff49f2473c374d5c0af5d0e04e6985
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2273604
> Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#783606}

TBR=dalecurtis@chromium.org,iclelland@chromium.org,chenleihu@google.com

Change-Id: I852afc10dee493b15f02c083762b9f2dae4d0f2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2273082
Reviewed-by: Charlie Hu <chenleihu@google.com>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Charlie Hu <chenleihu@google.com>
Cr-Commit-Position: refs/heads/master@{#783694}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This CL splits the parsing logic and semantic logic in
|FeaturePolicyParser| so that parsing logic can be easily
substituted.

This is pre-work to implement permission policy parser which
uses structured header format. (w3c/webappsec-permissions-policy#376)

Change-Id: I116e69a562974d21e0d3a53b15cfbb3276b37d72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2236726
Commit-Queue: Charlie Hu <chenleihu@google.com>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#779106}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 126ad87f6939e33404df74db93a9012195b6486f
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Implement permissions policy parser based on this proposal:
w3c/webappsec-permissions-policy#376

Change-Id: I50e160afa0f2e3039b67388c71bf5879957ad784
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2240900
Commit-Queue: Charlie Hu <chenleihu@google.com>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#783581}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: a96776d4cde1f37045889b799ce886ace501de5e
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This reverts commit a96776d4cde1f37045889b799ce886ace501de5e.

Reason for revert: Broke Linux bots:
https://ci.chromium.org/p/chromium/builders/ci/linux-blink-cors-rel/6631
https://ci.chromium.org/p/chromium/builders/ci/Cast%20Linux/95081

Original change's description:
> Implement permissions policy parser
> 
> Implement permissions policy parser based on this proposal:
> w3c/webappsec-permissions-policy#376
> 
> Change-Id: I50e160afa0f2e3039b67388c71bf5879957ad784
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2240900
> Commit-Queue: Charlie Hu <chenleihu@google.com>
> Reviewed-by: Ian Clelland <iclelland@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#783581}

TBR=iclelland@chromium.org,chenleihu@google.com

Change-Id: Iee0cbaa33aff49f2473c374d5c0af5d0e04e6985
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2273604
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#783606}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 9a47d288e4ddfea2123c312b19ae9cb5f157ac93
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This reverts commit 9a47d288e4ddfea2123c312b19ae9cb5f157ac93.

Original change's description:
> Revert "Implement permissions policy parser"
>
> This reverts commit a96776d4cde1f37045889b799ce886ace501de5e.
>
> Reason for revert: Broke Linux bots:
> https://ci.chromium.org/p/chromium/builders/ci/linux-blink-cors-rel/6631
> https://ci.chromium.org/p/chromium/builders/ci/Cast%20Linux/95081
>
> Original change's description:
> > Implement permissions policy parser
> >
> > Implement permissions policy parser based on this proposal:
> > w3c/webappsec-permissions-policy#376
> >
> > Change-Id: I50e160afa0f2e3039b67388c71bf5879957ad784
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2240900
> > Commit-Queue: Charlie Hu <chenleihu@google.com>
> > Reviewed-by: Ian Clelland <iclelland@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#783581}
>
> TBR=iclelland@chromium.org,chenleihu@google.com
>
> Change-Id: Iee0cbaa33aff49f2473c374d5c0af5d0e04e6985
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2273604
> Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#783606}

TBR=dalecurtis@chromium.org,iclelland@chromium.org,chenleihu@google.com

Change-Id: I852afc10dee493b15f02c083762b9f2dae4d0f2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2273082
Reviewed-by: Charlie Hu <chenleihu@google.com>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Charlie Hu <chenleihu@google.com>
Cr-Original-Commit-Position: refs/heads/master@{#783694}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 3f1b882cf5961f7c1fdeb5b2b7a523156f1b6c2d
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 a pull request may close this issue.

4 participants