Fix issue security issue in Gold

Fix issue security issue in Gold

To reliably fix internal b/26768421 (not just in gold) we need to wrap the error response in JSON object instead of returning a string.

BUG=skia:

Committed: https://skia.googlesource.com/buildbot/+/6753a5174a9a78020a328298a4fd45507090bfc3

[stephana, 2016-02-16 22:45:19]

The CQ bit was checked by stephana@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-16 22:45:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1704713002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1704713002/1

[stephana, 2016-02-16 22:48:33]

Description was changed from

==========
Fix issue

BUG=skia:
==========

to

==========
Fix issue security issue in Gold

To reliably fix this issue (not just in gold)

https://b.corp.google.com/u/0/issues/26768421

we need to wrap the error response in JSON object instead
of returning a string.

BUG=skia:
==========

[stephana, 2016-02-16 22:48:33]

stephana@google.com changed reviewers:
+ benjaminwagner@google.com, jcgregorio@google.com

[commit-bot: I haz the power, 2016-02-16 22:49:52]

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

[commit-bot: I haz the power, 2016-02-16 22:49:53]

Dry run: Try jobs failed on following builders:
  Infra-PerCommit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/b...)

[stephana, 2016-02-16 22:50:24]

PTAL: I cannot think of a better way to address this issue in general. I will
change all the necessary code to switch to the new format for error messages
once we agree on a solution.

[dogben, 2016-02-16 23:29:26]

Description was changed from

==========
Fix issue security issue in Gold

To reliably fix this issue (not just in gold)

https://b.corp.google.com/u/0/issues/26768421

we need to wrap the error response in JSON object instead
of returning a string.

BUG=skia:
==========

to

==========
Fix issue security issue in Gold

To reliably fix internal b/26768421 (not just in gold) we need to wrap the error
response in JSON object instead of returning a string.

BUG=skia:
==========

[dogben, 2016-02-16 23:49:50]

I'm not sure that JSON responses are safe either. See the link in the bug "(1)
If it is JSON:".

[stephana, 2016-02-17 03:15:03]

On 2016/02/16 23:49:50, Ben Wagner wrote:
> I'm not sure that JSON responses are safe either. See the link in the bug "(1)
> If it is JSON:".

If you look at link(1), it suggests to never return content type "text/plain"
which is exactly what we are currently doing. 
IMO, by placing the error message in a JSON string it should be automatically
escaped, but I'll investigate more. 

In the meantime I added all the header suggested by the link.

[jcgregorio, 2016-02-17 14:41:19]

On 2016/02/17 at 03:15:03, stephana wrote:
> On 2016/02/16 23:49:50, Ben Wagner wrote:
> > I'm not sure that JSON responses are safe either. See the link in the bug
"(1)
> > If it is JSON:".
> 
> If you look at link(1), it suggests to never return content type "text/plain"
which is exactly what we are currently doing. 
> IMO, by placing the error message in a JSON string it should be automatically
escaped, but I'll investigate more. 
> 
> In the meantime I added all the header suggested by the link.

The root of the problem is that the user input (query params, form values, etc)
are being reflected back to the browser when reporting the error. The simplest
fix would be to have ReportError only return the message in the HTTP response
and glog.Error the 'err'.

[stephana, 2016-02-17 15:21:25]

On 2016/02/17 14:41:19, jcgregorio wrote:
> On 2016/02/17 at 03:15:03, stephana wrote:
> > On 2016/02/16 23:49:50, Ben Wagner wrote:
> > > I'm not sure that JSON responses are safe either. See the link in the bug
> "(1)
> > > If it is JSON:".
> > 
> > If you look at link(1), it suggests to never return content type
"text/plain"
> which is exactly what we are currently doing. 
> > IMO, by placing the error message in a JSON string it should be
automatically
> escaped, but I'll investigate more. 
> > 
> > In the meantime I added all the header suggested by the link.
> 
> The root of the problem is that the user input (query params, form values,
etc)
> are being reflected back to the browser when reporting the error. The simplest
> fix would be to have ReportError only return the message in the HTTP response
> and glog.Error the 'err'.

I changed the behavior of ReportError, but that's not a good long term fix IMO.
I filed a bug to improve input validation and rethink the content types we
return. 
https://bug.skia.org/4961

[stephana, 2016-02-17 15:21:30]

On 2016/02/17 14:41:19, jcgregorio wrote:
> On 2016/02/17 at 03:15:03, stephana wrote:
> > On 2016/02/16 23:49:50, Ben Wagner wrote:
> > > I'm not sure that JSON responses are safe either. See the link in the bug
> "(1)
> > > If it is JSON:".
> > 
> > If you look at link(1), it suggests to never return content type
"text/plain"
> which is exactly what we are currently doing. 
> > IMO, by placing the error message in a JSON string it should be
automatically
> escaped, but I'll investigate more. 
> > 
> > In the meantime I added all the header suggested by the link.
> 
> The root of the problem is that the user input (query params, form values,
etc)
> are being reflected back to the browser when reporting the error. The simplest
> fix would be to have ReportError only return the message in the HTTP response
> and glog.Error the 'err'.

I changed the behavior of ReportError, but that's not a good long term fix IMO.
I filed a bug to improve input validation and rethink the content types we
return. 
https://bug.skia.org/4961

[jcgregorio, 2016-02-17 15:23:00]

lgtm

[stephana, 2016-02-17 21:06:08]

Description was changed from

==========
Fix issue security issue in Gold

To reliably fix internal b/26768421 (not just in gold) we need to wrap the error
response in JSON object instead of returning a string.

BUG=skia:
==========

to

==========
Fix issue security issue in Gold

To reliably fix internal b/26768421 (not just in gold) we need to wrap the error
response in JSON object instead of returning a string.

BUG=skia:
==========

[stephana, 2016-02-17 21:06:15]

The CQ bit was checked by stephana@google.com

[commit-bot: I haz the power, 2016-02-17 21:06:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1704713002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1704713002/40001

[commit-bot: I haz the power, 2016-02-17 21:12:27]

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

[commit-bot: I haz the power, 2016-02-17 21:12:28]

Try jobs failed on following builders:
  Infra-PerCommit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/b...)

[stephana, 2016-02-18 14:45:52]

The CQ bit was checked by stephana@google.com

[stephana, 2016-02-18 14:45:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
jcgregorio@google.com
Link to the patchset: https://codereview.chromium.org/1704713002/#ps60001
(title: "Fixed poller_test")

[commit-bot: I haz the power, 2016-02-18 14:46:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1704713002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1704713002/60001

[commit-bot: I haz the power, 2016-02-18 14:52:29]

Description was changed from

==========
Fix issue security issue in Gold

To reliably fix internal b/26768421 (not just in gold) we need to wrap the error
response in JSON object instead of returning a string.

BUG=skia:
==========

to

==========
Fix issue security issue in Gold

To reliably fix internal b/26768421 (not just in gold) we need to wrap the error
response in JSON object instead of returning a string.

BUG=skia:

Committed:
https://skia.googlesource.com/buildbot/+/6753a5174a9a78020a328298a4fd45507090...
==========

[commit-bot: I haz the power, 2016-02-18 14:52:30]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/buildbot/+/6753a5174a9a78020a328298a4fd45507090...