Page MenuHomePhabricator

MobileFrontendHooksTest::testOnBeforePageDisplay failing after security patches
Closed, ResolvedPublic

Description

The security patches was merged right now, but now the MobileFrontendHooksTest::testOnBeforePageDisplay test set is failing:

18:08:13 There were 9 failures:
18:08:13 
18:08:13 1) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #0 (true, true, true, true, true, array('a', '502-13'), 1, true, false)
18:08:13 in desktop view the cookie vary header should never be set
18:08:13 Failed asserting that true matches expected false.
18:08:13 
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:354
18:08:13 
18:08:13 2) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #1 (true, false, true, false, false, array('a', '502-13'), 0, true, false)
18:08:13 in desktop view the cookie vary header should never be set
18:08:13 Failed asserting that true matches expected false.
18:08:13 
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:354
18:08:13 
18:08:13 3) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #2 (false, true, true, true, true, array('a', '502-13'), 0, true, true)
18:08:13 in desktop view the cookie vary header should never be set
18:08:13 Failed asserting that true matches expected false.
18:08:13 
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:354
18:08:13 
18:08:13 4) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #3 (false, false, true, false, false, array('a', '502-13'), 0, true, false)
18:08:13 in desktop view the cookie vary header should never be set
18:08:13 Failed asserting that true matches expected false.
18:08:13 
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:354
18:08:13 
18:08:13 5) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #4 (true, true, false, true, true, array(), 1, false, false)
18:08:13 in desktop view the cookie vary header should never be set
18:08:13 Failed asserting that true matches expected false.
18:08:13 
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:354
18:08:13 
18:08:13 6) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #5 (true, false, false, false, false, array(), 0, false, false)
18:08:13 in desktop view the cookie vary header should never be set
18:08:13 Failed asserting that true matches expected false.
18:08:13 
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:354
18:08:13 
18:08:13 7) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #6 (false, true, false, true, true, array(), 0, false, true)
18:08:13 in desktop view the cookie vary header should never be set
18:08:13 Failed asserting that true matches expected false.
18:08:13 
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:354
18:08:13 
18:08:13 8) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #7 (false, false, false, false, false, array(), 0, false, false)
18:08:13 in desktop view the cookie vary header should never be set
18:08:13 Failed asserting that true matches expected false.
18:08:13 
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
18:08:13 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:354
18:08:13 
18:08:13 9) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #8 (false, false, false, false, true, array(), 0, false, false)
18:08:13 in desktop view the cookie vary header should never be set
18:08:13 Failed asserting that true matches expected false.

Possible related rMWaf3b10287e4b4de8360ea57c400c7b8ed71596a1

This was broken by

SECURITY: RawAction: Vary on the usual headers

This avoids edge cases where the user isn't logged in but we still need
varying for proper cache behavior.

Bug: T125283

Event Timeline

Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptMay 20 2016, 6:28 PM
Umherirrender triaged this task as Unbreak Now! priority.EditedMay 20 2016, 6:32 PM

This breaks all jenkins tests and merge of patch sets on mediawiki/core master

Restricted Application added subscribers: Luke081515, TerraCodes, Urbanecm. · View Herald TranscriptMay 20 2016, 6:32 PM
demon updated the task description. (Show Details)May 20 2016, 6:33 PM
demon updated the task description. (Show Details)May 20 2016, 6:37 PM

Change 289904 had a related patch set uploaded (by Gergő Tisza):
Disable MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/289904

Change 289904 merged by jenkins-bot:
Disable MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/289904

Umherirrender lowered the priority of this task from Unbreak Now! to Needs Triage.May 20 2016, 9:34 PM
Umherirrender removed a project: Patch-For-Review.
Tgr triaged this task as Normal priority.May 20 2016, 10:12 PM
Tgr added a subscriber: Tgr.

Leaving this open in case Readers-Web-Backlog wants to use this task to track fixing and reenabling of the test.

Tgr raised the priority of this task from Normal to Needs Triage.May 20 2016, 10:13 PM
Jdlrobson triaged this task as High priority.May 23 2016, 6:14 PM

I'm baffled @Tgr @Florian. Didn't this patch go out with a new release of MediaWiki ? So does that mean we shipped something with a broken MobileFrontend test? I would hope a broken test would block a release, so if this isn't the case why hasn't it done so?

The decision to disable the test blindly without understanding the test and what it protected is in my opinion a lazy and ill-made decision.

cc @BBlack the test "in desktop view the cookie vary header should never be set" is concerning as this could cause us all sorts of caching issues if true. Hopefully it's a false positive but we may want to stop the train until we can validate that.

@Jdlrobson thanks for the heads-up, and yes, that's a very serious issue. Vary must unconditionally be sent with the same value for any given URI. Varying a Vary header is a recipe for disaster.

(Also, why does MFE mess with Vary:Cookie? that should be a core thing, right?)

demon added a comment.May 23 2016, 6:48 PM

I'm baffled @Tgr @Florian. Didn't this patch go out with a new release of MediaWiki ? So does that mean we shipped something with a broken MobileFrontend test? I would hope a broken test would block a release, so if this isn't the case why hasn't it done so?

