Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Should Mithril vnodes be fully JSON-compatible? #2356

Closed
dead-claudia opened this issue Jan 7, 2019 · 1 comment
Closed

Should Mithril vnodes be fully JSON-compatible? #2356

dead-claudia opened this issue Jan 7, 2019 · 1 comment
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix

Comments

@dead-claudia
Copy link
Member

Description + Why

Currently, Mithril's vnodes are fully JSON-compatible, and this was previously exploited very deliberately in v0.2.x by mithril-objectify. However, as explained by Dan Abramov, it can run you into trouble if you're not careful with a JSON API. Of course, we typically err towards the side of trusting developers to do the right thing, but a hacked third-party server could just as easily return a {tag: "div", attrs: {innerHTML: "<img src='bad.png' onerror='alert(\"You just got pwned!\")'>"}, children: []}, leaving a server-driven XSS vector and a very annoying time debugging if the JSON happens to have a top-level tag.

Possible Implementation & Open Questions

I'd add a "vnode tag": Symbol.for("m.vnode") to each vnode and have Mithril detect that, falling back to the string "m.vnode tag". For convenience and internal use, I'd add the following method to detect this:

// This would be what I actually use for `vnode["vnode tag"]`
var vnodeTag = typeof Symbol === "function" && Symbol.for
	? Symbol.for("m.vnode")
	: "m.vnode tag"

m.m.vnode.isVnode = function (value) {
	return value && value["vnode.tag"] === vnodeTag
}

The open question is should I even do this? If there's no real significant risk and us documenting our detection (which we already do) is sufficient on its own, this bug could just be closed and if others run into this issue, we just show them the docs.

Is this something you're interested in working on?

Yes, if this ends up materializing into something we decide to do.

@dead-claudia dead-claudia added Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix labels Jan 7, 2019
@dead-claudia
Copy link
Member Author

This would very explicitly not be in v2, because of how it changes vnode detection. It won't break 99% of pure front-end users, but it will break major libraries like mithril-node-render who rely on tag being the sole source of truth.

@MithrilJS MithrilJS locked and limited conversation to collaborators Jan 29, 2022
@orbitbot orbitbot converted this issue into discussion #2733 Jan 29, 2022
@dead-claudia dead-claudia moved this to Under consideration in Feature requests/Suggestions Sep 2, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix
Projects
Status: Under consideration
Development

No branches or pull requests

1 participant