Skip to content
This repository was archived by the owner on Dec 6, 2018. It is now read-only.

Handle special properties like __proto__ #59

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Handle special properties like __proto__ #59

wants to merge 1 commit into from

Conversation

rvagg
Copy link
Contributor

@rvagg rvagg commented Aug 2, 2014

Because you're using sublevels as a plain map of course, I happened across this because my sublevels were generated by external data and one was called 'constructor', bzzt!

This is a breaking change if you choose to accept it because sublevels is intended (I guess) to be exposed publicly.

@dominictarr
Copy link
Owner

Hmm, okay this is an important issue.
This is certainly a simple way to deal with this,
it's a breaking change, but because it only fixes a small edge case,
but effects many dependant modules (multilevel for one, and most probably others)
I think I'd rather solve this another way...

What if instead sublevel just throws on reserved words (like functions from Object.prototype)
and then if the user wants user generated names they could prepend an escape char (such as $)
on all keys, or maybe just problematic names.

@rvagg
Copy link
Contributor Author

rvagg commented Aug 3, 2014

This is how we deal with this exact problem in MemDOWN fwiw. I don't really mind how you deal with it here as long as it's dealt with.

But, my opinion on throwing on use of a reserved word is that it would be a terribly leaky abstraction and generally a surprise to encounter it when you did. Would you document it in the README? Would you feel comfortable writing that documentation, admitting that it's a leaky abstraction?

What's the damage of the break here for multilevel? It's only the properties on that sublevels object that are changed, the keys are not altered so it's only metadata change and presumably if you are using the same version of level-sublevel on client and server then you're not going to have any troubles if it were changed?

@dominictarr
Copy link
Owner

Hmm, so wherether or not this is a leaky abstraction depends on what type of string is a valid sublevel name... I'll admit this was never clearly defined, but there are some implicit definitions...

Consider event emitters, if you did this: new EventEmitter().emit('__proto__', 'did it work?')
you'll get a type error, because this._events.__proto__ is not a function or an array.
So, you could interpret this as an implicit type declaration. That using javascript with common sense
means don't use __proto__ or things like that as keys. Sometimes this is a security hole.
I guess this is javascript's version of sql injection. Maybe try signing up for node.js apps as __proto__ and see what happens?

This would have always caused an error with sublevel, so I guess this is the first time someone has tried to do this, but what the valid keys are should be documented...
If you had used toString or __proto__ as a sublevel name you'd have hit this code:

level-sublevel/sub.js

Lines 61 to 63 in 4ec1f68

if(this.sublevels[prefix])
return this.sublevels[prefix]
return new SubDB(this, prefix, options || this._options)

And it would have returned the wrong object and given you a runtime type error.

I guess this question boils down to, in javascript, is "common sense" to not use these values, or is it to support them?

In memdown, that is a slightly different case, because a database should certainly be able to contain any value but a sublevel or an event is like a directory, it's better to choose a sensible value, and not use spaces, etc, etc.

@snowyu
Copy link

snowyu commented Oct 27, 2014

use property setter/getter can keep compatibility. see my branch: https://github.com/snowyu/level-sublevel/tree/hotfix/sublevels-compatibility. also I have opened a pull-request.

  • [bug] fixed the sublevels compatibility problem with special-names 446527d
  • [bug] fixed mergeOpts can not copy null/emptystring options. 4308fbc

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants