Skip to content
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

express-pouchdb: removes "/" from db names #213

Open
gr2m opened this issue Feb 21, 2017 · 8 comments · May be fixed by #214
Open

express-pouchdb: removes "/" from db names #213

gr2m opened this issue Feb 21, 2017 · 8 comments · May be fixed by #214

Comments

@gr2m
Copy link
Contributor

gr2m commented Feb 21, 2017

Currently express-pouchdb is removing / from filenames before initializing db instances for the current request in

return sanitize(name) || sanitize('__' + name + '__');
(via )

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
Copy link
Contributor Author

gr2m commented Feb 21, 2017

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
Copy link
Contributor Author

gr2m commented Feb 21, 2017

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

@gr2m
Copy link
Contributor Author

gr2m commented Feb 21, 2017

We use the current cleanFilename in 3 places

  1. set req.db:
  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
Copy link
Member

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
Copy link
Contributor Author

gr2m commented Feb 21, 2017

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

@nolanlawson
Copy link
Member

Sure thing, you can add tests in tests/express-pouchdb e.g. check out this one:

describe('redirects', function () {
it('GET /_utils should redirect to /_utils/', function (done) {
request(coreApp)
.get('/_utils')
.expect(301)
.end(done);
});
it('GET /_utils/ should return fauxton', function (done) {
request(coreApp)
.get('/_utils/')
.expect(200)
.expect(function (res) {
if (!/<title>PouchDB Server<\/title>/.test(res.text)) {
return "No '<title>PouchDB Server</title>' in response";
}
})
.end(done);
});
});

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 gr2m self-assigned this Feb 22, 2017
@gr2m
Copy link
Contributor Author

gr2m commented Feb 22, 2017

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 gr2m linked a pull request Feb 22, 2017 that will close this issue
@gr2m
Copy link
Contributor Author

gr2m commented Feb 22, 2017

there you go: #214

@gr2m gr2m removed their assignment Sep 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants