Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lowercase authorization header creates a broken request #188

Closed
tuupola opened this issue Mar 1, 2021 · 4 comments · Fixed by #195
Closed

Lowercase authorization header creates a broken request #188

tuupola opened this issue Mar 1, 2021 · 4 comments · Fixed by #195

Comments

@tuupola
Copy link
Contributor

tuupola commented Mar 1, 2021

Given the following example code:

<?php

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Slim\Factory\AppFactory;

require __DIR__  . '/vendor/autoload.php';

$app = AppFactory::create();


$app->get("/foo", function(ServerRequestInterface $request, ResponseInterface $response, array $arguments) {
    $line = $request->getHeaderLine("Authorization");
    $response->getBody()->write("$line\n");
    return $response;
});

$app->get("/bar", function(ServerRequestInterface $request, ResponseInterface $response, array $arguments) {
    $line = $request->getHeaderLine("X-Something");
    $response->getBody()->write("$line\n");
    return $response;
});

$app->run();

Yields the following results:

$ curl --include --header "Authorization: Basic cm9vdDp0MDBy" localhost:8080/foo
HTTP/1.1 200 OK
Host: localhost:8080
Date: Mon, 01 Mar 2021 10:52:46 GMT
Connection: close
X-Powered-By: PHP/8.0.1
Content-type: text/html; charset=UTF-8

Basic cm9vdDp0MDBy

$ curl --include --header "authorization: Basic cm9vdDp0MDBy" localhost:8080/foo
HTTP/1.1 200 OK
Host: localhost:8080
Date: Mon, 01 Mar 2021 10:53:07 GMT
Connection: close
X-Powered-By: PHP/8.0.1
Content-type: text/html; charset=UTF-8

Basic cm9vdDp0MDBy,Basic cm9vdDp0MDBy

The latter result can be considered broken. Taken from rfc2616:

"Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma."

Also author of the spec says it is not valid to specify multiple Authorization fields.

"You can only use multiple header fields when they are defined using list syntax."

I assume the bug is somewhere in slim/psr7 since problem goes away when changing the PSR-7 implementation:

$ composer remove slim/psr7
$ composer require nyholm/psr7
$ composer require nyholm/psr7-server

$ curl --include --header "authorization: Basic cm9vdDp0MDBy" localhost:8080/foo
HTTP/1.1 200 OK
Host: localhost:8080
Date: Mon, 01 Mar 2021 11:08:12 GMT
Connection: close
X-Powered-By: PHP/8.0.1
Content-type: text/html; charset=UTF-8

Basic cm9vdDp0MDBy

See also: tuupola/slim-basic-auth#105, tuupola/slim-basic-auth#89, #179

@l0gicgate
Copy link
Member

l0gicgate commented Mar 1, 2021

We should probably make it case insensitive then? Why do I feel like we've had this debate over and over again 🤔

The discrepancy is caused by this method:
https://github.com/slimphp/Slim-Psr7/blob/master/src/Headers.php#L180

@tuupola
Copy link
Contributor Author

tuupola commented Mar 2, 2021

I think so yes. RFC says header names are case-insensitive.

https://tools.ietf.org/html/rfc7230#section-3.2

"Each header field consists of a case-insensitive field name followed by a colon (":"), optional leading whitespace, the field value, and optional trailing whitespace."

@daniele-casavo
Copy link

hey, any news on this?

@l0gicgate
Copy link
Member

Fixed with this release https://github.com/slimphp/Slim-Psr7/releases/tag/1.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants