laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.56k stars 11.03k forks source link

HTTP header field name case-insensitivity is not being respected which leads to improper Session handling. #1639

Closed davemo closed 11 years ago

davemo commented 11 years ago

Overview

Laravel is incorrectly handling HTTP request header field names that are lowercase, which causes the Session handler to improperly issue a new session cookie on every request.

I encountered this bug when working with a NodeJS app configured to proxy requests to Laravel. The HTTP spec is pretty clear that header field names are case-insensitive:

4.2 Message Headers

... Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive. ...

Reproducing the Bug

Here's the breakdown of how I tested this and my configuration that lead me to believe the problem exists in the way Laravel parses request headers:

  1. I have laravel configured to run php artisan serve --host=127.0.0.1 --port=3000
  2. I have node running on port 8000, using the npm module http-proxy to forward requests to port 3000

To rule out node and http-proxy as a potential source of problems, I first used netcat to listen on port 3000 (nc -l 3000) and capture requests that http-proxy was sending, here's a dump of what http-proxy is forwarding to Laravel (note the lower case header field-names):

davidmosher@localhost:~/code/temp/laravel 
$ nc -l 3000
GET /auth/csrf_token HTTP/1.1
host: localhost:8000
connection: keep-alive
cache-control: no-cache
pragma: no-cache
accept: application/json, text/javascript
x-requested-with: XMLHttpRequest
user-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36
content-type: application/x-www-form-urlencoded
referer: http://localhost:8000/
accept-encoding: gzip,deflate,sdch
accept-language: en-US,en;q=0.8
cookie: laravel_session=doh0284ujk57ebnldlm2plp795
x-forwarded-for: 127.0.0.1
x-forwarded-port: 53245
x-forwarded-proto: http

When I use telnet 127.0.0.1 3000 in another terminal session I paste the above request that has lowercase header field-names in and receive the following response from Laravel:

HTTP/1.1 200 OK
Connection: close
X-Powered-By: PHP/5.4.14
Set-Cookie: laravel_session=kicg6iu0aobufkl036itar6km6; expires=Thu, 13-Jun-2013 18:48:12 GMT; path=/; HttpOnly
Set-Cookie: laravel_session=kicg6iu0aobufkl036itar6km6; expires=Thu, 13-Jun-2013 18:48:12 GMT; path=/; httponly
Cache-Control: no-cache
Date: Thu, 13 Jun 2013 16:48:12 GMT
Content-Type: application/json

Note the double Set-Cookie and the laravel_session has a different value than what was sent in the request. If I modify the header keys in my request to uppercase all the words like so:

GET /auth/csrf_token HTTP/1.1
Host: localhost:8000
Connection: keep-alive
Cache-Control: no-cache
Pragma: no-cache
Accept: application/json, text/javascript
X-Requested-With: XMLHttpRequest
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36
Content-Type: application/x-www-form-urlencoded
Referer: http://localhost:8000/
Accept-Encoding: gzip,deflate,sdch
Accept-Language: en-US,en;q=0.8
Cookie: laravel_session=doh0284ujk57ebnldlm2plp795
X-Forwarded-For: 127.0.0.1
X-Forwarded-Port: 53245
X-Forwarded-Proto: http

Then I get a correct response from Laravel, as follows:

HTTP/1.1 200 OK
Host: localhost:8000
Connection: close
X-Powered-By: PHP/5.4.14
Set-Cookie: laravel_session=doh0284ujk57ebnldlm2plp795; expires=Thu, 13-Jun-2013 18:49:40 GMT; path=/; httponly
Cache-Control: no-cache
Date: Thu, 13 Jun 2013 16:49:40 GMT
Content-Type: application/json

Conclusion

Having ruled out my proxy completely, using netcat and telnet I can only conclude that Laravel is treating http request header field names differently depending on the casing; this causes major problems for anyone setting up a proxy that may use lower case request header field names to forward requests to laravel and expects Session management to work properly.

Is this something that should be fixed by Laravel or at a lower level in Symfony?

fideloper commented 11 years ago

Interesting

When I cURL a Laravel build, I get 2 cookies back in the response (via Set-Cookie header). When I use the web browser (chrome), I get one - unless Chrome strips the duplicate out.

