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

Fix incorrect handling of non-node items inserted into the DOM #1533

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

BenjaminAster
Copy link
Contributor

Happy-DOM's current handling of inserting items into the DOM that are not nodes via .prepend(), .append(), .before(), .after(), .replaceWith() or .replaceChildren() is incorrect and might even allow for XSS vulnerabilities:
If an item is a string, Happy-DOM parses the string as HTML and inserts the parsed HTML. However, the actual behavior should be to add text nodes with the string as the text content, which is then automatically sanitized.

element.append("<script>");
console.log(element.innerHTML);
// Expected result: "&lt;script&gt;"
// Actual result: "<script></script>"

Additionally, if an item is neither a string nor a node, the browser converts it to a string automatically, but Happy-DOM errors.

element.append(123, true, {});
console.log(element.textContent);
// Expected result: "123true[object Object]"
// Actual result: [throws an error]

Note that the extra String() wrapper before passing it to document.createTextNode is necessary due to the special behavior of undefined in the DOM methods vs. the new Text() constructor.

element.append(new Text(undefined));
console.log(element.textContent);
// Expected result: "";
// BUT:
element.append(undefined);
console.log(element.textContent);
// Expected result: "undefined";

This PR (hopefully) fixes this to match the actual behavior of browsers. Let me know if I'm missing something and if I should add tests.

@BenjaminAster BenjaminAster marked this pull request as draft September 10, 2024 21:37
@BenjaminAster
Copy link
Contributor Author

BenjaminAster commented Sep 10, 2024

Alright, so apparently Happy-DOM uses the .replaceWith() method for the outerHTML setter, which then expects the wrong unsanitizing behavior... :/ Plus, the failing tests also seem to imply that the instanceof Node check isn't working reliably, or something...?

Anyways, I'll look into this this probably tomorrow; don't merge this yet.

@capricorn86
Copy link
Owner

Thank you for contributing @BenjaminAster! 🙂

Looking forward for the fix!

I believe that instanceof Node should be a reliable way of detecting if it is a Node. The tests are probably failing because the test the current behavior.

@BenjaminAster BenjaminAster marked this pull request as ready for review September 11, 2024 17:00
@BenjaminAster
Copy link
Contributor Author

@capricorn86 Ok, I have now modified the tests and the outerHTML setter, and it should be working now with the tests actually passing. GitHub still reports some integration-tests failing; I do not know what this means.


Btw, unrelated to this:
When I git commit on my machine, it reports the following errors:

2024-09-11 18:55:44.588 [info] husky - DEPRECATED

Please remove the following two lines from .husky/pre-commit:

#!/usr/bin/env sh
. "$(dirname -- "$0")/_/husky.sh"

They WILL FAIL in v10.0.0

C:\[MY_PERSONAL_PATH_TO]\happy-dom\node_modules\.bin\happy-lint-changed:2
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
          ^^^^^^^

SyntaxError: missing ) after argument list
    at wrapSafe (node:internal/modules/cjs/loader:1469:18)
    at Module._compile (node:internal/modules/cjs/loader:1491:20)
    at Module._extensions..js (node:internal/modules/cjs/loader:1691:10)
    at Module.load (node:internal/modules/cjs/loader:1317:32)
    at Module._load (node:internal/modules/cjs/loader:1127:12)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:217:24)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:166:5)
    at node:internal/main/run_main_module:30:49

Node.js v22.7.0
husky - pre-commit script failed (code 1)

, which I can circumvent by running git commit with --no-verify, but maybe this is something that should be inspected further? I have no idea about any of this, so I doubt I can help much here 😬.

Copy link
Owner

@capricorn86 capricorn86 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @BenjaminAster! 🌟

Not sure how this got implemented wrong, but great that it got fixed.

@capricorn86 capricorn86 merged commit afd256b into capricorn86:master Sep 11, 2024
3 checks passed
@capricorn86
Copy link
Owner

The failing unit test was because of some hiccup. It loads real websites such as github.com and npmjs.com into Happy DOM, which fails randomly from time to time.

@BenjaminAster
Copy link
Contributor Author

Thank you! And no worries, it's easy to get confused with all the hundreds of DOM methods and what they do exactly, and this error probably stayed undetected for a while because it's very rare to insert text that contains HTML tags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants