express-pouchdb: removes "/" from db names

[gr2m]

trafficstars

Currently express-pouchdb is removing / from filenames before initializing db instances for the current request in https://github.com/pouchdb/pouchdb-server/blob/55aa1932903083fcaf1b4c53898f98065c8a0600/packages/node_modules/express-pouchdb/lib/clean-filename.js#L9 (via https://github.com/pouchdb/pouchdb-server/blob/55aa1932903083fcaf1b4c53898f98065c8a0600/packages/node_modules/express-pouchdb/lib/utils.js#L67)

At Hoodie, we currently use express-pouchdb inside our server and we pass our PouchDB constructor. Our user databases have the format user/:uuid. The problem now is that if I start listening to changes of db user/abc4567 in Hoodie, then no changes will ever happen because express-pouchdb internally renames the database to userabc4567

[gr2m]

CouchDB allows

only lowercase characters (a-z), digits (0-9), or any of the characters _, $, (, ), +, -, and / are allowed.

See also http://docs.couchdb.org/en/2.0.0/api/database/common.html#put--db

If you’re familiar with Regular Expressions, the rules above could be written as ^[a-z][a-z0-9_$()+/-]*$.

[gr2m]

Instead of sanitizing a database name, we should throw an error for invalid database names.

[gr2m]

We use the current cleanFilename in 3 places

1. set req.db: https://github.com/pouchdb/pouchdb-server/blob/55aa1932903083fcaf1b4c53898f98065c8a0600/packages/node_modules/express-pouchdb/lib/utils.js#L67
2. PUT :db and DELETE :db
3. rewrite: https://github.com/pouchdb/pouchdb-server/blob/master/packages/node_modules/express-pouchdb/lib/routes/rewrite.js#L35

I’m not sure how CouchDB behaves for the url rewiretes (3.), but for 1. and 2. I’m pretty sure CouchDB throws errors

[garrensmith]

Good point @gr2m I agree. We should throw errors. I'm also not 100% about 3 but I would default to throwing an error as well if we are unsure.

[gr2m]

@garrensmith @nolanlawson @marten-de-vries I can just go ahead and fix it. Can you give me pointers regarding adding tests for it?

[nolanlawson]

Sure thing, you can add tests in tests/express-pouchdb e.g. check out this one: https://github.com/pouchdb/pouchdb-server/blob/55aa1932903083fcaf1b4c53898f98065c8a0600/tests/express-pouchdb/test.js#L231-L249

In general I am cool with just doing whatever CouchDB does. If it's a breaking change, then that's fine, we'll release 3.0.0.

[gr2m]

okay, I tested against my local CouchDB 1.6 to see what errors we should return. Sending any request (with any http verb) which includes an invalid databas name returns 400 Bad Request with the body {"error":"illegal_database_name","reason":"Name: '_invalid'. Only lowercase characters (a-z), digits (0-9), and any of the characters _, $, (, ), +, -, and / are allowed. Must begin with a letter."} (where '_invalid' is the invalid database in the requested path)

Current pouchdb-server does not complain:

[info] DELETE /_invalid 200 - 127.0.0.1
[info] GET /_invalid 404 - 127.0.0.1

I’m looking into this now

[gr2m]

there you go: https://github.com/pouchdb/pouchdb-server/issues/214