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.

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

Fix possible null value in AuthenticationSuccessHandler #1267

Merged
merged 4 commits into from Feb 7, 2020

Conversation

@bytehead
Copy link
Member

bytehead commented Jan 30, 2020

See #1263.

@bytehead bytehead added the defect label Jan 30, 2020
@bytehead bytehead added this to the 4.9 milestone Jan 30, 2020
@bytehead bytehead self-assigned this Jan 30, 2020
@bytehead bytehead changed the title Cast possible null value to string Fix possible null value in AuthenticationSuccessHandler Jan 30, 2020
@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jan 30, 2020

I don't know if it's really an issue - there was an old template in use in #1263. WDYT?

@bytehead bytehead added up for discussion and removed defect labels Jan 30, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 30, 2020

I'm not sure if it is wise to work around this – at least the error message makes the users aware that there is a problem. What happens if we silently ignore an empty target path?

@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jan 30, 2020

The user will possibly be redirected to the root page. Which is wrong at least for the backend.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 30, 2020

Will they still realize that there is a problem in their template?

@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jan 31, 2020

Probably not. But the error from #1263 was anyway not too obvious imho.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 31, 2020

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Feb 1, 2020

I think I would throw a 400 HTTP exception in this case. If it is a login request and it doesn't contain all the necessary information, that's a client request issue.

@leofeyer leofeyer added defect and removed up for discussion labels Feb 4, 2020
@leofeyer leofeyer merged commit 8c1a856 into contao:4.9 Feb 7, 2020
9 checks passed
9 checks passed
Coverage
Details
Coding Style
Details
PHP 7.2
Details
PHP 7.3
Details
PHP 7.4
Details
Prefer Lowest
Details
Bundles
Details
Windows
Details
codecov/project 89.89% (+<.01%) compared to 231e0e9
Details
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Feb 7, 2020

Thank you @bytehead.

@bytehead bytehead deleted the bytehead:bugfix/1263 branch Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.