[HttpFoundation] Fixed issue with empty headers

[GrahamCampbell]

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #35802
License	MIT
Doc PR	-

I was made aware of the issue due to:

1. https://stackoverflow.com/questions/60122162/laravel-undefined-offset-0-in-symfony-http-foundation-when-requiring-simplesaml
2. Undefined offset caused by symfony/http-foundation dependency. laravel/framework#31544
3. Undefined offset: 0 error with get function in HttpFoundation/HeaderBag.php #35802

---

Looking at the file history leads me to think it was caused by #33353, however, it would look like the old code had the bug too. It is also odd how it is claimed the bug is not present in 4.3.8, but is in 4.4.0...

Does this actually fix the real issue, or does it just mask it?

[Tobion]

please add a test case

[GrahamCampbell]

@frankgasking Please can you provide the test case for your issue.

[nicolas-grekas]

I agree this one is strange - why is key 0 unset? I'd be curious to see how this happens in practice.
Good to go on my side for the patch itself. Test case pending of course.

[frankgasking]

Apologies for the delay, here is my full test case and steps:

These steps were done using a PHP 7.2 Homestead vagrant box instance:

1. Set up base Laravel installation (latest build) - i.e.
   laravel new testinstance
2. Set up and install a test local SimpleSAMLPHP build
3. Under your laravel installation -> app/Http/Middleware
   - Create a "BeforeMiddleware.php" file with the following code as below (replacing PATH/TO with your server path to simplesamlphp).
   - Only need to add the require_once part, no auth required - as its this include which highlights the issue.

<?php
namespace App\Http\Middleware;

use Closure;

class BeforeMiddleware
{
    public function handle($request, Closure $next)
    {
        // Perform action
        require_once('PATH/TO/simplesamlphp/lib/_autoload.php');

        return $next($request);
    }
}

