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

Question about banned/disallowed tags #28

Closed
qqilihq opened this issue Jul 25, 2018 · 9 comments
Closed

Question about banned/disallowed tags #28

qqilihq opened this issue Jul 25, 2018 · 9 comments

Comments

@qqilihq
Copy link
Contributor

qqilihq commented Jul 25, 2018

Great tool! However, I was wondering:

domJSON defines the tags <link> and <script> as 'disallowed', and states the following:

A list of disallowed HTMLElement tags - there is no flexibility here, these cannot be processed by domJSON for security reasons!

What's the reason behind this? What would be the security implications of just serializing them as well?

@azaslavsky
Copy link
Owner

azaslavsky commented Jul 26, 2018

Thanks!

Hmm, to be quite honest, it written was 4 years ago, so I'm not entirely sure! I think the reasoning behind this was that if we serialize the arbitrary javascript in a <script> tag, and some other client re-injects that serialization into their DOM, that client is then exposed to whatever was in that script.

So for example, if browser A has some malicious extension installed that injects some sort of tracking javascript on every page, and that DOM gets serialized with .toJSON() on that browser and then rebuilt with .toDOM() on browser B, browser B now has the malicious code executing on their machine, without having the malicious extension installed! Arguably, you could put the onus for this security mistake on the owner of browser B for allowing .toDOM() to be executed without checking its contents, but it seems a little silly to expect clients to thoroughly scan a JSON string for <script> tags, and then vet those tags for potential maliciousness, before using this tool.

@azaslavsky
Copy link
Owner

If you feel there is a compelling use case for allowing the (de)serialization of those tags, I'd be willing to accept a pull request that allows them to be (de)serialized only if the user passes a (default false) option to .toJSON()/.toDOM() like dangerouslyAllowParsingOfScriptTags: true, similar to how React handles innerHTML injection.

@qqilihq
Copy link
Contributor Author

qqilihq commented Jul 27, 2018

Hi there,

many thanks for your detailed explanation, that makes total sense. I had admittedly only looked at the serialization aspect, and not the fact, the result will (usually) be deserialized within a browser again.

I'm working on a project, where this is not the case (instead, the serialized DOM is processed in a dedicated application, instead of a browser). In that environment, I need a “snapshot” as close as possible to the actual DOM content, so having the <link> and <script> tags is mission critical to me.

Thank you very much for being open for a PR -- I'm currently still in the evaluation phase, whether domJSON is the right tool (but I'm very confident in that regard :-) ). So, if we eventually decide to use it in our environment, I'll be glad to submit a PR.

Again, thank you for your efforts!

@qqilihq
Copy link
Contributor Author

qqilihq commented Sep 28, 2018

@azaslavsky It's been a while, but I made great experiences with domJSON, so I've actually added a dangerouslyAllowParsingOfScriptTags now to the serialization and deserialization in my fork. Beside that, I fixed an issue which occurred in Safari, and a typo.

  1. Would you be interested in merging these changes?
  2. If yes, would you prefer separate PRs, or all together?

@azaslavsky
Copy link
Owner

Separate PRs would be great. Would you mind also updating the docs (both gh-pages and README) if its not too hard? Thank you so much for the contribution! 💯

@qqilihq
Copy link
Contributor Author

qqilihq commented Oct 3, 2018

@azaslavsky Done -- see #31, #32, #33 😸

@azaslavsky
Copy link
Owner

You are an absolute champion. These are all excellent additions to the library, and I very much appreciate you taking the time to add such clean implementations. :)

@azaslavsky
Copy link
Owner

You're contributions also finally resolved #18 and #21. Thank you!

@qqilihq
Copy link
Contributor Author

qqilihq commented Oct 5, 2018

@azaslavsky Thanks to you for a great foundation 👍 Could you publish the updated version to NPM and add the dist JS here on GitHub as well? That would be awesome!

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

No branches or pull requests

2 participants