8000 [Form] Removed extra CSRF field on collection prototype by webmozart · Pull Request #3996 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Form] Removed extra CSRF field on collection prototype #3996

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

Merged
merged 1 commit into from
Apr 19, 2012

Conversation

webmozart
Copy link
Contributor

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

Travis Build Status

@webmozart
Copy link
Contributor Author

Wait a minute please, I oversaw some tests that have to be adapted.

{
if ($form->isRoot() && $form->hasChildren() && $form->hasAttribute('csrf_field_name')) {
if (!$view->hasParent() && $view->hasChildren() && $form->hasAttribute('csrf_field_name')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@bschussek I came to about the same conclusion: !$view->hasParent() && $form->hasChildren() && $form->hasAttribute('csrf_field_name'). I thought the children of the view might not be there yet ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see: "BottomUp"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I changed it to buildViewBottomUp, where the children are already there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check the children at all ? (what if the root form is an empty collection)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, otherwise a simple input field cannot be submitted. If an empty collection form has a CSRF field, it doesn't hurt anyone.

Copy link
Contributor
8000

Choose a reason for hiding this comment

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

empty collection -> hasChildren() === false -> no CSRF ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got me there :) An empty collection (or form in general for that sake) doesn't transmit data, so there's nothing to protect, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • couldn't this be used to erase a collection (not protected) ?
  • what if children are added dynamically ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have created a new issue as this PR is closed

@webmozart
Copy link
Contributor Author

Fixed. ping @fabpot

fabpot added a commit that referenced this pull request Apr 19, 2012
Commits
-------

ccd6bbc [Form] Removed extra CSRF field on collection prototype

Discussion
----------

[Form] Removed extra CSRF field on collection prototype

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

![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3990)

---------------------------------------------------------------------------

by bschussek at 2012-04-19T08:43:32Z

Wait a minute please, I oversaw some tests that have to be adapted.

---------------------------------------------------------------------------

by bschussek at 2012-04-19T09:22:45Z

Fixed. ping @fabpot
@fabpot fabpot merged commit ccd6bbc into symfony:master Apr 19, 2012
vicb added a commit to vicb/symfony that referenced this pull request Apr 27, 2012
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.

3 participants
0