Writing down what I found

The problem seems to be that by the time a special page that inherits from RedirectSpecialPage is rendered, the request has been changed to a DerivativeRequest which contains a FauxResponse that handles cookies and headers differently from WebResponse. Two problems I see:

1. FauxResponse has its own implementation for handling cookies and it doens't override hasCookies() and when $reponse->hasCookies() is called it executes the parent's which will incorrectly report back
2. FauxResponse overrides header() which does not set the headers to the global response but rather save them in an array on a member variable. Unlike WebResponse::header() that does send the headers, FauxResponse will never set the cookies even if it were able to find them.

It is my understanding that FauxRequest should only be used in tests and I'm not sure why it was used here. It might be tech debt(?) and it was never a problem until these new "set cookie changes" were implemented. I'm not able to find proper documentation on FauxRequest/DerivativeRequest so I think we should ask for advice on how to proceed here if we are going to fix this.

@dmaza After the server returns a 301/302 and the page gets redirected... why doesn't the next request (that results in a 200) have the cookies?

In T235047#5648040, @dbarratt wrote:

@dmaza After the server returns a 301/302 and the page gets redirected... why doesn't the next request (that results in a 200) have the cookies?

I believe it is because the headers are not being set on the request due to what I explained in my last comment

@Niharika This is a previously existing problem that happened to come to light after our work on setting the cookies on all uncached web requests. Should we tag other teams or do you still want us to work on it?

Tagging @WDoranWMF and @EvanProdromou to triage as this is a far-reaching change to our request handling that our team likely should not tackle.

In T235047#5665150, @dmaza wrote:

I believe it is because the headers are not being set on the request due to what I explained in my last comment

I don't understand how the subsequent request would have any knowledge of the initial request...

I've added this to the CPT inbox so we can review it this week.

In T235047#5646867, @dmaza wrote:

1. FauxResponse has its own implementation for handling cookies and it doens't override hasCookies() and when $reponse->hasCookies() is called it executes the parent's which will incorrectly report back

That appears to have been fixed recently in rMW441ff2003e60: Add tests to OutputPage::sendCacheControl().

1. FauxResponse overrides header() which does not set the headers to the global response but rather save them in an array on a member variable. Unlike WebResponse::header() that does send the headers, FauxResponse will never set the cookies even if it were able to find them.

This is intended behavior of FauxResponse, and should not be changed.

It is my understanding that FauxRequest should only be used in tests and I'm not sure why it was used here.

There are a few other use cases for FauxRequest. And DerivativeRequest, despite being a subclass, has some different semantics.

The documentation of DerivativeRequest says "Similar to FauxRequest, but only fakes URL parameters and method (POST or GET) and use the base request for the remaining stuff (cookies, session and headers)." That implies that it should return the base request's WebResponse too, but we'd have to check the places it's actually being used to try to determine whether that will break any existing uses or not.

Probably ok:

• includes/MediaWiki.php: The intent is to do a redirect internally instead of actually serving the redirect to the browser (see T109724).
• includes/specials/SpecialNewFiles.php: The intent is just swapping a start/end pair in the query string if they're out of order.
• includes/specials/SpecialWatchlist.php: Just remapping some query parameters for compatibility. Probably doesn't even use the response.
• includes/specialpage/AuthManagerSpecialPage.php: The intent is to restore POST data for a "return from callback" workflow, which I don't think we even use in production (it's for things like coming back from GoogleLogin).
• includes/specialpage/FormSpecialPage.php: The intent is to restore after coming back from a redirect to Special:UserLogin, and it's only passed to HTMLForm.
• includes/api/ApiEditPage.php: Faking up data for EditPage. Slightly scary, but I'd be willing to risk it.
• includes/api/ApiFeedRecentChanges.php: Remapping parameters for an internal call to SpecialRecentChanges/SpecialRecentChangesLinked.
• includes/htmlform/fields/HTMLFormFieldCloner.php: Unpacking nested form data for other HTMLFormFields.
• extensions/Video/includes/specials/SpecialNewVideos.php: Just remapping some query parameters for compatibility.

