400 Bad Request for invalid database names #214

gr2m · 2017-02-22T08:05:14Z

I would argue this is not a breaking change as it only fixes a bug to make express-pouchdb behave like CouchD. The side effect of this is that if a datbases that included special chars before were used they will now appear empty. E.g. "user/abc456" would have been stored as "userabc456" and will now be stored as "user/abc456". Renaming the files manually before restarting should fix the problem

closes #213

gr2m · 2017-02-22T08:15:50Z

packages/node_modules/express-pouchdb/lib/index.js

+          }
+        };
+      }
+    }


I’m not sure if there is a more efficient way to do this. We could for example find all internal /_* paths once and store it in some kind of app state to make the lookup immediate without the need to traverse to all routes every time

I agree, It might be better to just create a object hash with the routes on startup and we just look at that here.

Happy to change that, but I would need guidance on how to do that

Yeah, this solution seems a bit heavy. A simpler solution may be to check for all usages of new PouchDB() and isolate your logic to that. AFAICT this is only done in one single place, which is packages//node_modules/express-pouchdb/lib/create-or-delete-dbs.js, so you should be able to isolate your fix there.

Ah actually I see now why you had to do this; you need to send the 400.

I agree that looping through all routes for every single request seems like a potentially slow design, though. I'd prefer a more elegant solution but you'd probably have to dig through the code to see how Marten set up the routes because I'm not an expert in that.

@marten-de-vries: hmm I was not aware of PouchDB.new(); my goal with create-or-delete-dbs.js was to avoid memory leaks caused by creating new PouchDBs for every request. It seems that logic should be unified.

@gr2m I hadn't thought about that use case. You essentially want new PouchDB('user/foo') to map to express-pouchdb's user%2Ffoo. I think that may be impossible given that the standard logic for new PouchDB('some/really/long/path/here') is that it literally maps to a deep path somewhere, whereas express-pouchdb explicitly wants to avoid that... So it seems you would need to do new PouchDB('user%2Ffoo') within your app.

@nolanlawson are you sure? When I removed the filename sanitation then it worked all perfectly fine, without needing to do anything like new PouchDB('user%2Ffoo')

There's a good reason that that particular filename sanitization is in there, though; you wouldn't want users to be able to create a DB called ../../etc/passwords for instance. I think this needs a lot more thought and testing, and I'm also concerned about backwards compat. If we're going to break users who used to rely on new PouchDB('foo/bar') not actually creating a foo/ directory then it should at least be a major version release.

In any case all of this code is very hairy and lacks tests, so maybe that's the first problem we should attempt to solve...

../../etc/passwords would fail the regex ^[a-z][a-z0-9_$()+/-]*$

it should at least be a major version release.

Yes, agree. But you mentioned that’s no problem

Ah okay, that makes sense. And yeah 3.0.0 is no problem, just want to make sure that we finally fix db name escaping this time. 😃

garrensmith

This is really helpful. Nice work @gr2m. I don't know this well enough to approve. It would be nice to get @nolanlawson feedback as well

garrensmith · 2017-02-22T08:32:55Z

packages/node_modules/express-pouchdb/lib/index.js

+        if (route) for (var j = 0, method; j < route.stack.length; j++) {
+          method = route.stack[j].method.toUpperCase()
+          if (method === req.method && route.path.substr(1, dbName.length) === dbName) {
+            console.log('ok?')


I think you can remove this

garrensmith · 2017-02-22T08:34:13Z

packages/node_modules/express-pouchdb/lib/index.js

+          }
+        };
+      }
+    }


I agree, It might be better to just create a object hash with the routes on startup and we just look at that here.

garrensmith · 2017-02-22T08:35:45Z

packages/node_modules/express-pouchdb/lib/index.js

+        route = app._router.stack[i].route;
+        if (route) for (var j = 0, method; j < route.stack.length; j++) {
+          method = route.stack[j].method.toUpperCase()
+          if (method === req.method && route.path.substr(1, dbName.length) === dbName) {


I don't really understand express internals, why do you need to check the method as well?

That’s actually a good question. I think we don’t have to, now that I think about it :) I thought I had a case in mind why we need this, but can’t recall

Yes, I think you do if you go this way. You're checking if the current method is the same as the method of the route you're trying to match. If you don't check, it would allow you to do things like POST /_all_dbs (which is nonsensical).

yeah that was my thinking, but if you disallow /_all_dbs for all methods, you don’t have that problem. And I think there should be no problem with that

True, but you'd have to do that explicitly, which is messy as you might forget it somewhere. Now PouchDB itself might do that itself for databases starting with _, in which case you are right. But that's too long ago for me to recall.

nolanlawson · 2017-02-22T16:29:09Z

packages/node_modules/express-pouchdb/lib/index.js

+          }
+        };
+      }
+    }


Yeah, this solution seems a bit heavy. A simpler solution may be to check for all usages of new PouchDB() and isolate your logic to that. AFAICT this is only done in one single place, which is packages//node_modules/express-pouchdb/lib/create-or-delete-dbs.js, so you should be able to isolate your fix there.

nolanlawson · 2017-02-22T16:30:57Z

package.json

@@ -58,7 +58,6 @@
    "pouchdb-vhost": "^1.0.2",
    "pouchdb-wrappers": "^1.3.6",
    "raw-body": "^2.2.0",
-    "sanitize-filename": "^1.6.1",


One problem with removing sanitize-filename entirely is that it was doing some extra work for us to ensure that e.g. files named PRN or CON don't break on Windows (see this page; you really cannot create files or directories with those names)

This might be considered a minor thing, but if you run pouchdb-server on Windows and users can crash your system by creating DBs named PRN then it's kind of a big deal 😅

hmmm that’s a tough one. Would you be okay to simply forbid this names altogether for now for the time being? I’ll create an issue and log an error linking to it. Because renaming the db name internally will again break the initial reason why I want to fix this.

It seems like this is something that should be fixed on PouchDB adapter level

Yeah that's a good point. new PouchDB('prn') currently breaks in LevelDB-based PouchDBs, which is not express-pouchdb's fault. We could just throw an error on Windows. 🤷‍♂️

nolanlawson · 2017-02-22T16:34:31Z

packages/node_modules/express-pouchdb/lib/index.js

+          }
+        };
+      }
+    }


Ah actually I see now why you had to do this; you need to send the 400.

I agree that looping through all routes for every single request seems like a potentially slow design, though. I'd prefer a more elegant solution but you'd probably have to dig through the code to see how Marten set up the routes because I'm not an expert in that.

gr2m · 2017-06-15T15:52:37Z

hey @nolanlawson @garrensmith.

I’ve made lookup more efficient, it is now happening once on start up, I think that addressed all concerns? The remaining one is to release a breaking version with this change. What is our process for this?

gr2m mentioned this pull request Feb 22, 2017

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

Open

gr2m commented Feb 22, 2017

View reviewed changes

garrensmith reviewed Feb 22, 2017

View reviewed changes

nolanlawson requested changes Feb 22, 2017

View reviewed changes

gr2m mentioned this pull request Feb 22, 2017

🐛 Does not work when using PouchDB on the server neighbourhoodie/voice-of-interconnect#9

Open

gr2m added 5 commits June 15, 2017 14:48

remove filename sanitizing

c6218ef

add test for GET /_invalid

d4a367c

test for GET /_internal and GET /user%2Fabc4567

159a763

(#213) - 400 Bad Request for invalid db names

8456eed

remove console.log

09974e2

gr2m force-pushed the 213/db-name-validation branch 2 times, most recently from d93a549 to bbe30e6 Compare June 15, 2017 15:51

WIP lookup routes once to find interal paths starting with _

f9c7cc9

gr2m force-pushed the 213/db-name-validation branch from bbe30e6 to f9c7cc9 Compare June 15, 2017 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

400 Bad Request for invalid database names #214

400 Bad Request for invalid database names #214

gr2m commented Feb 22, 2017 •

edited

gr2m Feb 22, 2017

garrensmith Feb 22, 2017

gr2m Feb 22, 2017

nolanlawson Feb 22, 2017

nolanlawson Feb 22, 2017

nolanlawson Feb 23, 2017

gr2m Feb 23, 2017

nolanlawson Feb 23, 2017

gr2m Feb 23, 2017

nolanlawson Feb 23, 2017

garrensmith left a comment

garrensmith Feb 22, 2017

gr2m Feb 22, 2017

garrensmith Feb 22, 2017

garrensmith Feb 22, 2017

gr2m Feb 22, 2017

marten-de-vries Feb 22, 2017

gr2m Feb 22, 2017

marten-de-vries Feb 22, 2017

nolanlawson Feb 22, 2017

nolanlawson Feb 22, 2017

gr2m Feb 22, 2017

nolanlawson Feb 23, 2017

nolanlawson Feb 22, 2017

gr2m commented Jun 15, 2017

400 Bad Request for invalid database names #214

Are you sure you want to change the base?

400 Bad Request for invalid database names #214

Conversation

gr2m commented Feb 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrensmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr2m commented Jun 15, 2017

gr2m commented Feb 22, 2017 •

edited