Make WordPress Core

Opened 15 years ago

Closed 12 years ago

Last modified 12 years ago

#14184 closed defect (bug) (fixed)

HTTP API is not allowing '0' for headers and cookies.

Reported by: mailnew2ster's profile mailnew2ster Owned by: dd32's profile dd32
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: HTTP API Keywords:
Focuses: Cc:

Description

Hello!

I just saw you have a nice implementation of an http class, but while looking at it, I've noticed a minor bug:

You use the empty() function to check whether strings are empty, for example in the WP_Http_Cookie class. This is not a correct thing to do, because empty() returns true also for a "0" string, which is not really empty. That way, a cookie like that: "balloons=0", will just be skipped!

You'd better use a ($string == "") comperation.

Cheers :)

Attachments (3)

.diff (7.5 KB) - added by mailnew2ster 15 years ago.
14184.diff (2.7 KB) - added by jacobsantos 15 years ago.
Patch which tests for strings when required and comments areas where it is not.
14184.2.diff (2.7 KB) - added by mailnew2ster 15 years ago.
forgot the cookie name

Download all attachments as: .zip

Change History (31)

@mailnew2ster
15 years ago

#1 @dd32
15 years ago

  • Keywords has-patch needs-testing added; empty 0 zero http class removed

How many of those changes are actually required? From what i can see, Most of it is simply defensive coding rather than fixing the bug.

I also notice $length = hexdec( $match[1] ); in there, I guess thats a separate bug?

#2 follow-up: @mailnew2ster
15 years ago

Most of them are. empty() is used, for example, to check whether the response is empty, which "0" is not, etc.
As of $length = hexdec( $match[1] );, empty() used there to check for "0", but it does not return true for, eg., "00", thus it's better to use hexdec first, and then check the integer for 0.

#3 @jacobsantos
15 years ago

  • Line 243: Not required, empty is not only checking for the string is empty, but also for null, 0, etc. The next empty is making not only sure that the URL is "valid" but also that there is a scheme part of the URL.
  • Line 282: Again, $rbody? might be null, 0, false, so not change.
  • Line 428: Seriously, not everything is a string comparison. No change.
  • Line 437: No change required.
  • Line 447: Fixes original ticket, probably needed.
  • Line 515: Actually, for the Chunked Decode, every resource and test has reliably came up with '0' as the end of the chunks and not '00', so no modification required.
  • Line 697: Meh, either way is fine.
  • Line 758: If it isn't broke, don't 'fix' it.

There are more, but these are the ones I see so far.

  • Line 1037: Same as line 282, the empty check is required.

#4 in reply to: ↑ 2 @jacobsantos
15 years ago

Replying to mailnew2ster:

Most of them are. empty() is used, for example, to check whether the response is empty, which "0" is not, etc.

I disagree, in many of the cases the empty() check is needed, because we aren't just comparing against strings and others because it won't matter. Line 447 is the only one I can see so far looking through half the ticket where changing it fixes a behavior problem. The reason are either checking for values that are empty other than "", or are strings in situations where the other empty values won't be an issue.

As of $length = hexdec( $match[1] );, empty() used there to check for "0", but it does not return true for, eg., "00", thus it's better to use hexdec first, and then check the integer for 0.

During my research all of the examples and live tests had '0' as the last number. If you are seeing '00', then yes, I think for optimization, it should be corrected. Take for example if '00' is encountered, nothing adverse is going to happen. The length check is going to return 0, so nothing is going to be appended to the body and it will end the loop. I believe more servers are going to be sending a '0' for the ending chunk than '00'.

Correct me if I'm wrong.

#5 @jacobsantos
15 years ago

These look as if they need to be fixed as well, meaning the patch does so.

  1. Line 1738
  2. Line 1817 (second empty)

Question: Do you think we are amateurs and don't realize proper string comparisons?

I will repeat, many of the empty checks aren't simply checking for an empty string. Some of the checks are also checking for existence and if you were to do that comparison, it would require adding an isset() check. Some of the checks are also to prevent having to go back and correct server configurations that give the wrong data.

In the case of content-encoding, what if the server decides that 0 is the way for the server to say that there is none. If we're to use your 'fix', then we'd have to drill down first what is causing the problem and then change it back. The header is a good point, some custom headers might use 0 as a legitimate value and cookie values probably might use it as well. Everything else, I'd leave as is.

#6 @mailnew2ster
15 years ago

Well, yes, most of these cases will never be noticed... Like nobody will download a page with "0", or use a "0" protocol (0:sth/, I guess it's possible), and nobody will even notice that one of the cookies doesn't get saved, but imo it's a bad style of coding, and these are bugs.

Also, I don't really understand the logic behind empty("0") returning true. It's not empty, it's a string and has a length of one character, check strlen("0").

During my research...

There are rfcs, and I think code should be written according to what is written there, and not according to your own researches.
I did not read the rfcs, but php.net suggests 00 is also a posibility:
http://www.php.net/manual/en/function.http-chunked-decode.php

Question: Do you think we are amateurs and don't realize proper string comparisons?

Of cource not :)
I'm the amateur here, but I still managed to find some bugs quite easily, and I present some oppinions and suggestions according to what I think.

it would require adding an isset() check.

I'd prefer adding it. Maybe it's also a matter of coding style...

#7 @jacobsantos
15 years ago

I agree that the cookie and header are bugs and should be fixed. The others, I will say no. I have argued coding style before and it is more or less like talking to a brick wall. Get on IRC and talk to one of the core committers and have at it. If they decide that all string comparisons are going to be checked with $var == '', then so be it.

The research was using both the RFCs and using live data and looking at the raw chunk-encoded body. In all of the cases I've looked at (the raw chunked encoded body), the last was always a '0'. That is not to say that corrections or errata of the standard won't change that or that it will always be that way. It is to simply say that live data at that time did not suggest that two digits for the last chunk were used and the implementation reflected that.

I can't tell you how much time, testing, and effort went into checking and double checking against the RFCs, live data, and unit testing against real servers. What you read in the RFC is sometimes not what you see in live environments. Always be "liberal with input and conservative with output." http://www.postel.org/postel.html Meaning accept as much as you can and try to correct another mistakes.

@jacobsantos
15 years ago

Patch which tests for strings when required and comments areas where it is not.

#8 @jacobsantos
15 years ago

New patch only patches areas that match bug. Should satisfy bug report. Patch is based off of existing patch.

Suggestion for coding style changes is to create a new ticket, where it can be debated further. If we are going to change how the strings are tested, then it will have to be debated on wp-hackers and in another ticket. It will also require changes in other areas, as most likely the empty() check is used throughout WordPress.

#9 @jacobsantos
15 years ago

  • Summary changed from empty() and "0" in class-http.php to HTTP API is not allowing '0' for headers and cookies.

#10 @mailnew2ster
15 years ago

A name of a cookie can also be '0', you missed that.

If we are going to change how the strings are tested, then it will have to be debated on wp-hackers and in another ticket. It will also require changes in other areas, as most likely the empty() check is used throughout WordPress.

I agree that the comparison method has to be uniform, but it also has to be correct. As you can see, the usage of empty() led to several bugs, and after fixing them, the comparison method is no more uniform, so what's the point? I'd avoid using empty(), because it doesn't do what the name suggests (for the string type, at least). But it's up to you, I'm not going to open new tickets about it or discussing it on IRC channels.

@mailnew2ster
15 years ago

forgot the cookie name

#11 @dd32
15 years ago

thanks for the extra details mailnew2ster I'll go through both the original and updated patches and test them all out, Thanks for this and the other patches on the other tickets.

jacob: While empty() is the general coding standard used by WordPress, isset() / '' === is also used where appropriate, Doing a string comparison for every case is going to just end up as ugly code, but there are times that its appropriate, There are also times where empty() is appropriate due to the expected contents, and expected input (For example, having a scheme of 0 is unlikely, and completely unexpected in terms of HTTP access, therefor, empty() would be appropriate)

#12 @jacobsantos
15 years ago

I hope I never have to see a cookie named '0'. Actually, one of the problems with the header check was that, I believe all of the values would never fail the empty check, because of the space between the colon and the header value.

#13 @nacin
14 years ago

  • Milestone changed from Awaiting Review to Future Release

#14 @dd32
13 years ago

  • Owner set to dd32
  • Status changed from new to accepted

After testing cases around using "empty" values in headers and cookies, the only place I've been able to verify a bug exists, is sending a cookie such as 'test=0', as the 0 trips the empty() check, causing no cookie to be sent to the server (This doesn't affect receiving cookies).

As a result, The last chunk of 14184.2.diff is needed.

The 1st/2nd chunk revolving around !empty( $value ) causes a bug where headers in the format of 'header: 0' fail to be returned, the change doesn't seem to fix any cases I can find.

The 2nd last chunk around if ( empty($pair) ) is also not needed, as only non-empty values should ever exist within a cookie pair (That being, a 'key=name" combination, The only case this would fix would be a 0 standing alone in the cookie value - something that should not happen, and therefor is safe to discard. )

Last edited 13 years ago by dd32 (previous) (diff)

#15 @dd32
13 years ago

In [21227]:

WP_HTTP: Allow for cookies with "empty" values be sent, this affects sending cookies such as test=0, which would previously fail. Props mailnew2ster for initial patch. See #14184

#16 @dd32
13 years ago

  • Keywords has-patch needs-testing removed
  • Milestone changed from Future Release to 3.5

Leaving this open for triple confirming all issues discussed above are covered.

I do want to revisit the Header checks (Specifically the problem where empty() should never be true, given the space between the colon and the value) though to make sure I didn't miss something.

#17 @dd32
13 years ago

In [21230]:

WP_HTTP: Remove a empty() check that would never be empty due to the format of HTTP Headers, value would always contain at least a space followed by the value. See #14184

#18 @dd32
13 years ago

In addition to the above, both Streams and fsockopen can send empty headers ("Header: ") without problem, however, curl doesn't support this (the header isn't sent). They all send "Header: 0" without a problem.

According to http://curl.haxx.se/mail/lib-2006-01/0249.html you can use 'Header: ""' for clients servers that follow the RFC, unfortunately that didn't work with my testing (PHP see's "" as the value).

Other than adding the extra documentation link, I see nothing more that can be done here, nor any bugs related in our code.

Last edited 13 years ago by dd32 (previous) (diff)

#19 @dd32
13 years ago

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

In [21231]:

WP_HTTP: Add a @link to the Chunked Decode RFC for documentation of how to decode the data correctly. Props mailnew2ster. Fixes #14184

#20 @mailnew2ster
13 years ago

What about

empty($r['body'])

?

It makes your code fail to process a "0" body. Maybe there is near to zero chance one would actually send a request with 'body' equal to "0", but it's possible.

I don't like this empty construct, it's misleading. It defines "0" as empty, while it's a string with a length of 1 char, not so empty, is it?

I would avoid using it in my project. My 2 cents.

#21 @dd32
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It makes your code fail to process a "0" body. Maybe there is near to zero chance one would actually send a request with 'body' equal to "0", but it's possible.

Changing that seems like it'll be a bit harder than simply replacing it. Although ! isset( $r['body']) seems like the best fit there, It doesn't deal with empty arrays or objects, or cases where false is passed. Although you may think "That's not what you should be passing.." It's an expected behaviour due to php developers relying on the way that empty() works.

A personal vendetta against the way a PHP function works isn't a valid reason to stop using it in WordPress, however, I'm re-opening this to look into that case a bit closer.

#22 @dd32
13 years ago

  • Status changed from reopened to accepted

#23 @mailnew2ster
13 years ago

I suggest using the following empty_str function for testing string values for emptiness:

function empty_str(&$str)
{
        return empty($str) && !(isset($str) && $str === "0");
}

A test script:
http://ideone.com/3RaCK

What do you think?

#24 follow-up: @dd32
12 years ago

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

In [22047]:

Allow wp_remote_post to send a body consisting of entirely '0', which may be used when PUT'ing or POST'ing data to a API which accepts a raw chunk of data rather than key=value pairs (Such as some REST API's). Fixes #14184

#25 in reply to: ↑ 24 @duck_
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to dd32:

In [22047]:

Allow wp_remote_post to send a body consisting of entirely '0', which may be used when PUT'ing or POST'ing data to a API which accepts a raw chunk of data rather than key=value pairs (Such as some REST API's). Fixes #14184

Warning: strlen() expects parameter 1 to be string, array given in wp-includes/class-http.php on line 172

We're doing strlen( $r['body'] ) and then immediately checking if is_array( $r['body'] ) || is_object( $r['body'] ).

#26 @dd32
12 years ago

In [22055]:

Avoid a warning in wp_remote_post() when using arrays or objects in the body param by using a more verbose check, introduced with [22047]. See #14184

#27 @dd32
12 years ago

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

#28 @mailnew2ster
12 years ago

Quite an ugly solution, if you ask me. Oh well...

Note: See TracTickets for help on using tickets.