resources/odata: don't crash with surprising instanceId (#1158)

.github/workflows/standard-e2e.yml

@@ -0,0 +1,38 @@
+name: Standard E2E Tests
+
+on: push
+
+env:
+  LOG_LEVEL: DEBUG
+
+jobs:
+  standard-e2e:
+    timeout-minutes: 15
+    # TODO should we use the same container as circle & central?
+    runs-on: ubuntu-latest
+    services:
+      # see: https://docs.github.com/en/[email protected]/actions/using-containerized-services/creating-postgresql-service-containers
+      postgres:
+        image: postgres:14.10
+        env:
+          POSTGRES_PASSWORD: odktest
+        ports:
+          - 5432:5432
+        # Set health checks to wait until postgres has started
+        options: >-
+          --health-cmd pg_isready
+          --health-interval 10s
+          --health-timeout 5s
+          --health-retries 5
+    steps:
+    - uses: actions/checkout@v4
+    - name: Use Node.js 20
+      uses: actions/setup-node@v4
+      with:
+        node-version: 20.10.0
+        cache: 'npm'
+    - run: npm ci --legacy-peer-deps
+    - run: node lib/bin/create-docker-databases.js
+    - name: E2E Test
+      timeout-minutes: 10
+      run: ./test/e2e/standard/run-tests.sh

lib/resources/odata.js

@@ -46,13 +46,13 @@ module.exports = (service, endpoint) => {
       return matches[1];
     };
 
-    const singleRecord = endpoint.odata.json(({ Forms, Submissions, env }, { auth, params, query, originalUrl }) =>
-      getForm(Forms, auth, params)
-        .then((form) => Promise.all([
-          Forms.getFields(form.def.id).then(selectFields(query, getTableFromOriginalUrl(originalUrl))),
-          Submissions.getForExport(form.id, getUuid(params.uuid), draft).then(getOrNotFound), // may require s3 blob handling
-        ])
-          .then(([fields, row]) => singleRowToOData(fields, row, env.domain, originalUrl, query))));
+    const singleRecord = endpoint.odata.json(async ({ Forms, Submissions, env }, { auth, params, query, originalUrl }) => {
+      const form = await getForm(Forms, auth, params);
+      const allFields = await Forms.getFields(form.def.id);
+      const selectedFields = selectFields(query, getTableFromOriginalUrl(originalUrl))(allFields);
+      const row = await Submissions.getForExport(form.id, getUuid(params.uuid), draft).then(getOrNotFound);
+      return singleRowToOData(selectedFields, row, env.domain, originalUrl, query);
+    });
 
     // TODO: because of the way express compiles the *, we have to register this twice.
     service.get(`${base}/Submissions\\((:uuid)\\)`, singleRecord);

test/e2e/standard/run-tests.sh

@@ -0,0 +1,39 @@
+#!/bin/bash -eu
+set -o pipefail
+
+serverUrl="http://localhost:8383"
+userEmail="[email protected]"
+userPassword="secret1234"
+
+log() { echo "[test/e2e/standard/run-tests] $*"; }
+
+log "Building backend..."
+make base
+
+log "Attempting to create user..."
+echo "$userPassword" | node ./lib/bin/cli.js user-create  -u "$userEmail" && log "User created."
+log "Attempting to promote user..."
+node ./lib/bin/cli.js user-promote -u "$userEmail" && log "User promoted."
+
+kill_child_processes() {
+  log "Killing child processes..."
+  kill -- -$$
+}
+trap kill_child_processes EXIT
+
+log "Starting backend..."
+make run &
+
+log "Waiting for backend to start..."
+timeout 30 bash -c "while ! curl -s -o /dev/null $serverUrl; do sleep 1; done"
+log "Backend started!"
+
+cd test/e2e/standard
+npx mocha test.js
+
+if ! curl -s -o /dev/null "$serverUrl"; then
+  log "Backend died."
+  exit 1
+fi
+
+log "Test completed OK."

test/e2e/standard/submission.xml

@@ -0,0 +1,3 @@
+<data id="{{formId}}" version="{{formVersion}}">
+  <orx:meta><orx:instanceID>{{submissionId}}</orx:instanceID></orx:meta>
+</data>

test/e2e/standard/test-form.xml

@@ -0,0 +1,22 @@
+<?xml version="1.0"?>
+<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:jr="http://openrosa.org/javarosa" xmlns:orx="http://openrosa.org/xforms" xmlns:odk="http://www.opendatakit.org/xforms">
+  <h:head>
+    <h:title>instanceId Test</h:title>
+    <model odk:xforms-version="1.0.0">
+      <instance>
+        <data id="test-form" version="20240506130956">
+          <meta>
+            <instanceID/>
+          </meta>
+        </data>
+      </instance>
+      <instance id="image">
+        <root>
+        </root>
+      </instance>
+      <bind nodeset="/data/meta/instanceID" type="string" readonly="true()" jr:preload="uid"/>
+    </model>
+  </h:head>
+  <h:body>
+  </h:body>
+</h:html>

test/e2e/standard/test.js

@@ -0,0 +1,84 @@
+// Copyright 2024 ODK Central Developers
+// See the NOTICE file at the top-level directory of this distribution and at
+// https://github.com/getodk/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 assert = require('node:assert');
+const fs = require('node:fs');
+
+const SUITE_NAME = 'test/e2e/standard';
+const { apiClient } = require('../util/api');
+
+const serverUrl = 'http://localhost:8383';
+const userEmail = '[email protected]';
+const userPassword = 'secret1234';
+
+describe('#1157 - Backend crash when opening hostile-named submission detail', () => {
+  let api, projectId, xmlFormId, xmlFormVersion; // eslint-disable-line one-var, one-var-declaration-per-line
+
+  it('should handle weird submission instanceId gracefully', async () => {
+    // given
+    api = await apiClient(SUITE_NAME, { serverUrl, userEmail, userPassword });
+    projectId = await createProject();
+    await uploadForm('test-form.xml');
+    // and
+    const goodSubmissionId = 'good-id';
+    await uploadSubmission(goodSubmissionId);
+
+    // expect 200:
+    await api.apiGet(`projects/${projectId}/forms/${encodeURIComponent(xmlFormId)}.svc/Submissions('${goodSubmissionId}')`);
+
+    // given
+    const badSubmissionId = 'bad-id:';
+    await uploadSubmission(badSubmissionId);
+    // when
+    await assert.rejects(
+      () => api.apiGet(`projects/${projectId}/forms/${encodeURIComponent(xmlFormId)}.svc/Submissions('${badSubmissionId}')?%24select=__id%2C__system%2Cmeta`),
+      (err) => {
+        // then
+        assert.strictEqual(err.responseStatus, 404);
+        assert.deepStrictEqual(JSON.parse(err.responseText), {
+          message: 'Could not find the resource you were looking for.',
+          code: 404.1,
+        });
+        return true;
+      },
+    );
+
+    // and service has not crashed:
+    const rootRes = await fetch(serverUrl);
+    assert.strictEqual(rootRes.status, 404);
+    assert.strictEqual(await rootRes.text(), '{"message":"Expected an API version (eg /v1) at the start of the request URL.","code":404.2}');
+  });
+
+  async function createProject() {
+    const project = await api.apiPostJson(
+      'projects',
+      { name:`standard-test-${new Date().toISOString().replace(/\..*/, '')}` },
+    );
+    return project.id;
+  }
+
+  async function uploadForm(xmlFilePath) {
+    const res = await api.apiPostFile(`projects/${projectId}/forms?publish=true`, xmlFilePath);
+    xmlFormId = res.xmlFormId;
+    xmlFormVersion = res.version;
+  }
+
+  function uploadSubmission(submissionId) {
+    const xmlTemplate = fs.readFileSync('submission.xml', { encoding: 'utf8' });
+    const formXml = xmlTemplate
+      .replace('{{submissionId}}', submissionId)
+      .replace('{{formId}}', xmlFormId)
+      .replace('{{formVersion}}', xmlFormVersion);
+
+    return api.apiPostFile(`projects/${projectId}/forms/${encodeURIComponent(xmlFormId)}/submissions?deviceID=testid`, {
+      body: formXml,
+      mimeType: 'application/xml',
+    });
+  }
+});

test/e2e/util/api.js

@@ -38,10 +38,16 @@ async function apiClient(suiteName, { serverUrl, userEmail, userPassword, logPat
     return apiFetch('GET', path, undefined, headers);
   }
 
-  function apiPostFile(path, filePath) {
-    const mimeType = mimetypeFor(filePath);
-    const blob = fs.readFileSync(filePath);
-    return apiPost(path, blob, { 'Content-Type':mimeType });
+  function apiPostFile(path, opts) {
+    if(typeof opts === 'string') {
+      return apiPostFile(path, {
+        body: fs.readFileSync(opts),
+        mimeType: mimetypeFor(opts),
+      });
+    } else {
+      const { body, mimeType } = opts;
+      return apiPost(path, body, { 'Content-Type':mimeType });
+    }
   }
 
   function apiPostJson(path, body, headers) {
@@ -91,7 +97,16 @@ async function apiClient(suiteName, { serverUrl, userEmail, userPassword, logPat
 
     // eslint-disable-next-line no-use-before-define
     if(isRedirected(res)) return new Redirect(res);
-    if(!res.ok) throw new Error(`${res.status}: ${await res.text()}`);
+    if(!res.ok) {
+      const responseStatus = res.status;
+      const responseText = await res.text();
+
+      const err = new Error(`${responseStatus}: ${responseText}`);
+      err.responseStatus = responseStatus;
+      err.responseText = responseText;
+
+      throw err;
+    }
     return res;
   }
 }