WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#13317 closed defect (bug) (fixed)

Code Improvement in get_userdata

Reported by: hakre Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Minor code improvement.

Attachments (9)

13317.patch (375 bytes) - added by hakre 5 years ago.
13317.2.patch (497 bytes) - added by hakre 5 years ago.
if you're in for ! (instead of 0 == ), spare the absint() call while userid is false
13317-test_int.php (8.7 KB) - added by hakre 5 years ago.
Testcases
13317-test_int_result.txt (12.3 KB) - added by hakre 5 years ago.
Testresults
13317-a-little-more-strictness.patch (850 bytes) - added by hakre 5 years ago.
13317-better-not-propagate-absint.patch (26.5 KB) - added by hakre 5 years ago.
13317.3.patch (1.1 KB) - added by hakre 5 years ago.
One must not use absint() or intval()
13317-negative-values-equal-zero.patch (385 bytes) - added by hakre 5 years ago.
13317-negative-values-equal-zero.2.patch (475 bytes) - added by hakre 5 years ago.
updated using is_scalar() instead of is_numeric()

Download all attachments as: .zip

Change History (45)

@hakre5 years ago

comment:1 follow-up: @scribu5 years ago

  • Priority changed from normal to lowest

IMO, minor code improvements like this should be bundled with other patches.

The time wasted creating this ticket and committing the patch by a core dev outweighs it's usefulness.

comment:2 in reply to: ↑ 1 ; follow-up: @nacin5 years ago

Replying to scribu:

IMO, minor code improvements like this should be bundled with other patches.

+1

The time wasted creating this ticket and committing the patch by a core dev outweighs it's usefulness.

+10

This is a really, really silly patch. Not only that, but a == check on 0 should just be !$user_id instead.

comment:3 in reply to: ↑ 2 @hakre5 years ago

I thought patch-files were the common way to do it, but I'm flexible. So back on tra(c)k, please make your choice:

[ ] A - Commit
[ ] B - Close as wontfix
[ ] C - Punt to future release
[ ] D - Commit with nacin's additional suggestion
[ ] E - Commit with nacin's suggestion and take into account 
    that it is possible to spare the absint() call for the 
    case of false.
[ ] F - Wait for an additional patch and then start another 
    discussion right away after.

I would prefer E to finally give this discussion some value. Nacin, please let me know if you can do the change of those ca. three lines of code on your own or you want a patch-file for it. I personally now prefer the first variant. You're the committer, just let me know which way you prefer it.

comment:4 @hakre5 years ago

I finally must admit that those useless comments might have distracted me somehow and in kind of a bad influence.

Naturally my original patch is totally okay and nacin's suggestion to it are infact silly.

It's common practice in core code when it comes to a comparison against zero for a userid to do it with the numerical value and the standard php comparison operator ==. Using false or ! would be misleading.

So my new choice is A.

I hope some other comitter is able to concentrate on the actual work instead of trying to find an argumentation to no do things. It's especially a problem for me to see a really useless discussion here instead of a quick and straight ticket handling. It's not me who is wasting time in the end. Please deal with such minor changes more efficient next time to safe us all some time to concentrate on more important stuff. Thanks.

comment:5 @nacin5 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [14579]) Whitespace and standards in get_userdata. fixes #13317.

@hakre5 years ago

if you're in for ! (instead of 0 == ), spare the absint() call while userid is false

comment:6 @hakre5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

to give the discussion some value in collaborative work.

comment:7 @nacin5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

If the user_id is nonnumeric for some reason, that will absint() to 0 and thus compute to false. Moving the absint down would break that. I find ! $user_id easier to read. Yes, 0 == $user_id works, and perhaps is more clear with absint. I'm not inclined to change this again.

comment:8 @hakre5 years ago

A non-numeric user id? I have not seen that but for requesting the admin user obect:

$admin_user = get_userdata( array( 'theee_admin();' ) );

This one at least works for 99% of all installs and I think we must keep it backwards compatible. I've seen that with some plugins.

I was making up my mind a bit what the difference between just using (int) abs( $val ) instead of absint( abs( intval( $val ) ) ) would be. I have not found any difference by now, so it might be of some value to remove absint() from code and replace it with that more effective variant.

Please let me know if you want a patch.

comment:9 @hakre5 years ago

  • Keywords dev-feedback added

comment:10 follow-up: @Denis-de-Bernardy5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening, since there seems like there's potential for security issues here...

comment:11 @Denis-de-Bernardy5 years ago

  • Component changed from General to Security
  • Priority changed from lowest to high
  • Severity changed from normal to major

comment:12 in reply to: ↑ 10 @nacin5 years ago

Replying to Denis-de-Bernardy:

Reopening, since there seems like there's potential for security issues here...

Pardon me for having no idea what you're talking about.. but I have no idea what you're talking about.

comment:13 follow-up: @Denis-de-Bernardy5 years ago

What I'm meaning is that get_userdata(garbage) should not return an admin user on 99% of sites. It should fail and return false, zero, whatever. If, for any reason, a plugin uses the function improperly, it may very well introduce security risks. And nevermind that you can't see one. I'm sure you couldn't see any in the eval() calls used in permalinks either. But low and behold, they got exploited.

@hakre5 years ago

Testcases

@hakre5 years ago

Testresults

comment:14 @hakre5 years ago

Finally I was able to run some tests to find out more. The Absint() function should be put on the deprecated list and replaced with a single function call:

abs( (int) $val );

because it just does the job, this behaves absolutely the same as absint( $val );. I've attached a detailed testfile and exemplary output results.

comment:15 follow-up: @nacin5 years ago

The Absint() function should be put on the deprecated list and replaced with a single function call

We use absint() everywhere -- it's a well-known shortcut for plugin developers that does abs( intval( $val ) ), or alternatively abs( (int) $val ). It's not going anywhere. We didn't need the test cases to prove that they're the same :-)

What I'm meaning is that get_userdata(garbage) should not return an admin user on 99% of sites. It should fail and return false, zero, whatever.

Garbage will almost always evaluate to false on absint(), which will mean get_userdata will return false. I'm still missing the point.

comment:16 in reply to: ↑ 15 @Denis-de-Bernardy5 years ago

Replying to nacin:

Garbage will almost always evaluate to false on absint(), which will mean get_userdata will return false. I'm still missing the point.

That would mean you haven't read hakre's comment or tested the code in it further up.

comment:17 follow-up: @nacin5 years ago

I don't see why we need to protect against garbage inputs like array( 'theee_admin();' ). We're expecting an integer, so we absint() what we get. If we get an object, we'll throw a notice.

Are you looking for an is_int()? Should we is_int() check every variable before absint'ing it, when all we expect is an integer to begin with?

comment:18 in reply to: ↑ 17 @hakre5 years ago

Replying to nacin:
You should not concentrate too much why to not make a change but why. In this case (and be lucky it got noticed) this function should not return the admin user object on error. so, you do not need to run all core up and down and insert checks, but just in places where it can actually make sense.

Any way, I'll add a patch for that one and for the general replacement of the absint() calls.

comment:19 @hakre5 years ago

the absint() function is useless while casting is taken into account. therefore deprecate it and teach users a better way to use: abs( (int) $var );.

Related: #4762

I'll add another one that will get grip on needless intval() usage.

comment:20 follow-up: @nacin5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

absint() isn't going anywhere.

Not only is it way more readable -- you'd have to sneak it into a larger patch for me to commit that -- but we also use it throughout core for array_map callbacks.

We want to encourage integer casting for security. Let's not make it more difficult. It is also easier to document and teach a function than casting or coercion.

Additionally, it is *not* a "better way." That is your opinion. Both work quite well.

You should not concentrate too much why to not make a change but why.

I am firmly and proudly in the 'why not' camp. Changes should be sensical and not based on a "just because we can" philosophy.

Finally, there is a difference between returning an admin user object on error and stuffing absolute garbage into functions.

comment:21 @hakre5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Putting it into deprecated really got over the top. The usage as callback I have not checked, my fault.

But that does not make the overexagerated usage of absint() all over the place better.

I think you're in the 'why not' spread absint() all over the place "just because we can" camp. I do not share both points. Wether the why not nor the all over the place. You must really misread me.

I'll reopen, add (int) to where it is fit at least. But it might be even too much for you, because you use intval() just because you can, right? and yes, why not .... .

@hakre5 years ago

One must not use absint() or intval()

comment:22 in reply to: ↑ 20 @hakre5 years ago

Replying to nacin:

Finally, there is a difference between returning an admin user object on error and stuffing absolute garbage into functions.

Yeah, totally right. I just wonder why - when the docblocks already document that $user_id is to be int already you run absint() on it anyway. I mean, it's already an integer, and if some adds garbage into this function, like a negative integer, the function is not expceted to return a user, right?

You should really reflect what you say here in the end.

Getting the admin when passing -1 to that function is not an equally well thing either. But, let's better not do that strict, this is only about user-management which is knowing to be an area, where to do things in a secure manner isn't further important.

Sorry for so much irony.

comment:23 @hakre5 years ago

Gosh, there are really tons of the array_map('absint' calls. well, if I did knew that, I must say, why did no one thought of really making the job easier for the coders here by just allowing using absint on an array by design?

like so:

function absint( $maybeint ) {
	if ( is_array($maybeint) )
		return array_map('absint', $maybeint);				
	return abs( intval( $maybeint ) );
}

That would finally provide some use then for the wpquery subroutine. and it would make my live harder to remove it all over the place *gg*.

comment:24 @nacin5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Absolutely not. The whole point of absint is to return an integer. That would break its existing functionality of forcing an integer return. Maybe introduce absint_map(), but we cannot do what you are suggesting.

I see no compelling reason to make any changes to get_userdata, absint, or intval. "One must not use absint() or intval()" -- that's your opinion and you're welcome to have it, but we're done wasting my time here. Closing again as fixed on [14579].

comment:25 in reply to: ↑ 13 @westi5 years ago

If future can reporters please provide a more detailed ticket description hi-lighting the actual issue that they are trying to resolve.

It is very unproductive for everyone to spend the amount of time that has been invested in this ticket over a trivial coding style type discussion which is the meat of what is here.

comment:26 @westi5 years ago

  • Component changed from Security to General
  • Priority changed from high to normal
  • Severity changed from major to normal

comment:27 @westi5 years ago

(In [14695]) Don't return data for user 1 when passed in junk like an array. See #13317.

comment:28 @hakre5 years ago

  • Summary changed from Code Impriovement in get_userdata to Code Improvement in get_userdata

comment:29 @hakre5 years ago

Considering negative values as garbage as well. Unfixed for those. Please reopen and fix.

comment:30 @hakre5 years ago

is_numeric() check should be replaced with is_scalar() instead to change to a var-type-check instead of parsing the string - while it's at it.

@hakre5 years ago

updated using is_scalar() instead of is_numeric()

comment:31 @nacin5 years ago

  • Resolution fixed deleted
  • Severity changed from normal to blocker
  • Status changed from closed to reopened

is_numeric broke an instance of passing $post->post_author which was string "0" to get_userdata.

comment:32 @nacin5 years ago

(In [14780]) Revert [14695]. We need to be a bit less strict. see #13317.

comment:33 @nacin5 years ago

  • Severity changed from blocker to normal

comment:34 @nacin5 years ago

(In [14781]) False alarm, something else is going on here. Revert [14780], restore [14695]. see #13317.

comment:35 @nacin5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

For reference, handled in [14782].

comment:36 @hakre5 years ago

no comment: "hakre — 11 days ago -- updated using is_scalar() instead of is_numeric()"

Note: See TracTickets for help on using tickets.