Page MenuHomePhabricator

Disallow dynamic property access on ParserOutput
Closed, ResolvedPublic

Description

Per mediawiki 1.20 the way for extensions to attach custom data to the ParserOutput was to declare a dynamic property. Now the mechanism exists, ParserOutput::getExtensionData/setExtensionData to do this properly.

We need to find all the places in the deployed extensions where dynamic property access is still happening, replace usages with a fallback on __get, wait for ParserCache expiration period, drop support for it and trigger an error if an attempt is made to use dynamic properties on ParserOutput.

It seems like a good way of tracking the usages of this is by grepping for PhanUndeclaredProperty suppression.

Event Timeline

Pchelolo created this task.Sep 25 2020, 2:35 PM

Change 630190 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Allow back getting/setting dynamic properties on ParserOutput.

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

Change 630191 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/ProofreadPage@master] Use ParserOutput::setExtensionData instead of dynamic property access

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

Pchelolo added a comment.EditedSep 25 2020, 3:48 PM

Checking all the deployed extensions/core:

Change 630208 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/LiquidThreads@master] Replace use of dynamic property in ParserOutput for extension data.

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

Change 630210 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/GeoCrumbs@master] Remove fallback for ParserOutput->mCustomData

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

Change 630190 merged by jenkins-bot:
[mediawiki/core@master] Allow back getting/setting dynamic properties on ParserOutput.

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

Change 630191 merged by jenkins-bot:
[mediawiki/extensions/ProofreadPage@master] Use ParserOutput::setExtensionData instead of dynamic property access

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

Change 630208 merged by jenkins-bot:
[mediawiki/extensions/LiquidThreads@master] Replace use of dynamic property in ParserOutput for extension data.

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

Change 630212 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/GeoData@master] Unsuppress PhanUndeclaredProperty, reason for it was removed

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

Change 630210 merged by jenkins-bot:
[mediawiki/extensions/GeoCrumbs@master] Remove fallback for ParserOutput->mCustomData

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

Change 630214 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/Insider@master] Drop fallback to reading ParserOutput:mCustomData

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

Change 630216 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/TemplateSandbox@master] Unsuppress PhanUndeclaredProperty, reason for it removed

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

Change 630212 merged by jenkins-bot:
[mediawiki/extensions/GeoData@master] Unsuppress PhanUndeclaredProperty, reason for it was removed

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

Change 630216 merged by jenkins-bot:
[mediawiki/extensions/TemplateSandbox@master] Unsuppress PhanUndeclaredProperty, reason for it removed

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

Change 630214 merged by jenkins-bot:
[mediawiki/extensions/Insider@master] Drop fallback to reading ParserOutput:mCustomData

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

Pchelolo changed the task status from Open to Stalled.Sep 25 2020, 6:19 PM

Ok, now nothing in the deployed extensions or core writes to the dynamic properties in the ParserOutput, however LiquidThreads and ProofreadPage still fallback to the property since it could be coming from the ParserCache.

Now we need to wait for at least 1 month before removing the fallbacks to make sure ParserCache has expired, after that drop the last two fallbacks and either start emitting the deprecate warning or throwing an error if the dynamic property is accessed.

SemanticMediaWiki also uses dynamic properties. We started getting a bunch of logspam in translatewiki.net yesterday after we updated to get the security fixes. We had to revert the patch locally.

Pchelolo changed the task status from Stalled to Open.EditedSep 28 2020, 3:06 PM

SemanticMediaWiki also uses dynamic properties. We started getting a bunch of logspam in translatewiki.net yesterday after we updated to get the security fixes. We had to revert the patch locally.

With the latest latest version, with partially removed deprecation, you should not be getting logspam for dynamic properties, only direct access to static properties.

I can fix SemanticMediawiki as well.

Pchelolo claimed this task.Sep 28 2020, 3:07 PM
Pchelolo moved this task from Backlog to Doing on the Platform Team Workboards (Green) board.

Blocking until ParserCache has expired and we can drop fallbacks in remaining extensions.

With the latest latest version, with partially removed deprecation, you should not be getting logspam for dynamic properties, only direct access to static properties.

Thanks! We will test this on Wednesday.

@Nikerabbit Also, the semantic mediawiki PR above should fix it on their side. If you see some more logspam, please save it somewhere, I'd go through it and fix all issues

Change 631318 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[operations/mediawiki-config@master] Reject ParserCache entries from the last wmf.11 deployment

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

Change 631320 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[operations/mediawiki-config@master] Reject ParserCache entries from the last wmf.11 deployment (2)

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

Change 631320 merged by jenkins-bot:
[operations/mediawiki-config@master] Reject ParserCache entries from the last wmf.11 deployment (2)

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

Ok, now nothing in the deployed extensions or core writes to the dynamic properties in the ParserOutput, however LiquidThreads and ProofreadPage still fallback to the property since it could be coming from the ParserCache. Now we need to wait for at least 1 month before removing the fallbacks to make sure ParserCache has expired, after that drop the last two fallbacks and either start emitting the deprecate warning or throwing an error if the dynamic property is accessed.

1 month has passed, we can remove the fallback.

Change 639534 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/ProofreadPage@master] Remove fallback to undeclared ParserOutput property

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

Change 639535 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/LiquidThreads@master] Remove fallback to dynamic ParserOutput properties

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

Change 640281 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Hard-deprecate all public paroperty access on CacheTime and ParserOutput.

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

Change 639535 merged by jenkins-bot:
[mediawiki/extensions/LiquidThreads@master] Remove fallback to dynamic ParserOutput properties

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

Change 639534 merged by jenkins-bot:
[mediawiki/extensions/ProofreadPage@master] Remove fallback to undeclared ParserOutput property

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

Pchelolo closed this task as Resolved.Thu, Nov 19, 8:49 PM
Pchelolo moved this task from Blocked to Done on the Platform Team Workboards (Green) board.

Change 640281 merged by jenkins-bot:
[mediawiki/core@master] Hard-deprecate all public property access on CacheTime and ParserOutput.

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