Update callers of httputils.ReportError to specify message

Update callers of httputils.ReportError to specify message. Use "Unknown error" when message is not specified so that the 
error toast is never blank.

Context is in https://bugs.chromium.org/p/chromium/issues/detail?id=596183#c9

BUG=chromium:596183
Committed: https://skia.googlesource.com/buildbot/+/ace4c3c09b2c97d0fcf045d727473665348e4bf8

[rmistry, 2016-03-21 14:04:41]

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

[commit-bot: I haz the power, 2016-03-21 14:04:44]

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

[commit-bot: I haz the power, 2016-03-21 14:07:34]

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

[commit-bot: I haz the power, 2016-03-21 14:07:35]

Dry run: This issue passed the CQ dry run.

[rmistry, 2016-03-21 14:08:09]

rmistry@google.com changed reviewers:
+ stephana@google.com

[rmistry, 2016-03-21 14:13:23]

rmistry@google.com changed reviewers:
+ jcgregorio@google.com

[jcgregorio, 2016-03-21 14:46:44]

On 2016/03/21 at 14:13:23, rmistry wrote:
> 

The point of the security bug was that putting the 'err' in the response was a
security hole. For example, when you pass a string to strconv.Atoi() the error
message includes the text that was passed in, ie. "I couldn't parse: blah". This
is a security concern since the user could pass in text for the integer value,
for example "alert()", and with enough escaping could get a browser to execute
the result in the user's context.

I think a better route would be to simply return "Unknown error" if the message
is the empty string and then try to find all the places where message isn't
supplied and actually fill in messages.

[rmistry, 2016-03-21 14:48:07]

On 2016/03/21 14:46:44, jcgregorio wrote:
> On 2016/03/21 at 14:13:23, rmistry wrote:
> > 
> 
> The point of the security bug was that putting the 'err' in the response was a
> security hole. For example, when you pass a string to strconv.Atoi() the error
> message includes the text that was passed in, ie. "I couldn't parse: blah".
This
> is a security concern since the user could pass in text for the integer value,
> for example "alert()", and with enough escaping could get a browser to execute
> the result in the user's context.
> 
> I think a better route would be to simply return "Unknown error" if the
message
> is the empty string and then try to find all the places where message isn't
> supplied and actually fill in messages.

Ah k. I will go that route then, will update this CL soon.

[rmistry, 2016-03-21 18:01:12]

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

[commit-bot: I haz the power, 2016-03-21 18:01:32]

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

[commit-bot: I haz the power, 2016-03-21 18:04:22]

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

[commit-bot: I haz the power, 2016-03-21 18:04:22]

Dry run: This issue passed the CQ dry run.

[rmistry, 2016-03-21 18:05:01]

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

[commit-bot: I haz the power, 2016-03-21 18:05:12]

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

[commit-bot: I haz the power, 2016-03-21 18:08:02]

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

[commit-bot: I haz the power, 2016-03-21 18:08:03]

Dry run: This issue passed the CQ dry run.

[rmistry, 2016-03-21 18:10:07]

Description was changed from

==========
Use error string when message not specified

Context is in https://bugs.chromium.org/p/chromium/issues/detail?id=596183#c9

BUG=chromium:596183
==========

to

==========
Update callers of httputils.ReportError to specify message. Use "Unknown error"
when message is not specified so that the 
error toast is never blank.

Context is in https://bugs.chromium.org/p/chromium/issues/detail?id=596183#c9

BUG=chromium:596183
==========

[rmistry, 2016-03-21 18:10:41]

rmistry@google.com changed reviewers:
+ benjaminwagner@google.com

[rmistry, 2016-03-21 18:10:41]

PTAL. Also added Ben for CTFE sanity check.

[jcgregorio, 2016-03-21 18:15:39]

lgtm

[dogben, 2016-03-21 18:19:00]

nit: rather than duplicating message in fmt.Errorf and message, just pass nil
for err.

[dogben, 2016-03-21 18:22:30]

On 2016/03/21 at 18:19:00, Ben Wagner wrote:
> nit: rather than duplicating message in fmt.Errorf and message, just pass nil
for err.

(Just to clarify: For cases like the following, I think we want to do:
- httputils.ReportError(w, r, fmt.Errorf("No revision specified"), "")
+ httputils.ReportError(w, r, nil, "No revision specified")

OTOH, for cases like the following, I think we want to do exactly what you did
in this CL:
- httputils.ReportError(w, r, err, "")
+ httputils.ReportError(w, r, err, "Failed to get CL patch")

Sorry if it was unclear.)

[rmistry, 2016-03-21 18:36:02]

On 2016/03/21 18:19:00, Ben Wagner wrote:
> nit: rather than duplicating message in fmt.Errorf and message, just pass nil
> for err.

Done. PTAL.

[rmistry, 2016-03-21 18:36:07]

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

[commit-bot: I haz the power, 2016-03-21 18:36:10]

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

[commit-bot: I haz the power, 2016-03-21 18:39:00]

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

[commit-bot: I haz the power, 2016-03-21 18:39:01]

Dry run: This issue passed the CQ dry run.

[dogben, 2016-03-21 18:54:39]

One comment, otherwise LGTM.

https://codereview.chromium.org/1820863002/diff/120001/ct/go/ctfe/task_common...
File ct/go/ctfe/task_common/task_common.go (right):

https://codereview.chromium.org/1820863002/diff/120001/ct/go/ctfe/task_common...
ct/go/ctfe/task_common/task_common.go:218: httputils.ReportError(w, r, nil,
fmt.Sprintf("Could not find %s task with ID %s", prototype.GetTaskName(),
taskID))
taskID is not sanitized.

Same below.

[rmistry, 2016-03-21 18:56:42]

https://codereview.chromium.org/1820863002/diff/120001/ct/go/ctfe/task_common...
File ct/go/ctfe/task_common/task_common.go (right):

https://codereview.chromium.org/1820863002/diff/120001/ct/go/ctfe/task_common...
ct/go/ctfe/task_common/task_common.go:218: httputils.ReportError(w, r, nil,
fmt.Sprintf("Could not find %s task with ID %s", prototype.GetTaskName(),
taskID))
On 2016/03/21 18:54:38, Ben Wagner wrote:
> taskID is not sanitized.
> 
> Same below.

What do you mean by sanitized?

[dogben, 2016-03-21 19:00:28]

On 2016/03/21 at 18:56:42, rmistry wrote:
>
https://codereview.chromium.org/1820863002/diff/120001/ct/go/ctfe/task_common...
> File ct/go/ctfe/task_common/task_common.go (right):
> 
>
https://codereview.chromium.org/1820863002/diff/120001/ct/go/ctfe/task_common...
> ct/go/ctfe/task_common/task_common.go:218: httputils.ReportError(w, r, nil,
fmt.Sprintf("Could not find %s task with ID %s", prototype.GetTaskName(),
taskID))
> On 2016/03/21 18:54:38, Ben Wagner wrote:
> > taskID is not sanitized.
> > 
> > Same below.
> 
> What do you mean by sanitized?

taskID is provided by a URL param, so if it's echo'd back without sanitization,
it can be used in an XSS attack.

[rmistry, 2016-03-21 19:04:03]

On 2016/03/21 19:00:28, Ben Wagner wrote:
> On 2016/03/21 at 18:56:42, rmistry wrote:
> >
>
https://codereview.chromium.org/1820863002/diff/120001/ct/go/ctfe/task_common...
> > File ct/go/ctfe/task_common/task_common.go (right):
> > 
> >
>
https://codereview.chromium.org/1820863002/diff/120001/ct/go/ctfe/task_common...
> > ct/go/ctfe/task_common/task_common.go:218: httputils.ReportError(w, r, nil,
> fmt.Sprintf("Could not find %s task with ID %s", prototype.GetTaskName(),
> taskID))
> > On 2016/03/21 18:54:38, Ben Wagner wrote:
> > > taskID is not sanitized.
> > > 
> > > Same below.
> > 
> > What do you mean by sanitized?
> 
> taskID is provided by a URL param, so if it's echo'd back without
sanitization,
> it can be used in an XSS attack.

Sanitized it by removing it :)

[dogben, 2016-03-21 19:33:36]

lgtm

[rmistry, 2016-03-21 19:46:59]

The CQ bit was checked by rmistry@google.com

[rmistry, 2016-03-21 19:47:00]

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/1820863002/#ps140001
(title: "Remove taskID")

[commit-bot: I haz the power, 2016-03-21 19:47:02]

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

[commit-bot: I haz the power, 2016-03-21 19:53:14]

Description was changed from

==========
Update callers of httputils.ReportError to specify message. Use "Unknown error"
when message is not specified so that the 
error toast is never blank.

Context is in https://bugs.chromium.org/p/chromium/issues/detail?id=596183#c9

BUG=chromium:596183
==========

to

==========
Update callers of httputils.ReportError to specify message. Use "Unknown error"
when message is not specified so that the 
error toast is never blank.

Context is in https://bugs.chromium.org/p/chromium/issues/detail?id=596183#c9

BUG=chromium:596183

Committed:
https://skia.googlesource.com/buildbot/+/ace4c3c09b2c97d0fcf045d727473665348e...
==========

[commit-bot: I haz the power, 2016-03-21 19:53:16]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/buildbot/+/ace4c3c09b2c97d0fcf045d727473665348e...