Merge pull request #225 from opendatakit/issa/csrf-etc

v0.6 misc: CSRF, search(), keyId, urlencode bug, dependencies

Makefile

@@ -27,7 +27,7 @@ test-unit: node_modules
 	node node_modules/mocha/bin/mocha --recursive test/unit
 
 test-coverage: node_modules
-	node node_modules/.bin/nyc -x "**/migrations/**" --reporter=lcov node_modules/.bin/_mocha --recursive test
+	node node_modules/.bin/nyc -x "**/migrations/**" --reporter=lcov node_modules/.bin/_mocha --exit --recursive test
 
 lint:
 	node node_modules/.bin/eslint lib

docs/api.md

@@ -2033,8 +2033,8 @@ These are in alphabetic order, with the exception that the `Extended` versions o
 
 ## Form State (enum)
 + open (string) - _(Default)_ This form is available for download and accepts submissions.
-+ closing (string) - This form is _not_ available for download but still accepts submissions.
-+ closed (string) - This form is _not_ available for download, and it does _not_ accept submissions.
++ closing (string) - This form is _not_ available for download but still accepts submissions. This state will remove the form from the [OpenRosa FormList](/reference/openrosa-endpoints/openrosa-form-listing-api) that mobile clients use to determine which forms are available to download. The `closing` state does not affect the REST API at all: a `closing` form will still be available over all REST APIs.
++ closed (string) - This form is _not_ available for download, and it does _not_ accept submissions. This state is the same as the `closing` state, but additionally new submissions will not be accepted over either the [OpenRosa](/reference/openrosa-endpoints/openrosa-form-submission-api) or the [REST](/reference/forms-and-submissions/submissions/creating-a-submission) submission creation APIs.
 
 ## Extended App User (App User)
 + createdBy (Actor, required) - The full details about the `Actor` that created this `App User`.
@@ -2053,6 +2053,7 @@ These are in alphabetic order, with the exception that the `Extended` versions o
 ## Key (object)
 + id: `1` (number, required) - The numerical ID of the Key.
 + public: `bcFeKDF3Sg8W91Uf5uxaIlM2uK0cUN9tBSGoASbC4LeIPqx65+6zmjbgUnIyiLzIjrx4CAaf9Y9LG7TAu6wKPqfbH6ZAkJTFSfjLNovbKhpOQcmO5VZGGay6yvXrX1TFW6C6RLITy74erxfUAStdtpP4nraCYqQYqn5zD4/1OmgweJt5vzGXW2ch7lrROEQhXB9lK+bjEeWx8TFW/+6ha/oRLnl6a2RBRL6mhwy3PoByNTKndB2MP4TygCJ/Ini4ivk74iSqVnoeuNJR/xUcU+kaIpZEIjxpAS2VECJU9fZvS5Gt84e5wl/t7bUKu+dlh/cUgHfk6+6bwzqGQYOe5A==` (string, required) - The base64-encoded public key, with PEM envelope removed.
++ managed: `true` (boolean, optional) - If true, this is a key generated by Project managed encryption. If not, this key is self-supplied.
 + hint: `it was a secret` (string, optional) - The hint, if given, related to a managed encryption key.
 
 ## User (Actor)

lib/data/schema.js

@@ -18,9 +18,12 @@
 
 const { always } = require('ramda');
 const hparser = require('htmlparser2');
+const parse = require('csv-parse/lib/sync');
+const { decodeXML } = require('entities');
+const Option = require('../util/option');
 const Problem = require('../util/problem');
 const { sanitizeOdataIdentifier } = require('../util/util');
-const { stripNamespacesFromPath, traverseXml, findOne, findAll, and, root, node, hasAttr, tree, getAll, attr, text } = require('../util/xml');
+const { stripNamespacesFromPath, traverseXml, findOne, findAll, and, or, root, node, hasAttr, tree, getAll, attr, text } = require('../util/xml');
 
 ////////////////////////////////////////////////////////////////////////////////
 // SCHEMA EXTRACTION
@@ -195,9 +198,34 @@ const getJrPathName = (x) => {
 const mediaForms = { image: 'image', audio: 'audio', video: 'video', 'big-image': 'image' };
 const getMediaForm = (x) => mediaForms[x];
 
+// utility for expectedFormAttachments; find appearances that contain search, and then
+// extract the actual filename.
+const hasSearchAppearance = (_, attrs) => (attrs.appearance != null) && attrs.appearance.includes('search');
+const getSearchAppearance = (e, _, { appearance }) => {
+  if (e !== 'open') return getSearchAppearance;
+
+  const searchArgStr = /^\s*(?:\w*\s+)?search\s*\(\s*("|')(.+)\)\s*$/.exec(decodeXML(appearance));
+  if (searchArgStr == null) return Option.none();
+  const [ , quote, argStr ] = searchArgStr;
+
+  // l/rtrim allow spaces between " and , at the field delimit.
+  const [ searchArgs ] = parse(quote + argStr, { quote, ltrim: true, rtrim: true });
+  if (searchArgs[0] != null) {
+    // leading/trailing whitespace apparently cause Collect to just fail cryptically
+    // so we will just pretend the file does not exist.
+    const basename = searchArgs[0];
+    if (/^\s|\s$/.test(basename)) return Option.none();
+
+    const name = basename.endsWith('.csv') ? basename : `${basename}.csv`;
+    return Option.of(name);
+  }
+  return Option.none();
+};
+
 // gets all expected form attachment files from an xforms definition.
 const expectedFormAttachments = (xml) => {
   const modelNode = findOne(root('html'), node('head'), node('model'));
+  const bodyNode = findOne(root('html'), node('body'));
   return traverseXml(xml, [
     // gets <instance src="??"> from model children.
     modelNode(findAll(root(), and(node('instance'), hasAttr('src')))(attr('src'))),
@@ -207,8 +235,12 @@ const expectedFormAttachments = (xml) => {
 
     // gets <input query="*"> which implies itemsets.csv.
     // nested findOne so the <input> can be any number of levels deep in <body>.
-    findOne(root('html'), node('body'))(findOne(and(node('input'), hasAttr('query')))(always(true)))
-  ]).then(([ instanceSrcs, mediaLabels, hasItemsets ]) => {
+    bodyNode(findOne(and(node('input'), hasAttr('query')))(always(true))),
+
+    // gets <select|select1 appearance> nodes that contain the word search and
+    // extract the first argument if it is valid..
+    bodyNode(findAll(and(or(node('select'), node('select1')), hasSearchAppearance))(getSearchAppearance))
+  ]).then(([ instanceSrcs, mediaLabels, hasItemsets, selectSearches ]) => {
     // Option[Array[Option[text]]] => Array[text], filtering blank text.
     const externalInstances = instanceSrcs.map((srcs) => srcs
       .map((maybeSrc) => maybeSrc
@@ -230,6 +262,12 @@ const expectedFormAttachments = (xml) => {
     // if we have an <input query= at all we expect an itemsets.csv file.
     const itemsets = hasItemsets.orElse(false) ? [{ type: 'file', name: 'itemsets.csv' }] : [];
 
+    // filter down searches we can actually handle.
+    const searchCsvs = selectSearches.map((searches) => searches
+      .filter((maybe) => maybe.isDefined()) // TODO: it is silly that this pattern is so verbose.
+      .map((maybe) => ({ type: 'file', name: maybe.get() }))
+    ).orElse([]); // eslint-disable-line function-paren-newline
+
     // now we need to deduplicate our expected files. we consider type and name
     // a composite key, and we only add such pairs once. but duplicate names with
     // different types will be sent through, as they are different expectations.
@@ -239,7 +277,7 @@ const expectedFormAttachments = (xml) => {
     //    eventually complain about duplicate filenames.
     const seen = {};
     const result = [];
-    for (const attachment of externalInstances.concat(mediaFiles).concat(itemsets)) {
+    for (const attachment of externalInstances.concat(mediaFiles).concat(itemsets).concat(searchCsvs)) {
       if ((seen[attachment.name] == null) || (seen[attachment.name] !== attachment.type))
         result.push(attachment);
       seen[attachment.name] = attachment.type;

lib/http/preprocessors.js

@@ -8,7 +8,7 @@
 // except according to the terms contained in the LICENSE file.
 
 const { map } = require('ramda');
-const { isBlank, noop } = require('../util/util');
+const { isBlank, noop, without } = require('../util/util');
 const { isTrue } = require('../util/http');
 const Problem = require('../util/problem');
 const { QueryOptions } = require('../util/db');
@@ -75,9 +75,9 @@ const sessionHandler = ({ Session, User, Auth, crypto }, context) => {
           return reject(Problem.user.authenticationFailed());
         }));
 
-  // Cookie Auth, which is more relaxed about not doing anything on failures
-  // (but will absolutely not work on anything but GET):
-  } else if ((context.headers.cookie != null) && (context.method === 'GET')) {
+  // Cookie Auth, which is more relaxed about not doing anything on failures.
+  // but if the method is anything but GET we will need to check the CSRF token.
+  } else if (context.headers.cookie != null) {
     // do nothing if the user attempts multiple authentication schemes:
     if ((context.auth != null) && context.auth.isAuthenticated())
       return;
@@ -91,8 +91,25 @@ const sessionHandler = ({ Session, User, Auth, crypto }, context) => {
     if (token == null)
       return;
 
-    // actually try to authenticate with it. no Problem on failure.
-    return authBySessionToken(token[1]);
+    // actually try to authenticate with it. no Problem on failure. short circuit
+    // out if we have a GET request.
+    const maybeSession = authBySessionToken(token[1]);
+    if (context.method === 'GET') return maybeSession;
+
+    // if non-GET run authentication as usual but we'll have to check CSRF afterwards.
+    return maybeSession.then((cxt) => { // we have to use cxt rather than context for the linter
+      // if authentication failed anyway, just do nothing.
+      if ((cxt == null) || !cxt.auth.session().isDefined()) return;
+
+      // if csrf missing or mismatch; fail outright.
+      const csrf = cxt.body.__csrf;
+      if (isBlank(csrf) || (cxt.auth.session().get().csrf !== csrf))
+        return reject(Problem.user.authenticationFailed());
+
+      // delete the token off the body so it doesn't mess with downstream
+      // payload expectations.
+      return cxt.with({ body: without([ '__csrf' ], cxt.body) });
+    });
   }
 };
 

lib/model/instance/project.js

@@ -21,7 +21,7 @@ const { withCreateTime, withUpdateTime } = require('../../util/instance');
 const ExtendedProject = ExtendedInstance({
   fields: {
     joined: [ 'forms', 'lastSubmission', 'appUsers' ],
-    readable: [ 'id', 'name', 'archived', 'forms', 'lastSubmission', 'appUsers', 'createdAt', 'updatedAt' ]
+    readable: [ 'id', 'name', 'archived', 'keyId', 'forms', 'lastSubmission', 'appUsers', 'createdAt', 'updatedAt' ]
   },
   forApi() {
     const forms = this.forms || 0;
@@ -32,7 +32,7 @@ const ExtendedProject = ExtendedInstance({
 
 module.exports = Instance.with(ActeeTrait('project'), HasExtended(ExtendedProject))('projects', {
   all: [ 'id', 'name', 'archived', 'acteeId', 'keyId', 'createdAt', 'updatedAt', 'deletedAt' ],
-  readable: [ 'id', 'name', 'archived', 'createdAt', 'updatedAt' ],
+  readable: [ 'id', 'name', 'archived', 'keyId', 'createdAt', 'updatedAt' ],
   writable: [ 'name', 'archived' ]
 })(({ simply, fieldKeys, formDefs, projects, Key }) => class {
   forCreate() { return withCreateTime(this); }

lib/model/instance/session.js

@@ -17,8 +17,8 @@ const { generateToken } = require('../../util/crypto');
 
 
 module.exports = Instance('sessions', {
-  all: [ 'actorId', 'token', 'expiresAt', 'createdAt' ],
-  readable: [ 'token', 'expiresAt', 'createdAt' ]
+  all: [ 'actorId', 'token', 'csrf', 'expiresAt', 'createdAt' ],
+  readable: [ 'token', 'csrf', 'expiresAt', 'createdAt' ]
 })(({ Actor, Session, simply, sessions }) => class {
 
   forCreate() { return withCreateTime(this); }
@@ -28,7 +28,7 @@ module.exports = Instance('sessions', {
   static fromActor(actor) {
     const expiresAt = new Date();
     expiresAt.setDate(expiresAt.getDate() + 1);
-    return new Session({ actorId: actor.id, token: generateToken(), expiresAt });
+    return new Session({ actorId: actor.id, token: generateToken(), csrf: generateToken(), expiresAt });
   }
 
   // Returns Option[Session]. The resulting session object, if present, contains

lib/model/migrations/20171010-01-auth.js

@@ -8,7 +8,7 @@
 // except according to the terms contained in the LICENSE file.
 //
 
-const up = (knex, Promise) => {
+const up = (knex) => {
   const createActees = knex.schema.createTable('actees', (actees) => {
     actees.string('id', 36).primary();
     actees.string('species', 36);
@@ -78,7 +78,7 @@ const up = (knex, Promise) => {
   return Promise.all([ createActees, createActors, createUsers, createSessions, createMemberships, createGrants ]);
 };
 
-const down = (knex, Promise) => {
+const down = (knex) => {
   const tables = [ 'grants', 'memberships', 'sessions', 'users', 'actors', 'actees' ];
   const drop = (table) => knex.schema.dropTable(table);
   return Promise.all(tables.map(drop));

lib/model/migrations/20180108-01-expiring-actors.js

@@ -7,7 +7,7 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 //
-const up = (knex, Promise) => {
+const up = (knex) => {
   const addActorsColumn = knex.schema.table('actors', (actors) =>
     actors.date('expiresAt'));
   const renameSessionsColumn = knex.schema.table('sessions', (sessions) =>

lib/model/migrations/20180108-02-enum-to-varchar.js

@@ -7,12 +7,12 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 //
-const up = (knex, Promise) => Promise.all([
+const up = (knex) => Promise.all([
   knex.raw('alter table actors drop constraint actors_type_check'),
   knex.schema.table('actors', (actors) => actors.string('type', 15).alter())
 ]);
 
-const down = (knex, Promise) => Promise.all([]);
+const down = (knex) => Promise.all([]);
 
 module.exports = { up, down };
 

lib/model/migrations/20180125-03-add-blob-tables.js

@@ -7,7 +7,7 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 //
-const up = (knex, Promise) => {
+const up = (knex) => {
   const createBlobs = knex.schema.createTable('blobs', (blobs) => {
     blobs.increments('id');
     blobs.string('sha', 40).notNull().unique();
@@ -32,7 +32,7 @@ const up = (knex, Promise) => {
   return Promise.all([ createBlobs, createSubmissionsJoin ]);
 };
 
-const down = (knex, Promise) => Promise.all([
+const down = (knex) => Promise.all([
   knex.schema.dropTable('attachments'),
   knex.schema.dropTable('blobs')
 ]);

lib/model/migrations/20190618-01-add-csrf-token.js

@@ -0,0 +1,17 @@
+// Copyright 2019 ODK Central Developers
+// See the NOTICE file at the top-level directory of this distribution and at
+// https://github.com/opendatakit/central-backend/blob/master/NOTICE.
+// This file is part of ODK Central. It is subject to the license terms in
+// the LICENSE file found in the top-level directory of this distribution and at
+// https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central,
+// including this file, may be copied, modified, propagated, or distributed
+// except according to the terms contained in the LICENSE file.
+
+const up = (knex) =>
+  knex.schema.table('sessions', (sessions) => { sessions.string('csrf', 64); });
+
+const down = (knex) =>
+  knex.schema.table('sessions', (sessions) => { sessions.dropColumn('csrf'); });
+
+module.exports = { up, down };
+

lib/outbound/openrosa.js

@@ -38,21 +38,25 @@ const createdMessage = template(201, openRosaMessageBase());
 
 // Takes forms: [Form] and optional basePath: String, returns an OpenRosa xformsList
 // response. If basePath is given, it is inserted after domain in the downloadUrl.
-const formList = template(200, `<?xml version="1.0" encoding="UTF-8"?>
+const formListTemplate = template(200, `<?xml version="1.0" encoding="UTF-8"?>
   <xforms xmlns="http://openrosa.org/xforms/xformsList">
   {{#forms}}
     <xform>
       <formID>{{xmlFormId}}</formID>
       <name>{{name}}{{^name}}{{xmlFormId}}{{/name}}</name>
       <version>{{def.version}}</version>
       <hash>md5:{{def.hash}}</hash>
-      <downloadUrl>{{{domain}}}{{{basePath}}}/forms/{{xmlFormId}}.xml</downloadUrl>
+      <downloadUrl>{{{domain}}}{{{basePath}}}/forms/{{urlSafeXmlFormId}}.xml</downloadUrl>
       {{#hasAttachments}}
-      <manifestUrl>{{{domain}}}{{{basePath}}}/forms/{{xmlFormId}}/manifest</manifestUrl>
+      <manifestUrl>{{{domain}}}{{{basePath}}}/forms/{{urlSafeXmlFormId}}/manifest</manifestUrl>
       {{/hasAttachments}}
     </xform>
   {{/forms}}
   </xforms>`);
+const formList = (data) => formListTemplate(merge(data, {
+  forms: data.forms.map((form) =>
+    form.with({ urlSafeXmlFormId: encodeURIComponent(form.xmlFormId) }))
+}));
 
 const formManifestTemplate = template(200, `<?xml version="1.0" encoding="UTF-8"?>
   <manifest xmlns="http://openrosa.org/xforms/xformsManifest">

lib/util/xml.js

@@ -228,9 +228,8 @@ class Traversal {
 
   // we want to know if we have an im component so we can skip some work.
   get hasMisses() { return this.im > 0; }
-  // if we have traversed into negative space or we are running the imaginary
-  // origin axis, this traversal is no longer salient.
-  get useless() { return (this.re < 0) || ((this.re === 0) && (this.im > 0)); }
+  // if we no longer match anything (or never matched anything) we are useless.
+  get useless() { return this.re <= 0; }
 }
 
 // used by both findOne and findAll. parameter order modeled after those calls.
@@ -337,6 +336,12 @@ const and = (...conds) => (tagname, attr, isRoot) => {
       return false;
   return true;
 };
+const or = (...conds) => (tagname, attr, isRoot) => {
+  for (const cond of conds)
+    if (cond(tagname, attr, isRoot) === true)
+      return true;
+  return false;
+};
 
 // node() will match any node. node('name') will only match nodes of tagname name.
 const node = (expected) => (tagname) => ((expected === undefined)
@@ -416,7 +421,7 @@ const tree = (stack = []) => (e, tagname) => {
 module.exports = {
   traverseXml,
   findOne, findAll,
-  and, root, node, hasAttr,
+  and, or, root, node, hasAttr,
   getAll, attr, text, tree,
   stripNamespacesFromPath,