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 the comments link and markdown rendering issue #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rajat-Dabade
Copy link
Contributor

@Rajat-Dabade Rajat-Dabade commented Dec 17, 2024

Summary

The card with links in the comments was not opening inside the boards. Error when opening the card with a link on the hub in the console:

console.js:288 EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self' cdn.rudderlabs.com".

focalboard_9ce1515f8964ad29_bundle.js:2 [1734085501.12] error boundary redirecting to plugins/focalboard/error?id=unknown

This is because of the Content-Security-Policy Policy in webapp. This policy restricts the use of unsafe javascript methods like new Functions(), which can be exploited in injection attacks.

Before we were using the board markdown renderer which was working fine, but due to Respect display name configuration PR the boards now use the Markdown renderer from the web app (Channels).

The current Markdown renderer includes a custom link rendering function:

renderer.link = (href, title, contents) => {
            return '<a ' +
                'target="_blank" ' +
                'rel="noreferrer" ' +
                `href="${encodeURI(decodeURI(href || ''))}" ` +
                `title="${title || ''}" ` +
                `onclick="${(window.openInNewBrowser ? ' openInNewBrowser && openInNewBrowser(event.target.href);' : '')}"` +
            '>' + contents + '</a>'
        }

the link function in the above custom Markdown renderer contributed to the unsafe-eval CSP error, specifically due to the inline onclick function. The onclick attribute in the rendered HTML attempts to execute JavaScript inline. CSP policies typically disallow inline scripts unless explicitly permitted using a CSP directive like 'unsafe-inline'.

Removing the onclick should fix this issue. Anyways target='_blank' will always open the link in a new tab.

Also, there was a markdown rendering issue in the comments inside the cards. This was also fixed in this PR.

Ticket Link

https://mattermost.atlassian.net/browse/MM-62243

Before:

Screenshot 2024-12-17 at 7 35 21 PM

After:
Screenshot 2024-12-17 at 7 37 08 PM

@Rajat-Dabade Rajat-Dabade added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Dec 17, 2024
@Rajat-Dabade Rajat-Dabade self-assigned this Dec 17, 2024
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find! I'm amazed the original version of this worked before your last PR since the CSP header has been the same as far back as I can remember.

@@ -44,7 +44,6 @@ const Comment: FC<Props> = (props: Props) => {
const formattedText =
<Provider store={(window as any).store}>
{messageHtmlToComponent(formatText(comment.title, {
renderer: Utils.getMarkdownRenderer(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you double checked how Markdown tables render in cards? I see that the renderer returned by Utils.getMarkdownRenderer() also changes how those render as well

Copy link
Member

@harshilsharma63 harshilsharma63 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add tests fro this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants