Introduce "reporting endpoint" to COEP #8

yutakahirano · 2020-01-23T10:58:30Z

This change introduces "reporting endpoint" concept which will be used as an
reporting endpoint for potential COEP violations.

Reporting behavior will be specified in future changes.

Discussed at whatwg/html#5100.

This change introduces "reporting endpoint" concept which will be used as an reporting endpoint for potential COEP violations. Reporting behavior will be specified in future changes. Discussed at whatwg/html#5100.

zcorpan · 2020-01-23T19:37:16Z

This should probably follow the convention in #2

yutakahirano · 2020-01-24T03:49:19Z

I'll do that after that change lands.

mikewest

Thanks for this patch! I have a few questions:

Do we need a report-only mode? ( @arturjanc )
We could do this in one header rather than two (or four, if report-only is a thing). Wouldn't a param be simpler?

index.bs

yutakahirano · 2020-01-28T10:29:05Z

Thank you!

Do we need a report-only mode? ( @arturjanc )

What do you mean by report-only mode here? I'm planning to introduce the following behaviors:

If COEP is "none" and the reporting endpoint is null: do nothing
If COEP is "require-corp" and the reporting endpoint is null: COEP is effective without reporting
If COEP is "none" and the reporting endpoint is set: Do not block things but report "would-be" violations to the server.
If COEP is "require-corp" and the reporting endpoint is set: COEP is effective, and report violations to the server.

I thought the third settings would be the "report-only" mode.

We could do this in one header rather than two (or four, if report-only is a thing). Wouldn't a param be simpler?

I'm fine with this.

mikewest · 2020-01-28T10:32:58Z

I thought the third settings would be the "report-only" mode.

Got it. I'm fine with that. It's a little distinct from CSP, but that's not necessarily a bad thing.

If we go that route, the single-header variant I've suggested above would require introducing a new token. Perhaps none, or something more explicit (like report-corp-violations)?

yutakahirano · 2020-01-29T10:23:42Z

I fixed the header parsing part (I haven't changed the properties on context objects yet). Mike, can you take a look at that part?

yutakahirano · 2020-01-30T07:31:38Z

Now I think I addressed your comment. PTAL.

mikewest

Looks pretty reasonable to me. I'd like @annevk's opinion as well, as I think he disagrees about the header syntax, and we should probably chat about it rather than just landing this. :)

index.bs

arturjanc · 2020-01-30T11:50:14Z

Do we need a report-only mode? ( @arturjanc )

I think you've settled this already, but just mentioning that the approach above looks good to me (the 4 behaviors mentioned by @yutakahirano, possibly updated if we decide to go with the single-header variant).

In general, I'd posit that there are a lot of benefits to having a report-only mode for everything :)

annevk · 2020-02-03T09:43:11Z

Does anyone have some examples to evaluate? I find the diff a little hard to read.

Accept mikewset's suggestion Co-Authored-By: Mike West <mike@mikewest.org>

Accept mikewest's suggestion Co-Authored-By: Mike West <mike@mikewest.org>

yutakahirano · 2020-02-04T06:10:35Z

This change:

introduces "unsafe-none" as a value of the COEP header.
introduces "report-to" parameter to the COEP header
introduces the embedder policy concept which consists of COEP value and the reporting endpoint.

COEP headers now look like:

unsafe-none : Do nothing.
unsafe-none; report-to=endpoint Do not block things, but report would-be violations using "endpoint" reporting endpoint.
require-corp: COEP is effective without reporting
require-corp; report-to=endpoint: COEP is effective and report violations using "endpoint" reporting endpoint.

This change does NOT include how the reporting endpoint is used. I'm going to publish another PR for that part.

@annevk, does this summary help?

yutakahirano · 2020-02-05T11:17:23Z

I would like the syntax to be aligned with COOP reporting. @camillelamy, are you fine with the proposal?

annevk · 2020-02-05T12:43:55Z

The summary is great, thanks again!

After discussion on IRC I think I'm okay with this approach, as we'll likely keep COOP and COEP scoped to a simple enumerated value. If anyone foresees them using more complex values, now would be the time to bring that up. (If the value can be more complex, I think you want more headers eventually, as explained in https://github.com/w3c/webappsec-feature-policy/issues/362#issuecomment-582302307.)

zcorpan · 2020-02-05T12:59:01Z

@yutakahirano the : is supposed to be ; , correct?

yutakahirano · 2020-02-05T14:13:57Z

@zcorpan Yes, thanks. Fixed.

index.bs

mikewest

I'm happy with this % the discussion of parameter types. If @camillelamy and team are happy too, I'll pull it in.

index.bs

camillelamy · 2020-02-06T16:00:04Z

Our proposal for COOP does not have the same syntax. Currently we're looking at 3 headers:

Cross-Origin-Opener-Policy: the policy to enforce
Cross-Origin-Opener-Policy-Report-Only: allows to specify a policy which is not enforced and for which we only provide reports
Cross-Origin-Opener-Policy-Report-Endpoint: where to send the reports (which would be generated for both the COOP enforced and the COOP report-only mode).

We could move to a model closer to the one you propose. but we'd still have two headers:

Cross-Origin-Opener-Policy : value; report-to=endpoint
Cross-Origin-Opener-Policy-Report-Only : value; report-to=endpoint

This is because COOP has 3 values, so we cannot make the assumption that unsafe-none + an endpoint means we want a report only mode for same-origin (it could be same-origin-allow-popups).

The problem I see with the current approach for COEP is that if we want to add another value to the header, the report-only mode doesn't work anymore (like it wouldn't with COOP). If we foresee the need to eventually add another value, we should allow the developer to specify explicitly which policy they want report-only mode for instead of making this deduction.

annevk · 2020-02-06T16:35:08Z

Thanks Camille! @yutakahirano had an idea for that problem when COEP gets to it, but now you contrast it with COOP I think we ought to solve it now so we can have a consistent reporting story across these two features.

yutakahirano · 2020-02-07T05:44:40Z

Thank you, Camille.

Does the following work?

cross-origin-opener-policy: policy; reporting_as=another policy; reporting_to=endpoint

I'm also fine with the two-headers version. The three-headers version seems too verbose to me.

camillelamy · 2020-02-07T10:50:41Z

I think the two headers version matches what CSP is doing, right? If I'm not mistaken, CSP has two headers: Content-Security-Policy and Content-Security-Policy-Report-Only. It might be easier for developers to have the same model for COOP and COEP?

yutakahirano · 2020-02-12T11:15:03Z

Fixed. Now we have two headers: "Cross-Origin-Embedder-Policy" (COEP) and "Cross-Origin-Embedder-Policy-Report-Only" (COEP-Report-Only). I removed "unsafe-none" value from the header(s).

Cross-Origin-Embedder-Policy-Report-Only is ignored if there is (valid) Cross-Origin-Embedder-Policy attached; Hence there are four modes:

COEP: require-corp: CORP-Report-Only is ignored. COEP is effective. Violations are not reported.
COEP: require-corp; report-to="endpoint": CORP-Report-Only is ignored. COEP is effective. Violations are reported to "endpoint".
(COEP: missing,) COEP-Report-Only: require-corp: No effect
(COEP: missing,) COEP-Report-Only: require-corp; report-to="endpoint": Do not block things. Potential violations are reported to "endpoint".
(COEP: missing, COEP-Report-Only: missing): No effect

The "embedder policy" concept consists of value (either "require-corp" and "unsafe-none"), and reporting endpoint. Unlike the pair of headers above, this concept covers all modes by itself.

{value: "require-corp", reporting endpoint: null}: COEP is effective. Violations are not reported.
{value: "require-corp", reporting endpoint: "endpoint"}: COEP is effective. Violations are reported to "endpoint".
{value: "unsafe-none", reporting endpoint: null}: No effect
{value: "unsafe-none", reporting endpoint: "endpoint"}: Do not block things. Potential violations are reported to "endpoint".

The embedder policy concept is not exposed to HTTP, so if the syntax grows we can change the concept without breaking things.

@mikewest @annevk @camillelamy Are you fine with the latest change?

