1350909 - Make index.cgi cache-friendly for logged out requests

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, enhancement)

Description

[Dylan Hardison [:dylan] (he/him)]

The index page is pretty boring, it should be pretty easy to cache.

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1350909_1.patch

To do this, I need to be free of nonces. There are three CSP errors if I turn off nonces for that page. This patch fixes the first

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1350909_2.patch

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8851636 [details] [diff] [review]
1350909_2.patch

- expose encode_json to templates
- store BUGZILLA as data attribute on head, and load in new headers.js
- run index.cgi with no_yui
- only use a <script> in the head if passed javascript= into template or using YUI.

Comment 4

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1350909_remove_csp_nonce.patch

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1350909_add_cache_headers.patch

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8851786 [details] [diff] [review]
1350909_add_cache_headers.patch

a few problems I realized after spending even more time with caching:

1) max-age is not epoch seconds, just relative seconds so time() + is wrong.
2) tokens include IP addresses, we can't use those, so really we can only cache-private until I remove the rest of the login tokens out.
3) oh shit the CSRF protections from bug 1157395 while sufficient are not up to OWASP standards.

Comment 7

[:glob ✱]

Comment on attachment 8851786 [details] [diff] [review]
1350909_add_cache_headers.patch

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

::: index.cgi
@@ +41,2 @@
>  
> +my $weak_etag = q{W/"} . md5_hex(Bugzilla->user->id, Bugzilla->params->{bugzilla_version}) . q{"};

this etag isn't updated when dynamic content on the index page is updated (eg. custom lists in the footer, number of open requests in the header, email address, bug tagging, ...)

i think we can only set an etag for non-authenticated requests.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8851782 [details] [diff] [review]
1350909_remove_csp_nonce.patch

This is missing the #lob stuff -- just wasn't commited to the branch when I generated the diff, new one incoming

Comment 9

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1350909_remove_csp_nonce_2.patch

Comment 10

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1350909_add_cache_headers_2.patch

Comment 11

[:glob ✱]

Comment on attachment 8851986 [details] [diff] [review]
1350909_add_cache_headers_2.patch

clearing review as per discussions

Comment 12

[:glob ✱]

Comment on attachment 8851985 [details] [diff] [review]
1350909_remove_csp_nonce_2.patch

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

r=glob

::: js/global.js
@@ +47,5 @@
>  });
>  
> +function unhide_language_selector() {
> +    $('#lang_links_container').removeClass('bz_default_hidden');
> +}

as bmo is english only you could remove this.

::: template/en/default/global/header.html.tmpl
@@ +140,5 @@
> +        }
> +    };
> +    IF generate_api_token;
> +        js_BUGZILLA.api_token = get_api_token();
> +    END; 

nit: remove trailing spaces

@@ +210,5 @@
> +    [%# Make some Bugzilla information available to all scripts. 
> +      # We don't import every parameter and constant because we
> +      # don't want to add a lot of uncached JS to every page. 
> +      # %]
> +    

nit: remove trailing spaces

Comment 13

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1350909_add_cache_headers_3.patch

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8852468 [details] [diff] [review]
1350909_add_cache_headers_3.patch

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

::: index.cgi
@@ +16,4 @@
>  use Bugzilla::Constants;
>  use Bugzilla::Error;
>  use Bugzilla::Update;
> +use Bugzilla::Util qw(remote_ip);

ignore this, it won't be in the final version.

Comment 15

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1350909_add_cache_headers_4.patch

this time, minus the brain fart.

Comment 16

[:glob ✱]

Comment on attachment 8852474 [details] [diff] [review]
1350909_add_cache_headers_4.patch

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

::: index.cgi
@@ +26,5 @@
>  
> +# Yes, I really want to avoid two calls to the id method.
> +my $user_id = $user->id;
> +
> +# in the future caching logic might be a bit more complicated.

this comment doesn't add any value.  i would recommend instead stating that it's limited to logged out users due to the dynamic content in the header and footer.

@@ +50,3 @@
>  
> +# our weak etag is based on the bugzilla version parameter (BMO customization) and the announcehtml
> +# if either change, the cache will be considered invalid.

if my understanding of cache-control:public,max-age and etags is correct, the etag is only checked after the max-age for the cached item has expired.

https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/http-caching
"For example, in the above exchange, the server returns a 1024-byte response, instructs the client to cache it for up to 120 seconds, and provides a validation token ("x234dff") that can be used after the response has expired to check if the resource has been modified."

this likely isn't what we want, as it would mean changes could take up to 24 hours to take effect, and contradicts the comment.

maybe dropping the max-age down to 5 minutes would be a better balance between decreasing load and responsive updates.

Comment 17

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1350909_add_cache_headers_5.patch

Comment 18

[:glob ✱]

Comment on attachment 8852952 [details] [diff] [review]
1350909_add_cache_headers_5.patch

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

r=glob

Comment 19

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:mozilla-bteam/bmo.git
   30c35b3..8920743  master -> master

Comment 20

[Dylan Hardison [:dylan] (he/him)]

Sorry, more like:

To [e-mail]:mozilla-bteam/bmo.git
   5b80797..9f9e989  master -> master