update_post_meta removes all slashes

[5ubliminal]

update_post_meta with a new meta_key forwards the call to update_metadata and then it gets forwarded to add_metadata.

Funny thing is that both functions stripslashes_deep() the meta_value so I kind of remain slash-less in the process.

update_post_meta(1, 'new_meta_key', "x\\\\y"); // Adds 'xy' instead of 'x\y'

Am I missing something? This internal slashes abuse scares me. I can't come up with a fix because this slashes fetish is beyond me.

Thanks.

PS: It happens in 3.1.2 but can't find it in the list.

[Denis-de-Bernardy]

#12416 (and quite a few other slash-related tickets, e.g. #5791, #17018)

[jayarjo]

Not sure why this has been closed, since the bug is still present in emerging 3.3 (​http://core.trac.wordpress.org/browser/trunk/wp-includes/meta.php#L118). And while referenced tickets are related, they do not exactly cover mentioned bug. meta_value is stripslashed twice for meta that is being added (not updated) - here: ​http://core.trac.wordpress.org/browser/trunk/wp-includes/meta.php#L118 and then here: ​http://core.trac.wordpress.org/browser/trunk/wp-includes/meta.php#L47. This effectively strips off all slashes, even those that are meant to stay on the string.

[scribu]

Saying "This effectively strips off all slashes, even those that are meant to stay on the string." is equivalent to saying "update_post_meta() expects slashed values."

So #12416 does cover this.

[5ubliminal]

The just don't give a *#@!. Unless you're down with a Nacin dude, no fixes or improvements will go through. It's a cartel. They concentrate on the visual bling and keep the advanced plugin developers in pain. Don't hold your breath, rewrite the functions or hack the core. This fix ain't gonna happen any time soon.

Slash everything for security is like #$#! for virginity.

[SergeyBiryukov]

Version number is used to track when the bug was initially reported.

[scribu]

meta_value is stripslashed twice for meta that is being added (not updated)

I see what you mean.

[nacin]

Replying to 5ubliminal:

You do this all of the time. It's getting rather old. It's difficult enough keeping this on Twitter. Can you please confine it there? Some people here are trying to work.

There are something like 3000 open tickets right now. Dozens opened every week. When they get triaged, small details sometimes get missed. It happens. This does not mean there is a conspiracy. There is no cartel. If there is, I'm certainly not a member, let alone the leader. (That'd be cool, though, right? Running a bug-hunting cartel? I digress.)

You could have commented five months ago, offering clarification, but you didn't. scribu, a different contributor than the original reviewer, still didn't see it at first glance. Instead of clarifying -- in your first comment to your own ticket -- you went on a rant.

When I saw this ticket, I missed it just the same. But looking at the code with scribu, yes, we now see the bug. I've committed unit tests to confirm the bug (​[UT445]), and 17343.diff​ fixes it. I'll leave it for ryan's review in the morning.

[5ubliminal]

I posted a clear example. Somebody could have just run it.

Btw, you need to dig deeper in all functions where stripslashes(_deep) is called as this is not only in the function I pointed out. It's a widespread issue.

The stripslashes has to go. I understand the backward compatibility issues but it's impossible to guess where I need to slash, double slash, triple slash or no slash and nobody has time to check the core code in each function before using it. Those who don't escape their SQL properly deserve what's coming their way, it's not your job to enforce security and minimal programming practices. PHP 4 is long gone (I hope), RegisterGlobals also (I do hope) and those who don't escape SQL or typecast numbers should also go.

Think about plugin developers first, then pimp the UI which is already great. I understand WP is user and not dev-centric. That's why you're just popular, totally not developer friendly... and still far from your potential.

PS: I reserve the right to think that if any of my tickets was posted by your buddies, it would have gotten a second glance and maybe a fix.

[jane]

@5ubliminal: Denis-de-Bernardy is the one who closed the ticket, not Nacin, so you're basically being a jerk on this ticket, and it's not cool. Please keep the tone professional.

Lead devs and committers count on volunteer bug gardeners to help with triage, and the original reporters to keep an eye on their bug tickets. If your bug was closed by Denis 5 months ago, and you felt he was wrong to do so, you should have followed up on it. If you want to report bugs, that's awesome, but you also need to monitor your tickets and advocate for them if you want to be a trusted core contributor. Drive-by bug reports with no reporter follow-up sometimes are simple fixes that get committed quickly, but more often there is significant back-and-forth discussion to get to the best solution. Posting a bug on Trac rather than in the support forums or to the wp-testers list generally implies an interest in the development of a fix.

Oh, and if you want to imply conspiracy, I'd say the core team is more cabal than cartel. :)

[5ubliminal]

@jane: If you looked at my other reports, you'd have seen advocating. Those were improvements, this is an error. You advocate improvements (they're in my best interest) and you assume errors get checked and fixed (it's in everyone's best interest, especially yours).

Do you think I'm advocating here: ​http://core.trac.wordpress.org/ticket/17805? Please, next time you come break a fight, make sure you're in the know. Makes you look good ;)

[nacin]

Replying to 5ubliminal:

@jane: If you looked at my other reports, you'd have seen advocating.

If we can't see what the error is, we can't fix it. It's up to you to ensure we can understand it. #17805 is irrelevant here, and was wontfixed for the reasons stated therein. In fact, any other report is irrelevant. You did not advocate here. You therefore have no reason to complain.

We're now off topic for this ticket.

[jane]

Replying to 5ubliminal:

If you looked at my other reports, you'd have seen advocating.

Beside the point. A number of contributors focus on specific areas, so a) they wouldn't see your other tickets, and b) it's irrelevant if the point is that if you care about a ticket you'll give it the same attention you wont the committers to give it.

Those were improvements, this is an error. You advocate improvements (they're in my best interest) and you assume errors get checked and fixed (it's in everyone's best interest, especially yours).

Actually, in an open source project, the only way you can count on something getting fixed is to fix it. The committers are not servants, they're leaders, and the work they do includes prioritizing which bugs are high priority and which ones aren't as important because they affect fewer people. Aside from security issues (which immediately generate an all-hands-on-deck response but should not be posted in Trac anyway, but reported to security@…) and task (blessed) tickets (which leads have committed to themselves), this is an open development environment, and the assumption that "someone" will fix all reported bugs is incorrect. Bugs are fixed if/when individual people fix them. The best way to influence which things get priority attention is to contribute consistently good patches and earn trust and respect here. Insulting people who slog through hundreds of tickets per day doesn't help anyone.

[ryan]

Patch looks good.

[nacin]

In [18816]:

Pass unslashed values from update_metadata() to add_metadata(). fixes #17343.