From ce1bd86a4b3f563d6407a2a960211b3dcaca70ac Mon Sep 17 00:00:00 2001 From: "C. Scott Ananian" Date: Wed, 19 Jun 2019 14:22:42 -0400 Subject: [PATCH] Remove $wgUseKeyHeader and OutputPage::getKeyHeader(), deprecated in 1.32 These implemented a since-abandoned draft IETF spec, and the code was broken due to (1) case-(in)sensitivity issues with the Accept-Language header and (2) the BCP47 language code compatibility workaround we use. Change-Id: Ia53d07cd8ce8ab1497294ea244c13c7499f632c7 --- RELEASE-NOTES-1.34 | 2 + includes/DefaultSettings.php | 8 -- includes/OutputHandler.php | 4 - includes/OutputPage.php | 91 ++++------------------- includes/actions/RawAction.php | 3 - includes/api/ApiMain.php | 16 +--- includes/session/SessionProvider.php | 3 + tests/phpunit/includes/OutputPageTest.php | 78 ++++++++----------- 8 files changed, 53 insertions(+), 152 deletions(-) diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index 0e4081cfe5..2f656b8312 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -214,6 +214,8 @@ because of Phabricator reports. * OutputPage::addWikiText(), ::addWikiTextWithTitle(), ::addWikiTextTitleTidy(), ::addWikiTextTidy(), ::addWikiTextTitle(), deprecated in 1.32, have been removed. +* The $wgUseKeyHeader configuration option and the OutputPage::getKeyHeader() + method, deprecated in 1.32, have been removed. * … === Deprecations in 1.34 === diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 2f793b5857..c5a2f77782 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -2737,14 +2737,6 @@ $wgUseCdn = false; */ $wgUseESI = false; -/** - * Send the Key HTTP header for better caching. - * See https://datatracker.ietf.org/doc/draft-ietf-httpbis-key/ for details. - * @since 1.27 - * @deprecated in 1.32, the IETF spec expired without becoming a standard. - */ -$wgUseKeyHeader = false; - /** * Add X-Forwarded-Proto to the Vary and Key headers for API requests and * RSS/Atom feeds. Use this if you have an SSL termination setup diff --git a/includes/OutputHandler.php b/includes/OutputHandler.php index ba9e2d7a2c..1d1a193cbe 100644 --- a/includes/OutputHandler.php +++ b/includes/OutputHandler.php @@ -123,10 +123,6 @@ class OutputHandler { } if ( !$foundVary ) { header( 'Vary: Accept-Encoding' ); - global $wgUseKeyHeader; - if ( $wgUseKeyHeader ) { - header( 'Key: Accept-Encoding;match=gzip' ); - } } return $s; } diff --git a/includes/OutputPage.php b/includes/OutputPage.php index 95073c0a4c..fba75ecf50 100644 --- a/includes/OutputPage.php +++ b/includes/OutputPage.php @@ -265,11 +265,12 @@ class OutputPage extends ContextSource { private $mFollowPolicy = 'follow'; /** - * @var array Headers that cause the cache to vary. Key is header name, value is an array of - * options for the Key header. + * @var array Headers that cause the cache to vary. Key is header name, + * value should always be null. (Value was an array of options for + * the `Key` header, which was deprecated in 1.32 and removed in 1.34.) */ private $mVaryHeader = [ - 'Accept-Encoding' => [ 'match=gzip' ], + 'Accept-Encoding' => null, ]; /** @@ -2222,19 +2223,18 @@ class OutputPage extends ContextSource { * Add an HTTP header that will influence on the cache * * @param string $header Header name - * @param string[]|null $option Options for the Key header. See - * https://datatracker.ietf.org/doc/draft-fielding-http-key/ - * for the list of valid options. + * @param string[]|null $option Deprecated; formerly options for the + * Key header, deprecated in 1.32 and removed in 1.34. See + * https://datatracker.ietf.org/doc/draft-fielding-http-key/ + * for the list of formerly-valid options. */ public function addVaryHeader( $header, array $option = null ) { - if ( !array_key_exists( $header, $this->mVaryHeader ) ) { - $this->mVaryHeader[$header] = []; + if ( $option !== null && count( $option ) > 0 ) { + wfDeprecated( 'addVaryHeader $option is ignored', '1.34' ); } - if ( !is_array( $option ) ) { - $option = []; + if ( !array_key_exists( $header, $this->mVaryHeader ) ) { + $this->mVaryHeader[$header] = null; } - $this->mVaryHeader[$header] = - array_unique( array_merge( $this->mVaryHeader[$header], $option ) ); } /** @@ -2277,41 +2277,6 @@ class OutputPage extends ContextSource { return 'Link: ' . implode( ',', $this->mLinkHeader ); } - /** - * Get a complete Key header - * - * @return string - * @deprecated in 1.32; the IETF spec for this header expired w/o becoming - * a standard. - */ - public function getKeyHeader() { - wfDeprecated( '$wgUseKeyHeader', '1.32' ); - - $cvCookies = $this->getCacheVaryCookies(); - - $cookiesOption = []; - foreach ( $cvCookies as $cookieName ) { - $cookiesOption[] = 'param=' . $cookieName; - } - $this->addVaryHeader( 'Cookie', $cookiesOption ); - - foreach ( SessionManager::singleton()->getVaryHeaders() as $header => $options ) { - $this->addVaryHeader( $header, $options ); - } - - $headers = []; - foreach ( $this->mVaryHeader as $header => $option ) { - $newheader = $header; - if ( is_array( $option ) && count( $option ) > 0 ) { - $newheader .= ';' . implode( ';', $option ); - } - $headers[] = $newheader; - } - $key = 'Key: ' . implode( ',', $headers ); - - return $key; - } - /** * T23672: Add Accept-Language to Vary and Key headers if there's no 'variant' parameter in GET. * @@ -2327,33 +2292,7 @@ class OutputPage extends ContextSource { $lang = $title->getPageLanguage(); if ( !$this->getRequest()->getCheck( 'variant' ) && $lang->hasVariants() ) { - $variants = $lang->getVariants(); - $aloption = []; - foreach ( $variants as $variant ) { - if ( $variant === $lang->getCode() ) { - continue; - } - - // XXX Note that this code is not strictly correct: we - // do a case-insensitive match in - // LanguageConverter::getHeaderVariant() while the - // (abandoned, draft) spec for the `Key` header only - // allows case-sensitive matches. To match the logic - // in LanguageConverter::getHeaderVariant() we should - // also be looking at fallback variants and deprecated - // mediawiki-internal codes, as well as BCP 47 - // normalized forms. - - $aloption[] = "substr=$variant"; - - // IE and some other browsers use BCP 47 standards in their Accept-Language header, - // like "zh-CN" or "zh-Hant". We should handle these too. - $variantBCP47 = LanguageCode::bcp47( $variant ); - if ( $variantBCP47 !== $variant ) { - $aloption[] = "substr=$variantBCP47"; - } - } - $this->addVaryHeader( 'Accept-Language', $aloption ); + $this->addVaryHeader( 'Accept-Language' ); } } @@ -2464,10 +2403,6 @@ class OutputPage extends ContextSource { # maintain different caches for logged-in users and non-logged in ones $response->header( $this->getVaryHeader() ); - if ( $config->get( 'UseKeyHeader' ) ) { - $response->header( $this->getKeyHeader() ); - } - if ( $this->mEnableClientCache ) { if ( $config->get( 'UseCdn' ) && diff --git a/includes/actions/RawAction.php b/includes/actions/RawAction.php index 3e4e61422c..f6c4472f2a 100644 --- a/includes/actions/RawAction.php +++ b/includes/actions/RawAction.php @@ -88,9 +88,6 @@ class RawAction extends FormlessAction { // Set standard Vary headers so cache varies on cookies and such (T125283) $response->header( $this->getOutput()->getVaryHeader() ); - if ( $config->get( 'UseKeyHeader' ) ) { - $response->header( $this->getOutput()->getKeyHeader() ); - } // Output may contain user-specific data; // vary generated content for open sessions on private wikis diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php index ed17e07c9c..4efd230de3 100644 --- a/includes/api/ApiMain.php +++ b/includes/api/ApiMain.php @@ -916,32 +916,20 @@ class ApiMain extends ApiBase { return; } - $useKeyHeader = $config->get( 'UseKeyHeader' ); if ( $this->mCacheMode == 'anon-public-user-private' ) { $out->addVaryHeader( 'Cookie' ); $response->header( $out->getVaryHeader() ); - if ( $useKeyHeader ) { - $response->header( $out->getKeyHeader() ); - if ( $out->haveCacheVaryCookies() ) { - // Logged in, mark this request private - $response->header( "Cache-Control: $privateCache" ); - return; - } - // Logged out, send normal public headers below - } elseif ( MediaWiki\Session\SessionManager::getGlobalSession()->isPersistent() ) { + if ( MediaWiki\Session\SessionManager::getGlobalSession()->isPersistent() ) { // Logged in or otherwise has session (e.g. anonymous users who have edited) // Mark request private $response->header( "Cache-Control: $privateCache" ); return; - } // else no Key and anonymous, send public headers below + } // else anonymous, send public headers below } // Send public headers $response->header( $out->getVaryHeader() ); - if ( $useKeyHeader ) { - $response->header( $out->getKeyHeader() ); - } // If nobody called setCacheMaxAge(), use the (s)maxage parameters if ( !isset( $this->mCacheControl['s-maxage'] ) ) { diff --git a/includes/session/SessionProvider.php b/includes/session/SessionProvider.php index def3bc3125..80a400b057 100644 --- a/includes/session/SessionProvider.php +++ b/includes/session/SessionProvider.php @@ -402,6 +402,9 @@ abstract class SessionProvider implements SessionProviderInterface, LoggerAwareI * } * @endcode * + * Note that the $options parameter to addVaryHeader has been deprecated + * since 1.34, and should be `null` or an empty array. + * * @protected For use by \MediaWiki\Session\SessionManager only * @return array */ diff --git a/tests/phpunit/includes/OutputPageTest.php b/tests/phpunit/includes/OutputPageTest.php index 32ce08c0ad..5f0067deff 100644 --- a/tests/phpunit/includes/OutputPageTest.php +++ b/tests/phpunit/includes/OutputPageTest.php @@ -2246,14 +2246,13 @@ class OutputPageTest extends MediaWikiTestCase { * * @covers OutputPage::addVaryHeader * @covers OutputPage::getVaryHeader - * @covers OutputPage::getKeyHeader * * @param array[] $calls For each array, call addVaryHeader() with those arguments * @param string[] $cookies Array of cookie names to vary on * @param string $vary Text of expected Vary header (including the 'Vary: ') * @param string $key Text of expected Key header (including the 'Key: ') */ - public function testVaryHeaders( array $calls, array $cookies, $vary, $key ) { + public function testVaryHeaders( array $calls, array $cookies, $vary ) { // Get rid of default Vary fields $op = $this->getMockBuilder( OutputPage::class ) ->setConstructorArgs( [ new RequestContext() ] ) @@ -2264,22 +2263,19 @@ class OutputPageTest extends MediaWikiTestCase { ->will( $this->returnValue( $cookies ) ); TestingAccessWrapper::newFromObject( $op )->mVaryHeader = []; - $this->hideDeprecated( '$wgUseKeyHeader' ); + $this->hideDeprecated( 'addVaryHeader $option is ignored' ); foreach ( $calls as $call ) { $op->addVaryHeader( ...$call ); } $this->assertEquals( $vary, $op->getVaryHeader(), 'Vary:' ); - $this->assertEquals( $key, $op->getKeyHeader(), 'Key:' ); } public function provideVaryHeaders() { - // note: getKeyHeader() automatically adds Vary: Cookie return [ 'No header' => [ [], [], 'Vary: ', - 'Key: Cookie', ], 'Single header' => [ [ @@ -2287,7 +2283,6 @@ class OutputPageTest extends MediaWikiTestCase { ], [], 'Vary: Cookie', - 'Key: Cookie', ], 'Non-unique headers' => [ [ @@ -2297,26 +2292,26 @@ class OutputPageTest extends MediaWikiTestCase { ], [], 'Vary: Cookie, Accept-Language', - 'Key: Cookie,Accept-Language', ], 'Two headers with single options' => [ + // Options are deprecated since 1.34 [ [ 'Cookie', [ 'param=phpsessid' ] ], [ 'Accept-Language', [ 'substr=en' ] ], ], [], 'Vary: Cookie, Accept-Language', - 'Key: Cookie;param=phpsessid,Accept-Language;substr=en', ], 'One header with multiple options' => [ + // Options are deprecated since 1.34 [ [ 'Cookie', [ 'param=phpsessid', 'param=userId' ] ], ], [], 'Vary: Cookie', - 'Key: Cookie;param=phpsessid;param=userId', ], 'Duplicate option' => [ + // Options are deprecated since 1.34 [ [ 'Cookie', [ 'param=phpsessid' ] ], [ 'Cookie', [ 'param=phpsessid' ] ], @@ -2324,30 +2319,28 @@ class OutputPageTest extends MediaWikiTestCase { ], [], 'Vary: Cookie, Accept-Language', - 'Key: Cookie;param=phpsessid,Accept-Language;substr=en', ], 'Same header, different options' => [ + // Options are deprecated since 1.34 [ [ 'Cookie', [ 'param=phpsessid' ] ], [ 'Cookie', [ 'param=userId' ] ], ], [], 'Vary: Cookie', - 'Key: Cookie;param=phpsessid;param=userId', ], 'No header, vary cookies' => [ [], [ 'cookie1', 'cookie2' ], 'Vary: Cookie', - 'Key: Cookie;param=cookie1;param=cookie2', ], 'Cookie header with option plus vary cookies' => [ + // Options are deprecated since 1.34 [ [ 'Cookie', [ 'param=cookie1' ] ], ], [ 'cookie2', 'cookie3' ], 'Vary: Cookie', - 'Key: Cookie;param=cookie1;param=cookie2;param=cookie3', ], 'Non-cookie header plus vary cookies' => [ [ @@ -2355,16 +2348,15 @@ class OutputPageTest extends MediaWikiTestCase { ], [ 'cookie' ], 'Vary: Accept-Language, Cookie', - 'Key: Accept-Language,Cookie;param=cookie', ], 'Cookie and non-cookie headers plus vary cookies' => [ + // Options are deprecated since 1.34 [ [ 'Cookie', [ 'param=cookie1' ] ], [ 'Accept-Language' ], ], [ 'cookie2' ], 'Vary: Cookie, Accept-Language', - 'Key: Cookie;param=cookie1;param=cookie2,Accept-Language', ], ]; } @@ -2417,10 +2409,9 @@ class OutputPageTest extends MediaWikiTestCase { /** * @dataProvider provideAddAcceptLanguage * @covers OutputPage::addAcceptLanguage - * @covers OutputPage::getKeyHeader */ public function testAddAcceptLanguage( - $code, array $variants, array $expected, array $options = [] + $code, array $variants, $expected, array $options = [] ) { $req = new FauxRequest( in_array( 'varianturl', $options ) ? [ 'variant' => 'x' ] : [] ); $op = $this->newInstance( [], $req, in_array( 'notitle', $options ) ? 'notitle' : null ); @@ -2444,41 +2435,38 @@ class OutputPageTest extends MediaWikiTestCase { // This will run addAcceptLanguage() $op->sendCacheControl(); - - $this->hideDeprecated( '$wgUseKeyHeader' ); - $keyHeader = $op->getKeyHeader(); - - if ( !$expected ) { - $this->assertFalse( strpos( 'Accept-Language', $keyHeader ) ); - return; - } - - $keyHeader = explode( ' ', $keyHeader, 2 )[1]; - $keyHeader = explode( ',', $keyHeader ); - - $acceptLanguage = null; - foreach ( $keyHeader as $item ) { - if ( strpos( $item, 'Accept-Language;' ) === 0 ) { - $acceptLanguage = $item; - break; - } - } - - $expectedString = 'Accept-Language;substr=' . implode( ';substr=', $expected ); - $this->assertSame( $expectedString, $acceptLanguage ); + $this->assertSame( "Vary: $expected", $op->getVaryHeader() ); } public function provideAddAcceptLanguage() { return [ - 'No variants' => [ 'en', [ 'en' ], [] ], - 'One simple variant' => [ 'en', [ 'en', 'en-x-piglatin' ], [ 'en-x-piglatin' ] ], + 'No variants' => [ + 'en', + [ 'en' ], + 'Accept-Encoding, Cookie', + ], + 'One simple variant' => [ + 'en', + [ 'en', 'en-x-piglatin' ], + 'Accept-Encoding, Cookie, Accept-Language', + ], 'Multiple variants with BCP47 alternatives' => [ 'zh', [ 'zh', 'zh-hans', 'zh-cn', 'zh-tw' ], - [ 'zh-hans', 'zh-Hans', 'zh-cn', 'zh-Hans-CN', 'zh-tw', 'zh-Hant-TW' ], + 'Accept-Encoding, Cookie, Accept-Language', + ], + 'No title' => [ + 'en', + [ 'en', 'en-x-piglatin' ], + 'Accept-Encoding, Cookie', + [ 'notitle' ] + ], + 'Variant in URL' => [ + 'en', + [ 'en', 'en-x-piglatin' ], + 'Accept-Encoding, Cookie', + [ 'varianturl' ] ], - 'No title' => [ 'en', [ 'en', 'en-x-piglatin' ], [], [ 'notitle' ] ], - 'Variant in URL' => [ 'en', [ 'en', 'en-x-piglatin' ], [], [ 'varianturl' ] ], ]; } -- 2.20.1