fixed errors on /app/articles/app/page.tsx and added types for isomorphic-dompurify
#1179
Conversation
|
@petercr is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hello @petercr, thanks for opening your first Pull Request. The maintainers will review this Pull Request and provide feedback as soon as possible. Keep up the great work! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/(app)/articles/[slug]/page.tsx (3)
Line range hint
63-71: Consider enhancing DOMPurify configuration for stricter sanitization.While the current sanitization is good, consider adding specific DOMPurify configuration options to further restrict allowed content based on your application's needs.
const renderSanitizedTiptapContent = (jsonContent: JSONContent) => { const rawHtml = generateHTML(jsonContent, [...TiptapExtensions]); - return DOMPurify.sanitize(rawHtml); + return DOMPurify.sanitize(rawHtml, { + ALLOWED_TAGS: ['p', 'b', 'i', 'em', 'strong', 'a', 'ul', 'ol', 'li', 'code', 'pre'], + ALLOWED_ATTR: ['href', 'class'], + ALLOW_DATA_ATTR: false + }); };
121-123: Document security measures for future maintainers.While the content is properly sanitized, it's important to document the security measures in place when using
dangerouslySetInnerHTML.<div - dangerouslySetInnerHTML={{ __html: renderedContent ?? "" }} + // Security: Content is sanitized via DOMPurify in renderSanitizedTiptapContent + dangerouslySetInnerHTML={{ __html: renderedContent ?? "" }} className="tiptap-content" />🧰 Tools
🪛 Biome
[error] 122-122: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
Line range hint
90-102: Consider adding runtime type validation for content.To enhance robustness, consider adding runtime type validation for the content before rendering.
if (isTiptapContent && parsedBody) { + // Validate expected structure of Tiptap content + if (typeof parsedBody.content === 'undefined') { + console.error('Invalid Tiptap content structure'); + return <div>Error loading article content</div>; + } const jsonContent = parsedBody; renderedContent = renderSanitizedTiptapContent(jsonContent); } else { + // Validate Markdoc content + if (typeof post.body !== 'string') { + console.error('Invalid Markdoc content type'); + return <div>Error loading article content</div>; + } const ast = Markdoc.parse(post.body); const transformedContent = Markdoc.transform(ast, config);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
- app/(app)/articles/[slug]/page.tsx (2 hunks)
🧰 Additional context used
🪛 Biome
app/(app)/articles/[slug]/page.tsx
[error] 122-122: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (2)
app/(app)/articles/[slug]/page.tsx (2)
17-19: LGTM: Import changes look secure and correct.The switch to
@tiptap/coreand the use ofisomorphic-dompurifyfor sanitization are appropriate choices for secure content handling.
126-130: LGTM: Proper handling of non-Tiptap content.The use of
Markdoc.renderers.reactfor non-Tiptap content is correct and secure.
app/(app)/articles/[slug]/page.tsx
Outdated
| @@ -119,11 +119,15 @@ const ArticlePage = async ({ params }: Props) => { | |||
|
|
|||
| {isTiptapContent ? ( | |||
| <div | |||
| dangerouslySetInnerHTML={{ __html: renderedContent }} | |||
| dangerouslySetInnerHTML={{ __html: renderedContent ?? "" }} | |||
There was a problem hiding this comment.
If we fallback to "" should we not have an error thrown?
There was a problem hiding this comment.
So without the extra || "" you get a Typescript error warning you that if it ends up getting passes a null value, there will be problems.
It seems like an easy fix, but if you wanted to default to something else I will be happy to change it 👍🏻
Or if you want me to pass in or throw a new error I could add that too.
There was a problem hiding this comment.
I think if there is no content it should probably be notFound since there is no data (or content) for that page? That make sense?
There was a problem hiding this comment.
Great I will just bring in the NotFound component then, and replace the empty string instance with that 👍
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/(app)/articles/[slug]/page.tsx (2)
Line range hint
63-73: Consider adding error logging for JSON parsing failures.While the error handling is good, adding error logging would help diagnose content parsing issues in production.
const parseJSON = (str: string): JSONContent | null => { try { return JSON.parse(str); } catch (e) { + console.error('Failed to parse article content:', e); return null; } };
Potential XSS vulnerability in CoursePreview component
The verification revealed inconsistent HTML sanitization across components:
app/(app)/articles/[slug]/page.tsxproperly uses DOMPurify for sanitizationcomponents/Course/CoursePreview.tsxuses dangerouslySetInnerHTML without sanitization for the description propcomponents/ArticleMenu/ArticleMenu.tsxuses innerHTML safely (only for comparison, not for rendering)Action items:
- Add DOMPurify sanitization to CoursePreview component before rendering description
- Consider creating a shared sanitization utility to ensure consistent usage across components
🔗 Analysis chain
Line range hint
69-73: Verify HTML sanitization is consistently applied.While the current implementation properly sanitizes Tiptap content using DOMPurify, let's verify that all content paths are properly sanitized.
Also applies to: 123-125
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any direct HTML rendering without sanitization rg -n "dangerouslySetInnerHTML|innerHTML" --type tsx # Search for DOMPurify usage to ensure sanitization rg -n "DOMPurify.sanitize" --type tsxLength of output: 148
Script:
#!/bin/bash # Search for any direct HTML rendering without sanitization, trying different file extensions rg -n "dangerouslySetInnerHTML|innerHTML" --type-add 'web:*.{tsx,jsx,ts,js}' --type web # Search for DOMPurify usage to ensure sanitization rg -n "DOMPurify.sanitize" --type-add 'web:*.{tsx,jsx,ts,js}' --type web # Look for imports of DOMPurify rg -n "import.*DOMPurify" --type-add 'web:*.{tsx,jsx,ts,js}' --type webLength of output: 664
Script:
#!/bin/bash # Check the context around dangerouslySetInnerHTML usage in CoursePreview rg -B 5 -A 5 "dangerouslySetInnerHTML" components/Course/CoursePreview.tsx # Check if CoursePreview has DOMPurify import or sanitization rg "DOMPurify|sanitize" components/Course/CoursePreview.tsx # Check the full context of ArticleMenu's innerHTML usage rg -B 5 -A 5 "innerHTML" components/ArticleMenu/ArticleMenu.tsxLength of output: 758
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/(app)/articles/[slug]/page.tsx (2 hunks)
🧰 Additional context used
🪛 Biome
app/(app)/articles/[slug]/page.tsx
[error] 123-123: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (1)
app/(app)/articles/[slug]/page.tsx (1)
17-21: LGTM: Import changes successfully resolve the module resolution issue.The change from
@tiptap/htmlto@tiptap/coreforgenerateHTMLimport addresses the original crash issue, while maintaining the required functionality.
| dangerouslySetInnerHTML={{ | ||
| __html: renderedContent ?? <NotFound />, | ||
| }} |
There was a problem hiding this comment.
Fix NotFound component implementation.
The current implementation attempts to use JSX within dangerouslySetInnerHTML which won't work as expected. The NotFound component should be rendered conditionally.
- dangerouslySetInnerHTML={{
- __html: renderedContent ?? <NotFound />,
- }}
+ {...(renderedContent
+ ? {
+ dangerouslySetInnerHTML: {
+ __html: renderedContent,
+ },
+ }
+ : {}
+ )}
+ >
+ {!renderedContent && <NotFound />}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dangerouslySetInnerHTML={{ | |
| __html: renderedContent ?? <NotFound />, | |
| }} | |
| {...(renderedContent | |
| ? { | |
| dangerouslySetInnerHTML: { | |
| __html: renderedContent, | |
| }, | |
| } | |
| : {} | |
| )} | |
| > | |
| {!renderedContent && <NotFound />} |
🧰 Tools
🪛 Biome
[error] 123-123: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
✨ Codu Pull Request 💻
Fixes
It is related to issue #1168 and E2E testing, but more importantly it fixes an issue on the articles page which would not load the corresponding article without crashing.
Pull Request details
can't resolve module @tiptap/htmlAny Breaking changes
None.
Associated Screenshots
Screenshot of the issue now resolved with current PR:
