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

[HttpKernel] Prevent search engines from indexing dev applications #30325

Merged
merged 1 commit into from Mar 4, 2019

Conversation

@GaryPEGEOT
Copy link
Contributor

GaryPEGEOT commented Feb 20, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30318
License MIT
Doc PR TODO

Add the X-Robots-Tag: noindex to dev (and test) applications to prevent search engines to index them.

@nicolas-grekas nicolas-grekas added this to the next milestone Feb 21, 2019

@GaryPEGEOT GaryPEGEOT force-pushed the GaryPEGEOT:feature/noindex-listener branch from 29b9d02 to d294c4f Feb 26, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 3, 2019

@GaryPEGEOT Please, don't change code that your PR does not change (even if fabbot ask for it).

@GaryPEGEOT

This comment has been minimized.

Copy link
Contributor Author

GaryPEGEOT commented Mar 3, 2019

@javiereguiluz where should I document the new option?

@GaryPEGEOT GaryPEGEOT force-pushed the GaryPEGEOT:feature/noindex-listener branch from afa4a09 to 5b7f836 Mar 4, 2019

@fabpot

fabpot approved these changes Mar 4, 2019

@fabpot fabpot force-pushed the GaryPEGEOT:feature/noindex-listener branch from 67ab198 to 3dd8671 Mar 4, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 4, 2019

Thank you @GaryPEGEOT.

@fabpot fabpot merged commit 3dd8671 into symfony:master Mar 4, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 4, 2019

feature #30325 [HttpKernel] Prevent search engines from indexing dev …
…applications (GaryPEGEOT)

This PR was squashed before being merged into the 4.3-dev branch (closes #30325).

Discussion
----------

[HttpKernel] Prevent search engines from indexing dev applications

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30318
| License       | MIT
| Doc PR        | TODO

Add the *X-Robots-Tag: noindex* to dev (and test) applications to prevent search engines to index them.

Commits
-------

3dd8671 [HttpKernel] Prevent search engines from indexing dev applications
@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Mar 13, 2019

@GaryPEGEOT sorry I didn't reply to your comment. This feature is being documented in symfony/symfony-docs#11148. Thanks.

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Mar 13, 2019

Sorry I'm late on this one, but this feature could have been a bit more generic: the config could allow to set headers that will always be sent, including X-Robot-Tag, but also Content Security Policies, HSTS, HPKP settings, the Referrer header, CORS settings...

I can work on a PR.

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Mar 13, 2019

@dunglas can you please show an example of the HTTP header/s added to the response with your proposal and a sample of the YAML config used to do that? Thanks.

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Mar 13, 2019

on the other side, having rich configuration with sane defaults, per case is nice. But agree this could be aggregated into a single default header listener 👍

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Mar 13, 2019

@javiereguiluz:

framework:
    default_headers:
        Content-Security-Policy : "script-src 'self' www.google-analytics.com ajax.googleapis.com"
        Referrer-Policy: same-origin
        Strict-Transport-Security: "max-age=31536000; includeSubDomains; preload"
        X-Robot-Tag: noindex
        X-XSS-Protection: "1; mode=block"
        X-Frame-Options: SAMEORIGIN
        X-Content-Type-Options: nosniff

We could even provide sane defaults. WDYT?

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Mar 13, 2019

I like this idea @dunglas ! Perhaps it should be under framework.http.* though, because there are more http related settings such as session?

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Mar 13, 2019

@dunglas are we completely sure that 100% of existing apps are going to keep working if we apply those headers by default? Thanks.

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Mar 13, 2019

@javiereguiluz no! By default I mean "in the config generated by the recipe".

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Mar 13, 2019

Please don't call other people "idiots".

@stof

This comment has been minimized.

Copy link
Member

stof commented Mar 13, 2019

For all these security headers, I think using https://github.com/nelmio/NelmioSecurityBundle/ is better. Most of them require more configuration than just knowing whether we are in debug mode or no.

And this default_headers proposal would still have to be separate from the current feature, as adding X-Robot-Tags through it only in debug mode would be much more complex.

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Mar 13, 2019

@HTMLGuyLLC I agree with @linaori's comment and I ask you to please not call other people "idiots".

About your original comment, I'm afraid that those people who "run Symfony in dev because they cannot make it work in prod" will need to try again to put things in prod and report here any problem that they find in Symfony itself. Thanks!

@ostrolucky

This comment has been minimized.

Copy link
Contributor

ostrolucky commented Mar 13, 2019

Even if we don't go with an idea of having config for these like @dunglas suggested, at least listener class itself could be made in more generic way, so userland can write compiler pass with own rules. Having such single purpose listener which is completely inflexible seems such a waste to me. Changing it so headers can be injected should be trivial.

edit: on second thought, problem with this might be be that it needs to be re-registered in prod :/

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Mar 13, 2019

one can still provide default headers per environment config, given this node merges each. A sub config might want to clear an inherited header using X-Name: false

For all these security headers, I think using https://github.com/nelmio/NelmioSecurityBundle/ is better.

Agree, but for the simple case this config spares out some boilerplate code.

The flip side is we get more feature requests eventually; like expression support; better security defaults. I think that's reasonable, but so is a code solution or an external bundle.

Sticking with "config per case" avoids that decision path.

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 14, 2019

minor #11148 Document the disallow_search_engine_index option (javier…
…eguiluz)

This PR was merged into the master branch.

Discussion
----------

Document the disallow_search_engine_index option

This documents symfony/symfony#30325.

Commits
-------

a94b920 Document the disallow_search_engine_index option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.