From cb4d6a25908c48290ef76597db7decf4acaf9dec Mon Sep 17 00:00:00 2001 From: Taylor McKinnon Date: Tue, 21 Oct 2025 11:10:02 -0700 Subject: [PATCH] impr(ARSN-534): Fix website redirect for path style buckets --- lib/s3routes/routes/routeWebsite.ts | 12 +-- lib/s3routes/routesUtils.ts | 20 +++- tests/unit/s3routes/routeWebsite.spec.js | 2 +- .../routesUtils/redirectRequest.spec.js | 98 +++++++++++++++++-- 4 files changed, 109 insertions(+), 23 deletions(-) diff --git a/lib/s3routes/routes/routeWebsite.ts b/lib/s3routes/routes/routeWebsite.ts index 7255e312e..aec6fcf66 100644 --- a/lib/s3routes/routes/routeWebsite.ts +++ b/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) { diff --git a/lib/s3routes/routesUtils.ts b/lib/s3routes/routesUtils.ts index 8ff340c44..19aa19640 100644 --- a/lib/s3routes/routesUtils.ts +++ b/lib/s3routes/routesUtils.ts @@ -868,9 +868,8 @@ export function redirectRequest( redirectLocationHeader?: boolean; }, objectKey: string, - encrypted: boolean, + request: http.IncomingMessage, response: http.ServerResponse, - hostHeader: string, corsHeaders: Record, log: RequestLogger, ) { @@ -878,9 +877,13 @@ export function redirectRequest( 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; } diff --git a/tests/unit/s3routes/routeWebsite.spec.js b/tests/unit/s3routes/routeWebsite.spec.js index 1c22a8fb3..ddc449b0b 100644 --- a/tests/unit/s3routes/routeWebsite.spec.js +++ b/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, ); }); diff --git a/tests/unit/s3routes/routesUtils/redirectRequest.spec.js b/tests/unit/s3routes/routesUtils/redirectRequest.spec.js index 3284cb990..4c60d291b 100644 --- a/tests/unit/s3routes/routesUtils/redirectRequest.spec.js +++ b/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,24 +23,57 @@ 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, @@ -47,7 +81,29 @@ describe('routesUtils.redirectRequest', () => { 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);