r69339 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r69338‎ \| r69339 \| r69340 >
Date:	19:00, 14 July 2010
Author:	catrope
Status:	resolved (Comments)
Tags:
Comment:	API: Make output containing private or user-specific data uncacheable for logged-in users by setting Vary: Cookie or Cache-Control: private, whichever is appropriate. Fixes instances in core and WMF-deployed extensions only. Without this change, the output of requests like ?action=query&list=recentchanges&rcprop=patrolled&smaxage=3600 would be cached in Squid and viewable for anyone using the same URL, even if they don't have patrol rights. Other, more serious exploits are also possible. Also avoid using $wgUser in one place, kill some unused global $wgUser; instances and tweak a comment.
Modified paths:	/trunk/extensions/AbuseFilter/ApiQueryAbuseFilters.php (modified) (history) /trunk/extensions/AbuseFilter/ApiQueryAbuseLog.php (modified) (history) /trunk/extensions/CentralAuth/ApiQueryGlobalUserInfo.php (modified) (history) /trunk/extensions/CodeReview/api/ApiCodeComments.php (modified) (history) /trunk/extensions/CodeReview/api/ApiCodeDiff.php (modified) (history) /trunk/extensions/CodeReview/api/ApiCodeUpdate.php (modified) (history) /trunk/extensions/FlaggedRevs/api/ApiQueryOldreviewedpages.php (modified) (history) /trunk/extensions/GlobalBlocking/ApiQueryGlobalBlocks.php (modified) (history) /trunk/extensions/LiquidThreads/api/ApiQueryLQTThreads.php (modified) (history) /trunk/extensions/UsabilityInitiative/ClickTracking/ApiClickTracking.php (modified) (history) /trunk/phase3/includes/api/ApiBase.php (modified) (history) /trunk/phase3/includes/api/ApiLogout.php (modified) (history) /trunk/phase3/includes/api/ApiMain.php (modified) (history) /trunk/phase3/includes/api/ApiParse.php (modified) (history) /trunk/phase3/includes/api/ApiPatrol.php (modified) (history) /trunk/phase3/includes/api/ApiPurge.php (modified) (history) /trunk/phase3/includes/api/ApiQueryAllmessages.php (modified) (history) /trunk/phase3/includes/api/ApiQueryBlocks.php (modified) (history) /trunk/phase3/includes/api/ApiQueryDeletedrevs.php (modified) (history) /trunk/phase3/includes/api/ApiQueryFilearchive.php (modified) (history) /trunk/phase3/includes/api/ApiQueryInfo.php (modified) (history) /trunk/phase3/includes/api/ApiQueryRecentChanges.php (modified) (history) /trunk/phase3/includes/api/ApiQueryRevisions.php (modified) (history) /trunk/phase3/includes/api/ApiQueryUserContributions.php (modified) (history) /trunk/phase3/includes/api/ApiQueryUserInfo.php (modified) (history) /trunk/phase3/includes/api/ApiQueryUsers.php (modified) (history) /trunk/phase3/includes/api/ApiQueryWatchlist.php (modified) (history) /trunk/phase3/includes/api/ApiWatch.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiMain.php
—	—	@@ -124,7 +124,7 @@
125	125
126	126	private $mPrinter, $mModules, $mModuleNames, $mFormats, $mFormatNames;
127	127	private $mResult, $mAction, $mShowVersions, $mEnableWrite, $mRequest;
128		~~- private $mInternalMode, $mSquidMaxage, $mModule;~~
	128	+ private $mInternalMode, $mSquidMaxage, $mModule, $mVaryCookie;
129	129
130	130	private $mCacheControl = array( 'must-revalidate' => true );
131	131
—	—	@@ -169,6 +169,7 @@
170	170
171	171	$this->mSquidMaxage = - 1; // flag for executeActionWithErrorHandling()
172	172	$this->mCommit = false;
	173	+ $this->mVaryCookie = false;
173	174	}
174	175
175	176	/**
—	—	@@ -215,6 +216,14 @@
216	217	's-maxage' => $maxage
217	218	) );
218	219	}
	220	+
	221	+ /**
	222	+ * Make sure Cache-Control: private is set. Use this when the output of a request
	223	+ * is for the current recipient only and should not be cached in any shared cache.
	224	+ */
	225	+ public function setCachePrivate() {
	226	+ $this->setCacheControl( array( 'private' => true ) );
	227	+ }
219	228
220	229	/**
221	230	* Set directives (key/value pairs) for the Cache-Control header.
—	—	@@ -224,6 +233,35 @@
225	234	public function setCacheControl( $directives ) {
226	235	$this->mCacheControl = $directives + $this->mCacheControl;
227	236	}
	237	+
	238	+ /**
	239	+ * Make sure Vary: Cookie and friends are set. Use this when the output of a request
	240	+ * may be cached for anons but may not be cached for logged-in users.
	241	+ *
	242	+ * WARNING: This function must be called CONSISTENTLY for a given URL. This means that a
	243	+ * given URL must either always or never call this function; if it sometimes does and
	244	+ * sometimes doesn't, stuff will break.
	245	+ */
	246	+ public function setVaryCookie() {
	247	+ $this->mVaryCookie = true;
	248	+ }
	249	+
	250	+ /**
	251	+ * Actually output the Vary: Cookie header and its friends, if flagged with setVaryCookie().
	252	+ * Outputs the appropriate X-Vary-Options header and Cache-Control: private if needed.
	253	+ */
	254	+ private function outputVaryCookieHeader() {
	255	+ global $wgUseXVO, $wgOut;
	256	+ if ( $this->mVaryCookie ) {
	257	+ header( 'Vary: Cookie' );
	258	+ if ( $wgUseXVO ) {
	259	+ header( $wgOut->getXVO() );
	260	+ if ( $wgOut->hasCacheVaryCookies() ) {
	261	+ $this->setCacheControl( array( 'private' => true ) );
	262	+ }
	263	+ }
	264	+ }
	265	+ }
228	266
229	267	/**
230	268	* Create an instance of an output formatter by its name
—	—	@@ -276,6 +314,7 @@
277	315
278	316	// Error results should not be cached
279	317	$this->setCacheMaxAge( 0 );
	318	+ $this->setCachePrivate();
280	319
281	320	$headerStr = 'MediaWiki-API-Error: ' . $errCode;
282	321	if ( $e->getCode() === 0 ) {
—	—	@@ -291,6 +330,11 @@
292	331	$this->mPrinter->safeProfileOut();
293	332	$this->printResult( true );
294	333	}
	334	+
	335	+ // If this wiki is private, don't cache anything ever
	336	+ if ( in_array( 'read', User::getGroupPermissions( array( '*' ) ), true ) ) {
	337	+ $this->setCachePrivate();
	338	+ }
295	339
296	340	// If nobody called setCacheMaxAge(), use the (s)maxage parameters
297	341	if ( !isset( $this->mCacheControl['s-maxage'] ) ) {
—	—	@@ -322,6 +366,7 @@
323	367	}
324	368
325	369	header( "Cache-Control: $ccHeader" );
	370	+ $this->outputVaryCookieHeader();
326	371
327	372	if ( $this->mPrinter->getIsHtml() && !$this->mPrinter->isDisabled() ) {
328	373	echo wfReportTime();
—	—	@@ -477,7 +522,8 @@
478	523	*/
479	524	protected function checkExecutePermissions( $module ) {
480	525	global $wgUser, $wgGroupPermissions;
481		~~- if ( $module->isReadMode() && !$wgGroupPermissions['*']['read'] && !$wgUser->isAllowed( 'read' ) )~~
	526	+ if ( $module->isReadMode() && !in_array( 'read', User::getGroupPermissions( array( '*' ) ), true ) &&
	527	+ !$wgUser->isAllowed( 'read' ) )
482	528	{
483	529	$this->dieUsageMsg( array( 'readrequired' ) );
484	530	}
Index: trunk/phase3/includes/api/ApiBase.php
—	—	@@ -1103,9 +1103,12 @@
1104	1104	if ( $token == '' \|\| $token != $params['token'] ) {
1105	1105	$this->dieUsage( 'Incorrect watchlist token provided -- please set a correct token in Special:Preferences', 'bad_wltoken' );
1106	1106	}
1107		~~- } elseif ( !$wgUser->isLoggedIn() ) {~~
1108		~~- $this->dieUsage( 'You must be logged-in to have a watchlist', 'notloggedin' );~~
1109	1107	} else {
	1108	+ // User not determined by URL, so don't cache
	1109	+ $this->getMain()->setVaryCookie();
	1110	+ if ( !$wgUser->isLoggedIn() ) {
	1111	+ $this->dieUsage( 'You must be logged-in to have a watchlist', 'notloggedin' );
	1112	+ }
1110	1113	$user = $wgUser;
1111	1114	}
1112	1115	return $user;
Index: trunk/phase3/includes/api/ApiQueryUserInfo.php
—	—	@@ -40,6 +40,7 @@
41	41	}
42	42
43	43	public function execute() {
	44	+ $this->getMain()->setCachePrivate();
44	45	$params = $this->extractRequestParams();
45	46	$result = $this->getResult();
46	47	$r = array();
Index: trunk/phase3/includes/api/ApiQueryBlocks.php
—	—	@@ -127,6 +127,9 @@
128	128	'ipb_auto' => 0
129	129	) );
130	130	}
	131	+
	132	+ // Make sure private data (deleted blocks) isn't cached
	133	+ $this->getMain()->setVaryCookie();
