Allow splices in attribute names

[BadMannersXYZ]

Closes #444

[nubunto]

yup, I could use this too!

[untitaker]

I think customizable attributes should strip invalid characters from the attribute itself.

For example, if I want to set some dynamic data- attribute based on user input, and do <button (format!("data-{username}")=(somevalue) />, I would expect that maud ensures that the user can't break out by setting username = "x onclick=alert(42); " or something similar. With this PR that produces <button data-x onclick=alert(42); =somevalue />, which is bad.

You might say that's an unlikely usecase, but it's all strings to maud, and from a security perspective once you allow strings you will get arbitrary uses of strings IMO. Maybe you want to allow enums or a certain kind of trait only? idk.

[BadMannersXYZ]

Good call. I've implemented a very hacky solution to prevent XSS attacks, based on the HTML standard for attribute names. Ideally somebody else who's more familiar with Rust can propose a better fix, since this one is far from ideal.

[untitaker]

Render trait is used for other stringification purposes, and it calls into the escaper here: https://github.com/BadMannersXYZ/maud/blob/2c60c641814dc36be10b4e813528a07904aae200/maud/src/lib.rs#L113

So Display is not called for regular strings, which would make it faster. (formatting is slow, and in the case of rendering &str, an allocation is avoided) Of course, this may not matter for a fringe usecase such as this.

Maybe Render::render_to needs another argument that specifies the context in which something is rendered (attribute name vs other)

But this would be a breaking change on that trait. I don't know if it's worth it. I don't think there are many Render impls outside of maud, but not sure.

If this is added to the trait and compat breakage needs to be avoided, I think this can somehow be made backwards-compatible by having render_v2 and render_to_v2 whose default impls call back into the old render functions, but this seems error-prone.

Note: I'm not a maintainer, this is just a drive-by review as i saw your PR in the HTMX discord. Don't consider what I say as prescriptive.

[BadMannersXYZ]

Given the nature of the attribute names as strings, it might make more sense to change the bound to AsRef<str> instead. This would avoid the extra allocation. If the user wants to use a Display value, it's possible to pass x.to_string() on the splice, making the allocation explicit.

[BadMannersXYZ]

One issue I found is dealing with empty attribute names. Unfortunately, I don't think that there's a way to verify these at compile time. There are two possibilities for a runtime check at Maud's level:

• Aliasing to a default attribute name, for example 0, to avoid any issues; or
• Ignoring it and letting the browser handle the empty attribute name. This is the current implementation. In such cases, I tested Firefox and Chrome, and both seem to consider the ="value" part to be the name of an empty attribute.

[untitaker]

you could require that all use of dynamic attribute names is prefixed by a static string/ident. this is a strange limitation but it should work ok for HTMX usecases.

…

On Thu, Oct 31, 2024, at 01:52, Bad Manners wrote: One issue I found is dealing with empty attribute names. Unfortunately, I don't think that there's a way to verify these at compile time. There are two possibilities for a runtime check at Maud's level: • Aliasing to a default attribute name, for example `0`, to avoid any issues; or • Ignoring it and letting the browser handle the empty attribute name. This is the current implementation. In such cases, I tested Firefox and Chrome, and both seem to consider the `="value"` part to be the attribute name, which has an empty value. — Reply to this email directly, view it on GitHub <#445 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGMPROBTR5AOOSZNLEYHC3Z6F5LLAVCNFSM6AAAAABPAU5TT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBYG43TMMBWGI>. You are receiving this because you commented.Message ID: ***@***.***>

[BadMannersXYZ]

I see a few issues with that requirement:

1. That still wouldn't solve the underlying issue in the case of an empty prefix, or a prefix that consists of invalid characters.
2. In the case of eg. HTMX, if the spliced string already contains the prefix, it'd be awkward to work around the imposed limitation (i.e. if you already have the slice hx-get, you'd have to strip the hx- part from your str, just to readd it immediately afterwards, which is more error prone).
3. That would require a completely new interface separate from regular slices, adding to the complexity of the feature. This is not a huge deal technically, since the current PR already has some separation in the code to handle attribute names differently, but requiring two arguments instead of one might not be intuitive to users of the library.
4. It wouldn't be general enough, being limited to things that have clear-cut prefixes. For example, if somebody uses custom HTML elements with custom attributes, then there might not be a prefix.