BUG: XSS vulnerability in iframe attribute src #5743
Comments
|
This is unavoidable when using This would need to be addressed by preventing the malicious code from ever loading. This should be handled at the server on save. But, if on the client processing is needed, by wrapping the code in script tags, and then sanitizing the code prior to loading it in to the editor like this: https://jsfiddle.net/bernesto/5gcxa0jm/1/ I do see that when loading via the Personally, I believe that the server should be processing all saves and sanitizing. A malicious actor could always emulate a client request and save an malicious XSS to your server regardless of the editor used. @artf what do you think? In your opinion, should an XSS sanitizer be built into the editor? And if so, that begs the question; how do you feel about Not only would this allow us to sanitize for XSS vulnerabilities, but it would also eliminate the browser processing and painting those initial DOM elements twice (once on load, second to the new frame). This may actually speed up the editor load time as a happy benefit. |
|
Yeah indeed with I wouldn't push an entire sanitizer here (might be a pre-parser option though) but at least I can add a new option to prevent "unsafe" attribute values. |
|
I think the pre-parser option is a really good idea. It sticks to the 'plug-in' per feature concept. How about updating const editor = grapesjs.init({
container: '#gjs',
fromElement: '#mySource',
height: '100%',
storageManager: { type: 0 },
plugins: ['gjs-blocks-basic']
}); |
|
Thanks for the insightful discussion. I want to clarify that while I used Here's how I currently handle HTML compilation: export class HtmlTemplateEditorComponent
implements OnInit, ControlValueAccessor
{
....
private compileHtml(html: string, styles: string, script: string) {
const regexForIndex = /(<\/body>)/g;
const everyLastBodyTagIndex = html.search(regexForIndex);
const result =
`<head><style>${styles}</style></head>` +
html.slice(0, everyLastBodyTagIndex) +
`<script>${script}</script>` +
html.slice(everyLastBodyTagIndex);
return result;
}
private buildContentModel(): HtmlTemplateEditorContentModel {
const html = this.editor.getHtml();
const styles = this.editor.getCss() as string;
const script = this.editor.getJs();
return {
content: this.compileHtml(html, styles, script),
contentObject: { ...this.editor.getProjectData(), assets: null },
};
}
....
}My primary focus is on best practices for preventing XSS within this context. Will the upcoming pre-parser feature in GrapesJS adequately address these concerns? Looking forward to your suggestions. |
|
This will depend on how @artf decides he'd like that architected. I can see how it would make the sense to be inserted in the pipeline where projectData and components sources coalesce prior to being written to the frame DOM and/or when reading HTML/CSS out. That said, you mentioned 'best practices'. BP for security would be parsing and filtering the data at the server prior to storage, where it is beyond the manipulation of nefarious parties. e.g. A user may be well meaning, but a browser plugin may not. For instance it may take advantage of client RTEs with contenteditables to inject hidden code to mine bitcoin in a web worker for instance... (not a suggestion by any means lol) Client side code filtering should be exclusively used for user experience. i.e. Bubbling up warnings from user interactions prior to round tripping to the server to create a responsive and intuitive experience. I speculate that is part of why Artur is more inclined to not include this filtering in the client, favoring pre/post processor of some sort so you can float your own if you need. I personally really like his modular approach and flexibility, as this allows choice in implementation. |
|
Totally agree with @bernesto indeed no matter how hard we try to make it safe, it will never be enough and I don't want to give the impression that the library is "so safe" to justify a missing server-side validation. |
|
@davidgabrichidze I'll try to push the next release in a few days. |
GrapesJS version
What browser are you using?
Edge v122
Reproducible demo link
https://jsfiddle.net/bwreyq29/1/
Describe the bug
How to reproduce the bug?
open this link https://jsfiddle.net/bwreyq29/1/ and javascript code attached to
srcattribute will be executed automatically.What is the expected behaviour?
This code shouldn't run
What is the current behaviour?
XSS vulnerability exists
Code of Conduct
The text was updated successfully, but these errors were encountered: