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

Getting domconverter-unsafe-attribute-detected while setting the renderUnsafeAttributes #11879

Closed
NachoVazquez opened this issue Jun 6, 2022 · 2 comments · Fixed by #11929
Closed
Assignees
Labels
squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@NachoVazquez
Copy link

NachoVazquez commented Jun 6, 2022

📝 Provide detailed reproduction steps (if any)

  1. Create a custom Plugin with downcast conversion from attributeToElement adding an onclick attribute and bypassing security with renderUnsafeAttributes: ['onclick']
  2. Build the editor and plugin.
  3. Open the app and see the message domconverter-unsafe-attribute-detected after applying the plugin to a text element.

This is the full implementation

conversion.for('downcast').attributeToElement({
      model: 'terms-highlighter',
      view: (modelAttributeValue, { writer: viewWriter }) => {
        const viewElement = viewWriter.createAttributeElement(
          'terms-highlighter',
          {
            class: 'terms-highlighter',
            term: modelAttributeValue,
            onclick: `alert('hi')`,
          },
          {
            // Make sure the "onclick" attribute will pass through.
            renderUnsafeAttributes: ['onclick'],
          }
        );

        console.log(viewElement.shouldRenderUnsafeAttribute('onclick')); // Returns True

        return viewElement;
      },
      converterPriority: 'high',
    });

✔️ Expected result

The onclick event should not be removed and the message domconverter-unsafe-attribute-detected should not appear on the console.

❌ Actual result

The domconverter-unsafe-attribute-detected is shown on the console and the onclick attribute is removed from the DOM element.

{
    "domElement": terms-highlighter.terms-highlighter,
    "key": "onclick",
    "value": "alert('hi')"
}

❓ Possible solution

I have no idea. I saw the test for this behavior and they seem to be correct.

📃 Other details

  • Browser: Chrome 102.0.5005.61 (Official Build) (arm64)
  • OS: macOS Monterey 12.4
  • First affected CKEditor version: I have this from version 31-34
  • Versions used:
    "@ckeditor/ckeditor5-angular": "^4.0.0",
    "@ckeditor/ckeditor5-basic-styles": "^34.1.0",
    "@ckeditor/ckeditor5-core": "^34.1.0",
    "@ckeditor/ckeditor5-editor-classic": "^34.1.0",
    "@ckeditor/ckeditor5-essentials": "^34.1.0",
    "@ckeditor/ckeditor5-heading": "^34.1.0",
    "@ckeditor/ckeditor5-image": "^34.1.0",
    "@ckeditor/ckeditor5-inspector": "^4.1.0",
    "@ckeditor/ckeditor5-link": "^34.1.0",
    "@ckeditor/ckeditor5-list": "^34.1.0",
    "@ckeditor/ckeditor5-paragraph": "^34.1.0",
    "@ckeditor/ckeditor5-remove-format": "^34.1.0",
    "@ckeditor/ckeditor5-special-characters": "^34.1.0",
    "@ckeditor/ckeditor5-table": "^34.1.0",
    "@ckeditor/ckeditor5-theme-lark": "^34.1.0",
    "@ckeditor/ckeditor5-ui": "^34.1.0",
    "@ckeditor/ckeditor5-upload": "^34.1.0",
    "@ckeditor/ckeditor5-utils": "^34.1.0",
    "@ckeditor/ckeditor5-widget": "^34.1.0",
  • Installed CKEditor plugins:
  • Bold,
  • Essentials,
  • Heading,
  • Image,
  • ImageCaption,
  • ImageToolbar,
  • ImageUpload,
  • Italic,
  • Link,
  • List,
  • ListProperties,
  • Paragraph,
  • RemoveFormat,
  • GraphQLUploadAdapter,
  • SpecialCharacters,
  • SpecialCharactersArrows,
  • SpecialCharactersCurrency,
  • SpecialCharactersEssentials,
  • SpecialCharactersLatin,
  • SpecialCharactersMathematical,
  • SpecialCharactersText,
  • Strikethrough,
  • Subscript,
  • Superscript,
  • Table,
  • Underline,
  • TermsHighlighter,

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@NachoVazquez NachoVazquez added the type:bug This issue reports a buggy (incorrect) behavior. label Jun 6, 2022
@niegowski
Copy link
Contributor

This looks like a missing line in a view Element.clone():

cloned._unsafeAttributesToRender = this._unsafeAttributesToRender;

This bug is happening there because the AttributeElement is always cloned to wrap specific nodes.

@Mgsy Mgsy added support:2 An issue reported by a commercially licensed client. squad:core Issue to be handled by the Core team. labels Jun 7, 2022
@NachoVazquez
Copy link
Author

Please let me know if I can help, I could create a PR for this if that helps!
Thanks!

@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jun 20, 2022
arkflpc added a commit that referenced this issue Jun 21, 2022
Fix (engine): The view element renderUnsafeAttributes option should not be lost for an AttributeElements. Closes #11879.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jun 21, 2022
@CKEditorBot CKEditorBot added this to the iteration 54 milestone Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants