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

Store private fields on objects (take 2) #495

Merged
merged 5 commits into from Sep 8, 2016

Conversation

Projects
None yet
4 participants
@immerrr
Contributor

immerrr commented Aug 12, 2016

This is another take at what #487 was trying to accomplish.

The idea is to have the object represented by an empty (public) table forwarding all field accesses via a metatable to the real (private) table . The metatable would reject all unwanted accesses and the object methods would work on the private instance directly and thus have unhindered access to private fields.

"Privateness" is determined by a certain prefix, as before, but I had to change it to keep "private_mode" property name, so now it's "_" that marks a property or method as private.

I have also adjusted "is_wrapped" detection a bit. previously it would go and check the object's metatable for __wrapped field, but now the metatable is hidden, so I'm checking if the metatable is replaced by a certain placeholder.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 12, 2016

Current coverage is 86.11% (diff: 100%)

Merging #495 into master will decrease coverage by 0.48%

@@             master       #495   diff @@
==========================================
  Files            40         40          
  Lines          4978       4976     -2   
  Methods           0          0          
  Messages          0          0          
  Branches        690        688     -2   
==========================================
- Hits           4311       4285    -26   
- Misses          507        530    +23   
- Partials        160        161     +1   

Powered by Codecov. Last update 3caa40f...70eecee

codecov-io commented Aug 12, 2016

Current coverage is 86.11% (diff: 100%)

Merging #495 into master will decrease coverage by 0.48%

@@             master       #495   diff @@
==========================================
  Files            40         40          
  Lines          4978       4976     -2   
  Methods           0          0          
  Messages          0          0          
  Branches        690        688     -2   
==========================================
- Hits           4311       4285    -26   
- Misses          507        530    +23   
- Partials        160        161     +1   

Powered by Codecov. Last update 3caa40f...70eecee

@mike1808

This comment has been minimized.

Show comment
Hide comment
@mike1808

mike1808 Aug 12, 2016

Contributor

Mind blown solution. I'm amazed how this works.
👍

Contributor

mike1808 commented Aug 12, 2016

Mind blown solution. I'm amazed how this works.
👍

@mike1808

This comment has been minimized.

Show comment
Hide comment
@mike1808

mike1808 Aug 12, 2016

Contributor

Shouldn't this property be changed?

Contributor

mike1808 commented Aug 12, 2016

Shouldn't this property be changed?

@immerrr

This comment has been minimized.

Show comment
Hide comment
@immerrr

immerrr Aug 12, 2016

Contributor

This property was the reason I went for changing the "private" prefix from "private_" to "_", what do you have in mind?

Contributor

immerrr commented Aug 12, 2016

This property was the reason I went for changing the "private" prefix from "private_" to "_", what do you have in mind?

@mike1808

This comment has been minimized.

Show comment
Hide comment
@mike1808

mike1808 Aug 12, 2016

Contributor

I thought about decorator for marking methods as private, but I remembered that in Lua you cannot add a new property to a function value.

Contributor

mike1808 commented Aug 12, 2016

I thought about decorator for marking methods as private, but I remembered that in Lua you cannot add a new property to a function value.

@kmike kmike assigned immerrr and unassigned kmike Aug 17, 2016

@immerrr

This comment has been minimized.

Show comment
Hide comment
@immerrr

immerrr Aug 22, 2016

Contributor

Just tried to move method wrapping into object wrapping stage, but emulation tests started to fail, because they relied on modifying Splash class after the splash object was created. If a sandboxed piece of code could do require("splash") and mutate the class in the same manner, I think it caught a proper security problem.

If the class is not normally available, I guess we can move back to "on-access" wrapping and just disallow setting properties on private_self except for when it is done via property setters.

@kmike, what do you think?

Contributor

immerrr commented Aug 22, 2016

Just tried to move method wrapping into object wrapping stage, but emulation tests started to fail, because they relied on modifying Splash class after the splash object was created. If a sandboxed piece of code could do require("splash") and mutate the class in the same manner, I think it caught a proper security problem.

If the class is not normally available, I guess we can move back to "on-access" wrapping and just disallow setting properties on private_self except for when it is done via property setters.

@kmike, what do you think?

@kmike

This comment has been minimized.

Show comment
Hide comment
@kmike

kmike Aug 22, 2016

Member

@immerrr sandboxed code can't run require("splash"), but scripts stored on server can. Scripts stored on server are not sandboxed, so doing unsafe things in them is fine. There shouldn't be a way to get Splash class from a script submitted via HTTP API.

Member

kmike commented Aug 22, 2016

@immerrr sandboxed code can't run require("splash"), but scripts stored on server can. Scripts stored on server are not sandboxed, so doing unsafe things in them is fine. There shouldn't be a way to get Splash class from a script submitted via HTTP API.

@immerrr

This comment has been minimized.

Show comment
Hide comment
@immerrr

immerrr Aug 23, 2016

Contributor

Another quick stab at fixing this: I have made wait_restart_on_redirects public now that it's test-only.

Contributor

immerrr commented Aug 23, 2016

Another quick stab at fixing this: I have made wait_restart_on_redirects public now that it's test-only.

@kmike kmike merged commit 0979b04 into master Sep 8, 2016

2 checks passed

codecov/patch 100% of diff hit (target 90.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@immerrr immerrr deleted the make-private-fields-2 branch Sep 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment