impr(ARSN-534): Fix website redirect for path style buckets

lib/s3routes/routes/routeWebsite.ts

@@ -36,11 +36,7 @@ export default function routerWebsite(
                     }
                     // note that key might have been modified in websiteGet
                     // api to add index document
-                    return routesUtils.redirectRequest(redirectInfo,
-                        // TODO ARSN-217 encrypted does not exists in request.connection
-                        // @ts-ignore
-                        key, request.connection.encrypted,
-                        response, request.headers.host!, resMetaHeaders, log);
+                    return routesUtils.redirectRequest(redirectInfo, key, request, response, resMetaHeaders, log);
                 }
                 // user has their own error page
                 if (err && dataGetInfo) {
@@ -69,11 +65,7 @@ export default function routerWebsite(
                             'HEAD', redirectInfo, null, dataRetrievalParams,
                             response, resMetaHeaders, log);
                     }
-                    return routesUtils.redirectRequest(redirectInfo,
-                        // TODO ARSN-217 encrypted does not exists in request.connection
-                        // @ts-ignore
-                        key, request.connection.encrypted,
-                        response, request.headers.host!, resMetaHeaders, log);
+                    return routesUtils.redirectRequest(redirectInfo, key, request, response, resMetaHeaders, log);
                 }
                 // could redirect on err so check for redirectInfo first
                 if (err) {

lib/s3routes/routesUtils.ts

@@ -868,19 +868,22 @@ export function redirectRequest(
         redirectLocationHeader?: boolean;
     },
     objectKey: string,
-    encrypted: boolean,
+    request: http.IncomingMessage,
     response: http.ServerResponse,
-    hostHeader: string,
     corsHeaders: Record<string, string>,
     log: RequestLogger,
 ) {
     const { justPath, redirectLocationHeader, hostName, protocol,
         httpRedirectCode, replaceKeyPrefixWith,
         replaceKeyWith, prefixFromRule } = routingInfo;
 
+    // TODO ARSN-217 encrypted does not exists in request.connection
+    // @ts-ignore
+    const encrypted = request.socket.encrypted;
+
     const redirectProtocol = protocol || encrypted ? 'https' : 'http';
     const redirectCode = httpRedirectCode || 301;
-    const redirectHostName = hostName || hostHeader;
+    const redirectHostName = hostName || request.headers.host!;
 
     setCommonResponseHeaders(corsHeaders, response, log);
 
@@ -902,20 +905,27 @@ export function redirectRequest(
             redirectKey = replaceKeyPrefixWith + objectKey;
         }
     }
-    let redirectLocation = justPath ? `/${redirectKey}` :
-        `${redirectProtocol}://${redirectHostName}/${redirectKey}`;
+
+    const { gotBucketNameFromHost, bucketName } = request as any;
+    const redirectPath = gotBucketNameFromHost ? `/${redirectKey}` : `/${bucketName}/${redirectKey}`;
+
+    let redirectLocation = justPath ? redirectPath : `${redirectProtocol}://${redirectHostName}${redirectPath}`;
+
     if (!redirectKey && redirectLocationHeader && redirectLocation !== '/') {
         // remove hanging slash
         redirectLocation = redirectLocation.slice(0, -1);
     }
+
     log.end().info('redirecting request', {
         httpCode: redirectCode,
         redirectLocation: hostName,
     });
+
     response.writeHead(redirectCode, {
         Location: redirectLocation,
     });
     response.end();
+
     return undefined;
 }
 

tests/unit/s3routes/routeWebsite.spec.js

@@ -142,7 +142,7 @@ describe('routerWebsite', () => {
         routerWebsite(request, response, api, log, statsClient, dataRetrievalParams);
 
         expect(routesUtils.redirectRequest).toHaveBeenCalledWith(
-            mockRedirectInfo, 'some-key', true, response, request.headers.host, null, log,
+            mockRedirectInfo, 'some-key', request, response, null, log,
         );
     });
 

tests/unit/s3routes/routesUtils/redirectRequest.spec.js

@@ -1,5 +1,6 @@
 const assert = require('assert');
 
+const DummyRequest = require('../../../utils/DummyRequest');
 const DummyRequestLogger = require('../../storage/metadata/mongoclient/utils/DummyRequestLogger');
 const HttpResponseMock = require('../../../utils/HttpResponseMock');
 const routesUtils = require('../../../../lib/s3routes/routesUtils');
@@ -22,32 +23,87 @@ describe('routesUtils.redirectRequest', () => {
         const objectKey = '';
         const redirectLocationHeader = true;
 
-        it('should redirect to absolute url', () => {
+        it('should redirect to absolute url (path style bucket)', () => {
+            const requestMock = new DummyRequest({
+                socket: { encrypted: false },
+                gotBucketNameFromHost: false,
+                bucketName: 'test',
+            });
             const responseMock = new HttpResponseMock();
             const routing = {
                 redirectLocationHeader,
                 protocol: 'https',
-                hostName: 'scality.com/test',
+                hostName: 'scality.com',
             };
             routesUtils.redirectRequest(
-                routing, objectKey, encrypted, responseMock, hostHeader,
+                routing, objectKey, requestMock, responseMock,
                 corsHeaders, new DummyRequestLogger(),
             );
             assert.strictEqual(responseMock.statusCode, 301);
             assert.strictEqual(responseMock._body, null);
             assertCors(responseMock);
+            console.log({ location: responseMock._headers.Location })
             assert.strictEqual(responseMock._headers.Location, 'https://scality.com/test');
         });
 
-        it('should redirect to relative url', () => {
+        it('should redirect to absolute url (virtual-hosted style bucket)', () => {
+            const requestMock = new DummyRequest({
+                socket: { encrypted: false },
+                gotBucketNameFromHost: true,
+                bucketName: 'test',
+            });
+            const responseMock = new HttpResponseMock();
+            const routing = {
+                redirectLocationHeader,
+                protocol: 'https',
+                hostName: 'test.scality.com',
+            };
+            routesUtils.redirectRequest(
+                routing, objectKey, requestMock, responseMock,
+                corsHeaders, new DummyRequestLogger(),
+            );;
+            assert.strictEqual(responseMock.statusCode, 301);
+            assert.strictEqual(responseMock._body, null);
+            assertCors(responseMock);
+            assert.strictEqual(responseMock._headers.Location, 'https://test.scality.com');
+        });
+
+        it('should redirect to relative url (path style bucket)', () => {
+            const requestMock = new DummyRequest({
+                socket: { encrypted: false },
+                gotBucketNameFromHost: false,
+                bucketName: 'test',
+            });
             const responseMock = new HttpResponseMock();
             const routing = {
                 redirectLocationHeader,
                 justPath: true,
                 replaceKeyWith: 'testing/redirect.html',
             };
             routesUtils.redirectRequest(
-                routing, objectKey, encrypted, responseMock, hostHeader,
+                routing, objectKey, requestMock, responseMock,
+                corsHeaders, new DummyRequestLogger(),
+            );
+            assert.strictEqual(responseMock.statusCode, 301);
+            assert.strictEqual(responseMock._body, null);
+            assertCors(responseMock);
+            assert.strictEqual(responseMock._headers.Location, '/test/testing/redirect.html');
+        });
+
+        it('should redirect to relative url (virtual-hosted style bucket)', () => {
+            const requestMock = new DummyRequest({
+                socket: { encrypted: false },
+                gotBucketNameFromHost: true,
+                bucketName: 'test',
+            });
+            const responseMock = new HttpResponseMock();
+            const routing = {
+                redirectLocationHeader,
+                justPath: true,
+                replaceKeyWith: 'testing/redirect.html',
+            };
+            routesUtils.redirectRequest(
+                routing, objectKey, requestMock, responseMock,
                 corsHeaders, new DummyRequestLogger(),
             );
             assert.strictEqual(responseMock.statusCode, 301);
@@ -56,7 +112,35 @@ describe('routesUtils.redirectRequest', () => {
             assert.strictEqual(responseMock._headers.Location, '/testing/redirect.html');
         });
 
-        it('should redirect to root /', () => {
+        it('should redirect to root / (path style bucket)', () => {
+            const requestMock = new DummyRequest({
+                socket: { encrypted: false },
+                gotBucketNameFromHost: false,
+                bucketName: 'test',
+            });
+            const responseMock = new HttpResponseMock();
+            const routing = {
+                redirectLocationHeader,
+                justPath: true,
+                // cloudserver removes the /, arsenal puts it back
+                replaceKeyWith: '',
+            };
+            routesUtils.redirectRequest(
+                routing, objectKey, requestMock, responseMock,
+                corsHeaders, new DummyRequestLogger(),
+            );
+            assert.strictEqual(responseMock.statusCode, 301);
+            assert.strictEqual(responseMock._body, null);
+            assertCors(responseMock);
+            assert.strictEqual(responseMock._headers.Location, '/test');
+        });
+
+        it('should redirect to root / (dns style bucket)', () => {
+            const requestMock = new DummyRequest({
+                socket: { encrypted: false },
+                gotBucketNameFromHost: true,
+                bucketName: 'test',
+            });
             const responseMock = new HttpResponseMock();
             const routing = {
                 redirectLocationHeader,
@@ -65,7 +149,7 @@ describe('routesUtils.redirectRequest', () => {
                 replaceKeyWith: '',
             };
             routesUtils.redirectRequest(
-                routing, objectKey, encrypted, responseMock, hostHeader,
+                routing, objectKey, requestMock, responseMock,
                 corsHeaders, new DummyRequestLogger(),
             );
             assert.strictEqual(responseMock.statusCode, 301);