4. Add \App\Http\Middleware\BeforeMiddleware::class to app/Http/Kernel.php under protected $middlewareGroups and 'web'. This is so the include gets called on a page request .. I.e.:

    protected $middlewareGroups = [
        'web' => [
            \App\Http\Middleware\BeforeMiddleware::class

5. Visit the root of the laravel website in a browser to see the "Undefined Offset 0" error.

---

Sample $headers content:

Without any fixing to HeaderBag.php, during the Offset 0 error, $key was Content-Type and $headers contains:

array (size=3)
  'cache-control' => 
    array (size=1)
      0 => string 'no-cache, private' (length=17)
  'date' => 
    array (size=1)
      0 => string 'Mon, 24 Feb 2020 11:15:41 GMT' (length=29)
  'content-type' => 
    array (size=1)
      0 => string 'text/html' (length=9)

Fixing public function all(/*string $key = null*/) to public function all(string $key = null) then returns $headers as:

array (size=4)
  'cache-control' => 
    array (size=1)
      0 => string 'no-cache, private' (length=17)
  'date' => 
    array (size=1)
      0 => string 'Mon, 24 Feb 2020 11:18:25 GMT' (length=29)
  'content-type' => 
    array (size=1)
      0 => string 'text/html' (length=9)
  'set-cookie' => 
    array (size=2)
      0 => string 'XSRF-TOKEN=eyJpdiI6ImJiYUpjQ3Q5Ukl2K1BnOFJScDBiUmc9PSIsInZhbHVlIjoiNFBkQndIdjRIQlJtMUg0K3J2SjZTMEVHN0F6XC9LYjdPS3RuWERNWlMwcG14blh3KzFyNnVcL0RsbmVnNXhlRDg0IiwibWFjIjoiZTk2NTNlYzQ3MTI3YzFiY2I4NmFkNmI5YzY1ZWNkMDY5ZGI3NzcwYTY0OGIzYWQ3Mzk0OTVmNDBmZDBlZTk0MiJ9; expires=Mon, 24-Feb-2020 13:18:25 GMT; Max-Age=7200; path=/' (length=316)
      1 => string 'laravel_session=eyJpdiI6IlZtSjliNCtuXC9uK0o5c1pjKzlZY1VBPT0iLCJ2YWx1ZSI6IkRhSmdSVDNnRmE5bkZOTjlQbjU0ditLVm8xbVM0SmUxcnFHZ2VoKzFhcXZ6d2RrXC8yMmRON0lFRVwvUDBvNW8rQiIsIm1hYyI6ImMxNTZlYzE2ZDY1ODNkYWZjNGI3NTU1NTZiOTQ3NjhhOGNiMjc0ZTBkOWJiOGNiMDE3MTViZTI4N2JjZDUyZjcifQ%3D%3D; expires=Mon, 24-Feb-2020 13:18:25 GMT; Max-Age=7200; path=/; httponly' (length=339)

[GrahamCampbell]

Are you able to provide a more isolated way to reproduce this? Like a single php file or a test case?

[frankgasking]

Something just outside of Laravel? I can try, though it will have to be either later today or tomorrow. Web dev is only a small part of my role, so I don't get much time unfortunately. Will try and sort something asap.

[frankgasking]

My apologies. I've tried to replicate outside of Laravel, but i'm unable to catch the issue at all standalone. Something really strange is going on, which I think is due to the inclusion of the SimpleSAMLPHP library within Laravel's Middleware phase - causing some kind of strange redirect or overwrite of values. I think I can only demonstrate it by having it shown as via my steps above.

What is strange is when you look at what is being set and returned via this temporary code i've added before the null === $headers[0] call...

        if(!isset($headers[0]))
        {
            var_dump($key);
            var_dump($headers);
            $test = $this->headers[strtr($key, self::UPPER, self::LOWER)] ?? [];
            var_dump($test);
            die();
        }

.... it comes back and suggests that $key = "Content-Type" and $headers was:

array (size=3)
  'cache-control' => 
    array (size=1)
      0 => string 'no-cache, private' (length=17)
  'date' => 
    array (size=1)
      0 => string 'Mon, 24 Feb 2020 16:43:02 GMT' (length=29)
  'content-type' => 
    array (size=1)
      0 => string 'text/html' (length=9)

Hence the offset error.

However, $test (using the same line as in the all() function), returns correctly:

array (size=1)
  0 => string 'text/html' (length=9)

Why it doesn't return this via the very first line $headers = $this->all((string) $key); at the start of get() , I have absolutely no idea.

[frankgasking]

I've had another fresh look this morning. Laravel seems to make calls to get() and subsequently all() 10 times on a page request without any problems.

It seems to be on the 11th time all() is called, then some odd behaviour occurs.

At this point, the get() function is called with a $key of string "Content-Type". However for this occurrence, $key within the all() function is strangely translated to null. Basically, its defaulting to null - suggesting that no value was passed to all(), even though it clearly is in the code and nothing has changed.

So it returns all of the headers (with no 0 value accessible) and hence the offset error.

Checking the $key value again back in the get() function after the all() call ... it is still set to "Content-Type"!

On my isolated test, i've tried repeating the calls multiple times, and it works fine. Not seen an issue like this before. Because all() could be called without a value and default as null, there still needs to be a catch fix, but it of course doesn't make sense why the oddness is occurring above.

[nicolas-grekas]

I'm sorry completely missing how this can happen.
@frankgasking can you please provide a full reproducer - ie a repository on GitHub we could clone and that would reproduce the issue?

[frankgasking]

Hi Nicolas, my apologies - it is a strange one and i'm not entirely sure how my inclusion of SimpleSAMLPHP is causing it. I'll set up a standalone Laravel + SimpleSAMLPhp instance with something triggering the error hopefully before the end of the week and will post here.

…

On Wed, 4 Mar 2020 at 13:21, Nicolas Grekas ***@***.***> wrote: I'm sorry completely missing how this can happen. @frankgasking <https://github.com/frankgasking> can you please provide a full reproducer - ie a repository on GitHub we could clone and that would reproduce the issue? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35809?email_source=notifications&email_token=AAPUC6PYWD2KXMHEQMWABSTRFZIU5A5CNFSM4KYV22P2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENX2BKA#issuecomment-594518184>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPUC6M3K6ST2AEXPGWIHKLRFZIU5ANCNFSM4KYV22PQ> .

-- Cheers Frank

---------------------------------------- Visit my retro blog at: http://fgasking.wordpress.com/ Also the GTW project - http://www.gamesthatwerent.com (main) http://www.gtw64.co.uk (c64)

[frankgasking]

Hi Nicolas, apologies for the delay - I have set up a repository with Laravel + SimpleSAMLPHP at https://github.com/frankgasking/bughighlight

I've just ran this on a Homestead instance, but any PHP 7.2 stack should be fine. If you browse to laravel/public/ in a browser, then you'll see the offset error.

This has been triggered by the following Middleware, which just simply includes SimpleSAMLPHP:

https://github.com/frankgasking/bughighlight/blob/master/laravel/app/Http/Middleware/BeforeMiddleware.php

[nicolas-grekas]

Closing as explained in the linked issue.