-
Notifications
You must be signed in to change notification settings - Fork 148
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
[GH-692] Fix formatting inside code block in issue created notification #697
Conversation
…reated notification (#35) * [MI-3405] Fix issue: improper formatting inside code block in issue notification * [MI-3461] Add a comment to clearify the usage of code
server/plugin/webhook.go
Outdated
// Replacing the unicodes with the respective special characters to have proper rendering in code block. | ||
result := strings.ReplaceAll(policy.Sanitize(description), "'", "'") | ||
result = strings.ReplaceAll(result, """, "\"") | ||
result = strings.ReplaceAll(result, "&", "&") | ||
result = strings.ReplaceAll(result, ">", ">") | ||
result = strings.ReplaceAll(result, "<", "<") | ||
return strings.TrimSpace(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though I wonder if there is a more catch-all holistic approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister I explored other packages to sanitize the HTML but the one we are using right now was the best I could find. But this converts the special characters to their unicode value which are not formatted inside a code block, so we are replacing them again here. Please feel free to share your opinions if you have a better approach in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm so it's the policy.Sanitize
that's causing the issue. We really just want to use it to remove the <details>
elements from the description, and not escape those characters.
As this comment microcosm-cc/bluemonday#39 (comment) mentions on a related issue for the bluemonday
package, we can call html.UnescapeString
from the result to make those characters go back to what we want them to be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister Updated
server/plugin/webhook.go
Outdated
result = strings.ReplaceAll(result, ">", ">") | ||
result = strings.ReplaceAll(result, "<", "<") | ||
result := policy.Sanitize(description) | ||
html.UnescapeString(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to reassign the value right?
html.UnescapeString(result) | |
result = html.UnescapeString(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister Updated
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #697 +/- ##
==========================================
- Coverage 15.78% 15.78% -0.01%
==========================================
Files 15 15
Lines 5574 5575 +1
==========================================
Hits 880 880
- Misses 4652 4653 +1
Partials 42 42
☔ View full report in Codecov by Sentry. |
Does the |
@mickmister Yes, it's removing the HTML tags in the string. For some tags, it's just removing the tags and for some, it's removing the tags along with the text inside. |
@raghavaggarwal2308 Since the main thing we want to sanitize is |
@mickmister If we do so and there are other tags along with the details tag, then they will be removed as well. Is that fine? |
@raghavaggarwal2308 Yes that's fine 👍 |
…>" tag is present in it.
@mickmister Added logic to sanitize description conditionally. |
…notification (mattermost#697) * [MI-3405] Fix issue: improper formatting inside code block in issue created notification (#35) * [MI-3405] Fix issue: improper formatting inside code block in issue notification * [MI-3461] Add a comment to clearify the usage of code * Updated code to support less than and greater than symbols * Updated code to use html.UnescapeString * Review fix * [MI-3633] Updated logic to sanitize description only when a "<details>" tag is present in it.
* [GH-692] Fix formatting inside code block in issue created notification (#697) * [MI-3405] Fix issue: improper formatting inside code block in issue created notification (#35) * [MI-3405] Fix issue: improper formatting inside code block in issue notification * [MI-3461] Add a comment to clearify the usage of code * Updated code to support less than and greater than symbols * Updated code to use html.UnescapeString * Review fix * [MI-3633] Updated logic to sanitize description only when a "<details>" tag is present in it. * Bump plugin version to v2.1.7 --------- Co-authored-by: Raghav Aggarwal <raghav.aggarwal@brightscout.com>
Summary
Fixed the improper formatting of special characters inside the code block of issue-created channel notifications.
What to test?
Screenshots
Earlier:
![image](https://private-user-images.githubusercontent.com/55234496/267675450-198219bb-0075-4d07-813b-e22ed234b6ae.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ3NzIyOTksIm5iZiI6MTcyNDc3MTk5OSwicGF0aCI6Ii81NTIzNDQ5Ni8yNjc2NzU0NTAtMTk4MjE5YmItMDA3NS00ZDA3LTgxM2ItZTIyZWQyMzRiNmFlLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI3VDE1MTk1OVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTYyZGIzMzQ1N2M3NTA3NWJjNjJhOTczMzQ3ZWRhYzE3YTc3OGU2NDZjMWFhNzM3YmYyN2Q5NjUxODE4YmFjMWEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.OLqWVHxTPnldG9aJZ_Qr0zReK-1mZGBHS1oCaoulUWE)
Now:
![image](https://private-user-images.githubusercontent.com/55234496/267675232-c46a72ff-d08a-4ec7-874e-081839593767.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ3NzIyOTksIm5iZiI6MTcyNDc3MTk5OSwicGF0aCI6Ii81NTIzNDQ5Ni8yNjc2NzUyMzItYzQ2YTcyZmYtZDA4YS00ZWM3LTg3NGUtMDgxODM5NTkzNzY3LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI3VDE1MTk1OVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTI2ZTE5OGEyMDNjZTZiNjZjZjA0ZGNlYzRhMjYyMjRlY2ZmMTM1ZWUyMTJlNzA2ZGQ0YjQ0ZmM2MTJjNTU4NzYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.nF4bnTr970ARt4Alq_9NWxRgNvNN2cTPI-viy3tUa5I)
Ticket Link
Fixes #692