8000 [Form] CsrfValidationListener marks the token as invalid if it is not a string by umpirsky · Pull Request #29884 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] CsrfValidationListener marks the token as invalid if it is not a string #29884

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

Jump to bottom
Merged
merged 1 commit into from
Feb 7, 2019
Merged

[Form] CsrfValidationListener marks the token as invalid if it is not a string #29884

merged 1 commit into from
Feb 7, 2019

Conversation

umpirsky
Copy link
Contributor
@umpirsky umpirsky commented Jan 14, 2019 •
edited
Loading
Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29882
License MIT

@xabbuh
Copy link
Member
xabbuh commented Jan 14, 2019

3.4 is affected by this too, isn't it?

xabbuh
xabbuh requested changes Jan 14, 2019
View reviewed changes
@umpirsky
Copy link
Contributor Author

3.4 is affected by this too, isn't it?

Yes, the fix probably need to be applied there too.

@xabbuh xabbuh changed the base branch from 4.2 to 3.4 January 15, 2019 16:21
@xabbuh
Copy link
Member
xabbuh commented Jan 15, 2019

@umpirsky Can you rebase on the 3.4 branch? :)

@umpirsky
Copy link
Contributor Author

It will be easier to create a new PR. :)

@xabbuh
Copy link
Member
xabbuh commented Jan 15, 2019

@umpirsky if you'd like to, I can have a look at this PR and rebase it

@umpirsky
Copy link
Contributor Author

I will look into it soon...

@umpirsky
Copy link
Contributor Author

I reset to 3.4 and cherry-picked changes. Thanks for the patience!

@xabbuh xabbuh added this to the 3.4 milestone Jan 15, 2019
@umpirsky
Copy link
Contributor Author

Status?

nicolas-grekas
nicolas-grekas requested changes Jan 25, 2019
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Please rebase + change the PR title to make the changelog usefull.
Good to me otherwise!

@umpirsky umpirsky changed the title Fixes issue #29882 CsrfValidationListener marks the token as invalid if it is not a string Jan 26, 2019
@umpirsky
Copy link
Contributor Author

Updated.

@umpirsky umpirsky closed this Jan 26, 2019
@umpirsky umpirsky reopened this Jan 26, 2019
@xabbuh xabbuh requested a review from nicolas-grekas February 2, 2019 07:48
@xabbuh xabbuh added the Form label Feb 2, 2019
@nicolas-grekas nicolas-grekas changed the title CsrfValidationListener marks the token as invalid if it is not a string [Form] CsrfValidationListener marks the token as invalid if it is not a string Feb 7, 2019
@nicolas-grekas
Copy link
Member

Thank you @umpirsky.

@nicolas-grekas nicolas-grekas merged commit deb8e95 into symfony:3.4 Feb 7, 2019
nicolas-grekas added a commit that referenced this pull request Feb 7, 2019
@nicolas-grekas
bug #29884 [Form] CsrfValidationListener marks the token as invalid i…
5c7931c 
…f it is not a string (umpirsky)

This PR was squashed before being merged into the 3.4 branch (closes #29884).

Discussion
----------

[Form] CsrfValidationListener marks the token as invalid if it is not a string

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29882
| License       | MIT

Commits
-------

deb8e95 [Form] CsrfValidationListener marks the token as invalid if it is not a string
@umpirsky umpirsky deleted the fix-29882-4.2 branch February 7, 2019 11:26
This was referenced Mar 3, 2019
Merged
Merged
@COil
Copy link
Contributor
COil commented Mar 4, 2019 •
edited
Loading

Hello, my tests involving CSRF tokens are broken since 4.2.4. I guess it comes from this PR. Reverting to 4.2.3 fix the issue.

    $url.='/create';
    $client->request('POST', $url, [
        CategoryCreateType::FORM_ID => [
            'label' => 'New category', // id is 5
            '_token' => $this->getToken($client, CategoryCreateType::FORM_ID),
        ],
    ]);

It comes from:

protected function getToken(Client $client, string $tokenId): CsrfToken
{
    return $client->getContainer()->get('security.csrf.token_manager')->getToken($tokenId);
}

@umpirsky
Copy link
Contributor Author
umpirsky commented Mar 4, 2019

What does $this->getToken($client, CategoryCreateType::FORM_ID) returns?

@COil
Copy link
Contributor
COil commented Mar 4, 2019 •
edited
Loading

CsrfToken, casting to string fix the probem. :)

@COil
Copy link
Contributor
COil commented Mar 4, 2019 •
edited
Loading

@umpirsky What was the bug at first?

@xabbuh
Copy link
Member
xabbuh commented Mar 4, 2019

@COil When the submitted value was an array the listener resulted in a PHP error (see #29882).

@matks matks mentioned this pull request Mar 6, 2019
Merged
@matks
Copy link
matks commented Mar 6, 2019

CsrfToken, casting to string fix the probem. :)

Same for me, but instead I use $token->getValue() 😉

PrestaShop/PrestaShop#12804

@umpirsky
Copy link
Contributor Author
umpirsky commented Apr 2, 2019

Will there be 4.1.* version with this change?

@xabbuh
Copy link
Member
xabbuh commented Apr 2, 2019 •
edited
Loading

No, 4.1 doesn't receive bugfixes (except for security issues) anymore (see https://symfony.com/roadmap/4.1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
Bug Form Status: Reviewed Validator
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
7 participants
@umpirsky @xabbuh @nicolas-grekas @COil @matks @OskarStark @carsonbot
317C
0