CAS-1203: Allow login error messages to render HTML content

[mmoayyed]

https://issues.jasig.org/browse/CAS-1203

[battags]

If there's a chance an error can contain user provided data (i.e. username, etc.) then we should now allow this. It won't differentiate between the message template having HTML and the parameters, will it?

[mmoayyed]

If I understand your question correctly, the message that is displayed is statically always retrieved from the bundle and it does not include any dynamic parameters. That only "dynamic" thing that affects that is the exception indicating the message code:

messageContext.addMessage(new MessageBuilder().error().code(e.getCode()).defaultText(e.getCode()).build());

So, it does not differentiate between parameters (such as that of the form's) and html code, but there are no parameters unless the above line is modified by an adopter to pass in various parameters and args when building that message. So, it seems to me that this is not violating any security principals, but if someone chooses to modify the error to actually display sensitive data, then the onus is on them.

Does that make sense?

[mmoayyed]

I am going to further look into this and see if what Scott suggests can be duplicated. I'll report back in a little bit.

[mmoayyed]

I reviewed and tested the original changeset with a mocked exception throw that returned sensitive data in its message. What I realized was, the default text that is displayed in case no code is found by the AuthenticationViaFormAction, under certain conditions, actually turns out to the exception message itself.

In the next pull, this is what I have done:

• Refactored AuthenticationException and TicketException to reduce duplicate code and unify the behavior around getCode() for both
• Refactored AuthenticationViaFormAction to fall back to a default message code, rather than the exception message.

[leleuj]

Javadoc is required on class header...

[mmoayyed]

Good point. Will do.

[mmoayyed]

So, are you saying that instead of using BadCredentialsAuthenticationException.CODE, we should be using a better error message? Or, are you saying that the usage of BadCredentialsAuthenticationException.CODE is fine, and we just need a better "default" text in case the code mapped to BadCredentialsAuthenticationException.CODE isn't found?

[leleuj]

Sorry if I wasn't clear : the usage of BadCredentialsAuthenticationException.CODE is fine, but I proposed antoher "default" text in case the code mapped to BadCredentialsAuthenticationException.CODE isn't found.

[serac]

I would like to underscore Scott's concern. We got dinged on a security review of one of our internal apps for a case exactly like this: we had not considered that parameterized error messages could contain content provided by the user, which created an XSS vector when rendered as HTML to the client.

[mmoayyed]

I understand. I think this pull, in the way that it reorganizes the exception hierarchy and in particular, the way it re purposes getCode() should adequately address both concerns. I'll also account for the change in the default text in the newer pull.

[mmoayyed]

I have updated the default text message which may be displayed back to the client in case BadCredentialsAuthenticationException.CODE isn't found.

[leleuj]

+1

[mmoayyed]

Planning to merge this one by EOD, unless there are other concerns around the pull.