You can see that by CURLing fideloper.com (I get it when CURLing any Laravel build)

$ curl -I -v fideloper.com

# Request
> HEAD / HTTP/1.1
> User-Agent: curl/7.24.0 (x86_64-apple-darwin12.0) libcurl/7.24.0 OpenSSL/0.9.8x zlib/1.2.5
> Host: fideloper.com
> Accept: */*

# Response
< HTTP/1.1 200 OK
HTTP/1.1 200 OK
< Content-Type: text/html; charset=UTF-8
Content-Type: text/html; charset=UTF-8
< Set-Cookie: laravel_session=REDACT; expires=Thu, 13-Jun-2013 19:23:40 GMT; path=/
Set-Cookie: laravel_session=REDACT; expires=Thu, 13-Jun-2013 19:23:40 GMT; path=/
< Set-Cookie: laravel_session=REDACT; expires=Thu, 13-Jun-2013 19:23:40 GMT; path=/
Set-Cookie: laravel_session=REDACT; expires=Thu, 13-Jun-2013 19:23:40 GMT; path=/
< Date: Thu, 13 Jun 2013 17:23:40 GMT
Date: Thu, 13 Jun 2013 17:23:40 GMT
< Last-Modified: Tue, 04 Jun 2013 21:26:18 GMT
Last-Modified: Tue, 04 Jun 2013 21:26:18 GMT
< X-UA-Compatible: IE=Edge,chrome=1
X-UA-Compatible: IE=Edge,chrome=1

My "redacted" session data was all the same hash.

davemo commented 11 years ago

FWIW, I dug into Symfony symfony/http-foundation/Symfony/Component/HttpFoundation/HeaderBag.php and the code there seems to normalize header keys with strtolower, so the problem must exist at another layer.

/**
     * Returns a header value by name.
     *
     * @param string  $key     The header name
     * @param mixed   $default The default value
     * @param Boolean $first   Whether to return the first value or all header values
     *
     * @return string|array The first header value if $first is true, an array of values otherwise
     *
     * @api
     */
    public function get($key, $default = null, $first = true)
    {
        $key = strtr(strtolower($key), '_', '-');

        if (!array_key_exists($key, $this->headers)) {
            if (null === $default) {
                return $first ? null : array();
            }

            return $first ? $default : array($default);
        }

        if ($first) {
            return count($this->headers[$key]) ? $this->headers[$key][0] : $default;
        }

        return $this->headers[$key];
    }
fideloper commented 11 years ago

Right on, that's what I've seen in the past as well.

FYI, if you need to disable cookies in your proxy situation, you can edit app/config/session.php and change the handler to "array". This isn't a solution to the underlying "issue", but is something that might work in your use case for now.

davemo commented 11 years ago

I'll give that a shot as a stopgap, my problem comes in that I have an endpoint in Laravel that returns me a CSRF_TOKEN, which I use in the JavaScript app to attach to all relevant requests. When I get a new session cookie on every request the value of the CSRF_TOKEN also changes and subsequent requests yield a TokenMismatchException

filters.php

Route::filter('csrf_json', function() {
  if (Session::token() != Input::json('csrf_token')) {
    throw new Illuminate\Session\TokenMismatchException;
  }
});

routes.php

Route::post('/auth/login', array('before' => 'csrf_json', 'uses' => 'AuthController@login'));

POST /auth/login yields

{"error":{"type":"Illuminate\\Session\\TokenMismatchException","message":"","file":"\/Users\/davidmosher\/code\/temp\/laravel\/app\/filters.php","line":86}}
taylorotwell commented 11 years ago

I used the Postman Chrome extension to send a request with a lower-case header and I was able to retrieve it fine.

taylorotwell commented 11 years ago

Laravel does not parse the headers though, that is Symfony code, and looks to be in ServerBag.php -> getHeaders.

davemo commented 11 years ago

@taylorotwell the problem isn't with being able to retrieve the CSRF_TOKEN, I can do that.. the problem is that when requesting it via the unix command line utility telnet using telnet 127.0.0.1 3000 with lower case headers there is a double set-cookie header (a minor bug) and Laravel creates a new session for each request (a major bug). Changing the headers to uppercase fixes this bug, which leads me to believe Laravel or Symfony are responsible for this.

I downloaded Postman and am still seeing the same behaviour; not that I don't trust the Postman extension, but it's a little higher level and to be conclusive we should use the same tool, can you repeat your test using telnet and pasting in the request text with lower case header field names? You'll also need to be running Laravel using php artisan serve --host=127.0.0.1 --port=3000

taylorotwell commented 11 years ago

It's possible that Symfony is having trouble parsing the headers passed to PHP from the built-in PHP server. Like I said, Laravel doesn't parse headers at all. So, any issue with request header parsing would be an upstream Symfony issue and would be filed on their repositories.

davemo commented 11 years ago

Here's a video demonstrating the problem: http://cl.ly/1B0b030h410Y (open in chrome).

davemo commented 11 years ago

I suppose it could also be a problem with Artisan, right? I'm not opposed to filing an issue on the Symfony project as well, all I'm looking for is some confirmation other people are experiencing this bug; I think @fideloper's findings would indicate that it, at the very least, is a bug :)

taylorotwell commented 11 years ago

Artisan is simply a wrapper that calls "php -S" etc so you don't have to remember the command. I can guarantee you with 100% confidence that it is not a Laravel issue, because Laravel literally does not touch request headers in any form or fashion. Those headers are parsed by Symfony's HttpFoundation class.

davemo commented 11 years ago

Regardless of where the problem lies, I think it is a Laravel issue, because people using the framework behind any kind of proxy are going to run into this issue. Somewhere in the stack that Laravel sits on top of this is a bug, and a pretty nasty one :(

taylorotwell commented 11 years ago

But the problem is not in Laravel's codebase, it is in an upstream library's, and you should file the issue on their library since they handle the parsing of HTTP headers.

https://github.com/symfony/symfony

taylorotwell commented 11 years ago

There is literally nothing I can do to fix any bugs in parsing PHP headers. That is upstream code that I do not have commit rights to.

fideloper commented 11 years ago

Hey guys -

First

I think there's been a miscommunication in that the end result we're looking at here is why there might be 2 different Set-Cookie headers returned.

Whether Symfony or Laravel classes are to do with it is TBD.

I don't know why @davemo's changing of the capitalization of headers affects anything, but I have a feeling that's a "distraction" leading @davemo to the possibly incorrect conclusion about the root cause of the issue (Again, more of a feeling based on PHP debugging experience, I don't know this for sure).

However, in turn, that assumption is making it sound like it is Symfony which might be setting cookies weirdly; it may or may not be. @taylorotwell is therefore saying if it's something weird with Symfony reading headers, than he can't look into it (as Laravel just uses Symfony classes, it doesn't creating/contribute to them).

Digging in

Anyway, I dug into this a bit, and here's what I'm seeing (preliminary):

What I did find is that inside the Illuminate\Session\SessionServiceProvider class, there's a few methods which "touches" the cookie in order to update its expire time on each request:

Note that I've using $ curl -I to inspect the headers. I'm not sure if netcat/telnet would be better, but I think this is a matter of code rather than if the tool is displaying headers as actually sent/received.

    /**
     * Update the session cookie lifetime on each page load.
     *
     * @return void
     */
    protected function registerCookieToucher()
    {
        $me = $this;

        $this->app->close(function() use ($me)
        {
            if ( ! headers_sent()) $me->touchSessionCookie();
        });
    }

    /**
     * Update the session identifier cookie with a new expire time.
     *
     * @return void
     */
    public function touchSessionCookie()
    {
        $config = $this->app['config']['session'];

        $expire = $this->getExpireTime($config);

        setcookie($config['cookie'], session_id(), $expire, $config['path'], $config['domain'], false, true);
    }

If I comment out the setcookie() method call, I only get one Set-Cookie header, as I would expect.

With code "out of the box":

$curl -I header.ubuntu.dsdev 
HTTP/1.1 200 OK
Date: Fri, 14 Jun 2013 13:23:37 GMT
Server: Apache/2.2.22 (Ubuntu)
Set-Cookie: laravel_session=pda8975l58v7uinfugc56uhvk0; expires=Fri, 14-Jun-2013 15:23:37 GMT; path=/; HttpOnly
Set-Cookie: laravel_session=pda8975l58v7uinfugc56uhvk0; expires=Fri, 14-Jun-2013 15:23:37 GMT; path=/; httponly
Cache-Control: no-cache
Vary: Accept-Encoding
Transfer-Encoding: chunked
Content-Type: text/html; charset=UTF-8

With setcookie() inside of touchSessionCookie() commented out:

$ curl -I header.ubuntu.dsdev
HTTP/1.1 200 OK
Date: Fri, 14 Jun 2013 13:25:43 GMT
Server: Apache/2.2.22 (Ubuntu)
X-Powered-By: PHP/5.4.12-2~precise+1
Set-Cookie: laravel_session=d1b0shhdem5gn8i9kc18gfa9l2; expires=Fri, 14-Jun-2013 15:25:43 GMT; path=/; HttpOnly
Cache-Control: no-cache
Vary: Accept-Encoding
Transfer-Encoding: chunked
Content-Type: text/html; charset=UTF-8

Conclusion

I have no clue :D

Possibly Symfony is handling "touching" the cookie with the NativeSessionDriver itself, and so this SessionServiceProvider code is duplicating efforts? My tests aren't as thorough as they could be because I haven't been passing one session back and forth via curl.

There's still more to look into to see why that's happening, and if it's negatively affecting anything.

To be clear: I've used sessions successfully in Laravel 4 - there's no actual issue logging users in and persisting sessions via Cookie's that I know of.

Interesting SO articles to scan through (not that they say much...):

  1. Can setcookie in PHP result in multiple “Set-Cookie” headers? 2.Duplicated “set-cookie: ci-session” fields in header by codeigniter
taylorotwell commented 11 years ago

Yeah, I know exactly why there are multiple Set-Cookie headers. PHP sessions do not "touch" the session cookie once it is created. That is just plain goofy behavior, because people expect sessions to expire relative to the last activity, not relative to when the session is created. To get around this, Laravel has to call setcookie again with the touched cookie. PHP, for whatever reason, sends two Set-Cookie headers in that case.

The two issues aren't related.

Really my hunch is on the header casing issue is just that Symfony is not parsing lower-case headers sent from the internal PHP dev server correctly.

taylorotwell commented 11 years ago

Also, having the two Set-Cookie headers doesn't cause any problems. It's just something I have to do because of PHP's crazy session behavior, and the fact that I have no way of removing a cookie once it has been set via setcookie.

fideloper commented 11 years ago

Cool, thanks for the insight.

That's all I got for input, sounds like things are working right. It would be interesting to see if the issue persists under a "real" web server instead of PHP's.

I'll leave it to @davemo to decide if there's actually any issue then (something breaking his code?).

taylorotwell commented 11 years ago

Yeah. If the request headers aren't being parsed correctly for any reason that is 100% an upstream Symfony issue.

davemo commented 11 years ago

Thanks for your help @fideloper, I'd like to reiterate that the root issue here isn't the double set-cookie header, it's the fact that Laravel creates a new session on every request sent with lowercase header keys which changes the internal value of the key returned from csrf_token(); and makes it impossible to send a token that will match.

fideloper commented 11 years ago

Oh, I'm definitely "that guy" in this conversation then... I misunderstood the central part of your issue. Sorry to waste all your time :D

Is any of your node.js code available to play with? (Are you using node-proxy or something fancier?)

davemo commented 11 years ago

I'm using node-http-proxy with this setup inside of an express.js app. The proxy is only "problematic" in that it sends lowercase request header field-names (and that fact is what triggers the issue); to rule out the proxy as the root cause of the problem I used telnet to simulate the request that is coming in from the proxy which is where I encountered this bug.

It seems very strange to me that the simple act of changing the case of http header field-names would cause Laravel (or Symfony) to treat session management differently. Perhaps the lowercase cookie header just isn't being read?

davemo commented 11 years ago

I've been trying to isolate which part of Symfony is responsible for this issue, but I don't have a lot of experience debugging php. I've just been inserting var_dump calls into the Symfony code at various touchpoints but that doesn't seem to have any effect. Can you recommend the "right" way to debug or even log things in the Symfony framework code?

davemo commented 11 years ago

The only steps necessary to duplicate this bug are:

Route::get('/auth/csrf_token', function() {
  return Response::json(array('csrf_token' => csrf_token()));
});

php artisan serve --host=127.0.0.1 --port=3000

telnet 127.0.0.1 3000

GET /auth/csrf_token HTTP/1.1
host: localhost:8000
connection: keep-alive
cache-control: no-cache
pragma: no-cache
accept: application/json, text/javascript
x-requested-with: XMLHttpRequest
user-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36
content-type: application/x-www-form-urlencoded
referer: http://localhost:8000/
accept-encoding: gzip,deflate,sdch
accept-language: en-US,en;q=0.8
cookie: laravel_session=existing_session_id
x-forwarded-for: 127.0.0.1
x-forwarded-port: 53245
x-forwarded-proto: http
fideloper commented 11 years ago

Cool, thanks

When I can, I'll give that a go in apache on port 3000 and see if that changes anything.

As for debugging: I've never gone so far as to use a "real" debugger (aka something inside of Eclipse, Zend Studio, PHPStorm, etc - all those JRE-dependent monoliths). That's probably a side-effect of growing up as a PHP developer - no unified/standard toolset :D

I dig into the code just like you are. I'd start in Symfony's HttpFoundation\Request and HttpFoundation\Response. There is also Symfony's Session handlers - Laravel defaults to Symofny's NativeSessionHandler.

For logging - If you're in Laravel, you might be able to just sprinkle in Log::() in the Symfony classes to test. I've been creating a new Laravel project and then going to town in Symfony's code, rather than doing any step-through procedure.

davemo commented 11 years ago

It seems as though adding logging inside the Symfony code has no effect when running Laravel, for example I'm adding a simple Log::info('Request.php -> Headers', $this->headers); into HttpFoundation\Request in the initialize method, but nothing ever seems to get logged.

Is there something else I have to do?

davemo commented 11 years ago

To further simplify this, all you have to do is just provide the cookie header with lowercase and uppercase values, that is enough to trigger the difference.

good

GET /auth/csrf_token HTTP/1.1
Cookie: laravel_session=existing_session_id

bad

GET /auth/csrf_token HTTP/1.1
cookie: laravel_session=existing_session_id
davemo commented 11 years ago

From the Slim Documentation

The HTTP specification states that HTTP header names may be uppercase, lowercase, or mixed-case. Slim is smart enough to parse and return header values whether you request a header value using upper, lower, or mixed case header name, with either underscores or dashes. So use the naming convention with which you are most comfortable.

This is the stance I would like Laravel to take, regardless of whether the problem exists at a lower layer.

I'm going to experiment with creating some middleware that does this.

taylorotwell commented 11 years ago

I've tried to tell you that I literally do not maintain the code that parses HTTP headers.

davemo commented 11 years ago

I hear you @taylorotwell, I'm not adding comments to this issue to try and get you to do anything, but rather as documentation for anyone else who might come across this issue. I'm also actively working towards a solution :)

taylorotwell commented 11 years ago

You should file an issue with Symfony if you want to see this really fixed and get their input.

davemo commented 11 years ago

I did as you suggested last night https://github.com/symfony/symfony/issues/8278 :)

fideloper commented 11 years ago

Yea sorry, we're just discussing here, not thinking its a Laravel issue.

We can move our discussion off this issue so you can close it out - I'll talk directly on the Symfony issue or catch up with Dave on Twitter :D

Thanks again for your input.

On Friday, June 14, 2013, Taylor Otwell wrote:

You should file an issue with Symfony if you want to see this really fixed and get their input.

— Reply to this email directly or view it on GitHubhttps://github.com/laravel/framework/issues/1639#issuecomment-19468224 .

franzliedke commented 11 years ago

@taylorotwell Can't you remove cookies by sending a cookie with a past expiration date? Or are you talking about cookies that were sent in the same request?

taylorotwell commented 11 years ago

Same request.

On Jun 15, 2013, at 6:38 PM, Franz Liedke notifications@github.com wrote:

@taylorotwell Can't you remove cookies by sending a cookie with a past expiration date? Or are you talking about cookies that were sent in the same request?

— Reply to this email directly or view it on GitHub.