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

Change content type check and add basic auth support #1325

Merged
merged 18 commits into from
Nov 22, 2022

Conversation

georgeboot
Copy link
Contributor

@georgeboot georgeboot commented Nov 14, 2022

I've discovered that its also possible for browsers (or in my case, fetch on a Cloudflare worker) to append a suffix to the urlencoded form header: application/x-www-form-urlencoded;charset=UTF-8 is the value that was sent in my case.

According to the standard the values should always be utf-8 and hence it does not need to be specified, but here we are.

I've updated the code to check if the header starts with the value, instead of checking for equality.

Basic auth support

While in there, I've also implemented support for basic auth, as that relies on PHP setting $_SERVER vars.

$_SERVER vars

While in there, I've also added support for some widely used $_SERVER vars. Admitted, modern PHP apps should not depend on them, but to make the entry barrier to Bref as low as possible, I figured the most used ones should be included by default. Examples include HTTP headers but also:

  • CONTENT_LENGTH
  • CONTENT_TYPE
  • SERVER_NAME
  • SERVER_PORT
  • HTTP_HOST

Big thanks to @tillkruss who has done most of the work here!

Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

Would you be able to add tests?

  • 1 testing the basic auth variables
  • 1 testing the content type

src/Event/Http/Psr7Bridge.php Outdated Show resolved Hide resolved
src/Event/Http/Psr7Bridge.php Outdated Show resolved Hide resolved
src/Event/Http/Psr7Bridge.php Outdated Show resolved Hide resolved
src/Event/Http/Psr7Bridge.php Outdated Show resolved Hide resolved
src/Event/Http/Psr7Bridge.php Outdated Show resolved Hide resolved
@georgeboot
Copy link
Contributor Author

@mnapoli added some more fields, but all sourced directly from implemented methods of the HttpRequestEvent.

@mnapoli
Copy link
Member

mnapoli commented Nov 16, 2022

Oh wait I'm sorry, I just realized that the new $_SERVER variables were added to the PSR-7 requests, not the FastCGI requests.

To be honest, I'm not sure it makes sense. These variables make sense on $_SERVER (the actual variable) in FPM to keep backward compatibility with older PHP apps, but for apps using PSR-7 that makes much less sense. Especially for the basic auth: that's the role of a PSR-7 middleware/framework, not really of the PSR-7 request builder.

Just to clarify, I think these additions make sense for the FPM layer (to provide complete compatibility with FPM in traditional hosting).

Is there a specific scenario where these variables are useful to you?

@tillkruss
Copy link
Member

Is there a specific scenario where these variables are useful to you?

It's really backwards compatibility with 3rd party packages that may access their weirdest variables, because most develop on PHP-FPM.

@mnapoli
Copy link
Member

mnapoli commented Nov 16, 2022

Got it, but these work with PSR-7? Or could it be something related to Octane maybe? I'm having trouble seeing where a legacy codebase/package would run with the function layer, because that sounds really advanced.

@georgeboot
Copy link
Contributor Author

Yes I tend to agree with you. Octane indeed uses the PSR-7 bridge. Don't think there will be a lot of legacy apps running Octane out there.

Laravel for example, reads the basic auth user from these two variables. And there are plenty Laravel apps using basic auth out there.

And I bet there are some use cases for the other ones as well.

Shall I review all of them (including existing ones) one by one and check if they are used anywhere in Laravel or Symfony?

@mnapoli
Copy link
Member

mnapoli commented Nov 16, 2022

Laravel for example, reads the basic auth user from these two variables. And there are plenty Laravel apps using basic auth out there.

OK that's enough to convince me then 😄 This isn't the best in theory, but this is best in practice and practice trumps theory.

I'm all good for this approach then 👍

@mnapoli mnapoli added the bug label Nov 16, 2022
mnapoli added a commit that referenced this pull request Nov 22, 2022
@mnapoli mnapoli changed the title Change content type check to startsWith Change content type check and add basic auth support Nov 22, 2022
@mnapoli
Copy link
Member

mnapoli commented Nov 22, 2022

All good for me, it seems there are two failures in tests that might be a matter of a ?? '' or something similar.

@georgeboot
Copy link
Contributor Author

All good for me, it seems there are two failures in tests that might be a matter of a ?? '' or something similar.

Fixed now

@mnapoli
Copy link
Member

mnapoli commented Nov 22, 2022

Tests are failing on PHP 7.3 because of str_starts_with

@georgeboot
Copy link
Contributor Author

Ugh missed that. Fixed now.

You probably need allow Actions run once again

@mnapoli
Copy link
Member

mnapoli commented Nov 22, 2022

🙌 thank you @georgeboot!

@mnapoli mnapoli merged commit 33d3402 into brefphp:master Nov 22, 2022
@georgeboot georgeboot deleted the patch-1 branch November 22, 2022 15:55
mnapoli added a commit to brefphp/php-fpm-runtime that referenced this pull request Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants