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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
[Form] Removed extra CSRF field on collection prototype
  • Loading branch information
@webmozart
webmozart committed Apr 19, 2012
commit ccd6bbc0a1124960b9cf3f3d26e663d063895859
4 changes: 2 additions & 2 deletions src/Symfony/Component/Form/Extension/Csrf/Type/FormTypeCsrfExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ public function buildForm(FormBuilder $builder, array $options)
* @param FormView $view The form view
* @param FormInterface $form The form
*/
public function buildView(FormView $view, FormInterface $form)
public function buildViewBottomUp(FormView $view, FormInterface $form)
{
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

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

$name = $form->getAttribute('csrf_field_name');
$csrfProvider = $form->getAttribute('csrf_provider');
$intention = $form->getAttribute('csrf_intention');
Expand Down
64 changes: 32 additions & 32 deletions src/Symfony/Component/Form/Tests/AbstractDivLayoutTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,7 @@ public function testRest()
$html = $this->renderRest($view);

$this->assertMatchesXpath($html,
'/input
[@type="hidden"]
[@id="name__token"]
/following-sibling::div
'/div
[
./label[@for="name_field1"]
/following-sibling::input[@type="text"][@id="name_field1"]
Expand All @@ -112,6 +109,9 @@ public function testRest()
[count(../div)=2]
[count(..//label)=2]
[count(..//input)=3]
/following-sibling::input
[@type="hidden"]
[@id="name__token"]
'
);
}
Expand Down Expand Up @@ -144,8 +144,7 @@ public function testRestWithChildrenForms()
$html = $this->renderRest($view);

$this->assertMatchesXpath($html,
'/input[@type="hidden"][@id="parent__token"]
/following-sibling::div
'/div
[
./label[not(@for)]
/following-sibling::div[@id="parent_child1"]
Expand All @@ -172,6 +171,7 @@ public function testRestWithChildrenForms()
]
[count(//label)=4]
[count(//input[@type="text"])=2]
/following-sibling::input[@type="hidden"][@id="parent__token"]
'
);
}
Expand All @@ -189,15 +189,15 @@ public function testRestAndRepeatedWithRow()
$html = $this->renderRest($view);

$this->assertMatchesXpath($html,
'/input
[@type="hidden"]
[@id="name__token"]
/following-sibling::div
'/div
[
./label[@for="name_first"]
/following-sibling::input[@type="text"][@id="name_first"]
]
[count(.//input)=1]
/following-sibling::input
[@type="hidden"]
[@id="name__token"]
'
);
}
Expand All @@ -216,16 +216,16 @@ public function testRestAndRepeatedWithRowPerChild()
$html = $this->renderRest($view);

$this->assertMatchesXpath($html,
'/input
[@type="hidden"]
[@id="name__token"]
/following-sibling::div
'/div
[
./label[@for="name_first"]
/following-sibling::input[@type="text"][@id="name_first"]
]
[count(.//input)=1]
[count(.//label)=1]
/following-sibling::input
[@type="hidden"]
[@id="name__token"]
'
);
}
Expand All @@ -246,16 +246,16 @@ public function testRestAndRepeatedWithWidgetPerChild()
$html = $this->renderRest($view);

$this->assertMatchesXpath($html,
'/input
[@type="hidden"]
[@id="name__token"]
/following-sibling::div
'/div
[
./label[@for="name_first"]
/following-sibling::input[@type="text"][@id="name_first"]
]
[count(//input)=2]
[count(//label)=1]
/following-sibling::input
[@type="hidden"]
[@id="name__token"]
'
);
}
Expand Down Expand Up @@ -293,8 +293,7 @@ public function testCollectionRow()
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[
./input[@type="hidden"][@id="form__token"]
/following-sibling::div
./div
[
./label[not(@for)]
/following-sibling::div
Expand All @@ -311,6 +310,7 @@ public function testCollectionRow()
]
]
]
/following-sibling::input[@type="hidden"][@id="form__token"]
]
[count(.//input)=3]
'
Expand All @@ -327,8 +327,7 @@ public function testForm()
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[
./input[@type="hidden"][@id="name__token"]
/following-sibling::div
./div
[
./label[@for="name_firstName"]
/following-sibling::input[@type="text"][@id="name_firstName"]
Expand All @@ -338,6 +337,7 @@ public function testForm()
./label[@for="name_lastName"]
/following-sibling::input[@type="text"][@id="name_lastName"]
]
/following-sibling::input[@type="hidden"][@id="name__token"]
]
[count(.//input)=3]
'
Expand Down Expand Up @@ -383,8 +383,8 @@ public function testCsrf()
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[
./input[@type="hidden"][@id="name__token"][@value="foo&bar"]
/following-sibling::div
./div
/following-sibling::input[@type="hidden"][@id="name__token"][@value="foo&bar"]
]
[count(.//input[@type="hidden"])=1]
'
Expand All @@ -400,8 +400,7 @@ public function testRepeated()
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[
./input[@type="hidden"][@id="name__token"]
/following-sibling::div
./div
[
./label[@for="name_first"]
/following-sibling::input[@type="text"][@id="name_first"]
Expand All @@ -411,6 +410,7 @@ public function testRepeated()
./label[@for="name_second"]
/following-sibling::input[@type="text"][@id="name_second"]
]
/following-sibling::input[@type="hidden"][@id="name__token"]
]
[count(.//input)=3]
'
Expand All @@ -428,8 +428,7 @@ public function testRepeatedWithCustomOptions()
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[
./input[@type="hidden"][@id="name__token"]
/following-sibling::div
./div
[
./label[@for="name_first"][.="[trans]Test[/trans]"]
/following-sibling::input[@type="text"][@id="name_first"][@required="required"]
Expand All @@ -439,6 +438,7 @@ public function testRepeatedWithCustomOptions()
./label[@for="name_second"][.="[trans]Test2[/trans]"]
/following-sibling::input[@type="text"][@id="name_second"][@required="required"]
]
/following-sibling::input[@type="hidden"][@id="name__token"]
]
[count(.//input)=3]
'
Expand All @@ -454,12 +454,12 @@ public function testSearchInputName()
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[
./input[@type="hidden"][@id="full__token"]
/following-sibling::div
./div
[
./label[@for="full_name"]
/following-sibling::input[@type="search"][@id="full_name"][@name="full[name]"]
]
/following-sibling::input[@type="hidden"][@id="full__token"]
]
[count(//input)=2]
'
Expand Down Expand Up @@ -521,8 +521,7 @@ public function testThemeInheritance($parentTheme, $childTheme)
$this->assertWidgetMatchesXpath($view, array(),
'/div
[
./input[@type="hidden"]
/following-sibling::div
./div
[
./label[.="parent"]
/following-sibling::input[@type="text"]
Expand All @@ -539,6 +538,7 @@ public function testThemeInheritance($parentTheme, $childTheme)
]
]
]
/following-sibling::input[@type="hidden"]
]
'
);
Expand Down
16 changes: 8 additions & 8 deletions src/Symfony/Component/Form/Tests/AbstractLayoutTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,11 +646,11 @@ public function testSingleChoiceExpanded()
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[
./input[@type="hidden"][@id="name__token"]
/following-sibling::input[@type="radio"][@name="name"][@id="name_0"][@value="&a"][@checked]
./input[@type="radio"][@name="name"][@id="name_0"][@value="&a"][@checked]
/following-sibling::label[@for="name_0"][.="[trans]Choice&A[/trans]"]
/following-sibling::input[@type="radio"][@name="name"][@id="name_1"][@value="&b"][not(@checked)]
/following-sibling::label[@for="name_1"][.="[trans]Choice&B[/trans]"]
/following-sibling::input[@type="hidden"][@id="name__token"]
]
[count(./input)=3]
'
Expand All @@ -669,11 +669,11 @@ public function testSingleChoiceExpandedSkipEmptyValue()
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[
./input[@type="hidden"][@id="name__token"]
/following-sibling::input[@type="radio"][@name="name"][@id="name_0"][@checked]
./input[@type="radio"][@name="name"][@id="name_0"][@checked]
/following-sibling::label[@for="name_0"][.="[trans]Choice&A[/trans]"]
/following-sibling::input[@type="radio"][@name="name"][@id="name_1"][not(@checked)]
/following-sibling::label[@for="name_1"][.="[trans]Choice&B[/trans]"]
/following-sibling::input[@type="hidden"][@id="name__token"]
]
[count(./input)=3]
'
Expand All @@ -691,11 +691,11 @@ public function testSingleChoiceExpandedWithBooleanValue()
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[
./input[@type="hidden"][@id="name__token"]
/following-sibling::input[@type="radio"][@name="name"][@id="name_0"][@checked]
./input[@type="radio"][@name="name"][@id="name_0"][@checked]
/following-sibling::label[@for="name_0"][.="[trans]Choice&A[/trans]"]
/following-sibling::input[@type="radio"][@name="name"][@id="name_1"][not(@checked)]
/following-sibling::label[@for="name_1"][.="[trans]Choice&B[/trans]"]
/following-sibling::input[@type="hidden"][@id="name__token"]
]
[count(./input)=3]
'
Expand All @@ -714,13 +714,13 @@ public function testMultipleChoiceExpanded()
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[
./input[@type="hidden"][@id="name__token"]
/following-sibling::input[@type="checkbox"][@name="name[]"][@id="name_0"][@checked][not(@required)]
./input[@type="checkbox"][@name="name[]"][@id="name_0"][@checked][not(@required)]
/following-sibling::label[@for="name_0"][.="[trans]Choice&A[/trans]"]
/following-sibling::input[@type="checkbox"][@name="name[]"][@id="name_1"][not(@checked)][not(@required)]
/following-sibling::label[@for="name_1"][.="[trans]Choice&B[/trans]"]
/following-sibling::input[@type="checkbox"][@name="name[]"][@id="name_2"][@checked][not(@required)]
/following-sibling::label[@for="name_2"][.="[trans]Choice&C[/trans]"]
/following-sibling::input[@type="hidden"][@id="name__token"]
]
[count(./input)=4]
'
Expand Down
Loading
0