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

Collect current User-Agent when queueing a report #96

Merged
merged 3 commits into from
Jun 27, 2018

Conversation

dcreager
Copy link
Member

On mobile there's less of a guarantee that the User-Agent will be the same when reports are uploaded.

On mobile there's less of a guarantee that the User-Agent will be the
same when reports are uploaded.
@dcreager
Copy link
Member Author

@RByers I have not added the user agent to the Report object handed off to observers. Should I?

@RByers
Copy link
Collaborator

RByers commented Jun 25, 2018

@RByers I have not added the user agent to the Report object handed off to observers. Should I?

We discussed this a bit in #52 and #49 (please mention #52 in the commit description so people can trace the history). I don't see any value in adding it, but @mikewest seemed to think they'd be the same for the sake of consistency. We can always add it later, so I'd say keep it without it for now and we can always revisit in the future if there's value.

Copy link
Collaborator

@RByers RByers left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

@igrigorik
Copy link
Member

Silly question, but what's the value of mirroring this data within the report given that the UA will already automatically append the User-Agent string to the report's request header?

@dcreager
Copy link
Member Author

TAG suggested this in #52 in case User-Agent is different between the original request and the report upload. (Mobile, for instance, if you request a desktop site)

@RByers
Copy link
Collaborator

RByers commented Jun 26, 2018

Also relevant for example on Firefox where they have to spoof the UA string on some properties to get the best user experience :-(. The UA sent for the loaded page and the default UA string of the browser both seem potentially relevant to someone trying to understand browser errors.

@igrigorik
Copy link
Member

Hmm, ok. This still seems a bit odd to me, but I'll defer to you guys. FWIW, it might be worth documenting as to why we're capturing this in the spec, and explain where and why you should be looking at reported User-Agent vs what we provide in the report.

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.

FWIW, it might be worth documenting as to why we're capturing this in the spec

Done, PTAL

index.src.html Outdated
@@ -340,6 +340,13 @@ <h3 id="concept-reports">Reports</h3>
the value of the <code>User-Agent</code> <a>header</a> of the <a>request</a>
from which the report was generated.

Note: We collect the <code>User-Agent</code> header for each report, even
though we can also get this header from the report upload requests. This
Copy link
Collaborator

Choose a reason for hiding this comment

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

To me this wording is a little confusing - suggesting they're the same thing ("also get ..."). How about something roughly like:

The User-Agent header in the report represents the User-Agent sent by the browser for the page which generated the report. This is potentially distinct from the User-Agent sent in the HTTP headers for the report in case where the browser has chosen to use a non-default User-Agent string such as "request desktop site" feature.

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

@RByers
Copy link
Collaborator

RByers commented Jun 26, 2018

Hmm, ok. This still seems a bit odd to me, but I'll defer to you guys.

FWIW I do see this as pretty niche. I (and I suspect also TAG) would be fine treating this as a feature request for a future version which we'd prioritize only when a customer actually asks for it (or another implementor expesses interest in implementing it). But it's pretty low complexity, so if @dcreager wants to just do it now, I don't have any objection to that. I just hope it didn't seem like the blink API owners expect all TAG issues to be closed prior to ship - it's totally fine to say "this can be compatibly added later if deemed important, so doesn't blocking shipping a v0".

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.

I just hope it didn't seem like the blink API owners expect all TAG issues to be closed prior to ship - it's totally fine to say "this can be compatibly added later if deemed important, so doesn't blocking shipping a v0".

Yep yep, I was only getting this out of the way since it's not a huge change. Thanks for clarifying!

index.src.html Outdated
@@ -340,6 +340,13 @@ <h3 id="concept-reports">Reports</h3>
the value of the <code>User-Agent</code> <a>header</a> of the <a>request</a>
from which the report was generated.

Note: We collect the <code>User-Agent</code> header for each report, even
though we can also get this header from the report upload requests. This
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

@dcreager dcreager merged commit a98885a into w3c:master Jun 27, 2018
@dcreager dcreager deleted the user-agent branch June 27, 2018 13:52
@@ -1242,6 +1260,7 @@ <h2 id="sample-reports">Sample Reports</h2>
"type": "csp",
"age": 10,
"url": "https://example.com/vulnerable-page/",
"user_agent": "ReportingSpec/1",
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit confusing and easy to misinterpret as version of reporting API. Perhaps use a real-world UA string as an example?

Copy link
Member Author

Choose a reason for hiding this comment

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

#98

dcreager pushed a commit to dcreager/reporting that referenced this pull request Jul 7, 2018
* master:
  Fix typos (w3c#106)
  addressed comments
  Update "WICG" -> "W3C"
  Adding baseline CODE_OF_CONDUCT.md
  Use real User-Agent string in examples (w3c#98)
  Update links to JSON RFC (w3c#99)
  Collect current User-Agent when queueing a report (w3c#96)
  fixup s/report+json/reports+json/
  Define the MIME type in more detail.
dcreager pushed a commit to dcreager/reporting that referenced this pull request Jul 9, 2018
* master:
  Rename "report list" -> "report queue" (w3c#104)
  Clarify name of "observability" property (w3c#101)
  Add notes about load balancing mechanism (w3c#94)
  Fix typos (w3c#106)
  addressed comments
  Update "WICG" -> "W3C"
  Adding baseline CODE_OF_CONDUCT.md
  Use real User-Agent string in examples (w3c#98)
  Update links to JSON RFC (w3c#99)
  Collect current User-Agent when queueing a report (w3c#96)
  fixup s/report+json/reports+json/
  Define the MIME type in more detail.
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.

3 participants