camillelamy · 2020-02-12T12:22:43Z

Thanks! I am happy with the latest version.

yutakahirano · 2020-02-14T06:43:41Z

@annevk @mikewest can you take a look at the latest change? Thanks!

mikewest

LGTM % one comment.

mikewest · 2020-02-14T08:41:47Z

index.bs


-    1.  Set |header| to the result of [=isomorphic decoding=] |header|.
+    2.  Return a new [=/embedder policy=] whose [=embedder policy/value] is |parsed item|'s


This means that COEPRO is ignored in the presence of COEP. This is probably fine for the moment, since there's only one value, but would make it difficult to support enforcement of one value and reporting on another if we have multiple values in the future.

Perhaps add a note to that effect? I'm not sure it's worth adding complexity until we need it.

I'd say COEPRO is ignored if its value is equal to or weaker than the COEP value. That rule is natural, and we'll want to keep the rule even when we introduce more values. What do you think?

That's not the way CSP works.

What do we do with COEP: require-corp\nCOEPRO: require-corp; report-to=whatever? Should that not send reports?

Hmm... so you prefer having two values and endpoints. I'll try to modify the logic.

I think we will need to have two values and endpoints to support both enforce and report-only at the same time. I don't think that's actually relevant to any developer's life until we have multiple values (I have require-cors in the back of my head (because a value with a one-character difference to create radical behavioral differences is a great spelling option)), so I wouldn't mind explicitly punting on it. But doing it now certainly wouldn't be wasted effort.

Fixed. Now the embedder policy concept has four members.

value

reporting endpoint

report only value

report only reporting endpoint

reporting endpoint / report only reporting endpoint is not set if value / report only value is not "require-corp" but it doesn't affect the behavior because we'll have nothing to report for "unsafe-none".

yutakahirano · 2020-02-18T13:39:45Z

ping?

mikewest

LGTM. Let's land this and continue iterating. :)

yutakahirano · 2020-02-19T09:55:40Z

Thank you!

Being proposed as WICG/cross-origin-embedder-policy#8. This CL introduces a new mojo struct, CrossOriginEmbedderPolicyWithReporting to group related values. It has (virtually trivial) type mapping, because mojo::[Inlined]StructPtr is initialized as null while we don't want to allow null for CrossOriginEmbedderPolicyWithReporting. Bug: 1052764 Change-Id: I1985f24386c9dd3f76bfd2bab395b9103d14a4ff Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2060090 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#744490}

yutakahirano added 2 commits January 23, 2020 19:57

Introduce "reporting endpoint" to COEP

3b902d3

This change introduces "reporting endpoint" concept which will be used as an reporting endpoint for potential COEP violations. Reporting behavior will be specified in future changes. Discussed at whatwg/html#5100.

fix

8c2a500

yutakahirano mentioned this pull request Jan 23, 2020

COEP and report-only mode whatwg/html#5100

Closed

mikewest reviewed Jan 28, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

yutakahirano added 2 commits January 29, 2020 19:13

fix

ee36e23

fix

a302bae

yutakahirano added 2 commits January 30, 2020 16:24

fix

ca59f6b

fix

df422cc

mikewest reviewed Jan 30, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

yutakahirano and others added 4 commits February 4, 2020 14:28

Update index.bs

6f08b81

Accept mikewset's suggestion Co-Authored-By: Mike West <mike@mikewest.org>

Update index.bs

e7ec226

Accept mikewest's suggestion Co-Authored-By: Mike West <mike@mikewest.org>

fix

0dc1bbb

fix

3fa8c99

zcorpan reviewed Feb 5, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

mikewest approved these changes Feb 5, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

yutakahirano added 2 commits February 6, 2020 17:48

fix

b3cad88

fix

a926125

yutakahirano mentioned this pull request Feb 10, 2020

Proposal: Reporting API for Cross-Origin-Opener-Policy whatwg/html#5260

Closed

fix

a960783

mikewest approved these changes Feb 14, 2020

View reviewed changes

yutakahirano added 2 commits February 15, 2020 00:22

fix

55b8167

fix