131	134	if ( !$wgUser->isAllowed( 'hideuser' ) ) {
132	135	$this->addWhereFld( 'ipb_deleted', 0 );
133	136	}
Index: trunk/phase3/includes/api/ApiQueryInfo.php
—	—	@@ -253,6 +253,7 @@
254	254	}
255	255
256	256	if ( $this->fld_watched ) {
	257	+ $this->getMain()->setVaryCookie();
257	258	$this->getWatchedInfo();
258	259	}
259	260
—	—	@@ -298,6 +299,9 @@
299	300	}
300	301
301	302	if ( !is_null( $this->params['token'] ) ) {
	303	+ // Don't cache tokens
	304	+ $this->getMain()->setCachePrivate();
	305	+
302	306	$tokenFunctions = $this->getTokenFunctions();
303	307	$pageInfo['starttimestamp'] = wfTimestamp( TS_ISO_8601, time() );
304	308	foreach ( $this->params['token'] as $t ) {
—	—	@@ -534,7 +538,7 @@
535	539	}
536	540
537	541	/**
538		~~- * Get information about watched status and put it in $watched~~
	542	+ * Get information about watched status and put it in $this->watched
539	543	*/
540	544	private function getWatchedInfo() {
541	545	global $wgUser;
Index: trunk/phase3/includes/api/ApiQueryWatchlist.php
—	—	@@ -74,6 +74,7 @@
75	75	$this->fld_notificationtimestamp = isset( $prop['notificationtimestamp'] );
76	76
77	77	if ( $this->fld_patrol ) {
	78	+ $this->getMain()->setVaryCookie();
78	79	if ( !$user->useRCPatrol() && !$user->useNPPatrol() ) {
79	80	$this->dieUsage( 'patrol property is not available', 'patrol' );
80	81	}
—	—	@@ -141,9 +142,11 @@
142	143	}
143	144
144	145	// Check permissions. FIXME: should this check $user instead of $wgUser?
145		~~- if ( ( isset( $show['patrolled'] ) \|\| isset( $show['!patrolled'] ) ) && !$wgUser->useRCPatrol() && !$wgUser->useNPPatrol() )~~
146		~~- {~~
147		~~- $this->dieUsage( 'You need the patrol right to request the patrolled flag', 'permissiondenied' );~~
	146	+ if ( isset( $show['patrolled'] ) \|\| isset( $show['!patrolled'] ) ) {
	147	+ $this->getMain()->setVaryCookie();
	148	+ if ( !$wgUser->useRCPatrol() && !$wgUser->useNPPatrol() ) {
	149	+ $this->dieUsage( 'You need the patrol right to request the patrolled flag', 'permissiondenied' );
	150	+ }
148	151	}
149	152
150	153	/* Add additional conditions to query depending upon parameters. */
Index: trunk/phase3/includes/api/ApiQueryDeletedrevs.php
—	—	@@ -41,6 +41,7 @@
42	42
43	43	public function execute() {
44	44	global $wgUser;
	45	+ $this->getMain()->setVaryCookie();
45	46	// Before doing anything at all, let's check permissions
46	47	if ( !$wgUser->isAllowed( 'deletedhistory' ) ) {
47	48	$this->dieUsage( 'You don\'t have permission to view deleted revision information', 'permissiondenied' );
Index: trunk/phase3/includes/api/ApiPatrol.php
—	—	@@ -41,6 +41,7 @@
42	42	* Patrols the article or provides the reason the patrol failed.
43	43	*/
44	44	public function execute() {
	45	+ $this->getMain()->setCachePrivate();
45	46	$params = $this->extractRequestParams();
46	47
47	48	if ( !isset( $params['rcid'] ) ) {
Index: trunk/phase3/includes/api/ApiWatch.php
—	—	@@ -41,6 +41,7 @@
42	42
43	43	public function execute() {
44	44	global $wgUser;
	45	+ $this->getMain()->setCachePrivate();
45	46	if ( !$wgUser->isLoggedIn() ) {
46	47	$this->dieUsage( 'You must be logged-in to have a watchlist', 'notloggedin' );
47	48	}
Index: trunk/phase3/includes/api/ApiQueryFilearchive.php
—	—	@@ -43,6 +43,7 @@
44	44
45	45	public function execute() {
46	46	global $wgUser;
	47	+ $this->getMain()->setVaryCookie();
47	48	// Before doing anything at all, let's check permissions
48	49	if ( !$wgUser->isAllowed( 'deletedhistory' ) ) {
49	50	$this->dieUsage( 'You don\'t have permission to view deleted file information', 'permissiondenied' );
Index: trunk/phase3/includes/api/ApiQueryRevisions.php
—	—	@@ -411,6 +411,9 @@
412	412	}
413	413
414	414	if ( !is_null( $this->token ) ) {
	415	+ // Don't cache tokens
	416	+ $this->getMain()->setCachePrivate();
	417	+
415	418	$tokenFunctions = $this->getTokenFunctions();
416	419	foreach ( $this->token as $t ) {
417	420	$val = call_user_func( $tokenFunctions[$t], $title->getArticleID(), $title, $revision );
Index: trunk/phase3/includes/api/ApiParse.php
—	—	@@ -138,7 +138,7 @@
139	139	$p_result = false;
140	140	$pcache = ParserCache::singleton();
141	141	if ( $wgEnableParserCache ) {
142		~~- $p_result = $pcache->get( $articleObj, $wgUser );~~
	142	+ $p_result = $pcache->get( $articleObj, $popts );
143	143	}
144	144	if ( !$p_result ) {
145	145	$p_result = $wgParser->parse( $articleObj->getContent(), $titleObj, $popts );
—	—	@@ -162,6 +162,7 @@
163	163
164	164	if ( $params['pst'] \|\| $params['onlypst'] ) {
165	165	$text = $wgParser->preSaveTransform( $text, $titleObj, $wgUser, $popts );
	166	+ $this->getMain()->setVaryCookie();
166	167	}
167	168	if ( $params['onlypst'] ) {
168	169	// Build a result and bail out
Index: trunk/phase3/includes/api/ApiPurge.php
—	—	@@ -42,6 +42,7 @@
43	43	*/
44	44	public function execute() {
45	45	global $wgUser;
	46	+ $this->getMain()->setCachePrivate();
46	47	$params = $this->extractRequestParams();
47	48	if ( !$wgUser->isAllowed( 'purge' ) ) {
48	49	$this->dieUsageMsg( array( 'cantpurge' ) );
Index: trunk/phase3/includes/api/ApiQueryAllmessages.php
—	—	@@ -48,6 +48,9 @@
49	49	if ( !is_null( $params['lang'] ) && $params['lang'] != $wgLang->getCode() ) {
50	50	$oldLang = $wgLang; // Keep $wgLang for restore later
51	51	$wgLang = Language::factory( $params['lang'] );
	52	+ } else if ( is_null( $params['lang'] ) ) {
	53	+ // Language not determined by URL but by user preferences, so don't cache
	54	+ $this->getMain()->setVaryCookie();
52	55	}
53	56
54	57	$prop = array_flip( (array)$params['prop'] );
Index: trunk/phase3/includes/api/ApiQueryUsers.php
—	—	@@ -162,6 +162,9 @@
163	163	}
164	164
165	165	if ( !is_null( $params['token'] ) ) {
	166	+ // Don't cache tokens
	167	+ $this->getMain()->setCachePrivate();
	168	+
166	169	$tokenFunctions = $this->getTokenFunctions();
167	170	foreach ( $params['token'] as $t ) {
168	171	$val = call_user_func( $tokenFunctions[$t], $user );
Index: trunk/phase3/includes/api/ApiQueryUserContributions.php
—	—	@@ -164,6 +164,8 @@
165	165	);
166	166	}
167	167
	168	+ // Make sure private data (deleted revisions) isn't cached
	169	+ $this->getMain()->setVaryCookie();
168	170	if ( !$wgUser->isAllowed( 'hideuser' ) ) {
169	171	$this->addWhere( $this->getDB()->bitAnd( 'rev_deleted', Revision::DELETED_USER ) . ' = 0' );
170	172	}
—	—	@@ -215,9 +217,12 @@
216	218	$this->fld_patrolled )
217	219	{
218	220	global $wgUser;
	221	+ // Don't cache private data
	222	+ $this->getMain()->setVaryCookie();
219	223	if ( !$wgUser->useRCPatrol() && !$wgUser->useNPPatrol() ) {
220	224	$this->dieUsage( 'You need the patrol right to request the patrolled flag', 'permissiondenied' );
221	225	}
	226	+
222	227	// Use a redundant join condition on both
223	228	// timestamp and ID so we can use the timestamp
224	229	// index
Index: trunk/phase3/includes/api/ApiQueryRecentChanges.php
—	—	@@ -143,9 +143,11 @@
144	144
145	145	// Check permissions
146	146	global $wgUser;
147		~~- if ( ( isset( $show['patrolled'] ) \|\| isset( $show['!patrolled'] ) ) && !$wgUser->useRCPatrol() && !$wgUser->useNPPatrol() )~~
148		~~- {~~
149		~~- $this->dieUsage( 'You need the patrol right to request the patrolled flag', 'permissiondenied' );~~
	147	+ if ( isset( $show['patrolled'] ) \|\| isset( $show['!patrolled'] ) ) {
	148	+ $this->getMain()->setVaryCookie();
	149	+ if ( !$wgUser->useRCPatrol() && !$wgUser->useNPPatrol() ) {
	150	+ $this->dieUsage( 'You need the patrol right to request the patrolled flag', 'permissiondenied' );
	151	+ }
150	152	}
151	153
152	154	/* Add additional conditions to query depending upon parameters. */
—	—	@@ -412,6 +414,9 @@
413	415	}
414	416
415	417	if ( !is_null( $this->token ) ) {
	418	+ // Don't cache tokens
	419	+ $this->getMain()->setCachePrivate();
	420	+
416	421	$tokenFunctions = $this->getTokenFunctions();
417	422	foreach ( $this->token as $t ) {
418	423	$val = call_user_func( $tokenFunctions[$t], $row->rc_cur_id,
Index: trunk/phase3/includes/api/ApiLogout.php
—	—	@@ -42,6 +42,7 @@
43	43
44	44	public function execute() {
45	45	global $wgUser;
	46	+ $this->getMain()->setCachePrivate();
46	47	$oldName = $wgUser->getName();
47	48	$wgUser->logout();
48	49
Index: trunk/extensions/UsabilityInitiative/ClickTracking/ApiClickTracking.php
—	—	@@ -15,6 +15,7 @@
16	16	public function execute() {
17	17	global $wgUser, $wgTitle, $wgClickTrackContribGranularity1, $wgClickTrackContribGranularity2, $wgClickTrackContribGranularity3;
18	18
	19	+ $this->getMain()->setCachePrivate();
19	20	$params = $this->extractRequestParams();
20	21	$this->validateParams( $params );
21	22	$eventid_to_lookup = $params['eventid'];
—	—	@@ -74,7 +75,6 @@
75	76	$this->dieUsage( 'The URL to redirect to must be domain-relative, i.e. start with a /', 'badurl' );
76	77	}
77	78	}
78		~~- $this->getMain()->setCacheMaxAge( 0 );~~
79	79	}
80	80
81	81	/**
Index: trunk/extensions/GlobalBlocking/ApiQueryGlobalBlocks.php
—	—	@@ -36,7 +36,6 @@
37	37	}
38	38
39	39	public function execute() {
40		~~- global $wgUser;~~
41	40	$params = $this->extractRequestParams();
42	41
43	42	$prop = array_flip($params['prop']);
Index: trunk/extensions/CentralAuth/ApiQueryGlobalUserInfo.php
—	—	@@ -40,6 +40,7 @@
41	41	$prop = array_flip( (array)$params['prop'] );
42	42	if ( is_null( $params['user'] ) ) {
43	43	$params['user'] = $wgUser->getName();
	44	+ $this->getMain()->setVaryCookie();
44	45	}
45	46	$user = new CentralAuthUser( $params['user'] );
46	47	if ( !$user->exists() ) {
Index: trunk/extensions/FlaggedRevs/api/ApiQueryOldreviewedpages.php
—	—	@@ -58,6 +58,7 @@
59	59	$this->addWhere( 'GREATEST(page_len,rev_len)-LEAST(page_len,rev_len) <= ' .
60	60	intval( $params['maxsize'] ) );
61	61	if ( $params['filterwatched'] == 'watched' ) {
	62	+ $this->getMain()->setVaryCookie();
62	63	if ( !( $uid = $wgUser->getId() ) ) {
63	64	$this->dieUsage( 'You must be logged-in to have a watchlist', 'notloggedin' );
64	65	}
Index: trunk/extensions/CodeReview/api/ApiCodeDiff.php
—	—	@@ -4,6 +4,7 @@
5	5
6	6	public function execute() {
7	7	global $wgUser, $wgCodeReviewMaxDiffSize;
	8	+ $this->getMain()->setVaryCookie();
8	9	// Before doing anything at all, let's check permissions
9	10	if ( !$wgUser->isAllowed( 'codereview-use' ) ) {
10	11	$this->dieUsage( 'You don\'t have permission to view code diffs', 'permissiondenied' );
Index: trunk/extensions/CodeReview/api/ApiCodeUpdate.php
—	—	@@ -8,6 +8,7 @@
9	9	if ( !$wgUser->isAllowed( 'codereview-use' ) ) {
10	10	$this->dieUsage( 'You don\'t have permission to update code', 'permissiondenied' );
11	11	}
	12	+ $this->getMain()->setVaryCookie();
12	13	$params = $this->extractRequestParams();
13	14
14	15	if ( !isset( $params['repo'] ) ) {
Index: trunk/extensions/CodeReview/api/ApiCodeComments.php
—	—	@@ -30,6 +30,7 @@
31	31
32	32	public function execute() {
33	33	global $wgUser;
	34	+ $this->getMain()->setVaryCookie();
34	35	// Before doing anything at all, let's check permissions
35	36	if ( !$wgUser->isAllowed( 'codereview-use' ) ) {
36	37	$this->dieUsage( 'You don\'t have permission to view code comments', 'permissiondenied' );
Index: trunk/extensions/AbuseFilter/ApiQueryAbuseFilters.php
—	—	@@ -36,6 +36,7 @@
37	37
38	38	public function execute() {
39	39	global $wgUser;
	40	+ $this->getMain()->setVaryCookie();
40	41	if ( !$wgUser->isAllowed( 'abusefilter-view' ) )
41	42	$this->dieUsage( 'You don\'t have permission to view abuse filters', 'permissiondenied' );
42	43
Index: trunk/extensions/AbuseFilter/ApiQueryAbuseLog.php
—	—	@@ -36,6 +36,7 @@
37	37
38	38	public function execute() {
39	39	global $wgUser;
	40	+ $this->getMain()->setVaryCookie();
40	41	if ( !$wgUser->isAllowed( 'abusefilter-log' ) )
41	42	$this->dieUsage( 'You don\'t have permission to view the abuse log', 'permissiondenied' );
42	43
Index: trunk/extensions/LiquidThreads/api/ApiQueryLQTThreads.php
—	—	@@ -44,8 +44,6 @@
45	45	}
46	46
47	47	public function execute() {
48		~~- global $wgUser;~~
49		-
50	48	$params = $this->extractRequestParams();
51	49	$prop = array_flip( $params['prop'] );
52	50	$result = $this->getResult();
—	—	@@ -87,6 +85,7 @@
88	86	);
89	87
90	88	$this->addFields( $allFields );
	89	+ $this->getMain()->setVaryCookie();
91	90	}
92	91
93	92	$res = $this->select( __METHOD__ );

Follow-up revisions

Revision	Commit summary	Author	Date
r69342	1.16wmf4: MFT r69339	catrope	19:27, 14 July 2010
r69347	follow up r69339: Add a missing setVaryCookie for consistency and to avoid us...	mah	21:00, 14 July 2010
r69350	follow up r69339:...	mah	21:46, 14 July 2010
r69354	1.16: MFT r69339	mah	22:09, 14 July 2010
r69369	re r69339 use the actual function: s/hasCache/haveCache/	mah	01:15, 15 July 2010
r69376	MFT r69339	mah	02:05, 15 July 2010
r69579	Revert backport (r69376) of broken API cache header bug fix r69339.	tstarling	02:49, 20 July 2010
r69776	Rewrote r69339 etc. to clean up API cache header handling....	tstarling	07:17, 23 July 2010
r69784	Followup to r69776: remove calls to setCachePrivate() and setVaryCookie() fro...	catrope	11:17, 23 July 2010
r69932	* MFT r69776, and followups r69784, r69928, r69931, and superseded base revis...	tstarling	08:03, 26 July 2010

Comments

#Comment by MarkAHershberger (talk | contribs) 22:03, 14 July 2010

For ApiQueryInfo::get*Token() functions: should each of those results be cache-able as well?

#Comment by Catrope (talk | contribs) 22:31, 14 July 2010

No, but this is handled by setting uncachability in the code path that grabs the tokens rather than in each token function.

#Comment by MarkAHershberger (talk | contribs) 01:22, 15 July 2010

Hate to ask, but feel I must: How did you test this? Do you have hasCacheVaryCookies defined somewhere?

#Comment by Catrope (talk | contribs) 08:54, 15 July 2010

A way to test this programmatically (which I didn't do) would indeed be to add getters for $mVaryCookies and $mCacheControl. I tested this by inspecting the headers returned, but forgot to cover the cases where Vary: Cookie was set and didn't see the fatal error fixed in r69369 until just now.

Something that also bothers me is that you'll still get something like Cache-Control: private, must-revalidate, s-maxage=60, max-age=60 , which is contradictory; I'll ask Tim how bad this is.

#Comment by Midom (talk | contribs) 21:18, 18 July 2010

indeed, it is very much worth bothering, as it broke all the caching for opensearch.

if ( in_array( 'read', User::getGroupPermissions( array( '*' ) ), true ) ) {

I guess it misses one exclamation mark.

#Comment by Midom (talk | contribs) 21:22, 18 July 2010

I have some followup in r69521

#Comment by Tim Starling (talk | contribs) 02:24, 20 July 2010

As I said on IRC on July 15, setCachePrivate() should be used instead of setVaryCookie() in many of these cases. And yes, the bogus CC headers (with both private and maxage) need to be fixed. r69521 only fixes it in the public case, in the private case ($wgGroupPermissions['*']['read']=false) it's still broken.

#Comment by Tim Starling (talk | contribs) 06:18, 21 July 2010

What's the point in calling $this->setCacheControl() from ApiMain::outputVaryCookieHeader() when the Cache-Control header has already been sent?

Don't fix it, I'm working on it now.

#Comment by Tim Starling (talk | contribs) 04:57, 23 July 2010

You left out Vary: Accept-Encoding, so deploying this change will cause clients to get unexpected content encodings for cached objects. Again, please don't fix.

#Comment by Tim Starling (talk | contribs) 06:09, 26 July 2010

Fixed in r69776, r69784 and r69928, marking resolved.

Status & tagging log

06:09, 26 July 2010 Tim Starling (talk | contribs) changed the status of r69339 [removed: fixme added: resolved]
21:18, 18 July 2010 Midom (talk | contribs) changed the status of r69339 [removed: new added: fixme]