pouchdb-server icon indicating copy to clipboard operation
pouchdb-server copied to clipboard

pouchdb-server crashes due to request containing invalid rev

Open kwpeters opened this issue 6 years ago • 3 comments

Issue

When POSTing to _bulk_get, an invalid rev causes PouchDB Server to crash.

Info

  • Environment: Node.js v8.11.1
  • Platform: No browser involved. Both client and server running in Node.js v8.11.1.
  • Adapter: LevelDB
  • Server: PouchDB Server

Reproduce

All of the code needed to reproduce this issue can be found in this repo. I did not use glitch.me in order to demonstrate the server crash.

  1. Start PouchDB Server.

    pouchdb-server --port 3030 --dir ./dbs
    

    In the sample repo, this can be done with npm run start-server.

  2. Run the code found in this file

    This code sends a POST request to the _bulk_get endpoint. The code demonstrates that when rev is set to a valid value, the request succeeds. But, when rev is set to an invalid value, it causes the PouchDB Server to crash.

  3. Observe that the second POST request causes the PouchDB Server to crash with the following error and stack trace:

    Error: Unable to resolve latest revision for id person_1, rev 1-ecdf613faa5b4ba68c610c4d54dc0bca
        at Object.latest (/Users/kwpeters/dev/pouchdb_invalid_rev/node_modules/pouchdb-merge/lib/index.js:462:9)
        at /Users/kwpeters/dev/pouchdb_invalid_rev/node_modules/pouchdb-adapter-leveldb-core/lib/index.js:445:42
        at /Users/kwpeters/dev/pouchdb_invalid_rev/node_modules/sublevel-pouchdb/lib/index.js:245:9
        at /Users/kwpeters/dev/pouchdb_invalid_rev/node_modules/sublevel-pouchdb/lib/index.js:76:13
    

    Granted, the request is invalid, but I would expect the server to return an appropriate error rather than crash.

This is a duplicate posting of an issue submitted in the pouchdb repo. It is probably more appropriate here.

kwpeters avatar May 10 '19 12:05 kwpeters

Note that if you request a "bad" revision (i.e. a string that does not match the rev pattern - "sdfasdfasdf") you receive a "no revision found" error. This only happens if you request a non-existent revision that matches the correct pattern.

mohlsen avatar Jun 13 '19 11:06 mohlsen

More information: this does not happen for any _bulk_get request for a non-existent revision , it only happens if you specify /_bulk_get?latest=true and request a non-existent revision.

mohlsen avatar Jun 18 '19 14:06 mohlsen

After much investigation and analysis, I have been unable to find a relaiable fix for this in the express-poucdbcodebase. Due to the nested nature of all the async TaskQueues used in the adapters, the exception thrown by PouchDB does not bubble up to a single point that could be caught.

It would have seemed that we could have caught the exception by putting a try {...} catch on the call to bulkGet (code fragement below), but the exception takes down the server before this can be caught.

req.db.bulkGet(opts, function (err, response) {
      if (err) {
        return utils.sendError(res, err);
      }

      ...
      utils.sendJSON(res, 200, { results: httpResults });

    });

Example code performing all a variety of queries (and a summary of the results and differences) were added to @kwpeters example in this fork of his original repro case.

I added express-pouchdb unit tests in a fork of the pouchdb-server repo (which I can submit an MR for) illustrating this as well.

So, all this being said, my proposed fix for this is in PouchDB itself, which means this is really a fix for PouchDB Issue 7756. (can this be re-opened?). The code works fine for http Pouch requests, but fails for local (Node.js -> LeveDB) databases. This is because the implementation in the Level adapter is a bit different. The code in this commit fixes this issue, and has unit tests covering the scenerio (which fail before the fix to the adapter), and succeeds after the fix. All other unit tests pass with this fix as well.

Note in the unit test expectations, the results from the http and local adapters is different for bad revisions and also for these non-existing revisions. These responses should be the same I imagine, but that is a much larger issue to fix, and would have backwards compatibility issues (people with code expecting the bad behavior).

Does this seems like the right fix? Thoughts? Suggestions? Did I miss anything?

mohlsen avatar Jul 01 '19 18:07 mohlsen