1503efc

mikewest approved these changes Feb 19, 2020

View reviewed changes

mikewest merged commit 31591be into WICG:master Feb 19, 2020

annevk mentioned this pull request Apr 7, 2020

Use 'get a structured header' term from Fetch #2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce "reporting endpoint" to COEP #8

Introduce "reporting endpoint" to COEP #8

yutakahirano commented Jan 23, 2020

zcorpan commented Jan 23, 2020

yutakahirano commented Jan 24, 2020

mikewest left a comment

yutakahirano commented Jan 28, 2020

mikewest commented Jan 28, 2020

yutakahirano commented Jan 29, 2020

yutakahirano commented Jan 30, 2020

mikewest left a comment

arturjanc commented Jan 30, 2020

annevk commented Feb 3, 2020

yutakahirano commented Feb 4, 2020 •

edited

Loading

yutakahirano commented Feb 5, 2020 •

edited

Loading

annevk commented Feb 5, 2020

zcorpan commented Feb 5, 2020

yutakahirano commented Feb 5, 2020

mikewest left a comment

camillelamy commented Feb 6, 2020

annevk commented Feb 6, 2020

yutakahirano commented Feb 7, 2020

camillelamy commented Feb 7, 2020

yutakahirano commented Feb 12, 2020

camillelamy commented Feb 12, 2020

yutakahirano commented Feb 14, 2020

mikewest left a comment

mikewest Feb 14, 2020

yutakahirano Feb 14, 2020

mikewest Feb 14, 2020

yutakahirano Feb 14, 2020

mikewest Feb 14, 2020

yutakahirano Feb 14, 2020 •

edited

Loading

yutakahirano Feb 14, 2020

yutakahirano commented Feb 18, 2020

mikewest left a comment

yutakahirano commented Feb 19, 2020


		1. Set \|header\| to the result of [=isomorphic decoding=] \|header\|.
		2. Return a new [=/embedder policy=] whose [=embedder policy/value] is \|parsed item\|'s

Introduce "reporting endpoint" to COEP #8

Introduce "reporting endpoint" to COEP #8

Conversation

yutakahirano commented Jan 23, 2020

zcorpan commented Jan 23, 2020

yutakahirano commented Jan 24, 2020

mikewest left a comment

Choose a reason for hiding this comment

yutakahirano commented Jan 28, 2020

mikewest commented Jan 28, 2020

yutakahirano commented Jan 29, 2020

yutakahirano commented Jan 30, 2020

mikewest left a comment

Choose a reason for hiding this comment

arturjanc commented Jan 30, 2020

annevk commented Feb 3, 2020

yutakahirano commented Feb 4, 2020 • edited Loading

yutakahirano commented Feb 5, 2020 • edited Loading

annevk commented Feb 5, 2020

zcorpan commented Feb 5, 2020

yutakahirano commented Feb 5, 2020

mikewest left a comment

Choose a reason for hiding this comment

camillelamy commented Feb 6, 2020

annevk commented Feb 6, 2020

yutakahirano commented Feb 7, 2020

camillelamy commented Feb 7, 2020

yutakahirano commented Feb 12, 2020

camillelamy commented Feb 12, 2020

yutakahirano commented Feb 14, 2020

mikewest left a comment

Choose a reason for hiding this comment

mikewest Feb 14, 2020

Choose a reason for hiding this comment

yutakahirano Feb 14, 2020

Choose a reason for hiding this comment

mikewest Feb 14, 2020

Choose a reason for hiding this comment

yutakahirano Feb 14, 2020

Choose a reason for hiding this comment

mikewest Feb 14, 2020

Choose a reason for hiding this comment

yutakahirano Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

yutakahirano Feb 14, 2020

Choose a reason for hiding this comment

yutakahirano commented Feb 18, 2020

mikewest left a comment

Choose a reason for hiding this comment

yutakahirano commented Feb 19, 2020

yutakahirano commented Feb 4, 2020 •

edited

Loading

yutakahirano commented Feb 5, 2020 •

edited

Loading

yutakahirano Feb 14, 2020 •

edited

Loading