belgattitude / soluble-japha

PHP Java integration
https://belgattitude.github.io/soluble-japha/
MIT License
69 stars 15 forks source link

SimpleHttpHandler: fix PHP's $_COOKIE serialization #23

Closed diegosainz closed 8 years ago

diegosainz commented 8 years ago

Hi,

First for all thank you for the excellent work you have done by making the PHP/Java bridge up to date and compatible with PHP7 - we were happy at the office to find it in our transition to PHP7.

This pull request fixes an issue when there are values in PHP's $_COOKIE super global that are not simple scalars (like arrays). We stumbled upon the problem when there was a cookie in the form name[0]=somevalue set from the client that caused an "array to string conversion" exception on the bridge.

The prolematic piece of code is the following:

    /**
     *
     * @return string
     */
    public function getCookies()
    {
        $str = "";
        $first = true;
        foreach ($_COOKIE as $k => $v) {
            $str .=($first ? "Cookie: $k=$v" : "; $k=$v");
            $first = false;
        }
        if (!$first) {
            $str .="\r\n";
        }
        return $str;
    }

If $v is an array or other non-scalar value there will be an implicit conversion to string.

In this PR you'll see that objects are unsupported - I don't know if this is the right approach, but definitively I wouldn't recommend to serialize them due to the security implications.

belgattitude commented 8 years ago

Hi Diego,

Thank for your contribution... Great to feel it helps.

I'll merge the fix for now as it looks good to me, but I'm not really cookie expert. Still I'll need few hours to test it on internal projects before releasing a new version.

But could be nice if we extract the getCookie() method in it's own class (Soluble\Japha\Bridge\Http\Cookie for example)... and create an associated test class. This way we ensure it gets tested... for later releases (I might add PSR-7 support later on)...

What do you think of a method called like

namespace Soluble\Japha\Bridge\Http;

class Cookie {

    /**
     * @return string
     */
   static public function getCookiesHeaderLine() {
//...
   } 
}  

And modify SimpleHTTPHandler and SimpleHTTPTunnelHandler... to use the static method instead

Let me know, I'll review an release anyway. But always interested in discussion :)

And

diegosainz commented 8 years ago

Hi Sébastien,

That was fast! - yeah, I agree on putting the Cookie logic in its own class. It will be better isolated. I can do that and send the PR.

I can also create the unit tests for that. Any special considerations/guidelines I should follow on that?