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

Are getAttributeType and getPropertyType methods neccessary? #384

Closed
lukewarlow opened this issue Jan 8, 2024 · 7 comments
Closed

Are getAttributeType and getPropertyType methods neccessary? #384

lukewarlow opened this issue Jan 8, 2024 · 7 comments
Labels
proposed-removal Issues concerning potential removal of functionality from the API
Milestone

Comments

@lukewarlow
Copy link
Member

lukewarlow commented Jan 8, 2024

This has been raised elsewhere but worth making a standalone issue for it.

In #381

Even better would be to remove both methods, since it is unclear if they are needed.

In Mozilla's standards position mozilla/standards-positions#20 (comment):

First and foremost, there is some functionality (e.g., getPropertyType, getAttributeType that seem a bit odd and their usage in the wild isn't clear to us.

I (implementing in WebKit) have also questioned if they're actually useful when reading the spec. After more time I think they could be valuable.

@tosmolka
Copy link

tosmolka commented Jan 8, 2024

FYI, usage of getAttributeType API in DOMPurify:

@lukewarlow
Copy link
Member Author

As a rough test of what it would be like I did a quick local patch of htmx to make it pass trusted types requirements and have come across a few places where getAttributeType is useful.

Example below:

           forEach(mergeFrom.attributes, function (attr) {
                if (shouldSettleAttribute(attr.name)) {
                    const type = trustedTypes.getAttributeType(mergeTo.tagName, attr.name);
                    if (type === "TrustedHTML") {
                        mergeTo.setAttribute(attr.name, htmxPolicy.createHTML(attr.value));
                    } else if (type === "TrustedScript") {
                        mergeTo.setAttribute(attr.name, htmxPolicy.createScript(attr.value));
                    } else if (type === "TrustedScriptURL") {
                        mergeTo.setAttribute(attr.name, htmxPolicy.createScriptURL(attr.value));
                    }
                }
            });

That being said I'm not sure if this is something that couldn't just be achieved with a small userland code snippet. The polyfill of these functions isn't very big and most people probably only need a very small subset?

@koto koto added the proposed-removal Issues concerning potential removal of functionality from the API label Jan 18, 2024
@mbrodesser-Igalia mbrodesser-Igalia added this to the v1 milestone Jan 23, 2024
@mozfreddyb
Copy link
Collaborator

I think the html and DOMPurify examples are good presentations of real world use cases: Whenever you have code that is assigning attributes on elements in a generic way (e.g., templating) you want to make sure that this assignment works properly ans safely. Asking the TT API for which enforcement is in place and being able to call an appropriate TT constructor seems only reasonable.

Looking at this again, the alternative is indeed that a script is writing their list of TT-controlled HTML themselves, as {element, attribute, type} triple, where element is an HTML element, attribute an attribute and type would be a Trusted Type (TrustedHTML etc.).

With software evolving, I'd assume that it would be nice for developers to rely on the platform to have its back and co-evolve rather than having to hand-roll this list and keep it up to date. The overhead for the web platform as a somewhat simple list lookup seems relatively low but the benefits of co-evolving this with HTML rather than have pages break if new XSS sinks arise seems preferable to me.

@lukewarlow
Copy link
Member Author

Given the above comment I'm going to go ahead and close this issue out as there's a valid use case for them.

@mozfreddyb
Copy link
Collaborator

Sorry, to clarify. My comment had lots of "I" and not a lot of "we" intentionally. I'd like to hear @smaug----'s and @petervanderbeken's take on this too :)

@lukewarlow
Copy link
Member Author

lukewarlow commented Feb 8, 2024

Just found Chromium also has a getTypeMapping function shipped to stable which isn't in the current spec at all. #432 we should similarly decide if we can remove it or if we can keep it and it needs speccing.

@mozfreddyb
Copy link
Collaborator

I've asked around internally. There's agreement to keep getAttributeType and getPropertyType. I think we can close it and discuss getTypeMapping in #432.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposed-removal Issues concerning potential removal of functionality from the API
Projects
None yet
Development

No branches or pull requests

5 participants