From 67bd0ad3def294a89fc17a8be8e1001a37303f80 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   |  2 ++
 files/nginx/odk.conf.template      | 16 ++++++++++++++++
 test/mock-http-server/index.js     |  9 +++++++++
 test/nginx.test.docker-compose.yml |  8 ++++++++
 test/run-tests.sh                  |  2 ++
 test/test-nginx.js                 | 20 ++++++++++++++++++++
 6 files changed, 57 insertions(+)

diff --git a/.github/workflows/test-nginx.yml b/.github/workflows/test-nginx.yml
index 79061efd..efb321f4 100644
--- a/.github/workflows/test-nginx.yml
+++ b/.github/workflows/test-nginx.yml
@@ -22,3 +22,5 @@ jobs:
       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 e04e9f42..74ad96a5 100644
--- a/test/mock-http-server/index.js
+++ b/test/mock-http-server/index.js
@@ -14,6 +14,15 @@ app.get('/reset',       withStdLogging((req, res) => {
   res.json('OK');
 }));
 
+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
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) {