[14:10:46] James_F: In https://gerrit.wikimedia.org/r/c/mediawiki/extensions/TimedMediaHandler/+/232214/64..65/i18n/api/en.json you fixed the on line 17 but overlooked line 18. [15:02:49] anomie: Fixed, thanks. [15:39:26] Hello CPT, does anyone have an update about the export-CodeReview-as-static-pages work, T205361? It's blocking undeploying that extension, which is a RelEng goal… [15:39:27] T205361: Make an HTML dump of the output of the CodeReview extension on MediaWiki.org - https://phabricator.wikimedia.org/T205361 [15:40:10] Amir1, Krinkle: I'm not seeing the point of https://gerrit.wikimedia.org/r/c/mediawiki/core/+/506386 without also doing something like what happens if you visit https://en.wikipedia.org/wiki/WP:Sandbox?action=purge. As it is the GET request still works, all you're doing is adding JS UI that seems to have no advantage over the non-JS UI. [15:41:59] anomie: What it adds is to not regress the user expectation for the common case that one-click will log you out, and to keep the overhead in terms of user effort at one interaction instead of two interactions that also cross an entire page load. [15:42:06] same as ajax watch, ajax patrol. [15:42:09] anomie: yes, the patch is the first step and it improves UX by giving notification and refreshing the page instead of getting them to a special page but once it's properly in place, I will start removing the support for token in get [15:42:30] the same is also being done for rollback, and (if we actually had a UI for it), for purge. Most purge gadgets do ajax as well. [15:42:56] Krinkle: I think you misunderstood. Amir1 has the good answer: the purge-like confirmation page is coming in a followup. [15:43:28] However, I do think Amir1's commit message is confusing, because it suggests we're adding JS to avoid GET requests with csrf which afaik cannot happen. We're just avoiding an extra form step that submits to POST, right? [15:44:02] OK. we usually do it the other way around, but ok. [15:44:03] I even already made the changes in my code, haven't polishsed it enough to push it to geritt [15:44:31] Krinkle: well, most people use the js ajax, it definitely reduces number of GET requests with CSRF tokens in it [15:44:46] but it doesn't remove the support [15:45:09] My concern was that right now there is no extra form step, so the JS seems pointless. Adding the extra form step will give it a point. Personally I'd do both in one patch, but I don't care enough to try to insist. [15:45:35] Krinkle: also, right now, you can log out with js csrf token in GET (similar to rollback) [15:47:22] anomie: one part is that the current situation leaves you at special:Logout after you clicked on it, but with this change, when you click on logout, you will stay on the page but you get a refresh instead [15:47:39] so it's some improvement on its own as well [15:48:35] Krinkle: I still don't know where to put the js module :( [15:49:35] Amir1: I'd think that most often after someone clicks logout they either click "log in" to log into a different account, leave the site, or close the browser. Is there research that shows people instead keep reading the same page after logging out? [15:49:35] Amir1: my point was that I find it very unexpected that we've introduced csrf-over-GET as a new feature in 2019. I was expecting that to not exist in the first place and thus to use POST already, and then add JS to make it nicer. Anyway, makes sense given it wasn't using a token before so no worries there. [15:50:48] Amir1: I don't think we should change that as part of this. redirect to Special:Logout instead, and have it do nothing if already logged-out (just state you are logged out, which is not a lie even if you visit it randomly). [15:51:17] I'd leave it to design/UX to debata on that with product if it's worth changing and if so research why [15:51:40] anomie: Krinkle : Noted, sure. [15:53:03] Regarding CSRF over get, yes, we should remove that and API actually requires POST while the special page doesn't. This js is only there to remove this csrf-over-get [15:53:24] and once it's there, I remove any support of it in a followup [15:53:30] How badly would it break things to stop accepting GETs on the special page? [15:53:51] I presume that random gadgets aren't going to be relying on that? [15:54:09] James_F: without my js patch, everyone needs to do an extra step to logout [15:54:20] Oh, sure. [15:54:29] with my patch, only people who disabled js [15:57:53] Krinkle: None of the canaries run PHP7, so it's not like changing the fatalmonitor query would help. [16:06:41] James_F: PHP7 is opt-in by random header/cookie set in Varnish. It is used regardless of which server it routes to, and that includes debug and canaries. [16:06:45] They have it installed and support it. [16:07:05] canaries receive a portion of real traffic, which should include the PHP7 test sampling which we keep ramping up [16:07:31] Krinkle: Oh, right, it's just the API servers where that's not going to bbe the case. [16:07:37] Don't mind me. [16:07:57] Right, we don't forcefully opt-in API server traffic yet (afaik). [16:08:32] but for beta feature usage and WikimediaDebug, I think it applies to api.php requests as well, not sure. [16:08:54] and certainly when we switch the default, it will affect those. so we do need to monitor them before then. [16:09:05] is there a task for API traffcic? [16:10:20] Krinkle: T219129 [16:10:21] T219129: Allow directing a percentage of API traffic to PHP7 - https://phabricator.wikimedia.org/T219129 [16:50:01] TimStarling: tgr: Curious if you have any concerns/suggestions regarding T218207, specifically wgCacheDirectory [16:50:01] T218207: Use disk-based LCStore by default in MediaWiki 1.34 - https://phabricator.wikimedia.org/T218207 [17:16:33] Krinkle: the security implications seem a bit scary [17:16:58] is it that much faster thank JSON or some other nonexecutable format? [17:19:09] I guess for small installations we'll offer a web updater at some point, which will come with the same risks, so not that much of a deal [17:19:49] for larger ones, not sure about making this silently the default [17:19:53] tgr: "at some point" - do we not have a web updater? [17:20:34] Any sizable install presumably has wgCacheDirectory set already to a non-web directory, in which case nothing changes, except the format of files stored (.php array instead of .cdb today). [17:23:07] tgr: I'd be happy to make a balance there, but what would be on the balance in static-array's disinterest? And is it something WMF would not be concerned about? [18:43:40] Krinkle: a codebase updater, I mean [18:44:05] static array means arbitrary code execution (I imagine we are not parsing it ourselves?) [18:44:31] JSON or CDB does not; even serialized PHP arrays are reasonably safe from it [18:45:14] as for the WMF being concerned about it, I'm a little surprised that we are not [18:46:31] but then I guess if an attacker has write access to PHP temp files in production, they can probably use it in different but equally bad ways already [18:47:52] but at a minimum it could be used to turn a reflected or otherwise hard code execution attack into a permanent one [19:01:43] tgr: OK. I'll stall the parent-parent task on security review. But meanwhile, I suppose we could move forward by keeping the default on CDB. the main point is to enable caching on disk, the php array is mainly an optimisation what we're doing for WMF, but if we're not yet ready to do that by default, I'm fine with post-poning that. [19:02:10] tgr: also for WMF (and other sites using recache=false) the files can be made read-only for MW web, and write from a different user, like we do in prod. [19:02:22] generated from a maintenance script/scap [19:02:36] They should/could already be read-only. [21:26:46] Krinkle: in general for the DB -> file switch, the main issue will be permissions, as others said. That's no reason not to do it (reducing page load times is great!), but there should be an easy way to revert back to the old settings for a while, IMO [21:27:11] which is to say, the old DB-based class should be kept around as non-default for a few releases [21:28:15] tgr: yeah, I have no plans to remove support for it. It is already configurable via wgLocationCacheConf['store']