[seba_dos1]

> However, these are no fixes. You just introduce a new variable, that you never use, and re-assign the same contents of that new variable back to the $_REQUEST

While this whole takeover thing is completely ridiculous, it's you who displays an "inexperienced eye" here. What do you think the $original_post variable (which was already there) is doing, huh?

[DanielLestrange]

The same. Nothing. The “security” issue here would be that the user callback can access post and request data. Tell me one place in the entire wp code base where that is NOT possible?

Security issues can be fixed WITHOUT renaming the plugin or removing links and text even if the original author has no access anymore

And that “fix” is ridiculous. If anything it breaks code of users who were actually adding callbacks using that data. It’s the nature of php that you can access those details - it’s up to the caller to know what to do with it. If anything, the usage of user callback is an issue here.

And in any thinkable case this ain’t a security fix that was done. A security fix would include that and only that change.

[DanielLestrange]

For some context how you MIGHT actually “fix” the true security concern in this code: $allowed_callbacks = ['some_function', 'another_function']; // Example of allowed functions if ( in_array($original_cb, $allowed_callbacks, true) && is_callable($original_cb) ) { $return = call_user_func($original_cb, $post); } else { // Log or handle invalid callbacks safely $return = false; }

Tampering with global variables or else is NOT a fix, and this one in particular is like pointing out a crumb on the child’s mouth and grounding it for not brushing its teeth.

You could apply a filter to allow filtering the allowed callbacks, if you really want to allow more than the hardcoded whitelist.

In the end it still boils down to “do not use user callbacks” as the better security fix, which again shows how “they” didn’t fix a thing here. This is a blatant excuse for legal CYA.

[seba_dos1]

If the codebase was built on the assumption that user callbacks will execute in a context where POST data is sanitized (which is evidenced by the code that was already there), then failing to sanitize $_REQUEST in addition to $_POST is certainly a security issue.

Of course, relying on such simplistic measures is still brittle and inelegant, but that's another matter. Reworking it would likely be quite invasive to that codebase and far beyond the scope of a security patch.

(also, frankly, the entire WordPress ecosystem isn't particularly known for its high quality codebase... this kind of "fix" is exactly what you'd expect there even without all that drama around)

> Security issues can be fixed WITHOUT renaming the plugin or removing links and text even if the original author has no access anymore

Not sure who you're arguing with there, but certainly not with me.

You have plenty of shitty behavior to call out there, so not sure why you decided to announce that there's no security issue being handled at all instead. It only makes your point weaker for no good reason.

[DanielLestrange]

If anything, the problem here is call_user_func, which when an attacker HAS ACCESS TO THE CODE, can be dangerous.

How on earth does emptying POST or REQUEST solve anything at all in regards? How on earth does, no matter what crap ACF added BEFORE the takeover, this "Fix" justify a hostile takeover? If or not there is a security issue with this code (which there IS, but not with POST or REQUEST data) is not even the matter anymore - it was and is posed and defended as a "urgent action to fix a security issue in a plugin the author has no access to"

And I repeat - there has not been any security fix!!

Read my root comment: > because the only relevant changes are actually neither introducing fixes, nor ever changing the plugin core code in a way that fixes security issues.

And I stand by that. Anyone reading this code can see it.

[seba_dos1]

> How on earth does, no matter what crap ACF added BEFORE the takeover, this "Fix" justify a hostile takeover?

You can continue arguing with yourself, but I don't need to be there.

[DanielLestrange]

Why do you not actually provide some researched facts? I mean, I am all ears to stand corrected. Yet it appears all you (and other automatticians, and/or else employees) can do is deflect and talk down pretending you know better. Do you? Then teach your fellow humans where they are wrong. So far, I still have not been proven wrong about this pretended fix, which fixed nothing at all.

[seba_dos1]

I don't intend to defend Automattic's shitty behavior at all, it's indisputably shitty and I take offense at your suggestions that I may think otherwise, as I never gave you any reason to believe that.

That said, you clearly seem to be confused about the nature of the issue being fixed there. It's clear from the existing code that the contract between this function and the callback getting executed is that the callback is expected to execute in a kind of a sanitized environment with restricted execution abilities.

You can argue that it's a really weak sandbox and that the whole design is smelly, and I'll agree with you. However, that's how this code was designed and that's what users are running on their servers, and that's how they expect it to behave. It prevents the callback from calling functions with `wp_` prefix and it disallows it from reading or making changes to user's POST data.

Now, if you find a way to circumvent those restrictions, it's an obvious security issue. Someone may have deployed some code that relied on that contract, and now that contract is known to be invalid (it always had been, but it wasn't known before). Therefore, stopping that from happening is a security fix.

It may be a poor fix - and in fact, this one is, as it's incomplete. But it's a fix. The upstream project recognized that and applied a similar, but a bit more thorough approach in its repo:

https://github.com/AdvancedCustomFields/acf/commit/a60034f8a...

It does exactly what the change we're discussing does, plus a bit more.

You still wouldn't convince me that it's a bulletproof sandbox and I already have some other ideas in my head on how it could potentially be circumvented after reading that code (though my PHP is so rusty I may very well be wrong), but the change in question is clearly a security fix, recognized and applied by both Automattic and WP Engine and I really can't understand why you're so keen on implying otherwise. As I already said, it only made your other (good) points seem weaker.