{"action":"created","issue":{"url":"https://api.github.com/repos/tc39/proposal-decorators/issues/136","repository_url":"https://api.github.com/repos/tc39/proposal-decorators","labels_url":"https://api.github.com/repos/tc39/proposal-decorators/issues/136/labels{/name}","comments_url":"https://api.github.com/repos/tc39/proposal-decorators/issues/136/comments","events_url":"https://api.github.com/repos/tc39/proposal-decorators/issues/136/events","html_url":"https://github.com/tc39/proposal-decorators/pull/136","id":347538494,"node_id":"MDExOlB1bGxSZXF1ZXN0MjA2MTQwNzQx","number":136,"title":"Normative: Restrict the use of PrivateNames in class decorators","user":{"login":"littledan","id":189835,"node_id":"MDQ6VXNlcjE4OTgzNQ==","avatar_url":"https://avatars3.githubusercontent.com/u/189835?v=4","gravatar_id":"","url":"https://api.github.com/users/littledan","html_url":"https://github.com/littledan","followers_url":"https://api.github.com/users/littledan/followers","following_url":"https://api.github.com/users/littledan/following{/other_user}","gists_url":"https://api.github.com/users/littledan/gists{/gist_id}","starred_url":"https://api.github.com/users/littledan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/littledan/subscriptions","organizations_url":"https://api.github.com/users/littledan/orgs","repos_url":"https://api.github.com/users/littledan/repos","events_url":"https://api.github.com/users/littledan/events{/privacy}","received_events_url":"https://api.github.com/users/littledan/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-08-03T21:15:46Z","updated_at":"2018-08-04T17:46:27Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/tc39/proposal-decorators/pulls/136","html_url":"https://github.com/tc39/proposal-decorators/pull/136","diff_url":"https://github.com/tc39/proposal-decorators/pull/136.diff","patch_url":"https://github.com/tc39/proposal-decorators/pull/136.patch"},"body":"There's been some concern about the trust involved in decorating\r\nprivate class elements. Decorating a private field or method is\r\nspecified to give access to the private name which is declared.\r\nAlthough this notion of privacy may be defensible in theory, we\r\ntake a couple additional precautions to ensure that it's private\r\nin practice:\r\n- #134: PrivateName is a defensive class, which is deeply frozen and\r\n  does not permit monkey patching.\r\n- #133: Class decorators get \"restricted\" PrivateName instances,\r\n  which cannot be used to read or write private class elements.\r\n\r\nPreviously, the idea was that class decorators would be trusted,\r\nenabling class decorators to be used to deliberately grant access to\r\nprivate fields and methods to things outside of the class. For\r\nexample, a @testable decorator could expose all private class elements\r\nto a testing framework.\r\n\r\nThe new idea is, class decorators are less trusted, and can only see\r\nthat private class elements are present, insert or delete them, but\r\nnot read or write them. This increases the level of privacy in\r\npractice (e.g., if a decorator is used for wrapping purposes and not\r\nexpected to be able to trusted reading private fields), but removes\r\ncertain use cases.\r\n\r\nThis patch enables class decorators to add and remove private elements,\r\nwhich may be considered to have integrity implications with respect\r\nto usages of private class elements as a \"brand\". The trust level of\r\nclass decorators is intermediate: they are trusted with these branding\r\nrights, but not with the ability to actually read and write everything.\r\n\r\nIn a follow-on proposal, we could permit class decorators to get\r\nun-restricted private names through a `@trusted: decorator` syntax;\r\nwe could also decide to \"relax\" the restriction on all class\r\ndecorators.\r\n\r\nThe logic here could be used to implement \"restricted\" private fields\r\nas proposed in https://github.com/tc39/proposal-decorators/issues/24#issuecomment-405377193\r\nwith the following implementation:\r\n\r\n```js\r\nfunction restricted(privateName) {\r\n  let restrictedName;\r\n  function classDecorator({[{key}]}) {\r\n    restrictedName = key;\r\n  }\r\n  function elementDecorator(descriptor) {\r\n    return {...descriptor, key: privateName};\r\n  }\r\n  @classDecorator class X {\r\n    @elementDecorator x;\r\n  }\r\n  return restrictedName;\r\n}\r\n```"},"comment":{"url":"https://api.github.com/repos/tc39/proposal-decorators/issues/comments/410466149","html_url":"https://github.com/tc39/proposal-decorators/pull/136#issuecomment-410466149","issue_url":"https://api.github.com/repos/tc39/proposal-decorators/issues/136","id":410466149,"node_id":"MDEyOklzc3VlQ29tbWVudDQxMDQ2NjE0OQ==","user":{"login":"allenwb","id":505435,"node_id":"MDQ6VXNlcjUwNTQzNQ==","avatar_url":"https://avatars2.githubusercontent.com/u/505435?v=4","gravatar_id":"","url":"https://api.github.com/users/allenwb","html_url":"https://github.com/allenwb","followers_url":"https://api.github.com/users/allenwb/followers","following_url":"https://api.github.com/users/allenwb/following{/other_user}","gists_url":"https://api.github.com/users/allenwb/gists{/gist_id}","starred_url":"https://api.github.com/users/allenwb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/allenwb/subscriptions","organizations_url":"https://api.github.com/users/allenwb/orgs","repos_url":"https://api.github.com/users/allenwb/repos","events_url":"https://api.github.com/users/allenwb/events{/privacy}","received_events_url":"https://api.github.com/users/allenwb/received_events","type":"User","site_admin":false},"created_at":"2018-08-04T17:46:27Z","updated_at":"2018-08-04T17:46:27Z","author_association":"MEMBER","body":"> It would be great to hear from you if you feel that #136 addresses the problem, or if more changes are needed.\r\n\r\nI'm afraid it really doesn't.  The main problem I see is that a spoofed or Trojan horse decorator can be used as an attack vector to penetrate hard private encapsulation in some situations.  If I understand this change, what it has done is narrowed but not eliminated that attack vector.  Basically, it means that an attacker would now focus on identifying  and attacking situations where high security private state is associated with an private field that has an element decorator.  Oh, also branding use cases which I suspect will be a very common use of private fields.\r\n\r\nA smaller security hole is still a security hole.\r\n\r\nAs a cautionary tale, consider that for much of the ES2015 development cycle we thought that Symbols would be a mechanism would support strong encapsulation use cases.  We even had syntactic support (@names) in the spec. to enhance the usability of Symbol keyed properties.  But then we realized Proxy provided a way to penetrate symbol based encapsulation and we couldn't come up with a way to close that hole while preserving the full functionality of Proxy that was needed for some of its use cases. So we backed away from the idea of Symbols proving strong encapsulation and deferred solving that problem to  post ES2015. We didn't really change anything WRT Symbols (other than removing the @name connivance syntax) but stopped talking about them as solution for strong encapsulation.\r\n\r\nLexically scoped private names were then developed as a strong encapsulation (ie, \"hard private\") solution.  And they work for that purpose until decorators (or some subset of decorators) are introduced that can expose (some) private fields to access that is not lexically constrained.  In the presence of the possibility of such decorators, private fields do not provide \"hard private\" strong encapsulation.  They are just \"less soft\" than Symbol based fields. If we continue on the current path, even with this revision, what is the solution for the use cases that actually needs truly \"hard private\"? In the next round do we add truly private fields (##name??) that are not exposed to decorators?\r\n\r\nIt seems to me we have only two alternatives ahead: 1) restrict the functionality of decorators to preserve the strong lexical encapsulation of private fields; or 2) stop taking about private fields as being \"hard private\" and start working on a new solution for that use case.\r\n\r\nBut here's the thing, we could be there today if we follow the first alternative. Soft private via Symbol keyed fields is good enough for encapsulation use cases that don't require impenetrable strong encapsulation.  If a developer understands that decorating a private field potentially destroys its strong encapsulation then they should never decorate one.  But if decorating them is allowed then sometimes private fields will be intentionally decorated when the author knows that only soft private is required. In that case, if you are reading code in a class definition and see an undecorated private field, how are you going to determine whether that field requires \"hard private\" and hence should never be decorated or really only needs to be \"soft private\" hence is ok to decorate. \r\n\r\nI suggest that you stop trying to fix what is probably an unfixable conflict between strong encapsulation and the use of decorations, even as refined in this patch.  Instead remove reification of private names from the decorator proposal and make it clear that decorators don't work with private fields.  Start marketing symbol based fields as the encapsulation solution in all cases expect where impenetrable strong encapsulation is a requirement.\r\n\r\nRegardless, do that security review focusing on ways that a decorator might be spoofed. Don't forget to consider the things that bundlers do with ES modules and how that may expanded the attack surface for spoofing decorators.\r\n\r\n"}}
{"actor":{"display_login":"allenwb"}}