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

Include custom request/response headers in reports #96

Merged
merged 2 commits into from
Nov 5, 2018

Conversation

dcreager
Copy link
Member

The new request_headers and response_headers policy fields allow you to instruct the user agent to include the values of arbitrary request and response headers when creating a NEL report about a request.

Issue #93

The new `request_headers` and `response_headers` policy fields allow you
to instruct the user agent to include the values of arbitrary request
and response headers when creating a NEL report about a request.

Issue w3c#93
@dcreager
Copy link
Member Author

The main open question I have about this is whether there are any request or response headers that we have to prohibit for security or privacy reasons. I don't think we do, but I'd like to hear other people's opinions, too.

My reasoning: a NEL report is only supposed to contain information that the server would already have visibility into. Since the server can always see every request header sent by the user agent, and has full control over which response headers it sends back, then there isn't any new information that we could leak to the server owner using this mechanism.

To make sure that we handle the kinds of attacks mentioned in #74, we make sure to clear out any custom headers when downgrading a report (which happens when we have a policy for a particular domain, but can't verify that the domain owner is the same as the server owner).

@dcreager
Copy link
Member Author

Adding several reviewers who are participating in the discussion on w3c/trace-context#69; please feel free to add others!

index.html Outdated
end user request, and uses a distributed tracing framework to correlate
events about all of these requests together. The tracing framework
relies on the browser generating a unique trace ID for each outgoing
request, placing it in the request's <code>TraceParent</code> header.
Copy link
Member

Choose a reason for hiding this comment

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

This bit assumes the browser supports such a thing, but that's not the case today, nor is it clear where / how that will be defined. Should we omit this for now? If we want to keep this, I'd propose pulling this content out into a non-normative note, with a link to relevant discussion / WIP spec that attempts to defines this.

Copy link
Member Author

@dcreager dcreager Oct 25, 2018

Choose a reason for hiding this comment

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

There was also confusion at TPAC about this — consensus was to discuss other use cases (and possibly remove/move this one as you suggest) to clarify that this feature is not specific to Distributed Tracing.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the example to talk about cache validation (ETag and If-None-Match) instead.

index.html Outdated
data-lt="policy request headers">request headers</dfn> and a list of <dfn
data-lt-noDefault data-lt="policy response headers">response
headers</dfn>, each of which is a list of
header names.
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with IETF nomenclature and be more precise, let's use "header field names" throughout?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was following Fetch, which uses "header name". (Though I should link this to the fetch definition)

index.html Outdated

<p>
The OPTIONAL <dfn><code>response_headers</code></dfn> member defines the
list of <a data-lt="policy response headers">response headers</a> that
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth clarifying that you intend to append both header field name and value in these reports? Ditto for both request and response.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@igrigorik
Copy link
Member

Re, security / privacy and exposing headers: I believe your reasoning makes sense. The server would see all these headers already, so we shouldn't be exposing anything new, short of allowing said origin to delegate delivery to alternate reporting endpoint.

That said, I'd love for @mikewest to sanity check before we land this.

@dcreager
Copy link
Member Author

Re, security / privacy and exposing headers: I believe your reasoning makes sense. The server would see all these headers already, so we shouldn't be exposing anything new, short of allowing said origin to delegate delivery to alternate reporting endpoint.

We discussed this at TPAC today and the consensus was that this should be fine from a security/privacy standpoint.

Copy link
Member Author

@dcreager dcreager left a comment

Choose a reason for hiding this comment

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

PTAL

index.html Outdated
data-lt="policy request headers">request headers</dfn> and a list of <dfn
data-lt-noDefault data-lt="policy response headers">response
headers</dfn>, each of which is a list of
header names.
Copy link
Member Author

Choose a reason for hiding this comment

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

I was following Fetch, which uses "header name". (Though I should link this to the fetch definition)

index.html Outdated

<p>
The OPTIONAL <dfn><code>response_headers</code></dfn> member defines the
list of <a data-lt="policy response headers">response headers</a> that
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

index.html Outdated
end user request, and uses a distributed tracing framework to correlate
events about all of these requests together. The tracing framework
relies on the browser generating a unique trace ID for each outgoing
request, placing it in the request's <code>TraceParent</code> header.
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the example to talk about cache validation (ETag and If-None-Match) instead.

Copy link
Member

@igrigorik igrigorik left a comment

Choose a reason for hiding this comment

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

👍

Awesome work on the detailed example. :)

@dcreager
Copy link
Member Author

dcreager commented Nov 5, 2018

Thanks Ilya! Any other comments/questions before we merge this?

@igrigorik
Copy link
Member

Looks good from this end.

@dcreager dcreager merged commit 3562767 into w3c:gh-pages Nov 5, 2018
@dcreager dcreager deleted the custom-headers branch November 5, 2018 19:28
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.

2 participants