665792 - Factor out header sending and parameter requiring

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Markus Stange [:mstange]]

The most important change here is to reject non-string values. We don't want anybody to submit stars with funny array values in the "who" field, for example.

Comment 1

[Markus Stange [:mstange]]

Attachment: v1

Comment 2

[Arpad Borsos [:Swatinem]]

Comment on attachment 540619 [details] [diff] [review]
v1

Review of attachment 540619 [details] [diff] [review]:
-----------------------------------------------------------------

You forgot to hg add inc/Communication.php :-D

Anyway, I think using 3 different functions is not that good, it does not make things any simpler. Just make that one function sendHeaders($asText, $allowCrossOrigin, $cache)

Or maybe make that first parameter $type a string with fallback to text/html, and make the json output text/javascript like it should be.

::: php/updateBuilders.php
@@ +20,3 @@
>  
> +sendContentTypeHeaders("text/plain");
> +sendLiberalAccessControlHeaders();

We don’t want that for updateBuilders. Imagine that one could just hide all the builds from a malicious site. We still use a password but I see no reason for this to be cross origin.

Comment 3

[Arpad Borsos [:Swatinem]]

Comment on attachment 540619 [details] [diff] [review]
v1

Review of attachment 540619 [details] [diff] [review]:
-----------------------------------------------------------------

::: php/submitBuildStar.php
@@ +2,5 @@
>  
> +require_once 'inc/Communication.php';
> +
> +sendContentTypeHeaders("text/plain");
> +sendLiberalAccessControlHeaders();

I don’t want this to be open either.

Comment 4

[Markus Stange [:mstange]]

Attachment: v2

(In reply to comment #2)
> You forgot to hg add inc/Communication.php :-D

Oops, minor oversight. :P

> Anyway, I think using 3 different functions is not that good, it does not
> make things any simpler. Just make that one function sendHeaders($asText,
> $allowCrossOrigin, $cache)

I've made it sendHeaders($flags, $mimeType). Passing multiple booleans isn't very readable from the caller's perspective because you need to look up the function definition in order to understand what the parameters mean.

> Or maybe make that first parameter $type a string with fallback to
> text/html

I made the fallback not send a content-type header at all because getLogExcerpt.php only wants allow_cross_origin and sends its own content-type header later.

> and make the json output text/javascript like it should be.

A quick google search told me that application/json is preferred over text/javascript, so I've used that.

Interestingly, I then had to remove to instances of JSON.parse in HiddenBuildsAdminUI.js because jQuery.ajax started to do the parsing automatically (due to the right mime type).

> ::: php/updateBuilders.php
> @@ +20,3 @@
> >  
> > +sendContentTypeHeaders("text/plain");
> > +sendLiberalAccessControlHeaders();
> 
> We don’t want that for updateBuilders. Imagine that one could just hide all
> the builds from a malicious site.

Not sending the header here wouldn't make a difference: If the browser wants to know the header's value, it needs to send the request first, and by then the damage is already done. This header only prevents the result to be sent back to the XHR. It doesn't block the other direction.

Also, the malicious website could just send the same request from its server. Sending it from the client doesn't give it any elevated privileges because we don't use cookies anywhere.

> We still use a password but I see no
> reason for this to be cross origin.

Somebody might want to run his own TBPL client somewhere (e.g. to hack on the UI) but still want to use it with the tbpl.mozilla.org server side (using Config.baseURL). Why should we disallow that?

Comment 5

[Arpad Borsos [:Swatinem]]

Comment on attachment 540742 [details] [diff] [review]
v2

Review of attachment 540742 [details] [diff] [review]:
-----------------------------------------------------------------

> A quick google search told me that application/json is preferred over
> text/javascript, so I've used that.

You are right, I just don’t like it because of bug 606488 (it is offered as download instead of displayed in the browser).
But we should do it the right way instead of working around a browser bug.

> Interestingly, I then had to remove to instances of JSON.parse in
> HiddenBuildsAdminUI.js because jQuery.ajax started to do the parsing
> automatically (due to the right mime type).

Very good side effect :-)

> Not sending the header here wouldn't make a difference: If the browser wants
> to know the header's value, it needs to send the request first, and by then
> the damage is already done. This header only prevents the result to be sent
> back to the XHR. It doesn't block the other direction.

WHAT? I mean thats what the HEAD method is for, isn’t it? But yes, I remember PHP still executes the whole script, so maybe we can just exit() if we detect a HEAD request?

> Also, the malicious website could just send the same request from its
> server. Sending it from the client doesn't give it any elevated privileges
> because we don't use cookies anywhere.

Yes, unfortunately we are inherently open to spam :-(

So r+ with the missing case fixed and Leading-case Headers class, I don’t have a strong preference about the other issues.

::: php/getParsedLog.php
@@ +9,5 @@
>  require_once 'inc/GzipUtils.php';
>  require_once 'inc/RunForLog.php';
> +require_once 'inc/Communication.php';
> +
> +sendLiberalAccessControlHeaders();

you forgot to update this one.

::: php/inc/Communication.php
@@ +1,5 @@
> +<?php
> +/* -*- Mode: PHP; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set sw=2 ts=2 et tw=80 : */
> +
> +class HEADERS {

I like it better if classes are not uppercase, even if they just contain const members.
But now that I think about it: why not make that a static Headers::send() method?

@@ +8,5 @@
> +}
> +
> +function sendHeaders($flags = 0, $mimeType = "") {
> +  if ($mimeType)
> +    header("Content-Type: {$mimeType}, charset=utf-8");

I’m not sure if defaulting to text/html would be a better idea still, to make sure we send the correct charset, I think the php default is not utf-8.

Comment 6

[Markus Stange [:mstange]]

Attachment: v3

(In reply to comment #5)
> > A quick google search told me that application/json is preferred over
> > text/javascript, so I've used that.
> 
> You are right, I just don’t like it because of bug 606488 (it is offered as
> download instead of displayed in the browser).

Yeah, I've noticed that, too.

> > Not sending the header here wouldn't make a difference: If the browser wants
> > to know the header's value, it needs to send the request first, and by then
> > the damage is already done. This header only prevents the result to be sent
> > back to the XHR. It doesn't block the other direction.
> 
> WHAT? I mean thats what the HEAD method is for, isn’t it?

https://developer.mozilla.org/en/HTTP_access_control says something about an OPTIONS header that is used for this scenario, but it's not kicking in for us because we're using simple POST requests... or something like that.

> But yes, I
> remember PHP still executes the whole script, so maybe we can just exit() if
> we detect a HEAD request?

Maybe, I don't know. But I don't think it's worth doing it.
Conceptually, I see tbpl.mozilla.org/php/ as the authoritative data provider, and all TBPL clients as equally-valued consumers of that data, no matter if hosted on tbpl.mozilla.org or elsewhere.

> ::: php/inc/Communication.php
> @@ +1,5 @@
> > +<?php
> > +/* -*- Mode: PHP; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* vim: set sw=2 ts=2 et tw=80 : */
> > +
> > +class HEADERS {
> 
> I like it better if classes are not uppercase, even if they just contain
> const members.
> But now that I think about it: why not make that a static Headers::send()
> method?

Good idea, done. Let's hope that people won't go looking for php/inc/Headers.php after reading Headers::send(...).

> @@ +8,5 @@
> > +}
> > +
> > +function sendHeaders($flags = 0, $mimeType = "") {
> > +  if ($mimeType)
> > +    header("Content-Type: {$mimeType}, charset=utf-8");
> 
> I’m not sure if defaulting to text/html would be a better idea still, to
> make sure we send the correct charset, I think the php default is not utf-8.

OK.

Comment 7

[Arpad Borsos [:Swatinem]]

Comment on attachment 540765 [details] [diff] [review]
v3

Review of attachment 540765 [details] [diff] [review]:
-----------------------------------------------------------------

::: php/getHiddenBuilderNames.php
@@ +10,4 @@
>  
> +Headers::send(Headers::ALLOW_CROSS_ORIGIN | Headers::NO_CACHE, "application/json");
> +$branch = requireStringParameter('branch', $_GET);
> +echo json_encode(getHiddenBuilderNames($branch)) . "\n";

Some whitespace would be good.