janl commented on PR #9215: URL: https://github.com/apache/pouchdb/pull/9215#issuecomment-4186809066
Excuse the detour, but I want to put all the pieces on the board to make sure we solve this correctly: 1. I think this is an issue worth fixing. 2. This only really applies to non-browser environments, that currently* means the LevelDB `pouchdb-adapter-leveldb` adapter is affected (the `-memory` and `-localstorage` variants use in-browser and in-memory backends that are not affected. So a fix should probably live in `pouchdb-adapter-leveldb`. 3. Next let’s consider what we are trying to say here: there are certain database names that should not be allowed. As usual for PouchDB, we can look at CouchDB for inspiration and [CouchDB has a definition of a valid database name](https://docs.couchdb.org/en/stable/api/database/common.html#put--db), it even gives a regex: `^[a-z][a-z0-9_$()+/-]*$`. This regex is designed to only allow characters that are allowed as filenames on all file systems. This definition has not changed since it was first introduced in CouchDB at around (IIRC) 2007 or 2008, so I am happy to say it survived real world contact. 4. But PouchDB-in-Node.js scenarios use the name in one additional way that differs from CouchDB: specification of database directory. By default your LevelDB databases go into `./` and you can override this with a `./data/` prefix for any name to put all databases into that subdir. This is a feature we should keep supporting, but without overloading the database name, and thus allowing file-system-traversal-relevant characters. 5. CouchDB also allows the configuration of a database directory, but that happens in CouchDB’s configuration, not by inferring a path from the database name (there is an exception for `/` in db names that do create subdirs, but those never allow upwards traversal), but by having a `database_dir` configuration value that is set when CouchDB starts. 6. Given all this, I suggest we do the following: 1. inside `pouchdb-adapter-leveldb` during database initialisation, we only allow the regex as defined by CouchDB for valid database names. 2. we also introduce a new `option` to be passed to `new PouchDB(name, options)` called `databaseDir`. The `name` and `databaseDir` are then `path.join()`ed before handing them to LevelDB or using them for separate cleanup operations. 3. this is a backwards-incompatible or breaking change. We try to avoid these, but I think it could be warranted here. I could be convinced to allow a fallback with yet another option, so folks can just enable the old behaviours without a semantic port to the new system that might cause issues. I’m a bit on the fence about about it. Maybe the answer is to release the fix as opt-in in a bugfix release and then flip it to opt-out in the next major release. But please discuss. *we’ll keep this in mind for the ongoing SQLite adapter work, cc @Realtin @AlbaHerrerias. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
