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

Editor crashes if the attribute name is a number #13841

Closed
Mgsy opened this issue Apr 7, 2023 · 2 comments · Fixed by #13923
Closed

Editor crashes if the attribute name is a number #13841

Mgsy opened this issue Apr 7, 2023 · 2 comments · Fixed by #13923
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

@Mgsy
Copy link
Member

Mgsy commented Apr 7, 2023

📝 Provide detailed reproduction steps (if any)

  1. Go to https://ckeditor.com/docs/ckeditor5/37.0.1/features/html/general-html-support.html.
  2. Execute editor.setData('<p><foo 200="test">test</foo></p>').

❌ Actual result

The editor crashes.

snippet.js:4 Uncaught Error: Failed to execute 'setAttribute' on 'Element': '200' is not a valid attribute name.
    at na.setDomElementAttribute (snippet.js:4:121865)
    at na.viewToDom (snippet.js:4:121421)
    at na.viewChildrenToDom (snippet.js:4:122402)
    at viewChildrenToDom.next (<anonymous>)
    at na.viewToDom (snippet.js:4:121502)
    at Pc.toData (snippet.js:4:234200)
    at model (snippet.js:4:568008)
    at snippet.js:4:214567
    at Tc.<anonymous> (snippet.js:4:214600)
    at Tc.fire (snippet.js:4:5713)

📃 Other details

  • It crashes also after pasting the content.

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

@Mgsy Mgsy added type:bug This issue reports a buggy (incorrect) behavior. support:2 An issue reported by a commercially licensed client. squad:core Issue to be handled by the Core team. labels Apr 7, 2023
@arkflpc
Copy link
Contributor

arkflpc commented Apr 11, 2023

Actually, it can fail whenever the attribute name starts with non-letter. In example (in Firefox and Chrome):

// Both fail:
element.setAttribute('2x', 'abc')
element.setAttribute('-x', 'abc')

// Both seem to be accepted by browsers:
element.setAttribute('x2', 'abc')
element.setAttribute('x-', 'abc')

@niegowski
Copy link
Contributor

We should filter out invalid DOM attributes. We are doing it already but for custom elements, we have a custom support that lacks the filtering:

const htmlContent = editor.data.processor.toData( documentFragment );

as while we process attributes, we skip invalid ones:

if ( !isValidAttributeName( key as string ) ) {
attributes.delete( key );
}

@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Apr 12, 2023
@arkflpc arkflpc self-assigned this Apr 17, 2023
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Apr 17, 2023
arkflpc added a commit that referenced this issue Apr 21, 2023
Fix (engine): Editor should not crash when a custom element with an invalid attribute name is pasted. Closes #13841.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Apr 21, 2023
@CKEditorBot CKEditorBot added this to the iteration 63 milestone Apr 21, 2023
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.

5 participants
@niegowski @arkflpc @Mgsy @CKEditorBot and others