8000 Improve a security related exception · Issue #45913 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

Improve a security related exception #45913

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
Closed
javiereguiluz opened this issue Apr 1, 2022 · 6 comments
Closed

Improve a security related exception #45913

javiereguiluz opened this issue Apr 1, 2022 · 6 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Security

Comments

@javiereguiluz
Copy link
Member
javiereguiluz commented Apr 1, 2022 •
edited
Loading

Description

Via an issue in a third-party bundle I've seen that there's a security-related exception message that could be improved:

symfony/src/Symfony/Component/Security/Http/Logout/LogoutUrlGenerator.php

Line 150 in f3ec7a0

throw new \InvalidArgumentException('Unable to find the current firewall LogoutListener, please provide the provider key manually.');

The exception message mentions which is the problem, but it's not very precise about the possible solution. A better exception message would mention the name of the firewall to configure and the exact config option name that should be added or updated.

Thanks!
@xabbuh xabbuh added Security DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Apr 3, 2022
@stof
Copy link
Member
stof commented Apr 7, 2022

If we were able to know the firewall name, we would not need the exception at all. Except in advanced setups, the provider key is precisely the firewall name.

@javiereguiluz
Copy link
Member Author

@stof I'm sorry but I don't fully understand your comment. Are you saying that we can't (for some technical reasons) improve the exception message with more details ... or are you saying that we shouldn't do that? Thanks!

@wouterj
Copy link
Member
wouterj commented Apr 7, 2022

The message is saying "We can't figure out the firewall, so we can't find the logout listener to use" (but with technical terms like "provider key").

We can improve the message to no longer use the outdated terms like "provider key", but we can't add the firewall name, because that's exactly what is missing here.

If I read the code correctly, 2 scenarios can produce this error:

  1. Calling this code in a request that is not behind a firewall
  2. Calling this code in a request that is behind a firewall, but that firewall has no logout defined

Maybe we can also add this to the exception message?

@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@carsonbot
Copy link

Friendly ping? Should this still be open? I will close if I don't hear anything.

@alamirault alamirault mentioned this issue Nov 14, 2022
Closed
@alamirault
Copy link
Contributor

I created PR to manage these 2 cases as suggested by @wouterj

@carsonbot carsonbot removed the Stalled label Nov 14, 2022
@fabpot fabpot closed this as completed in 631673c Nov 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Security
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Security] Improve LogoutUrlGenerator exception message alamirault/symfony
6 participants
@javiereguiluz @stof @wouterj @xabbuh @alamirault @carsonbot
0