Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[HttpFoundation] HTTP Basic authentication is broken with php-cgi under Apache#3551

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:2.0fromkepten:ticket_1813
May 21, 2012

Conversation

@kepten
Copy link
Contributor

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets:#1813
Todo: -

In order to work, add this to the .htaccess:

RewriteEngine on
RewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization}]
RewriteCond %{REQUEST_FILENAME} !-f
RewriteRule ^(.*)$ app.php [QSA,L]

Copy link
Member

Choose a reason for hiding this comment

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

the indentation should use 4 spaces, not 8

and instead modifying$this->parameters, I would modify$parameters before calling the parent constructor

Choose a reason for hiding this comment

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

you should also use camelCase variable names ($authorizationHeader instead of$authorization_header).

Copy link
Member

Choose a reason for hiding this comment

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

how did I miss that ? 😳

Choose a reason for hiding this comment

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

I think I want a badge for that!

@stof
Copy link
Member

you should also add a unit test for this

@kepten
Copy link
ContributorAuthor

Thanks for the feedback, I committed the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

indention wrong

@stof
Copy link
Member

stof commentedApr 4, 2012

@fabpot could you review it ?

@fabpot
Copy link
Member

My comments:

  • ServerBag represents what we have in the$_SERVER global variables. As such, the code should be moved to thegetHeaders() method instead like the other tweaks we do for the HTTP headers.
  • A comment must be added explaining why this is needed and the configuration the user must have to make it work (then remove the Github URLs).
  • The code should only be executed whenPHP_AUTH_USER is not available (to not have any overhead when not needed).

@danielholmes
Copy link
Contributor

A quick note on that .htaccess/apache configuration required, if adding to the Symfony SE htaccess file, then it will need to look like this:

<IfModule mod_rewrite.c>    RewriteEngine On    RewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization}]    RewriteCond %{REQUEST_FILENAME} !-f    RewriteRule ^(.*)$ app.php [QSA,L]</IfModule>

NOTE: No,L in the Authorization Rewrite as in the original example - it prevents the front controller rewrite from happening

Choose a reason for hiding this comment

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

Sf standard would be to use null !== when testing $authorizationHeader

@towards
Copy link

@kepten you were faster than me applying@fabpot's comments :) nevertheless part of the bug hunt day I also modified the ServerBag class and tested them on a productive LAMP hosting server using Apache and FastCGI

@kepten
Copy link
ContributorAuthor

ok, so is my PR is useless or should I still fix problems?

@towards
Copy link

your PR is fine for sure and I don't want to interfere, just wanted to mention that part of the bug hunt day of Symfony I had a go at this PR as an "exercise" but just saw later on that you already fixed the problem, so you can ignore my pushes

@vicb
Copy link
Contributor

I have been working with@towards: your PR is useful, please implement his comments and squash your PR.

…fastCGI under ApacheBug fix: yesFeature addition: noBackwards compatibility break: noSymfony2 tests pass: yesFixes the following tickets:symfony#1813Todo: -In order to work, add this to the .htaccess:RewriteEngine onRewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization}]RewriteCond %{REQUEST_FILENAME} !-fRewriteRule ^(.*)$ app.php [QSA,L]
@kepten
Copy link
ContributorAuthor

never squashed before, is it okay now? :)

@stof
Copy link
Member

it is

@vicb
Copy link
Contributor

@fabpot this should be ready to be merged

fabpot added a commit that referenced this pull requestMay 21, 2012
Commits-------a450d00 [HttpFoundation] HTTP Basic authentication is broken with PHP as cgi/fastCGI under ApacheDiscussion----------[HttpFoundation] HTTP Basic authentication is broken with php-cgi under ApacheBug fix: yesFeature addition: noBackwards compatibility break: noSymfony2 tests pass: yesFixes the following tickets:#1813Todo: -In order to work, add this to the .htaccess:RewriteEngine onRewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization}]RewriteCond %{REQUEST_FILENAME} !-fRewriteRule ^(.*)$ app.php [QSA,L]---------------------------------------------------------------------------by stof at 2012-03-10T17:34:26Zyou should also add a unit test for this---------------------------------------------------------------------------by kepten at 2012-03-11T15:34:04ZThanks for the feedback, I committed the changes.---------------------------------------------------------------------------by stof at 2012-04-04T01:59:53Z@fabpot could you review it ?---------------------------------------------------------------------------by fabpot at 2012-04-04T07:15:34ZMy comments: * `ServerBag` represents what we have in the `$_SERVER` global variables. As such, the code should be moved to the `getHeaders()` method instead like the other tweaks we do for the HTTP headers. * A comment must be added explaining why this is needed and the configuration the user must have to make it work (then remove the Github URLs). * The code should only be executed when `PHP_AUTH_USER` is not available (to not have any overhead when not needed).---------------------------------------------------------------------------by danielholmes at 2012-04-14T13:27:09ZA quick note on that .htaccess/apache configuration required, if adding to the Symfony SE htaccess file, then it will need to look like this:```<IfModule mod_rewrite.c>    RewriteEngine On    RewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization}]    RewriteCond %{REQUEST_FILENAME} !-f    RewriteRule ^(.*)$ app.php [QSA,L]</IfModule>```NOTE: No **,L** in the Authorization Rewrite as in the original example - it prevents the front controller rewrite from happening---------------------------------------------------------------------------by towards at 2012-04-20T16:12:49Z@kepten you were faster than me applying@fabpot's comments :) nevertheless part of the bug hunt day I also modified the ServerBag class and tested them on a productive LAMP hosting server using Apache and FastCGI---------------------------------------------------------------------------by kepten at 2012-04-20T16:15:57Zok, so is my PR is useless or should I still fix problems?---------------------------------------------------------------------------by towards at 2012-04-20T16:20:26Zyour PR is fine for sure and I don't want to interfere, just wanted to mention that part of the bug hunt day of Symfony I had a go at this PR as an "exercise" but just saw later on that you already fixed the problem, so you can ignore my pushes---------------------------------------------------------------------------by vicb at 2012-04-20T16:20:36ZI have been working with@towards: your PR is useful, please implement his comments and squash your PR.---------------------------------------------------------------------------by kepten at 2012-04-20T16:59:07Znever squashed before, is it okay now? :)---------------------------------------------------------------------------by stof at 2012-04-20T17:21:07Zit is---------------------------------------------------------------------------by vicb at 2012-05-20T19:57:51Z@fabpot this should be ready to be merged
@fabpotfabpot merged commita450d00 intosymfony:2.0May 21, 2012
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@kepten@stof@fabpot@danielholmes@towards@vicb@immutef@Tobion

[8]ページ先頭

©2009-2025 Movatter.jp