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

Safer fields (escaping fields in xhtm) #21

Closed
aral opened this issue Mar 7, 2023 · 4 comments
Closed

Safer fields (escaping fields in xhtm) #21

aral opened this issue Mar 7, 2023 · 4 comments

Comments

@aral
Copy link
Contributor

aral commented Mar 7, 2023

Based on this issue on vhtml, I don’t think the hyperscript → HTML renderer is the right place to escape (untrusted) data interpolated into templates. Since hyperscript is stateless, a renderer like vhtml has no way I can see of carrying out proper escaping of code – what is referred to as sanitisation in vhtml, although it’s not really sanitisation but escaping – without being open to the linked replay/injection attack. (Whether or not that attack has meaningful real-world exploits is another issue but I’d rather not find out.) What we can do, however, is easily carry out escaping of fields in xhtm because we know what is an interpolated string (“field”) and what is not.

In my fork of vhtml, I’ve removed the “sanitisation” code from tag contents. And I’m going to open a pull request next with that functionality added to xhtm instead.

Would love your thoughts, @dy.

Also, while vhtml is aware of attributes and can do attribute escaping properly (I’ve kept that for the time being), it feels wrong to separate that functionality between two modules. If we’re going to escape fields here, we should probably be escaping attributes here too. If this is a direction you think is worth pursuing? Should I add that to that PR? Or open a different one?

(I also completely understand if this is not something you feel belongs in xhtm. I’m not sure of your other use cases. While it makes sense for me, I realise it might not for you when using a stateful DOM-based renderer in the browser, for example. I’m happy to keep a soft fork that does sanitisation and keep it synced with xhtm.) :)

@dy
Copy link
Owner

dy commented Mar 7, 2023

Sorry for delay. A bit loaded these days. Will look at it today.

@aral
Copy link
Contributor Author

aral commented Mar 7, 2023

@dy No rush whatsoever. You’re not blocking me in any way. Whenever you can is fine :)

@dy dy closed this as completed in 2430ce6 Mar 9, 2023
@dy
Copy link
Owner

dy commented Mar 9, 2023

I gave it a thought - I wonder why escaping wouldn't work for you if implemented on attributes level? I don't think merging sanititzer is right move here, a bit mixing concerns and also some unnecessary slowdown?

@dy
Copy link
Owner

dy commented Mar 9, 2023

The other concern here is that sanitizing doesn't necessarily include all html rules or html at all. Ie. xhtml can be used with some XML without these escaping rules.

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