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.

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

Typo in SimpleFormAuthenticationListener #30341

Closed
nikic opened this issue Feb 22, 2019 · 5 comments
Closed

Typo in SimpleFormAuthenticationListener #30341

nikic opened this issue Feb 22, 2019 · 5 comments
Labels
Bug Good first issue Ideal for your first contribution! (some Symfony experience may be required) Security Status: Needs Review

Comments

@nikic
Copy link
Contributor

nikic commented Feb 22, 2019

The check https://github.com/symfony/security/blob/baa22f665a7ea04c2bfb4d6236a2020a25979f10/Http/Firewall/SimpleFormAuthenticationListener.php#L100 should be one of

if (!\is_string($username))
// or
if (!\is_string($username) && !(\is_object($username) && !\method_exists($username, '__toString')))

As currently written, the second condition will always be false.

Noticed this while looking into an opcache optimization bug caused by this piece of code :)
@nicolas-grekas nicolas-grekas added the Good first issue Ideal for your first contribution! (some Symfony experience may be required) label Feb 22, 2019
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 22, 2019 •
edited

Thanks, the fix should be submitted on branch 3.4, with a test case if possible.
I'm labeling as "good first issue" if anyone wants to take it.

@chalasr
Copy link
Member

chalasr commented Feb 22, 2019

Same for

symfony/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php

Line 88 in 848a830

if (!\is_string($username) || (\is_object($username) && !\method_exists($username, '__toString'))) {

@PReimers PReimers mentioned this issue Feb 22, 2019
Merged
@ghost
Copy link

ghost commented Feb 22, 2019

I'd like to take it if possible. Should be able to do it over the weekend.

@xabbuh
Copy link
Member

xabbuh commented Feb 22, 2019

@cardillomarcelo I am sorry but someone already opened a pull request (see #30347).

@ghost
Copy link

ghost commented Feb 22, 2019

np 👍

nicolas-grekas added a commit that referenced this issue Feb 23, 2019
@nicolas-grekas
bug #30347 [Security] Change FormAuthenticator if condition (PReimers)
1aac865 
This PR was squashed before being merged into the 3.4 branch (closes #30347).

Discussion
----------

[Security] Change FormAuthenticator if condition

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

I changed the if condition in `SimpleFormAuthenticationListener` and `UsernamePasswordFormAuthenticationListener` based on the solution provided by @nikic in issue #30341

#OpenSourceFriday

Commits
-------

67ae121 [Security] Change FormAuthenticator if condition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Good first issue Ideal for your first contribution! (some Symfony experience may be required) Security Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nikic @nicolas-grekas @xabbuh @chalasr @carsonbot