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

Trusted proxies were removed when URL signing took over #6541

Conversation

rawkode
Copy link
Contributor

@rawkode rawkode commented May 6, 2016

.. index::
single: Request; Trusted Proxies

Trusting Proxies
Copy link
Member

Choose a reason for hiding this comment

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

this removal is wrong. We still have trusted proxies (even though ESI accepts signed requests from anywhere)

@stof
Copy link
Member

stof commented May 6, 2016

the commit you linked does not remove trusted proxies at all. It removed their usage in a single place which was not working properly

@rawkode rawkode force-pushed the bugfix/remove-all-mention-of-trusted-proxies branch from da10763 to 1bf084d Compare May 6, 2016 18:33
@rawkode
Copy link
Contributor Author

rawkode commented May 6, 2016

Sorry, @stof - silly assumption on my part. I've removed all the daft removals.

TIL more about Symfony and the FragmentListener 👍

@xabbuh
Copy link
Member

xabbuh commented May 21, 2016

👍

wouterj added a commit that referenced this pull request May 21, 2016
…rawkode)

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #6541).

Discussion
----------

Trusted proxies were removed when URL signing took over

Since: symfony/http-kernel@fa8f4f8

Commits
-------

853825b Removing incorrect reference that the FragmentListener only listens to ``trusted_proxies``
@wouterj
Copy link
Member

wouterj commented May 21, 2016

Thank you @rawkode for updating the code.

As this security fix was merged into the 2.3 version of the code, I've merged this into the 2.3 version of the docs as well. I'll take care of merging it into the other versions.

@wouterj wouterj closed this May 21, 2016
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.

4 participants