Page MenuHomePhabricator

Mobile-HTML: CSP header updates not appearing on recently re-rendered pages
Closed, ResolvedPublic

Description

In mobileapps commit c7e1cf9, an allowed connect-src value of app://*.wikimedia.org was added to the content security policy for mobile-html. However, the updated value is not appearing on recently rendered mobile-html responses:

BAD:

https://en.wikipedia.org/api/rest_v1/page/mobile-html/Edwin_Catmull
https://en.wikipedia.org/api/rest_v1/page/mobile-html/Tokyo
https://en.wikipedia.org/api/rest_v1/page/mobile-html/User_talk:MHolloway_%28WMF%29 (note: the correct value did appear initially for this URL, and then more recently disappeared)

(Aside: Where are the HTTP response headers such as CSP coming from when a response is served from RESTBase storage? Looking at https://github.com/wikimedia/restbase/blob/master/v1/pcs/stored_endpoint.js, it looks like only the response body from mobileapps is stored, not the headers.)

Event Timeline

It also depends on the location the request is made from. I don't even get app://*.wikimedia.org in connect-src for the first page in @Mholloway's user talk space: connect-src https://*.wikipedia.org;

Mholloway renamed this task from Mobile-HTML: CSP header updates appear on some recently rerendered pages and not others to Mobile-HTML: CSP header updates not appearing on recently re-rendered pages.Mar 30 2020, 12:30 PM
Mholloway updated the task description. (Show Details)

@dr0ptp4kt borderline UBN. Blocking apps release but there's still more beta testing we can do.

Thanks @JoeWalsh . @Pchelolo how should we route this? We believe this probably requires CPT treatment, although I wasn't sure there was a particular workflow we should follow - I didn't think it was necessarily appropriate for me to just assign it to you, although of course your name came to mind!

The problem is here: https://github.com/wikimedia/restbase/blob/master/lib/security_response_header_filter.js#L6

As a workaround we're indeed not storing the CSP for mobile stuff, thus for the responses obtained from the storage we copy-paste the headers. Before I said that storing the headers is not a correct way of doing things since updating the headers will require purging all the PCS content.. I still think that's the case.

This is another example why we need to make PCS manage it's own storage..

Here's a PR for the current problem: https://github.com/wikimedia/restbase/pull/1251

Thanks @Pchelolo. That's good to know. I agree it's best to avoid storing the CSP headers with each page and so we don't need to purge content when it needs to be updated. It's better to always send the latest and greatest for all pages.

So, basically we currently have 3 different places where one could think the production CSP headers for mobile-html are defined, but only the one in RB is the one used. These 3 places are:

Should we just get rid of the ones in mobileapps altogether as to avoid any confusion? Should we remove sending of any CSP?

Pchelolo claimed this task.

Should we just get rid of the ones in mobileapps altogether as to avoid any confusion? Should we remove sending of any CSP?

IMHO the one to be removed is RESTBase one, but that's the hardest to remove. We can only do it together with removing RESTBase as a whole from this.

Donno why I resolved it. Accident.

It's somewhat of a broader issue, but IMO the presence of config.prod.yaml is confusing almost to the point of being misleading, since it's never actually used in production. I wouldn't be opposed to removing it.

Mentioned in SAL (#wikimedia-operations) [2020-04-02T19:23:44Z] <ppchelko@deploy1001> Started deploy [restbase/deploy@7923c1f]: Update CSP headers for mobileapps T248431

Mentioned in SAL (#wikimedia-operations) [2020-04-02T19:38:57Z] <ppchelko@deploy1001> Finished deploy [restbase/deploy@7923c1f]: Update CSP headers for mobileapps T248431 (duration: 15m 13s)

I'm still seeing the old headers on some pages after the deploy. Is this expected?

I'm still seeing the old headers on some pages after the deploy. Is this expected?

It might still be service some stale data from Varnish, it's totally possible. You can check by verifying the 'age' header.

Thanks, it looks like that's it: Age: 268489

What remains to be done here? Where should the be on the board?