Skip to content

Test whatthediff #4

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.

Sign up for GitHub

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

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

Test whatthediff #4

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/components/assetLoader/HTMLContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import React, {FC} from 'react';
import {CSSAssetSchemaRenderDescription} from '../../utils/renderDescriptionSchema';
import {useDynamicStyle} from '../../hooks/useDynamicStyle';

type HTMLContainerProps = CSSAssetSchemaRenderDescription & {
htmlContent: string;
dynamicStyle?: any;
};
Comment on lines +5 to +8
Copy link

@coderabbitai coderabbitai bot Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The dynamicStyle prop is typed as any. This could lead to potential type errors or bugs in the future. It would be better to define a more specific type for this prop, if possible.

-	dynamicStyle?: any;
+	dynamicStyle?: Record<string, unknown>;

Copy link
Owner Author

Choose a reason for hiding this comment

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

Is using dangerouslySetInnerHTML safe for this component?

Copy link

Choose a reason for hiding this comment

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

@alfonsograziano The use of dangerouslySetInnerHTML can potentially expose your application to cross-site scripting (XSS) attacks if the HTML content is not properly sanitized. This is because it directly inserts HTML content into your component, which could include malicious scripts.

In this case, the safety of using dangerouslySetInnerHTML depends on the source and handling of the htmlContent prop. If htmlContent is always static, controlled by developers, and never includes user input or data from untrusted sources, then it should be safe. However, if htmlContent can come from an untrusted source or includes user input, it should be sanitized before being passed to dangerouslySetInnerHTML.

If you need to render user-supplied or otherwise untrusted HTML, consider using a library like DOMPurify to sanitize the HTML content before rendering it.

Here's how you might do that:

import DOMPurify from 'dompurify';

// ...

export const HTMLContainer: FC<HTMLContainerProps> = ({
	htmlContent,
	dynamicStyle,
	...props
}) => {
	const currentFrameStyle = useDynamicStyle(dynamicStyle);
	const sanitizedHTML = DOMPurify.sanitize(htmlContent);

	return (
		<div
			// eslint-disable-next-line react/no-danger
			dangerouslySetInnerHTML={{__html: sanitizedHTML}}
			style={{...props.style, ...currentFrameStyle}}
		/>
	);
};

Please note that sanitizing HTML is a complex task and using a well-tested library like DOMPurify is generally safer than trying to do it yourself.


export const HTMLContainer: FC<HTMLContainerProps> = ({
htmlContent,
dynamicStyle,
...props
}) => {
const currentFrameStyle = useDynamicStyle(dynamicStyle);
return (
<div
// eslint-disable-next-line react/no-danger
dangerouslySetInnerHTML={{__html: htmlContent}}
Copy link
Owner Author

Choose a reason for hiding this comment

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

/wtd is this safe?

Copy link

Choose a reason for hiding this comment

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

Suggested change
dangerouslySetInnerHTML={{__html: htmlContent}}
dangerouslySetInnerHTML={{__html: htmlContent}}

style={{...props.style, ...currentFrameStyle}}
/>
);
};
Comment on lines +1 to +23
Copy link
Owner Author

Choose a reason for hiding this comment

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

/wtd give me a feedback on this

Copy link

Choose a reason for hiding this comment

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

Suggested change
import React, {FC} from 'react';
import {CSSAssetSchemaRenderDescription} from '../../utils/renderDescriptionSchema';
import {useDynamicStyle} from '../../hooks/useDynamicStyle';
type HTMLContainerProps = CSSAssetSchemaRenderDescription & {
htmlContent: string;
dynamicStyle?: any;
};
export const HTMLContainer: FC<HTMLContainerProps> = ({
htmlContent,
dynamicStyle,
...props
}) => {
const currentFrameStyle = useDynamicStyle(dynamicStyle);
return (
<div
// eslint-disable-next-line react/no-danger
dangerouslySetInnerHTML={{__html: htmlContent}}
style={{...props.style, ...currentFrameStyle}}
/>
);
};
import React, { FC } from 'react';
import { CSSAssetSchemaRenderDescription } from '../../utils/renderDescriptionSchema';
import { useDynamicStyle } from '../../hooks/useDynamicStyle';
type HTMLContainerProps = CSSAssetSchemaRenderDescription & {
htmlContent: string;
dynamicStyle?: any;
};
export const HTMLContainer: FC<HTMLContainerProps> = ({
htmlContent,
dynamicStyle,
...props
}) => {
const currentFrameStyle = useDynamicStyle(dynamicStyle);
return (
<div
// eslint-disable-next-line react/no-danger
dangerouslySetInnerHTML={{ __html: htmlContent }}
style={{ ...props.style, ...currentFrameStyle }}
/>
);
};