From 30162cc7cf2fde8675af7b3b2a0b918afc0be71a Mon Sep 17 00:00:00 2001
From: Alex Anderson <191496+alxndrsn@users.noreply.github.com>
Date: Thu, 19 Sep 2024 15:35:02 +0300
Subject: [PATCH] s3: handle 307 redirects

---
 .github/workflows/test-nginx.yml   |  9 +++++++++
 files/nginx/odk.conf.template      | 16 ++++++++++++++++
 test/mock-http-server/index.js     | 15 ++++++++++++---
 test/nginx.test.docker-compose.yml |  8 ++++++++
 test/run-tests.sh                  |  2 ++
 test/test-nginx.js                 | 20 ++++++++++++++++++++
 6 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/.github/workflows/test-nginx.yml b/.github/workflows/test-nginx.yml
index 36097032..6af8bed4 100644
--- a/.github/workflows/test-nginx.yml
+++ b/.github/workflows/test-nginx.yml
@@ -15,3 +15,12 @@ jobs:
         node-version: 20
     - run: cd test && npm i
     - run: cd test && ./run-tests.sh
+
+    - if: always()
+      run: docker logs test-nginx-1
+    - if: always()
+      run: docker logs test-service-1 
+    - if: always()
+      run: docker logs test-enketo-1
+    - if: always()
+      run: docker logs test-mock_s3-1
diff --git a/files/nginx/odk.conf.template b/files/nginx/odk.conf.template
index 663cb874..2e5aa171 100644
--- a/files/nginx/odk.conf.template
+++ b/files/nginx/odk.conf.template
@@ -58,6 +58,22 @@ server {
     proxy_request_buffering on;
     proxy_buffering off;
     proxy_read_timeout 2m;
+
+    # transparently follow 307 redirects
+    # See: https://serverfault.com/a/792035
+    proxy_intercept_errors on;
+    error_page 307 = @follow_redirect_transparently;
+  }
+
+  location @follow_redirect_transparently {
+    # TODO 127.0.0.11 is docker DNS, and may not be required in production
+    # TODO 8.8.8.8 is google DNS, and may not be palatable to some users
+    # TODO resolver config will need testing IRL
+    # See: https://nginx.org/en/docs/http/ngx_http_core_module.html#resolver
+    # See: https://stackoverflow.com/a/40331256
+    resolver 127.0.0.11 8.8.8.8;
+    set $hdr_location '$upstream_http_location';
+    proxy_pass '$hdr_location';
   }
 
   location / {
diff --git a/test/mock-http-server/index.js b/test/mock-http-server/index.js
index 2e729084..74ad96a5 100644
--- a/test/mock-http-server/index.js
+++ b/test/mock-http-server/index.js
@@ -14,8 +14,17 @@ app.get('/reset',       withStdLogging((req, res) => {
   res.json('OK');
 }));
 
-app.get('/*', logRequestType('GET'));
-app.post('/*', logRequestType('POST'));
+app.get('/blob-server/*', withStdLogging((req, res) => {
+  res.send(`blob:${req.path.replace('/blob-server/', '')}`);
+}));
+
+app.get('/*blob/*', withStdLogging((req, res) => {
+  // NOTE this may require tweaking when reality of using real nginx server is understood.
+  res.redirect(307, 'http://mock_s3:33333/blob-server/' + req.path.replace(/.*blob\//, ''));
+}));
+
+app.get('/*', ok('GET'));
+app.post('/*', ok('POST'));
 // TODO add more methods as required
 
 app.listen(port, () => {
@@ -29,7 +38,7 @@ function withStdLogging(fn) {
   };
 }
 
-function logRequestType(method) {
+function ok(method) {
   return withStdLogging((req, res) => {
     requests.push({ method, path:req.path });
     res.send('OK');
diff --git a/test/nginx.test.docker-compose.yml b/test/nginx.test.docker-compose.yml
index 1615c8f8..d4f27bfb 100644
--- a/test/nginx.test.docker-compose.yml
+++ b/test/nginx.test.docker-compose.yml
@@ -13,6 +13,13 @@ services:
       - "8383:8383"
     environment:
       - PORT=8383
+  mock_s3:
+    build:
+      dockerfile: mock-http-service.dockerfile
+    ports:
+      - "33333:33333"
+    environment:
+      - PORT=33333
   nginx:
     build:
       context: ..
@@ -20,6 +27,7 @@ services:
     depends_on:
       - service
       - enketo
+      - mock_s3
     environment:
       - DOMAIN=odk-nginx.example.test
       - SENTRY_ORG_SUBDOMAIN=example
diff --git a/test/run-tests.sh b/test/run-tests.sh
index 3833414f..f2be0642 100755
--- a/test/run-tests.sh
+++ b/test/run-tests.sh
@@ -38,6 +38,8 @@ log "Waiting for mock backend..."
 wait_for_http_response  5 localhost:8383/health 200
 log "Waiting for mock enketo..."
 wait_for_http_response  5 localhost:8005/health 200
+log "Waiting for mock s3..."
+wait_for_http_response  5 localhost:33333/health 200
 log "Waiting for nginx..."
 wait_for_http_response 90 localhost:9000 301
 
diff --git a/test/test-nginx.js b/test/test-nginx.js
index 878e3e00..5628a689 100644
--- a/test/test-nginx.js
+++ b/test/test-nginx.js
@@ -104,6 +104,26 @@ describe('nginx config', () => {
       { method:'GET', path:'/v1/some/central-backend/path' },
     );
   });
+
+  it('should transparently follow backend 307 redirects', async () => {
+    // when
+    const res = await fetchHttps('/v1/blob/some-bucket/some-id');
+
+    // then
+    assert.isTrue(res.ok);
+    assert.equal(res.status, 200);
+    assert.equal(await res.text(), 'blob:some-bucket/some-id');
+  });
+
+  it('should not modify enketo 307 redirects', async () => {
+    // when
+    const res = await fetchHttps('/-/blob/some-bucket/some-id');
+
+    // then
+    assert.isFalse(res.ok);
+    assert.equal(res.status, 307);
+    assert.equal(await res.headers.get('location'), 'http://mock_s3:33333/blob-server/some-bucket/some-id');
+  });
 });
 
 function fetchHttp(path, options) {