Worrying:

• Internal Action API calls. We probably don't want API response headers to be passed through.
  • extensions/Collection/includes/Specials/SpecialCollection.php:
  • extensions/Collection/includes/DataProvider.php
  • extensions/MobileFrontend/includes/specials/SpecialMobileLanguages.php
  • extensions/GWToolset/includes/Adapters/Php/MediawikiTemplatePhpAdapter.php:
  • extensions/GWToolset/includes/Handlers/UploadHandler.php
  • extensions/Thanks/includes/SpecialThanks.php
  • extensions/LiquidThreads/api/ApiThreadAction.php
  • extensions/WikiLove/includes/ApiWikiLove.php
  • extensions/GettingStarted/includes/MoreLikePageSuggester.php
  • extensions/ContentTranslation/api/ApiContentTranslationPublish.php
  • extensions/MassMessage/includes/content/MassMessageListContentHandler.php
  • extensions/PropertySuggester/src/GetSuggestions.php
  • extensions/PageTriage/includes/Api/ApiPageTriageTagging.php
  • extensions/CollaborationKit/includes/specials/SpecialCreateHubFeature.php
  • extensions/CollaborationKit/includes/specials/SpecialCreateCollaborationHub.php
  • extensions/CollaborationKit/includes/content/CollaborationListContentHandler.php
  • extensions/CollaborationKit/includes/content/CollaborationHubContentHandler.php
  • extensions/VisualEditor/includes/ApiVisualEditorEdit.php
  • extensions/VisualEditor/includes/ApiVisualEditor.php
  • extensions/Newsletter/includes/content/NewsletterContentHandler.php
• extensions/MassMessage/includes/job/MassMessageJob.php: Internal API request that also overrides $wgRequest for some reason.
• extensions/Wikibase/repo/includes/LinkedData/EntityDataFormatProvider.php: Used for ApiMain, but not to run a request. Apparently only to fetch the list of Action API formatting modules available.
• extensions/Wikibase/repo/includes/LinkedData/EntityDataSerializationService.php: Used for ApiMain, but not to run a request. It populates an ApiResult, gets an ApiFormatBase instance, and uses that to format output.

Non-deployed extensions:

• Internal Action API calls. We probably don't want API response headers to be passed through.
  • extensions/BlueSpiceFoundation/includes/skins/BSSkinSidebarTreeParser.php
  • extensions/BlueSpiceNamespaceManager/includes/NamespaceNuker.php
  • extensions/BlueSpiceSMWConnector/includes/BSSMWConnectorHooks.php
  • extensions/BlueSpiceSMWConnector/maintenance/MigrateSemanticFormsToPageForms.php
  • extensions/PGFTikZ/PGFTikZ.parser.php
  • extensions/Html2Wiki/specials/SpecialHtml2Wiki.php
  • extensions/BlueSpiceExtendedSearch/src/MediaWiki/Specials/SearchAdmin.php
  • extensions/BlueSpiceRSSFeeder/includes/api/BSApiTasksRSSFeeder.php
  • extensions/QuickSearchLookup/includes/QuickSearchLookup.php
  • skins/Poncho/Poncho.php
• extensions/BlueSpiceFoundation/includes/utility/PageContentProvider.class.php
• extensions/MultiUpload/SpecialMultiUpload.php

So, yeah, probably not safe to change the default behavior of DerivativeRequest thanks to all those extensions doing internal API calls.

You might add an option to DerivativeRequest (to instruct it to use the parent WebResponse, or even to inject a different WebResponse), and use the new option in the MediaWiki.php codepath that's the problem here. Extra credit for following a deprecation process to switch all those extension internal API calls over to explcitly specifying "use a FauxResponse" then either making not-specifying an error or switching the default to returning the parent's response.