MobileFrontend is not bundled with the MW tarballs, so we didn't ship a broken anything.

Jdlrobson added a comment.EditedMay 23 2016, 6:53 PM

@demon but we cut extension branches at the same time no? So if someone upgrades to mediawiki with MobileFrontend they could potentially have this issue?

@BBlack am going to look into this in the afternoon to see if something has indeed been broken. It messes with the vary cookie for sites which do not have a mobile site url but use MobileFrontend. It should only apply for 3rd parties so am a little worried that may have gone wrong. Will update as soon as I know more!

demon added a comment.May 23 2016, 7:19 PM

@demon but we cut extension branches at the same time no? So if someone upgrades to mediawiki with MobileFrontend they could potentially have this issue?

This was a security release so no new branches were cut.

okay thanks for clarifying @demon !

@Jdlrobson thanks for the heads-up, and yes, that's a very serious issue. Vary must unconditionally be sent with the same value for any given URI. Varying a Vary header is a recipe for disaster.

It seems https://github.com/wikimedia/mediawiki/commit/af3b10287e4b4de8360ea57c400c7b8ed71596a1 adds the Cookie Vary Header unconditionally if getCacheVaryCookies returns something. Thus, thankfully this does appear to be a false positive and it is my opinion that it can be safely removed.

(The above assumes that the fix to T125283 is sane)

Change 290285 had a related patch set uploaded (by Jdlrobson):
Revert "Disable MobileFrontendHooksTest::testOnBeforePageDisplay"

https://gerrit.wikimedia.org/r/290285

Tgr added a comment.May 23 2016, 9:47 PM

As I said in the commit summary,

The test tries to assert that no Vary headers are set in OutputPage,
which is not something a test should be doing without lots of mocking
(OutputPage is not isolated very well and MobileFrontend is certainly
not the only component messing with headers).

It was clear the test was wrong; it wasn't clear what would be right (does MF add a vary header under certain circumstances and was the test asserting that that does not happen under certain other circumstances?); it was breaking CI and blocking the work of all WMF developers, so something had to be done about it. I'm not sure what else could have been done differently. The point of UBN bugs is that they should be handled fast (and ideally by the team who owns the component - I tried to call attention to it on #wikimedia-mobile, but might have been too subtle about it). Even if I wasn't sure the test is a false positive (and I was - maybe the commit summary did not express that clearly), whether it is significant enough to make the issue a release blocker is not a call that I can make, with zero knowledge of the product. That should be done by someone familiar with MF.

The timing of your reply suggests that the web team only became aware of this issue at the weekly triage meeting. If you are looking for improvements to the process, IMO that's the place to start. There should be at least one person in Reading Web (but preferably more, to account for vacations) who gets notifications about UBN bugs. You can set up a Herald rule to do that.

As I said in the commit summary,

The test tries to assert that no Vary headers are set in OutputPage,
which is not something a test should be doing without lots of mocking
(OutputPage is not isolated very well and MobileFrontend is certainly
not the only component messing with headers).

It was clear the test was wrong; it wasn't clear what would be right (does MF add a vary header under certain circumstances and was the test asserting that that does not happen under certain other circumstances?); it was breaking CI and blocking the work of all WMF developers, so something had to be done about it. I'm not sure what else could have been done differently. The point of UBN bugs is that they should be handled fast (and ideally by the team who owns the component - I tried to call attention to it on #wikimedia-mobile, but might have been too subtle about it). Even if I wasn't sure the test is a false positive (and I was - maybe the commit summary did not express that clearly), whether it is significant enough to make the issue a release blocker is not a call that I can make, with zero knowledge of the product. That should be done by someone familiar with MF.

The timing of your reply suggests that the web team only became aware of this issue at the weekly triage meeting. If you are looking for improvements to the process, IMO that's the place to start. There should be at least one person in Reading Web (but preferably more, to account for vacations) who gets notifications about UBN bugs. You can set up a Herald rule to do that.

We have a triage every morning except Friday which is arguably a bad day to be pushing potentially breaking changes. I was personally at an event Friday. I'd suggest contacting Adam in future for anything urgent like this.

Anyway, yes it seems like there is nothing to worry about in this particular instance. The original revert hadn't given me the confidence I needed to but it seems you handled this affair correctly so thank you for taking care of it. Hopefully rolling this out won't prove otherwise due to some badly undocumented piece of MobileFrontend code :)

Change 290496 had a related patch set uploaded (by Gergő Tisza):
Remove Vary header check from MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/290496

Change 290285 merged by jenkins-bot:
Revert "Disable MobileFrontendHooksTest::testOnBeforePageDisplay"

https://gerrit.wikimedia.org/r/290285

Change 290496 merged by jenkins-bot:
Remove Vary header check from MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/290496

Change 290579 had a related patch set uploaded (by Gergő Tisza):
Remove Vary header check from MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/290579

Change 290579 merged by Brian Wolff:
Remove Vary header check from MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/290579