Skip to content

[Security] Verifying if the password field is null #36925

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
Aug 18, 2020
Merged

[Security] Verifying if the password field is null #36925

merged 1 commit into from
Aug 18, 2020

Conversation

Shine-neko
Copy link

@Shine-neko Shine-neko commented May 23, 2020 •
edited
Loading

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #36926
License MIT

@Shine-neko Shine-neko mentioned this pull request May 23, 2020
Closed
@nicolas-grekas nicolas-grekas changed the base branch from master to 3.4 May 23, 2020 16:20
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 23, 2020
@xabbuh
Copy link
Member

xabbuh commented May 24, 2020

Shouldn't we rather check how the credentials could end up being null at all instead?

@wouterj
Copy link
Member

wouterj commented May 24, 2020

I think I agree with @xabbuh here. I think this should be fixed in UsernamePasswordFormAuthenticationListener and a LogicException should be thrown instead - this is not a user error (like blank password), but a developer error.

@Shine-neko
Copy link
Author

I think I agree with @xabbuh here. I think this should be fixed in UsernamePasswordFormAuthenticationListener and a LogicException should be thrown instead - this is not a user error (like blank password), but a developer error.

A similar thing ? https://gist.github.com/Shine-neko/76f97ee46950a480074434fa3259f797/revisions

@wouterj
Copy link
Member

wouterj commented May 25, 2020

Yes, seems good. Although I think the error message can be changed, so we can give the developer guidelines on how to fix it (e.g. adding the expected password parameter named - $this->options['password_parameter']).

@fabpot
Copy link
Member

fabpot commented Jun 23, 2020

Any news? It looks the gist code is more in line with what we should do.

@Shine-neko Shine-neko force-pushed the fix-nullable-password branch 3 times, most recently from cb5e269 to 564361e Compare June 26, 2020 17:44
@Shine-neko
Copy link
Author

@fabpot done

@Shine-neko Shine-neko force-pushed the fix-nullable-password branch from 564361e to cfc946a Compare June 29, 2020 06:40
@Shine-neko Shine-neko requested a review from fabpot June 29, 2020 06:40
fabpot
fabpot requested changes Aug 17, 2020
View reviewed changes
src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php Outdated
@@ -95,6 +95,10 @@ protected function attemptAuthentication(Request $request)
throw new BadCredentialsException('Invalid username.');
}

if (null === $password) {
throw new \LogicException(sprintf('The key "%s" cannot be null.', $this->options['password_parameter']));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe try to explain the problem?

sprintf('The key password cannot be null; check that the password field name of the form matches "%s".', $this->options['password_parameter'], $this->options['password_parameter']);

@Shine-neko Shine-neko force-pushed the fix-nullable-password branch from cfc946a to d9d9cec Compare August 17, 2020 15:36
@Shine-neko Shine-neko requested a review from fabpot August 17, 2020 15:36
@fabpot fabpot changed the base branch from 3.4 to master August 18, 2020 06:32
@fabpot fabpot force-pushed the fix-nullable-password branch from d9d9cec to e4a14ac Compare August 18, 2020 06:32
@fabpot
Copy link
Member

fabpot commented Aug 18, 2020

Thank you @Shine-neko.

@fabpot fabpot merged commit 09ff501 into symfony:master Aug 18, 2020
@ogizanagi ogizanagi mentioned this pull request Aug 19, 2020
Merged
fabpot added a commit that referenced this pull request Aug 19, 2020
@fabpot
minor #37886 [Security] Fix tests (ogizanagi)
7e24fc1 
This PR was merged into the 5.2-dev branch.

Discussion
----------

[Security] Fix tests

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | N/A <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | N/A

Fixes https://travis-ci.org/github/symfony/symfony/jobs/719243848#L6234-L6242 (relates to #36925)

Commits
-------

e04386c [Security] Fix tests
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

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

Successfully merging this pull request may close these issues.

Authentication provider throw empty password if password is null
6 participants
@Shine-neko @xabbuh @wouterj @fabpot @nicolas-grekas @carsonbot