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

Consider allowing creating a policy via a constructor. #258

Closed
koto opened this issue Feb 14, 2020 · 11 comments
Closed

Consider allowing creating a policy via a constructor. #258

koto opened this issue Feb 14, 2020 · 11 comments
Labels
future In consideration for the future releases of the API spec
Milestone

Comments

@koto
Copy link
Member

koto commented Feb 14, 2020

new TrustedTypePolicy(name, rules)

might be simpler than

trustedTypes.createPolicy(name, rules)

trustedTypes should still be left to hold other functions like isHTML and getAttributeType, but using a constructor pattern seems simpler to the developers (while TT applications almost always must create a policy, they don't need to use other trustedTypes functions).

It should still be possible to easily locate policy creation statically in code, and dynamically we'll have events at policy creation (#222 (comment)).

/cc @otherdaniel @xtofian

@otherdaniel
Copy link
Member

That also works.

Just so I read this correctly: The trustedTypes accessor would remain (for feature presence testing; the isXXX helper functions; "default" policy access, the metadata functions, etc.), but trustedTypes.createPolicy would move to a constructor?

I'd find the current spec more logical, since policy creation fails (or succeeds) due to the state of the "factory"; that is, due to whether another policy with that name has already been created. That, in my opinion, sounds more like a method on the instance that "knows" all the policies rather than a constructor. But that is admittedly just an opinion, and either approach would work.

@koto
Copy link
Member Author

koto commented Feb 14, 2020

FWIW all checks are done actually at the Realm level, so this might work.

@koto
Copy link
Member Author

koto commented Mar 4, 2020

We've looked into this, and the broader issue all of the API being exposed under window.trustedTypes.

It always takes two actions to obtain a trusted type - first creating a policy, and then creating a type instance through it. That's by design, as we believe policies are neccessary for controlling how types can be created (more background behint it here and here).

Creating a policy through the constructor is definitely more idiomatic than calling trustedTypes.createPolicy, but at the same time we still need the other funtions under trustedTypes to exist, and keeping all of them under the same "umbrella" object makes feature detection much easier for authors, as they may check for one symbol only.

IOW, extracting just the policy creation to use a constructor pattern would be natural only if all other functionality moved off trustedTypes (e.g. to Element.getAttributeType, TrustedHTML.isValid() and TrustedScript.EMPTY), but at that point the feature detection logic has to depend on multiple symbols. We think a single entry point for all-things-trusted-types has advantages here.

That said, this surfaced the problem of #257, and we decided to remove the unforgeability of the API, such that we could add new functionality or alias existing ones, in a way that makes it easy to polyfill it.

@koto koto closed this as completed Mar 4, 2020
@mikewest
Copy link
Member

mikewest commented Mar 7, 2020

@koto: It might be reasonable to add some bits of this conversation to the wiki entry for clarity.

/cc @othermaciej

@othermaciej
Copy link
Member

I think a constructor would be clearer. Elsewhere in Web APIs where a create...() function pattern has been used because there is a handy object around anyway, it's generally been less clear, and often has later been changed to allow direct construction. This quirk also makes the API look more complicated than it actually is.

Note also, it's ok for a constructor to have static functions on it (that aren't in objects it creates). It's not clear if TrustedTypePolicy.whatever() would make sense in place of trustedTypes.whatever() though.

@mikewest
Copy link
Member

mikewest commented Mar 9, 2020

As a thought experiment, this might replace TrustedTypePolicyFactory with something like the following additions to other interfaces:

  • TrustedTypePolicy would gain:
    • a constructor to replace TrustedTypePolicyFactory.createPolicy()
    • a static defaultPolicy member to replace TrustedTypePolicyFactory.defaultPolicy.
  • TrustedHTML, TrustedScript, and TrustedScriptURL would gain:
    • static isWhatever() methods to replace TrustedTypePolicyFactory.isWhatever()
    • static emptyWhatever members to replace TrustedTypePolicyFactory.emptyWhatever.

This leaves getAttributeType() and getPropertyType(). We'd need to either move these to something like static methods on TrustedTypePolicy (something more central like Element?), or invert the check into multiple instances of something like TrustedHTML.isHTMLAttribute().

These changes represent a somewhat different aesthetic than the existing API, and they're all reasonable to suggest. There's some cost to making them given code that's been written against the existing API, and internally there would be some friction with regard to the integration with the SafeHTML typing mechanism that Google's been using for some time now, but no one's shipped yet, and it's certainly doable. Still, I'm a bit reluctant to block on this if we're just going to make spelling changes; I know Google's security team is champing at the bit to begin rolling this mechanism out at some scale.

With all that said, @othermaciej: would a change along these lines make WebKit more willing to implement the API, all other things being equal? I'd be excited about putting Chrome's implementation on hold for another release cycle if it made cross-vendor support more likely!

@otherdaniel
Copy link
Member

That design would also work. (The current design still strikes me as both more logical and more implementation-friendly, but I'll gladly let people with more web and/or WebIDL experience have the last word here. It seems we're moving complexity around, rather than genuinely reducing it.)


If you do want to go with the proposed alternative design aesthetic, you could move the meta-data functions also to the specific types. That is, instead of a TrustedTypesPolicyFactory.getAttributeType(..) that returns a type name we could have Nx TrustedXXX.isAttributeTrusted(name) that returns a boolean. Likewise for getTypeMapping, which could be replaced with Nx TrustedXXX.getMapping() for the specific type.

(As said above, I'm not convinced that'd improve developer ergonomics much, but... it's a viable alternative.)

@mikewest
Copy link
Member

mikewest commented Mar 9, 2020

It seems we're moving complexity around, rather than genuinely reducing it.

For clarity, I do agree with this. I don't think the sketch above actually reduces complexity, but if folks would be happier with it, it seems like it would work. I think the bar should be somewhat higher than the floor for shifting things around at this point, but changes are certainly possible if it would lead to more adoption. :)

@othermaciej
Copy link
Member

I think the constructor change is an improvement. It makes this more like other Web APIs. So one less piece of strangeness, and it resolves the sense that creation is very indirect.

Static functions on the constructor vs on a dedicated singleton object is an aesthetic choice. Reasonable to do it either way. I don’t really have an opinion on that, I just brought it up due to the argument that because there are global functions in this API, it should not do natural construction.

Making the constructor change would probably make Apple incrementally more likely to implement. How much exactly is hard to say. I will ask for other opinions, as mine is not the only one that is relevant.

@koto koto added this to the v2 milestone Mar 11, 2020
@koto koto reopened this Mar 11, 2020
@koto koto added the future In consideration for the future releases of the API label Mar 11, 2020
@koto
Copy link
Member Author

koto commented Mar 11, 2020

After dropping the [Unforgeable] the polyfill for the constructor approach is simply:

TrustedTypePolicy = new Proxy(TrustedTypePolicy, { 
  construct(target, args) { 
    return trustedTypes.createPolicy(...args) 
  } 
});

@koto
Copy link
Member Author

koto commented Mar 19, 2020

We talked about this internally and decided to launch with the createPolicy function. The constructor approach is appealing (and definitely more idiomatic), but is a minor improvement, and we'd like to start seeing how developers are using Trusted Types without delaying the launch of the current shape of it. If there seems to be a repeated preference to use a constructor pattern, we'd be more than happy to introduce it, likely deprecating createPolicy, with polyfills for both approaches, but dropping createPolicy now already hurts early adopters, so we have to take that into account.

We're closing the bug only to mark that this is not under active work, and would reopen if the circumstances change. Developers, please let us know what y'all would prefer - this API is for you.

@koto koto closed this as completed Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future In consideration for the future releases of the API spec
Projects
None yet
Development

No branches or pull requests

4 participants