8000 Improved nginx config to not expose other php files by peterrehm · Pull Request #6008 · symfony/symfony-docs · GitHub
[go: up one dir, main page]

Skip to content

Improved nginx config to not expose other php files #6008

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

Closed
wants to merge 2 commits into from
Closed

Improved nginx config to not expose other php files #6008

wants to merge 2 commits into from

Conversation

peterrehm
Copy link
Contributor
Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets #6005

This should improve the security as all non defined php files will return
a 404 error code instead of providing them as text which is a more sane
solution.

@wouterj
Copy link
Member
wouterj commented Dec 13, 2015

I'm a bit -1 on denying access to all other files. Many people just copy/past this code example without even reading it. Having some custom php file that doesn't work anymore will result in questions. I think we should explicitely match config.php and app_dev.php.

the web directory. All other files will be denied. You **must** also make
sure that if you *do* deploy ``app_dev.php`` or ``config.php`` that these
files are secured and not available to any outside user (the IP address
checking code at the top of each file does this by default).
Copy link
Member

Choose a reason for hiding this comment

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

this last sentence no longer makes sense, if you do deploy app_dev.php or config.php, a 404 is returned so you don't have to secure it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still the user should be aware of it... If he removes the config above he will get the issue again.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, perhaps we should just talk about the end result to avoid.

  1. Move this out of a tip block - just make it an explanation
  2. Turn the following into a caution block.
.. caution::

    After you deploy to production, make sure that you **cannot** access the ``app_dev.php``
    or ``config.php`` scripts (i.e. ``http://example.com/app_dev.php`` and ``http://example.com/config.php``).
    If you *can* access these, be sure to remove the ``DEV`` section from the above configuration.

@peterrehm
Copy link
Contributor Author

The thing is matching app_* and config does not make sense for dev environments and could cause confusion and troubles. Especially if people copy and paste the chances are higher that they remove the dev clauses than removing the files. If they manage to do that they should be safe and can enable other files if they really need it.

@peterrehm
Copy link
Contributor Author

Can we get that merged or shall I close the PR?

@xabbuh
Copy link
Member
xabbuh commented Feb 9, 2016

Is there no way to let nginx execute other PHP files too instead of returning them as plain text?

@wouterj
Copy link
Member
wouterj commented Feb 9, 2016

@xabbuh yes, that would mean changing the current return 404; line to the three fastcgi_pass lines to execute PHP.

However, for security reasons, you don't want to run the config.php and other scripts in the SE.

@xabbuh
Copy link
Member
xabbuh commented Feb 9, 2016

We could still have the return 404; for app_dev.php and config.php but use fastcgi_pass for all other PHP files.

@wouterj
Copy link
Member
wouterj commented Feb 9, 2016

Yeah, I think that's the best option. @peterrehm can you maybe update your PR?

@peterrehm
Copy link
Contributor Author

Don't you think that the current approach is better as it forces you to define the additional files?
With the current approach files are not being passed to fastcgi unless named app.... The highest security level is achieved with my proposal, everything else weakens it.

If we would allow more files the internal setting also won't work any more as it is not any more a front controller setup.

@xabbuh
Copy link
Member
xabbuh commented May 21, 2016

I am still a bit mixed on this. I agree with you on the Security perspective, but I am afraid that this config will add confusion for people who have to ship there own PHP files.

@xabbuh
Copy link
Member
xabbuh commented May 21, 2016

On the other hand thinking of this again how common is it that you have to serve your own PHP files in a Symfony application?

@peterrehm
Copy link
Contributor Author

Rather uncommon. And I think security is more important.

@xabbuh
Copy link
Member
xabbuh commented May 21, 2016

👍

also make sure that if you *do* deploy ``app_dev.php`` or ``config.php``
that these files are secured and not available to any outside user (the
IP address checking code at the top of each file does this by default).
the web directory. All other files will be denied. You **must** also make
Copy link
Member

Choose a reason for hiding this comment

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

All other files ending in .php will be denied.

@peterrehm
Copy link
Contributor Author

Updated according to @weaverryan's comments.

@weaverryan
Copy link
Member
weaverryan commented May 22, 2016

Thanks Peter!

@peterrehm peterrehm deleted the patch-2 branch May 24, 2016 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
De 3C75 velopment

Successfully merging this pull request may close these issues.

4 participants
0