Add Content-Type header to net::ReportSender reports

Add Content-Type header to net::ReportSender reports

Two sites that are opted into OCSP Expect-Staple reporting
(www.dropbox.com, www.caddyserver.com) reported that they were receiving
reports from Chrome with empty request bodies. This turned out to be
their logging code not handling requests with no Content-Type
header. When TransportSecurityState sends JSON violation reports for
HPKP and Expect-Staple, it ought to be sending a Content-Type header of
application/json. Thus, this CL adds a |content_type| parameter to
net::ReportSender::Send() and updates the users of net::ReportSender
(including TransportSecurityState) to set |content_type| appropriately.

BUG=650327
Committed: https://crrev.com/b1716e2e3830aa031dc335b0403913efadb199dd
Cr-Commit-Position: refs/heads/master@{#421459}

[estark, 2016-09-26 18:31:24]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 18:32:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-09-26 18:32:50]

estark@chromium.org changed reviewers:
+ eroman@chromium.org

[estark, 2016-09-26 18:32:50]

eroman, PTAL?

[eroman, 2016-09-26 18:37:24]

https://codereview.chromium.org/2365353004/diff/20001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2365353004/diff/20001/net/http/transport_secu...
net/http/transport_security_state.h:260: virtual void SetContentTypeHeader(const
std::string& content_type) = 0;
API question: Why not make this a parameter of the Send() method?

[estark, 2016-09-26 18:38:45]

https://codereview.chromium.org/2365353004/diff/20001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2365353004/diff/20001/net/http/transport_secu...
net/http/transport_security_state.h:260: virtual void SetContentTypeHeader(const
std::string& content_type) = 0;
On 2016/09/26 18:37:24, eroman wrote:
> API question: Why not make this a parameter of the Send() method?

There are other users of this class that don't care about setting a Content-Type
header. I could either force them to care (maybe that's a good thing to do), or
I could add a new SendWithContentType() method that TransportSecurityState uses.
Any preference among those options?

[commit-bot: I haz the power, 2016-09-26 18:49:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 18:49:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[eroman, 2016-09-26 18:55:11]

https://codereview.chromium.org/2365353004/diff/20001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2365353004/diff/20001/net/http/transport_secu...
net/http/transport_security_state.h:260: virtual void SetContentTypeHeader(const
std::string& content_type) = 0;
On 2016/09/26 18:38:45, estark wrote:
> On 2016/09/26 18:37:24, eroman wrote:
> > API question: Why not make this a parameter of the Send() method?
> 
> There are other users of this class that don't care about setting a
Content-Type
> header. I could either force them to care (maybe that's a good thing to do),
or
> I could add a new SendWithContentType() method that TransportSecurityState
uses.
> Any preference among those options?

I think it would be clearer for Send() to capture this information.
Right now the |report| parameter (at least in the comments) is an opaque blob,
and doesn't answer the questions of what is the content-type, and what is the
text encoding (charset).

What are the other content types that need to be supported? I am assuming
UTF-encoded JSON, is there anything else?

My thinking would be to either internalize this and mandate what |report|
represents. Or if that is not possible, then make it a required parameter in
Send().

If the UTF-8 JSON is the common case can have a non-virtual implementation that
feeds the right parameters:

 SendUtf8Json(uri, json) {...}

WDYT?

[estark, 2016-09-26 18:59:43]

On 2016/09/26 18:55:11, eroman wrote:
>
https://codereview.chromium.org/2365353004/diff/20001/net/http/transport_secu...
> File net/http/transport_security_state.h (right):
> 
>
https://codereview.chromium.org/2365353004/diff/20001/net/http/transport_secu...
> net/http/transport_security_state.h:260: virtual void
SetContentTypeHeader(const
> std::string& content_type) = 0;
> On 2016/09/26 18:38:45, estark wrote:
> > On 2016/09/26 18:37:24, eroman wrote:
> > > API question: Why not make this a parameter of the Send() method?
> > 
> > There are other users of this class that don't care about setting a
> Content-Type
> > header. I could either force them to care (maybe that's a good thing to do),
> or
> > I could add a new SendWithContentType() method that TransportSecurityState
> uses.
> > Any preference among those options?
> 
> I think it would be clearer for Send() to capture this information.
> Right now the |report| parameter (at least in the comments) is an opaque blob,
> and doesn't answer the questions of what is the content-type, and what is the
> text encoding (charset).
> 
> What are the other content types that need to be supported? I am assuming
> UTF-encoded JSON, is there anything else?
> 
> My thinking would be to either internalize this and mandate what |report|
> represents. Or if that is not possible, then make it a required parameter in
> Send().
> 
> If the UTF-8 JSON is the common case can have a non-virtual implementation
that
> feeds the right parameters:
> 
>  SendUtf8Json(uri, json) {...}
> 
> WDYT?

components/certificate_reporting uses this to send serialized protobufs:
https://cs.chromium.org/chromium/src/components/certificate_reporting/error_r...

Given that, I guess required parameter on Send() is the right way to go?

[eroman, 2016-09-26 19:28:03]

sure sgtm

On Mon, Sep 26, 2016 at 11:59 AM, <estark@chromium.org> wrote:

> On 2016/09/26 18:55:11, eroman wrote:
> >
> https://codereview.chromium.org/2365353004/diff/20001/net/
> http/transport_security_state.h
> > File net/http/transport_security_state.h (right):
> >
> >
> https://codereview.chromium.org/2365353004/diff/20001/net/
> http/transport_security_state.h#newcode260
> > net/http/transport_security_state.h:260: virtual void
> SetContentTypeHeader(const
> > std::string& content_type) = 0;
> > On 2016/09/26 18:38:45, estark wrote:
> > > On 2016/09/26 18:37:24, eroman wrote:
> > > > API question: Why not make this a parameter of the Send() method?
> > >
> > > There are other users of this class that don't care about setting a
> > Content-Type
> > > header. I could either force them to care (maybe that's a good thing
> to do),
> > or
> > > I could add a new SendWithContentType() method that
> TransportSecurityState
> > uses.
> > > Any preference among those options?
> >
> > I think it would be clearer for Send() to capture this information.
> > Right now the |report| parameter (at least in the comments) is an opaque
> blob,
> > and doesn't answer the questions of what is the content-type, and what
> is the
> > text encoding (charset).
> >
> > What are the other content types that need to be supported? I am assuming
> > UTF-encoded JSON, is there anything else?
> >
> > My thinking would be to either internalize this and mandate what |report|
> > represents. Or if that is not possible, then make it a required
> parameter in
> > Send().
> >
> > If the UTF-8 JSON is the common case can have a non-virtual
> implementation
> that
> > feeds the right parameters:
> >
> > SendUtf8Json(uri, json) {...}
> >
> > WDYT?
>
> components/certificate_reporting uses this to send serialized protobufs:
> https://cs.chromium.org/chromium/src/components/
> certificate_reporting/error_reporter.cc?rcl=1474888691&l=152
>
> Given that, I guess required parameter on Send() is the right way to go?
>
> https://codereview.chromium.org/2365353004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[estark, 2016-09-26 19:55:22]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 19:56:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-09-26 19:57:35]

Description was changed from

==========
Add Content-Type header to TransportSecurityState violation reports

Two sites that are opted into OCSP Expect-Staple reporting
(www.dropbox.com, www.caddyserver.com) reported that they were receiving
reports from Chrome with empty request bodies. This turned out to be
their logging code not handling requests with no Content-Type
header. When TransportSecurityState sends JSON violation reports for
HPKP and Expect-Staple, it ought to be sending a Content-Type header of
application/json. Thus, this CL adds a SetContentTypeHeader() method to
net::ReportSender, which TransportSecurityState uses to set its
violation reports to application/json.

BUG=650327
==========

to

==========
Add Content-Type header to TransportSecurityState violation reports

Two sites that are opted into OCSP Expect-Staple reporting
(www.dropbox.com, www.caddyserver.com) reported that they were receiving
reports from Chrome with empty request bodies. This turned out to be
their logging code not handling requests with no Content-Type
header. When TransportSecurityState sends JSON violation reports for
HPKP and Expect-Staple, it ought to be sending a Content-Type header of
application/json. Thus, this CL adds a |content_type| parameter to
net::ReportSender::Send() and updates the users of net::ReportSender
(including TransportSecurityState) to set |content_type| appropriately.

BUG=650327
==========

[estark, 2016-09-26 19:58:17]

Ok, I updated the CL to add |content_type| as a parameter to Send() -- PTAL.
Thanks!

[eroman, 2016-09-26 21:14:07]

lgtm

https://codereview.chromium.org/2365353004/diff/40001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/2365353004/diff/40001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:179:
report_sender_->Send(report_uri, "application/json", serialized_report);
What is your take on using "application/json; charset=utf-8" ?

In practice I think application/json is going to be interpreted as UTF-8, but if
the goal here is fixing a server-side compatibility issue, it might be better to
be explicit.

https://codereview.chromium.org/2365353004/diff/40001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2365353004/diff/40001/net/http/transport_secu...
net/http/transport_security_state.h:256: // header as specified in
|content_type|.
Can you add a note that content-type should be non-empty?
(to avoid any confusion on interpretation -- empty is not a means to omit the
content-type parameter).

https://codereview.chromium.org/2365353004/diff/40001/net/http/transport_secu...
net/http/transport_security_state.h:258: const std::string& content_type,
What about base::StringPiece for these parameters?

I don't feel too strongly either way.
However since these are just being supplied to make a copy, StringPiece does
eliminate a redundant copy.

[commit-bot: I haz the power, 2016-09-26 21:23:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 21:23:29]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[estark, 2016-09-26 22:52:17]

Description was changed from

==========
Add Content-Type header to TransportSecurityState violation reports

Two sites that are opted into OCSP Expect-Staple reporting
(www.dropbox.com, www.caddyserver.com) reported that they were receiving
reports from Chrome with empty request bodies. This turned out to be
their logging code not handling requests with no Content-Type
header. When TransportSecurityState sends JSON violation reports for
HPKP and Expect-Staple, it ought to be sending a Content-Type header of
application/json. Thus, this CL adds a |content_type| parameter to
net::ReportSender::Send() and updates the users of net::ReportSender
(including TransportSecurityState) to set |content_type| appropriately.

BUG=650327
==========

to

==========
Add Content-Type header to net::ReportSender reports

Two sites that are opted into OCSP Expect-Staple reporting
(www.dropbox.com, www.caddyserver.com) reported that they were receiving
reports from Chrome with empty request bodies. This turned out to be
their logging code not handling requests with no Content-Type
header. When TransportSecurityState sends JSON violation reports for
HPKP and Expect-Staple, it ought to be sending a Content-Type header of
application/json. Thus, this CL adds a |content_type| parameter to
net::ReportSender::Send() and updates the users of net::ReportSender
(including TransportSecurityState) to set |content_type| appropriately.

BUG=650327
==========

[estark, 2016-09-26 23:24:17]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 23:24:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-09-26 23:26:09]

estark@chromium.org changed reviewers:
+ nparker@chromium.org

[estark, 2016-09-26 23:26:10]

Thanks, Eric. Adding nparker: can you please take a look at changes to
chrome/browser/safe_browsing?

https://codereview.chromium.org/2365353004/diff/40001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/2365353004/diff/40001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:179:
report_sender_->Send(report_uri, "application/json", serialized_report);
On 2016/09/26 21:14:07, eroman wrote:
> What is your take on using "application/json; charset=utf-8" ?
> 
> In practice I think application/json is going to be interpreted as UTF-8, but
if
> the goal here is fixing a server-side compatibility issue, it might be better
to
> be explicit.

Sure, that seems reasonable. Done.

https://codereview.chromium.org/2365353004/diff/40001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2365353004/diff/40001/net/http/transport_secu...
net/http/transport_security_state.h:256: // header as specified in
|content_type|.
On 2016/09/26 21:14:07, eroman wrote:
> Can you add a note that content-type should be non-empty?
> (to avoid any confusion on interpretation -- empty is not a means to omit the
> content-type parameter).

Done.

https://codereview.chromium.org/2365353004/diff/40001/net/http/transport_secu...
net/http/transport_security_state.h:258: const std::string& content_type,
On 2016/09/26 21:14:07, eroman wrote:
> What about base::StringPiece for these parameters?
> 
> I don't feel too strongly either way.
> However since these are just being supplied to make a copy, StringPiece does
> eliminate a redundant copy.

Done.

[eroman, 2016-09-26 23:40:55]

lgtm

https://codereview.chromium.org/2365353004/diff/60001/net/url_request/report_...
File net/url_request/report_sender.cc (right):

https://codereview.chromium.org/2365353004/diff/60001/net/url_request/report_...
net/url_request/report_sender.cc:56: new std::vector<char>(report.data(),
report.data() + report.length())));
nit: For terser code: (report.begin(), report.end())

[Nathan Parker, 2016-09-26 23:53:25]

LGTM for safe_browsing

Mine are all nits, do what you can with them.

https://codereview.chromium.org/2365353004/diff/60001/components/certificate_...
File components/certificate_reporting/error_reporter.cc (right):

https://codereview.chromium.org/2365353004/diff/60001/components/certificate_...
components/certificate_reporting/error_reporter.cc:142:
certificate_report_sender_->Send(upload_url_, "application/octet-stream",
Is there a const string somewhere you can use instead of this?  This could have
a typo...

https://codereview.chromium.org/2365353004/diff/60001/components/certificate_...
File components/certificate_reporting/error_reporter_unittest.cc (right):

https://codereview.chromium.org/2365353004/diff/60001/components/certificate_...
components/certificate_reporting/error_reporter_unittest.cc:45: void Send(const
GURL& report_uri,
Since there are several tests reusing this pattern, could they reuse some code?

https://codereview.chromium.org/2365353004/diff/60001/net/url_request/report_...
File net/url_request/report_sender.cc (right):

https://codereview.chromium.org/2365353004/diff/60001/net/url_request/report_...
net/url_request/report_sender.cc:38: DCHECK(!content_type.empty());
Can you DCHECK that content_type is a probably-valid content type?  I'm not sure
how you'd do that, but it would catch the case where the caller accidentally
swaps the two StringPieces (since the compiler won't catch that).

[eroman, 2016-09-27 00:03:34]

https://codereview.chromium.org/2365353004/diff/60001/net/url_request/report_...
File net/url_request/report_sender.cc (right):

https://codereview.chromium.org/2365353004/diff/60001/net/url_request/report_...
net/url_request/report_sender.cc:38: DCHECK(!content_type.empty());
On 2016/09/26 23:53:25, Nathan Parker wrote:
> Can you DCHECK that content_type is a probably-valid content type?  I'm not
sure
> how you'd do that, but it would catch the case where the caller accidentally
> swaps the two StringPieces (since the compiler won't catch that).

Or if JSON and application/octet-stream are going to be the only supported types
in practice, could make this an enum

[commit-bot: I haz the power, 2016-09-27 00:30:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 00:30:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2016-09-27 04:04:12]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 04:04:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-09-27 04:05:20]

https://codereview.chromium.org/2365353004/diff/60001/components/certificate_...
File components/certificate_reporting/error_reporter.cc (right):

https://codereview.chromium.org/2365353004/diff/60001/components/certificate_...
components/certificate_reporting/error_reporter.cc:142:
certificate_report_sender_->Send(upload_url_, "application/octet-stream",
On 2016/09/26 23:53:25, Nathan Parker wrote:
> Is there a const string somewhere you can use instead of this?  This could
have
> a typo...

Alas, not that I can find. :/

https://codereview.chromium.org/2365353004/diff/60001/components/certificate_...
File components/certificate_reporting/error_reporter_unittest.cc (right):

https://codereview.chromium.org/2365353004/diff/60001/components/certificate_...
components/certificate_reporting/error_reporter_unittest.cc:45: void Send(const
GURL& report_uri,
On 2016/09/26 23:53:25, Nathan Parker wrote:
> Since there are several tests reusing this pattern, could they reuse some
code?

Yeah, I think it might make sense to have a net::MockReportSender that they can
all use. I think I'll save it for a follow-up though because I might want to try
to merge this CL back to M54 (so I want it as small as possible).

https://codereview.chromium.org/2365353004/diff/60001/net/url_request/report_...
File net/url_request/report_sender.cc (right):

https://codereview.chromium.org/2365353004/diff/60001/net/url_request/report_...
net/url_request/report_sender.cc:38: DCHECK(!content_type.empty());
On 2016/09/27 00:03:34, eroman wrote:
> On 2016/09/26 23:53:25, Nathan Parker wrote:
> > Can you DCHECK that content_type is a probably-valid content type?  I'm not
> sure
> > how you'd do that, but it would catch the case where the caller accidentally
> > swaps the two StringPieces (since the compiler won't catch that).
> 
> Or if JSON and application/octet-stream are going to be the only supported
types
> in practice, could make this an enum

I like the enum idea, but I can't figure out how to do it in a non-weird way.
That is, if I define the enum in net::TransportSecurityState, that means that
things like components/certificate_reporting that otherwise know nothing about
net::TransportSecurityState would be using the net::TransportSecurityState enum
to call ReportSender::Send(). That seems weird and probably indicates that
there's something weird about the way these classes are set up...

https://codereview.chromium.org/2365353004/diff/60001/net/url_request/report_...
net/url_request/report_sender.cc:56: new std::vector<char>(report.data(),
report.data() + report.length())));
On 2016/09/26 23:40:55, eroman wrote:
> nit: For terser code: (report.begin(), report.end())

Done.

[commit-bot: I haz the power, 2016-09-27 05:11:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 05:11:31]

Dry run: This issue passed the CQ dry run.

[estark, 2016-09-28 05:07:43]

The CQ bit was checked by estark@chromium.org

[estark, 2016-09-28 05:07:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2365353004/#ps80001
(title: "eroman comment")

[commit-bot: I haz the power, 2016-09-28 05:08:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 06:04:06]

Description was changed from

==========
Add Content-Type header to net::ReportSender reports

Two sites that are opted into OCSP Expect-Staple reporting
(www.dropbox.com, www.caddyserver.com) reported that they were receiving
reports from Chrome with empty request bodies. This turned out to be
their logging code not handling requests with no Content-Type
header. When TransportSecurityState sends JSON violation reports for
HPKP and Expect-Staple, it ought to be sending a Content-Type header of
application/json. Thus, this CL adds a |content_type| parameter to
net::ReportSender::Send() and updates the users of net::ReportSender
(including TransportSecurityState) to set |content_type| appropriately.

BUG=650327
==========

to

==========
Add Content-Type header to net::ReportSender reports

Two sites that are opted into OCSP Expect-Staple reporting
(www.dropbox.com, www.caddyserver.com) reported that they were receiving
reports from Chrome with empty request bodies. This turned out to be
their logging code not handling requests with no Content-Type
header. When TransportSecurityState sends JSON violation reports for
HPKP and Expect-Staple, it ought to be sending a Content-Type header of
application/json. Thus, this CL adds a |content_type| parameter to
net::ReportSender::Send() and updates the users of net::ReportSender
(including TransportSecurityState) to set |content_type| appropriately.

BUG=650327
==========

[commit-bot: I haz the power, 2016-09-28 06:04:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-28 06:06:10]

Description was changed from

==========
Add Content-Type header to net::ReportSender reports

Two sites that are opted into OCSP Expect-Staple reporting
(www.dropbox.com, www.caddyserver.com) reported that they were receiving
reports from Chrome with empty request bodies. This turned out to be
their logging code not handling requests with no Content-Type
header. When TransportSecurityState sends JSON violation reports for
HPKP and Expect-Staple, it ought to be sending a Content-Type header of
application/json. Thus, this CL adds a |content_type| parameter to
net::ReportSender::Send() and updates the users of net::ReportSender
(including TransportSecurityState) to set |content_type| appropriately.

BUG=650327
==========

to

==========
Add Content-Type header to net::ReportSender reports

Two sites that are opted into OCSP Expect-Staple reporting
(www.dropbox.com, www.caddyserver.com) reported that they were receiving
reports from Chrome with empty request bodies. This turned out to be
their logging code not handling requests with no Content-Type
header. When TransportSecurityState sends JSON violation reports for
HPKP and Expect-Staple, it ought to be sending a Content-Type header of
application/json. Thus, this CL adds a |content_type| parameter to
net::ReportSender::Send() and updates the users of net::ReportSender
(including TransportSecurityState) to set |content_type| appropriately.

BUG=650327

Committed: https://crrev.com/b1716e2e3830aa031dc335b0403913efadb199dd
Cr-Commit-Position: refs/heads/master@{#421459}
==========

[commit-bot: I haz the power, 2016-09-28 06:06:11]

Patchset 5 (id:??) landed as
https://crrev.com/b1716e2e3830aa031dc335b0403913efadb199dd
Cr-Commit-Position: refs/heads/master@{#421459}