Templates get transcluded in (un)delete reason for associated talk page
Closed, ResolvedPublic2 Estimated Story PointsBUG REPORT
Actions

Description

What is the problem?

When deleting or restoring a page with its associated talk page, if the reason includes a template it will be transcluded in the associated talk page reason.

For example, deleting a page with reason {{age|1989|7|23}} will lead to two entries in Special:Log and Recent Changes (also see screenshot):

<timestamp> <user> deleted page <page> (<num revisions deleted>) ({{age|1989|7|23}})
<timestamp> <user> deleted page <talk page> (<num revisions deleted>) (Deleted together with the associated page with reason: 32)

In the database, the first is stored as:

+-------------------+
| comment_text      |
+-------------------+
| {{age|1989|7|23}} |
+-------------------+

The second as:

+------------------------------------------------------------+
| comment_text                                               |
+------------------------------------------------------------+
| Restored together with the associated page with reason: 32 |
+------------------------------------------------------------+

Steps to reproduce problem

Login as an admin to beta
Go to any page with a talk page (or create one for an existing page)
More > Delete
In "Other/additional reason:" enter {{age|1989|7|23}}
Check "Delete all revisions of the associated talk page"
Submit
Go to Special:Log

Expected behavior: The two log entries for the page you just deleted is something like:

<timestamp> <user> deleted page <page> (<num revisions deleted>) ({{age|1989|7|23}})
<timestamp> <user> deleted page <talk page> (<num revisions deleted>) (Deleted together with the associated page with reason: {{age|1989|7|23}})

Observed behavior: The two entries like:

<timestamp> <user> deleted page <page> (<num revisions deleted>) ({{age|1989|7|23}})
<timestamp> <user> deleted page <talk page> (<num revisions deleted>) (Deleted together with the associated page with reason: 32)

Environment

Wiki(s): https://en.wikipedia.beta.wmflabs.org MediaWiki 1.39.0-alpha (b6d486b) 08:01, 19 April 2022.

Screenshots

Details

Subject	Repo	Branch	Lines +/-
DeletePage, UndeletePage: use plaintextParams when creating log message	mediawiki/core	wmf/1.39.0-wmf.8	+2 -2
DeletePage: use plaintextParams when creating log message	mediawiki/core	REL1_38	+1 -1
DeletePage, UndeletePage: use plaintextParams when creating log message	mediawiki/core	master	+2 -2

Customize query in gerrit

Related Objects
Search...

		Status	Subtype	Assigned	Task
		Resolved	Release	jeena	T305214 1.39.0-wmf.8 deployment blockers
		Resolved	BUG REPORT	MusikAnimal	T306431 Templates get transcluded in (un)delete reason for associated talk page

Event Timeline

dom_walden created this task.Apr 19 2022, 10:51 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 19 2022, 10:51 AM

I think the call to MessageValue::param here should be replaced with either plaintextParam() or rawParam(). Which one to use depends on the escaping, which I didn't check.

dom_walden mentioned this in T305523: Undelete associated talk page does not give same reason as deleting.Apr 19 2022, 1:07 PM

Change 784285 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):

[mediawiki/core@master] DeletePage: use plaintextParams when creating log message

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

Change 783911 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):

[mediawiki/core@wmf/1.39.0-wmf.8] DeletePage: use plaintextParams when creating log message

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

Change 783911 abandoned by MusikAnimal:

[mediawiki/core@wmf/1.39.0-wmf.8] DeletePage: use plaintextParams when creating log message

Reason:

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

Change 783911 restored by MusikAnimal:

[mediawiki/core@wmf/1.39.0-wmf.8] DeletePage: use plaintextParams when creating log message

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

MusikAnimal claimed this task.Apr 19 2022, 4:34 PM

MusikAnimal edited projects, added Community-Tech (CommTech-Sprint-23); removed Community-Tech.

In T306431#7863898, @Daimona wrote:

I think the call to MessageValue::param here should be replaced with either plaintextParam() or rawParam(). Which one to use depends on the escaping, which I didn't check.

Thank you! I went with plaintextParam() which sounds like what we want, but to be honest I am not certain. At any rate, even if rawParams() is better, we for sure don't want to ship this feature using params() as we could permanently corrupt data in the log. I.e. if the admin uses the default content was: message, it could contain a template that has a LOT of content that then gets substituted. We'll backport this fix to 1.38 as well.

Thanks Dom for finding this!

Change 784285 merged by jenkins-bot:

[mediawiki/core@master] DeletePage, UndeletePage: use plaintextParams when creating log message

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

Change 783913 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):

[mediawiki/core@REL1_38] DeletePage, UndeletePage: use plaintextParams when creating log message

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

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.9; 2022-04-25).Apr 19 2022, 5:00 PM

Change 783913 merged by jenkins-bot:

[mediawiki/core@REL1_38] DeletePage: use plaintextParams when creating log message

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

MusikAnimal added a parent task: T305214: 1.39.0-wmf.8 deployment blockers.Apr 19 2022, 5:57 PM

ReleaseTaggerBot added a project: MW-1.38-notes.Apr 19 2022, 6:00 PM

Chances are somewhat slim, but given this could cause permanent data corruption of log entires, I've added this as a train blocker. Hence raising to UBN

In T306431#7864890, @MusikAnimal wrote:

In T306431#7863898, @Daimona wrote:

I think the call to MessageValue::param here should be replaced with either plaintextParam() or rawParam(). Which one to use depends on the escaping, which I didn't check.

Thank you! I went with plaintextParam() which sounds like what we want, but to be honest I am not certain.

I think it's better to be safe and use plaintextParam: rawParam skips escaping, so you may introduce some kind of injections if the input is not already escaped (which does seem to be the case).

At any rate, even if rawParams() is better, we for sure don't want to ship this feature using params() as we could permanently corrupt data in the log. I.e. if the admin uses the default content was: message, it could contain a template that has a LOT of content that then gets substituted. We'll backport this fix to 1.38 as well.

I don't think any corruption could happen: the template is parsed when inserting the log entry, and the expanded version should be saved in the database.

Also... This made me realize that the message used as reason is formatted as text (i.e. Message::text()). I wonder if it should use Message::plain() instead.

In T306431#7865412, @Daimona wrote:

At any rate, even if rawParams() is better, we for sure don't want to ship this feature using params() as we could permanently corrupt data in the log. I.e. if the admin uses the default content was: message, it could contain a template that has a LOT of content that then gets substituted. We'll backport this fix to 1.38 as well.

I don't think any corruption could happen: the template is parsed when inserting the log entry, and the expanded version should be saved in the database.

Right, but saving the expanded version is irreversible, is what I mean by corruption. As unlikely as it may be, the deleted content could have {{some template containing lots of inappropriate text}} and they use the automatic deletion summary (which I would guess is a more common practice on group0). Hopefully I'm not causing unnecessary ruckus by making this a train blocker! But I would feel bad if this shipped and folks had to hide deletion summaries.

Also... This made me realize that the message used as reason is formatted as text (i.e. Message::text()). I wonder if it should use Message::plain() instead.

I'm not sure. In my testing we can still use internal links (i.e. [[test]]) but everything else is saved as plain text, which I think is what we want.

Change 783911 merged by jenkins-bot:

[mediawiki/core@wmf/1.39.0-wmf.8] DeletePage, UndeletePage: use plaintextParams when creating log message

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

Mentioned in SAL (#wikimedia-operations) [2022-04-19T20:26:54Z] <urbanecm@deploy1002> Synchronized php-1.39.0-wmf.8/includes/page/DeletePage.php: rMWf1ebd29c3c62: DeletePage, UndeletePage: use plaintextParams when creating log message (T306431; 1/2) (duration: 00m 50s)

Mentioned in SAL (#wikimedia-operations) [2022-04-19T20:27:45Z] <urbanecm@deploy1002> Synchronized php-1.39.0-wmf.8/includes/page/UndeletePage.php: rMWf1ebd29c3c62: DeletePage, UndeletePage: use plaintextParams when creating log message (T306431; 2/2) (duration: 00m 50s)

@MusikAnimal can this be marked as resolved or are we waiting on something else?

Yes I think we can go ahead and mark as resolved. Tested working on testwiki, and it's in the wmf.8 branch. Thanks to everyone who helped fix this bug, and especially to Dom for identifying it!

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.8; 2022-04-18); removed MW-1.39-notes (1.39.0-wmf.9; 2022-04-25).Apr 19 2022, 9:00 PM

Templates get transcluded in (un)delete reason for associated talk pageClosed, ResolvedPublic2 Estimated Story PointsBUG REPORTActions

Description

What is the problem?

Steps to reproduce problem

Environment

Screenshots

Details

Related ObjectsSearch...

Event Timeline

Templates get transcluded in (un)delete reason for associated talk page
Closed, ResolvedPublic2 Estimated Story PointsBUG REPORT
Actions

Related Objects
Search...