AdminActivityLogTableRow: fix bug in displaying error message #8608 #8609
Conversation
| @@ -145,7 +146,7 @@ public String getLogId() { | |||
| } | |||
|
|
|||
| public String getActionName() { | |||
| return activityLog.getActionName(); | |||
There was a problem hiding this comment.
Can we rename to getDisplayedActionName and move the method above the Forwarding activityLog methods as it should belong to Enhancement to the fields
| @@ -157,7 +158,7 @@ public boolean getIsMasqueradeUserRole() { | |||
| } | |||
|
|
|||
| public String getMessage() { | |||
| public void testSanitizeForLogMessage() { | ||
| assertNull("should return null if given null", SanitizationHelper.sanitizeForLogMessage(null)); | ||
|
|
||
| String unsanitized = "<span class=\"text-danger\"> A <span class=\"bold\">typical</span> log message <br>" |
There was a problem hiding this comment.
Some HTML elements with other class?
There was a problem hiding this comment.
currently i'm only allowing bold and text-danger as that's what i found the log messages to be using. Other classes will be escaped.
There was a problem hiding this comment.
That is exactly I am asking for, to test that scenario also and make sure it works
| return null; | ||
| } | ||
| return unsanitizedString | ||
| .replaceAll("<(?!(/?(span( class=\"(bold|text-danger)\")?|br)>))", "<") |
There was a problem hiding this comment.
I believe there is a more readable way to write this. Check richTextPolicy in SanitizationHelper
There was a problem hiding this comment.
richTextPolicy uses the OWASP sanitization library for Java which will remove disallowed elements instead of escaping them. Hence I had to write my own regular expression to do this. I went for conciseness here. The more readable way would be to list out all the full-string possibilities, i.e. something like
"<(?!(span class=\"bold\">|span class=\"text-danger\">|/span>|br>))"
Do you think we should use this form instead?
| assertNull("should return null if given null", SanitizationHelper.sanitizeForLogMessage(null)); | ||
|
|
||
| String unsanitized = "<span class=\"text-danger\"> A <span class=\"bold\">typical</span> log message <br>" | ||
| + " It contains some <script>dangerous</script> elements </span>"; |
There was a problem hiding this comment.
This is to make sure ' is also sanitised.
Opps. See your changes already :P
|
|
||
| unsanitized = "Hmm. <span class=\"text-info\"> How about this? </span> and <span> this</span>"; | ||
| correctSanitized = | ||
| "Hmm. <span class="text-info"> How about this? </span> and <span> this</span>"; |
There was a problem hiding this comment.
The html tag is not well-formed :(
But seems no better solution for this.
There was a problem hiding this comment.
yeah, this is ugly but there's no simple solution for this.
There was a problem hiding this comment.
Can be fixed together with sub-issue in #6502 - give a standard way to generate statusToAdmin
There was a problem hiding this comment.
Ideally, the log message should just contain plaintext but have a predefined form based on the result type. Then we can sanitize the whole log message, and use the predefined form to parse and format the message according to what we want.
| unsanitized = "Hmm. <span class=\"text-info\"> How about this? </span> and <span> this</span>"; | ||
| correctSanitized = | ||
| "Hmm. <span class="text-info"> How about this? </span> and <span> this</span>"; | ||
| assertEquals("Should escape <span class=\"text-danger\">, <span class=\"bold\"> and <br>", |
There was a problem hiding this comment.
The message is bit confusing
Should escape tag other than <span class=\"text-danger\">, <span class=\"bold\"> and <br> right?
There was a problem hiding this comment.
My mistake. Copy pasted from above and didn't edit properly
xpdavid
added
s.FinalReview
damithc
added
s.ToMerge
Fixes #8608
Outline of Solution
Use the correct function to retrieve the response result in a log message.