symfony/symfony-docsPublic

NotificationsYou must be signed in to change notification settings
Fork5.3k
Star2.3k

document old way of checking validity of CSRF token#5818

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

Jump to bottom

Closed

snoek09 wants to merge5 commits intosymfony:2.3fromsnoek09:document-old-way-checking-validity-csrf-token

Closed

document old way of checking validity of CSRF token#5818

snoek09 wants to merge5 commits intosymfony:2.3fromsnoek09:document-old-way-checking-validity-csrf-token

Conversation

Copy link

snoek09 commentedOct 20, 2015

Q	A
Doc fix?	yes
New docs?	yes
Applies to	all
Fixed tickets	Related to#4668

document old way of checking validity of CSRF token

d6cfb36

snoek09 mentioned this pull request

Oct 20, 2015

document old way of checking validity of CSRF token#5340

Closed

Copy link

Member

xabbuh commentedOct 20, 2015

👍

xabbuh added the Status: Reviewed label

Oct 21, 2015

OskarStark reviewed

Oct 22, 2015

View reviewed changes

book/controller.rst

Copy link

Contributor

Choose a reason for hiding this comment

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

DELETE in backticks?

Copy link

Author

Choose a reason for hiding this comment

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

I checked the docs for mentions of HTTP methods. In a sentence they are never in backticks.

Copy link

Contributor

Choose a reason for hiding this comment

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

👍

xabbuh added Status: Needs Work and removed Status: Reviewed labels

Oct 22, 2015

Henry Snoek added2 commits

October 22, 2015 19:36

fix code example

eda7332

use generateCsrfToken()

73209d0

Copy link

Author

snoek09 commentedOct 24, 2015

@xabbuh please let me know if the example code is fine now.

Copy link

Author

snoek09 commentedDec 1, 2015

@xabbuh is this PR finished?

xabbuh reviewed

Dec 1, 2015

View reviewed changes

book/controller.rst Outdated

Copy link

Member

Choose a reason for hiding this comment

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

I think this should link to the method in the interface instead (it doesn't matter which implementation is registered as the service).

Copy link

Member

xabbuh commentedDec 1, 2015

@snoek09 Sorry for missing your last comment. I left a last minor comment. After that this looks good to be merged to me.

link to interface

7b57421

Copy link

Author

snoek09 commentedDec 1, 2015

@xabbuh No problem. I updated the link.

xabbuh added Status: Reviewed and removed Status: Needs Work labels

Dec 1, 2015

Copy link

Contributor

OskarStark commentedDec 2, 2015

👍

Copy link

Author

snoek09 commentedDec 9, 2015

@xabbuh I think this is ready to be merged.

Copy link

Member

xabbuh commentedDec 9, 2015

@weaverryan @wouterj @javiereguiluz Ready to go here? :)

weaverryan reviewed

Dec 10, 2015

View reviewed changes

book/controller.rst

Copy link

Member

Choose a reason for hiding this comment

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

Does it make sense to show this without showing a template? When you generate the CSRF token in the template, you would need to also includeauthenticate then, right?

Copy link

Member

weaverryan commentedDec 10, 2015

I added some comments, but I like it! Short and simple.

To be clear, this is not an "old way" of checking CSRF, correct? It seems totally valid and current.

Thanks!

Copy link

Contributor

ogizanagi commentedDec 10, 2015

@weaverryan : No it isn't, because theform.csrf_provider service is deprecated since 2.4, as well as the wholeCsrfProviderInterface. We usesecurity.csrf.token_manager instead.

However, I wonder if thebook/controller.rst page is the right place for this ? In#4668,@wouterj suggested to create a new entry in the cookbook instead, and to me it actually makes much more sense to have this in a Controller or Security cookbook section.
I know PRs for 2.6+ are already merged (#5325), but what do you think ?

more realistic example

fb1c73d

Copy link

Author

snoek09 commentedDec 15, 2015

@ogizanagi is right. The example code has to work with 2.3 using the 'old way'.
I also agree that this documentation should be moved to the cookbook.

Copy link

Member

xabbuh commentedDec 15, 2015

I think we can merge it just as it is and then work on converting this into a cookbook recipe afterwards. What do you think?

Copy link

Contributor

ogizanagi commentedDec 15, 2015

👍

Copy link

Author

snoek09 commentedDec 15, 2015

I like that idea@xabbuh.

Copy link

Member

xabbuh commentedDec 31, 2015

@weaverryan @wouterj @javiereguiluz Okay with merging and thinking about how to rework the whole part afterwards?

Copy link

Member

javiereguiluz commentedDec 31, 2015

@xabbuh yes!

Copy link

Member

weaverryan commentedJan 10, 2016

👍

Copy link

Member

xabbuh commentedJan 11, 2016

Thank you Henry.

xabbuh added a commit that referenced this pull request

Jan 11, 2016

minor#5818document old way of checking validity of CSRF token (snoe…

8b8a48a

…k09)This PR was squashed before being merged into the 2.3 branch (closes#5818).Discussion----------document old way of checking validity of CSRF token| Q             | A| ------------- | ---| Doc fix?      | yes| New docs?     | yes| Applies to    | all| Fixed tickets | Related to#4668Commits-------8257cc8 document old way of checking validity of CSRF token

xabbuh closed this

Jan 11, 2016

snoek09 deleted the document-old-way-checking-validity-csrf-token branch

January 11, 2016 10:22

Labels

Status: Reviewed

7 participants

Movatterモバイル変換

Uh oh!

document old way of checking validity of CSRF token#5818

document old way of checking validity of CSRF token#5818

Uh oh!

Conversation

snoek09 commentedOct 20, 2015

Uh oh!

xabbuh commentedOct 20, 2015

Uh oh!

OskarStarkOct 22, 2015

Choose a reason for hiding this comment

Uh oh!

snoek09Oct 22, 2015

Choose a reason for hiding this comment

Uh oh!

OskarStarkOct 22, 2015

Choose a reason for hiding this comment

Uh oh!

snoek09 commentedOct 24, 2015

Uh oh!

snoek09 commentedDec 1, 2015

Uh oh!

xabbuhDec 1, 2015

Choose a reason for hiding this comment

Uh oh!

xabbuh commentedDec 1, 2015

Uh oh!

snoek09 commentedDec 1, 2015

Uh oh!

OskarStark commentedDec 2, 2015

Uh oh!

snoek09 commentedDec 9, 2015

Uh oh!

xabbuh commentedDec 9, 2015

Uh oh!

weaverryanDec 10, 2015

Choose a reason for hiding this comment

Uh oh!

weaverryan commentedDec 10, 2015

Uh oh!

ogizanagi commentedDec 10, 2015

Uh oh!

snoek09 commentedDec 15, 2015

Uh oh!

xabbuh commentedDec 15, 2015

Uh oh!

ogizanagi commentedDec 15, 2015

Uh oh!

snoek09 commentedDec 15, 2015

Uh oh!

xabbuh commentedDec 31, 2015

Uh oh!

javiereguiluz commentedDec 31, 2015

Uh oh!

weaverryan commentedJan 10, 2016

Uh oh!

xabbuh commentedJan 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants