From 99ee5d15c2994b6806cc5cbd5a127fdeb484cc0d Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Tue, 2 Jun 2026 11:27:06 +0000 Subject: [PATCH 1/3] nginx: reject form previews with unexpected query params --- .github/workflows/main.yml | 2 +- files/nginx/odk.conf.template | 4 + test/nginx/src/mocha/nginx.spec.js | 114 ++++++++++++++++++++++++++++- 3 files changed, 115 insertions(+), 5 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 32ac23ddb..9c954f614 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -93,7 +93,7 @@ jobs: fetch-depth: 0 fetch-tags: true submodules: recursive - - run: ./test/check-docker-context.sh --min-size 50000 --max-size 100000 --min-count 1500 --max-count 1700 + - run: ./test/check-docker-context.sh --min-size 90000 --max-size 110000 --min-count 1500 --max-count 1700 - run: ./test/test-images.sh - if: always() run: docker compose logs diff --git a/files/nginx/odk.conf.template b/files/nginx/odk.conf.template index 1e4dab06b..896505dd6 100644 --- a/files/nginx/odk.conf.template +++ b/files/nginx/odk.conf.template @@ -165,6 +165,10 @@ server { # For that iframe to work, we'll need another path prefix (enketo-passthrough) under which we can # reach Enketo — this one will not be intercepted. location ~ ^/(?:-|enketo-passthrough)(?:/|$) { + if ($args ~* "(^|&|;)(x|%78|%58)?(f|%66|%46)(o|%6f|%4f)(r|%72|%52)(m|%6d|%4d)(=[^;&]*)?(&|;|$)" ) { + return 400; + } + rewrite ^/enketo-passthrough(/.*)?$ /-$1 break; proxy_pass http://enketo:8005; proxy_redirect off; diff --git a/test/nginx/src/mocha/nginx.spec.js b/test/nginx/src/mocha/nginx.spec.js index 1cdb9ab28..515c7649b 100644 --- a/test/nginx/src/mocha/nginx.spec.js +++ b/test/nginx/src/mocha/nginx.spec.js @@ -1,3 +1,4 @@ +const { isIPv6 } = require('node:net'); const tls = require('node:tls'); const { Readable } = require('stream'); @@ -609,6 +610,100 @@ function standardTestSuite({ fetchHttp, fetchHttp6, apiFetch, apiFetch6, forward }); }); + describe('enketo query param filtering', () => { + [ + '/-/preview', + '/-/preview/', + '/enketo-passthrough/preview', + '/enketo-passthrough/preview/', + ].forEach(pathRoot => { + [ + 'form', + 'form=', + 'form=123', + 'form&a=1', + 'form=&a=1', + 'form=123&a=1', + 'a=1&form', + 'a=1&form=', + 'a=1&form=123', + 'a=1&form&b=2', + 'a=1&form=&b=2', + 'a=1&form=123&b=2', + 'a=1&form&form=123', + 'a=1&form=123&form=123', + + 'xform', + 'xform=', + 'xform=123', + 'xform&a=1', + 'xform=&a=1', + 'xform=123&a=1', + 'a=1&xform', + 'a=1&xform=', + 'a=1&xform=123', + 'a=1&xform&b=2', + 'a=1&xform=&b=2', + 'a=1&xform=123&b=2', + 'a=1&xform&xform=123', + 'a=1&xform=123&xform=123', + + 'form=1&xform=1&form=2&xform=2&form=3&xform=3&form=4&xform=4&form=5&xform=5&form=6&xform=6&', + + '%66orm=123', + + 'XFORM=123', + + 'form;a=1', + 'form=;a=1', + 'form=123;a=1', + 'a=1;form', + 'a=1;form=', + 'a=1;form=123', + 'a=1;form;b=2', + 'a=1;form=;b=2', + 'a=1;form=123;b=2', + 'a=1;form;form=123', + 'a=1;form=123;form=123', + ].forEach(queryString => { + const path = pathRoot + '?' + queryString; + + it(`should reject path ${path}`, async () => { + // when + const res = await apiFetch(path); + + // then + assert.equal(res.status, 400); + // and + await assertEnketoReceivedNoRequests(); + }); + }); + + [ + 'platform=mac', + 'form_id=99', + 'uniform=true', + 'a=1&deform=false&b=2', + 'information=detailed', + 'performance=good', + ].forEach(queryString => { + const path = pathRoot + '?' + queryString; + + it(`should NOT reject path ${path}`, async () => { + // when + const res = await apiFetch(path); + + // then + assert.equal(res.status, 200); + // and + await assertEnketoReceived( + { method:'GET', path:'/-/preview' + (pathRoot.endsWith('/') ? '/' : '') + '?' + queryString }, + ); + }); + }); + }); + }); + describe('blank.html', () => { [ '/blank.html', @@ -1087,13 +1182,20 @@ async function resetMock(port) { // // 1. do not follow redirects // 2. allow overriding of fetch's "forbidden" headers: https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name -function request(url, { body, ...options }={}) { +function request(urlString, { body, ...options }={}) { if(!options.headers) options.headers = {}; if(!options.headers.host) options.headers.host = 'odk-nginx.example.test'; + const url = new URL(urlString); + if(url.username || url.password) throw new Error('Basic auth creds not yet supported.'); + + options.host = safeIpv6(url.hostname); + options.port = url.port; + options.path = urlString.replace(/^http(s?):\/\/[^/]*/, '') || '/'; + return new Promise((resolve, reject) => { try { - const req = getProtocolImplFrom(url).request(url, options, res => { + const req = getProtocolImplFrom(url).request(options, res => { res.on('error', reject); const body = new Readable({ read:() => {} }); @@ -1129,8 +1231,7 @@ function request(url, { body, ...options }={}) { }); } -function getProtocolImplFrom(url) { - const { protocol } = new URL(url); +function getProtocolImplFrom({ protocol }) { switch(protocol) { case 'http:': return require('node:http'); case 'https:': return require('node:https'); @@ -1194,3 +1295,8 @@ function assertCsp(actual, expected) { ); } } + +function safeIpv6(hostname) { + const maybeV6 = hostname.replace(/^\[(.*)\]$/, (_, $1) => $1); + return isIPv6(maybeV6) ? maybeV6 : hostname; +} From cdf8586d9eeb37c115729b465ff989fcdfe5fc6a Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Thu, 4 Jun 2026 14:46:31 +0000 Subject: [PATCH 2/3] revert ci change --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 9c954f614..32ac23ddb 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -93,7 +93,7 @@ jobs: fetch-depth: 0 fetch-tags: true submodules: recursive - - run: ./test/check-docker-context.sh --min-size 90000 --max-size 110000 --min-count 1500 --max-count 1700 + - run: ./test/check-docker-context.sh --min-size 50000 --max-size 100000 --min-count 1500 --max-count 1700 - run: ./test/test-images.sh - if: always() run: docker compose logs From c16555ace41181c29a1274bda4aaaf04736e15f2 Mon Sep 17 00:00:00 2001 From: alxndrsn Date: Thu, 4 Jun 2026 14:50:30 +0000 Subject: [PATCH 3/3] revert irrelevant changes --- test/nginx/src/mocha/nginx.spec.js | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/test/nginx/src/mocha/nginx.spec.js b/test/nginx/src/mocha/nginx.spec.js index 515c7649b..e3f6b4a1e 100644 --- a/test/nginx/src/mocha/nginx.spec.js +++ b/test/nginx/src/mocha/nginx.spec.js @@ -1,4 +1,3 @@ -const { isIPv6 } = require('node:net'); const tls = require('node:tls'); const { Readable } = require('stream'); @@ -1182,20 +1181,13 @@ async function resetMock(port) { // // 1. do not follow redirects // 2. allow overriding of fetch's "forbidden" headers: https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name -function request(urlString, { body, ...options }={}) { +function request(url, { body, ...options }={}) { if(!options.headers) options.headers = {}; if(!options.headers.host) options.headers.host = 'odk-nginx.example.test'; - const url = new URL(urlString); - if(url.username || url.password) throw new Error('Basic auth creds not yet supported.'); - - options.host = safeIpv6(url.hostname); - options.port = url.port; - options.path = urlString.replace(/^http(s?):\/\/[^/]*/, '') || '/'; - return new Promise((resolve, reject) => { try { - const req = getProtocolImplFrom(url).request(options, res => { + const req = getProtocolImplFrom(url).request(url, options, res => { res.on('error', reject); const body = new Readable({ read:() => {} }); @@ -1231,7 +1223,8 @@ function request(urlString, { body, ...options }={}) { }); } -function getProtocolImplFrom({ protocol }) { +function getProtocolImplFrom(url) { + const { protocol } = new URL(url); switch(protocol) { case 'http:': return require('node:http'); case 'https:': return require('node:https'); @@ -1295,8 +1288,3 @@ function assertCsp(actual, expected) { ); } } - -function safeIpv6(hostname) { - const maybeV6 = hostname.replace(/^\[(.*)\]$/, (_, $1) => $1); - return isIPv6(maybeV6) ? maybeV6 : hostname; -}