test-results: Get handler cleanup

test-results: Get handler cleanup

- Test wrapCallback
- Clean up an error check block in frontend/get.go
- renamed GetHandler -> getHandler

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/6b8aea58af01681d1ef9e445538ca7b7f27491ae

[nishanths, 2016-08-16 05:04:24]

nishanths@google.com changed reviewers:
+ estaab@chromium.org, martiniss@chromium.org, vadimsh@chromium.org

[estaab, 2016-08-16 15:33:23]

lgtm

https://codereview.chromium.org/2251623002/diff/1/go/src/infra/appengine/test...
File go/src/infra/appengine/test-results/frontend/get.go (left):

https://codereview.chromium.org/2251623002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/get.go:36: var callbackNameRx =
regexp.MustCompile(`^[A-Za-z0-9_]+$`)
What's the consequence of removing this? Any security concerns?

[nishanths, 2016-08-16 16:14:37]

On 2016/08/16 15:33:23, estaab wrote:
> lgtm
> 
>
https://codereview.chromium.org/2251623002/diff/1/go/src/infra/appengine/test...
> File go/src/infra/appengine/test-results/frontend/get.go (left):
> 
>
https://codereview.chromium.org/2251623002/diff/1/go/src/infra/appengine/test...
> go/src/infra/appengine/test-results/frontend/get.go:36: var callbackNameRx =
> regexp.MustCompile(`^[A-Za-z0-9_]+$`)
> What's the consequence of removing this? Any security concerns?

Removal reasons: 
* be consistent with the behavior in the GET /builders handler: 
 
https://chromium.googlesource.com/infra/infra/+/021639ccbe7b1bee56df9e82968c0...
* Valid JavaScript function names aren't restricted to that regex.

No security concerns from what I've read, will ask Vadim too.

[nishanths, 2016-08-16 16:15:13]

Description was changed from

==========
test-results: get handler cleanup

- do not check that the callback param matches callbackNameRx
- get_test.go: test wrapCallback
- clean up an error check block
- handlers: renamed GetHandler -> getHandler

BUG=
==========

to

==========
test-results: get handler cleanup

- do not check that the callback param matches callbackNameRx
  - be consistent with the behavior in the GET /builders handler: 
  - valid JavaScript function names aren't restricted to that regex.
- get_test.go: test wrapCallback
- clean up an error check block
- handlers: renamed GetHandler -> getHandler

BUG=
==========

[nishanths, 2016-08-16 23:19:16]

On 2016/08/16 16:14:37, nishanths wrote:
> On 2016/08/16 15:33:23, estaab wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2251623002/diff/1/go/src/infra/appengine/test...
> > File go/src/infra/appengine/test-results/frontend/get.go (left):
> > 
> >
>
https://codereview.chromium.org/2251623002/diff/1/go/src/infra/appengine/test...
> > go/src/infra/appengine/test-results/frontend/get.go:36: var callbackNameRx =
> > regexp.MustCompile(`^[A-Za-z0-9_]+$`)
> > What's the consequence of removing this? Any security concerns?
> 
> Removal reasons: 
> * be consistent with the behavior in the GET /builders handler: 
>  
>
https://chromium.googlesource.com/infra/infra/+/021639ccbe7b1bee56df9e82968c0...
> * Valid JavaScript function names aren't restricted to that regex.
> 
> No security concerns from what I've read, will ask Vadim too.

Spoke to Vadim. There is an XSS risk. 
For example, an attacker could make you click:
https://test-results.appspot.com/testfile?callback="evil_func(); foo"

Will re-add the regex check here and in the builders handler.

[nishanths, 2016-08-17 00:03:16]

Description was changed from

==========
test-results: get handler cleanup

- do not check that the callback param matches callbackNameRx
  - be consistent with the behavior in the GET /builders handler: 
  - valid JavaScript function names aren't restricted to that regex.
- get_test.go: test wrapCallback
- clean up an error check block
- handlers: renamed GetHandler -> getHandler

BUG=
==========

to

==========
test-results: Get handler cleanup

- Test wrapCallback
- Clean up an error check block in frontend/get.go
- renamed GetHandler -> getHandler

BUG=
==========

[nishanths, 2016-08-17 00:03:41]

The CQ bit was checked by nishanths@google.com

[nishanths, 2016-08-17 00:03:41]

The patchset sent to the CQ was uploaded after l-g-t-m from estaab@chromium.org
Link to the patchset: https://codereview.chromium.org/2251623002/#ps60001
(title: "Add callbackNameRx")

[commit-bot: I haz the power, 2016-08-17 00:03:50]

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-08-17 00:05:19]

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

[commit-bot: I haz the power, 2016-08-17 00:05:20]

Try jobs failed on following builders:
  Infra Linux Precise 32 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30aea15a89048610)
  Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30aea15a23f6f510)
  Infra Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30aea15d340b0810)

[nishanths, 2016-08-17 00:12:05]

The CQ bit was checked by nishanths@google.com

[nishanths, 2016-08-17 00:12:05]

The patchset sent to the CQ was uploaded after l-g-t-m from estaab@chromium.org
Link to the patchset: https://codereview.chromium.org/2251623002/#ps80001
(title: "(Rebase)")

[commit-bot: I haz the power, 2016-08-17 00:12:10]

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

[nishanths, 2016-08-17 00:22:35]

On 2016/08/17 00:05:20, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   Infra Linux Precise 32 Tester on luci.infra.try (JOB_FAILED,
> https://luci-milo.appspot.com/swarming/task/30aea15a89048610)
>   Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
> https://luci-milo.appspot.com/swarming/task/30aea15a23f6f510)
>   Infra Presubmit on luci.infra.try (JOB_FAILED,
> https://luci-milo.appspot.com/swarming/task/30aea15d340b0810)

Needed rebase.

[commit-bot: I haz the power, 2016-08-17 00:25:49]

Description was changed from

==========
test-results: Get handler cleanup

- Test wrapCallback
- Clean up an error check block in frontend/get.go
- renamed GetHandler -> getHandler

BUG=
==========

to

==========
test-results: Get handler cleanup

- Test wrapCallback
- Clean up an error check block in frontend/get.go
- renamed GetHandler -> getHandler

BUG=

Committed:
https://chromium.googlesource.com/infra/infra/+/6b8aea58af01681d1ef9e445538ca...
==========

[commit-bot: I haz the power, 2016-08-17 00:25:50]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/6b8aea58af01681d1ef9e445538ca...