Skip to content

DefaultLoginPageGeneratingFilter should be able to handle AuthenticationExceptions without message #13768

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

Closed
pctF opened this issue Sep 1, 2023 · 1 comment
Labels
status: waiting-for-triage An issue we've not yet triaged type: bug A general bug

Comments

@pctF
Copy link

pctF commented Sep 1, 2023

Describe the bug

Right now DefaultLoginPageGeneratingFilter#getLoginErrorMessage can return nullable exception message. This message passed to org.springframework.web.util.HtmlUtils#htmlEscape with can not handle null with results in exception in.. exception handling.

I think filter should have normal flow because:

  1. It is default filter for many applications
  2. Message is nullable attribute and there is always a chance to catch one with null message and there is really no valid way to enforce otherwise at this point

To Reproduce

Create an filter/user service that throws org.springframework.security.core.AuthenticationException with null message.

java.lang.IllegalArgumentException: Input is required
        at org.springframework.util.Assert.notNull(Assert.java:201)
	at org.springframework.web.util.HtmlUtils.htmlEscape(HtmlUtils.java:83)
	at org.springframework.web.util.HtmlUtils.htmlEscape(HtmlUtils.java:63)
	at org.springframework.security.web.authentication.ui.DefaultLoginPageGeneratingFilter.createError(DefaultLoginPageGeneratingFilter.java:372)

Expected behavior

Here should be some discussion:

  1. We already have default message: "Invalid credentials". IMHO it doesn't really fit
  2. We can show at least exception name ie: exception.getClass().getSimpleName(). It doesn't expose much but can provide some information
  3. Utilize org.springframework.context.MessageSourceAware with some default message like "Unexpected error while performing login" and code for overriding it. Seems like commitment for future backward compatibility but possible.
@pctF pctF added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Sep 1, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Nov 7, 2023

Thanks for the suggestion, @pctF. I agree that the code should be more resilient to null values.

That said, I would recommend just addressing the null value for the time being, instead of doing 1, 2, and 3 as you described.

  1. We already have default message: "Invalid credentials". IMHO it doesn't really fit

You may have a point here, but this would be a conversation about the page as a whole, so I'd recommend it go into a separate ticket.

  1. We can show at least exception name ie: exception.getClass().getSimpleName(). It doesn't expose much but can provide some information

I don't think we should do this. It exposes the underlying technology. Developers can find exceptions in their logs; showing the exception to the end user provides no actionable information for them.

  1. Utilize org.springframework.context.MessageSourceAware with some default message like "Unexpected error while performing login" and code for overriding it. Seems like commitment for future backward compatibility but possible.

This filter is not intended to be internationalized. Instead, it should typically be replaced with a custom login page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants