Skip to content

Commit 91eb289

Browse files
tmacrotmacro
committed
impr(ARSN-534): Fix website redirect for path style buckets
1 parent d92702e commit 91eb289

File tree

4 files changed

+109
-23
lines changed

4 files changed

+109
-23
lines changed

‎lib/s3routes/routes/routeWebsite.ts‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,7 @@ export default function routerWebsite(
3636
}
3737
// note that key might have been modified in websiteGet
3838
// api to add index document
39-
return routesUtils.redirectRequest(redirectInfo,
40-
// TODO ARSN-217 encrypted does not exists in request.connection
41-
// @ts-ignore
42-
key, request.connection.encrypted,
43-
response, request.headers.host!, resMetaHeaders, log);
39+
return routesUtils.redirectRequest(redirectInfo, key, request, response, resMetaHeaders, log);
4440
}
4541
// user has their own error page
4642
if (err && dataGetInfo) {
@@ -69,11 +65,7 @@ export default function routerWebsite(
6965
'HEAD', redirectInfo, null, dataRetrievalParams,
7066
response, resMetaHeaders, log);
7167
}
72-
return routesUtils.redirectRequest(redirectInfo,
73-
// TODO ARSN-217 encrypted does not exists in request.connection
74-
// @ts-ignore
75-
key, request.connection.encrypted,
76-
response, request.headers.host!, resMetaHeaders, log);
68+
return routesUtils.redirectRequest(redirectInfo, key, request, response, resMetaHeaders, log);
7769
}
7870
// could redirect on err so check for redirectInfo first
7971
if (err) {

‎lib/s3routes/routesUtils.ts‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -868,19 +868,22 @@ export function redirectRequest(
868868
redirectLocationHeader?: boolean;
869869
},
870870
objectKey: string,
871-
encrypted: boolean,
871+
request: http.IncomingMessage,
872872
response: http.ServerResponse,
873-
hostHeader: string,
874873
corsHeaders: Record<string, string>,
875874
log: RequestLogger,
876875
) {
877876
const { justPath, redirectLocationHeader, hostName, protocol,
878877
httpRedirectCode, replaceKeyPrefixWith,
879878
replaceKeyWith, prefixFromRule } = routingInfo;
880879

880+
// TODO ARSN-217 encrypted does not exists in request.connection
881+
// @ts-ignore
882+
const encrypted = request.socket.encrypted;
883+
881884
const redirectProtocol = protocol || encrypted ? 'https' : 'http';
882885
const redirectCode = httpRedirectCode || 301;
883-
const redirectHostName = hostName || hostHeader;
886+
const redirectHostName = hostName || request.headers.host!;
884887

885888
setCommonResponseHeaders(corsHeaders, response, log);
886889

@@ -902,20 +905,27 @@ export function redirectRequest(
902905
redirectKey = replaceKeyPrefixWith + objectKey;
903906
}
904907
}
905-
let redirectLocation = justPath ? `/${redirectKey}` :
906-
`${redirectProtocol}://${redirectHostName}/${redirectKey}`;
908+
909+
const { gotBucketNameFromHost, bucketName } = request as any;
910+
const redirectPath = gotBucketNameFromHost ? `/${redirectKey}` : `/${bucketName}/${redirectKey}`;
911+
912+
let redirectLocation = justPath ? redirectPath : `${redirectProtocol}://${redirectHostName}${redirectPath}`;
913+
907914
if (!redirectKey && redirectLocationHeader && redirectLocation !== '/') {
908915
// remove hanging slash
909916
redirectLocation = redirectLocation.slice(0, -1);
910917
}
918+
911919
log.end().info('redirecting request', {
912920
httpCode: redirectCode,
913921
redirectLocation: hostName,
914922
});
923+
915924
response.writeHead(redirectCode, {
916925
Location: redirectLocation,
917926
});
918927
response.end();
928+
919929
return undefined;
920930
}
921931

‎tests/unit/s3routes/routeWebsite.spec.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ describe('routerWebsite', () => {
142142
routerWebsite(request, response, api, log, statsClient, dataRetrievalParams);
143143

144144
expect(routesUtils.redirectRequest).toHaveBeenCalledWith(
145-
mockRedirectInfo, 'some-key', true, response, request.headers.host, null, log,
145+
mockRedirectInfo, 'some-key', request, response, null, log,
146146
);
147147
});
148148

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

Lines changed: 91 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
const assert = require('assert');
22

3+
const DummyRequest = require('../../../utils/DummyRequest');
34
const DummyRequestLogger = require('../../storage/metadata/mongoclient/utils/DummyRequestLogger');
45
const HttpResponseMock = require('../../../utils/HttpResponseMock');
56
const routesUtils = require('../../../../lib/s3routes/routesUtils');
@@ -22,32 +23,87 @@ describe('routesUtils.redirectRequest', () => {
2223
const objectKey = '';
2324
const redirectLocationHeader = true;
2425

25-
it('should redirect to absolute url', () => {
26+
it('should redirect to absolute url (path style bucket)', () => {
27+
const requestMock = new DummyRequest({
28+
socket: { encrypted: false },
29+
gotBucketNameFromHost: false,
30+
bucketName: 'test',
31+
});
2632
const responseMock = new HttpResponseMock();
2733
const routing = {
2834
redirectLocationHeader,
2935
protocol: 'https',
30-
hostName: 'scality.com/test',
36+
hostName: 'scality.com',
3137
};
3238
routesUtils.redirectRequest(
33-
routing, objectKey, encrypted, responseMock, hostHeader,
39+
routing, objectKey, requestMock, responseMock,
3440
corsHeaders, new DummyRequestLogger(),
3541
);
3642
assert.strictEqual(responseMock.statusCode, 301);
3743
assert.strictEqual(responseMock._body, null);
3844
assertCors(responseMock);
45+
console.log({ location: responseMock._headers.Location })
3946
assert.strictEqual(responseMock._headers.Location, 'https://scality.com/test');
4047
});
4148

42-
it('should redirect to relative url', () => {
49+
it('should redirect to absolute url (dns style bucket)', () => {
50+
const requestMock = new DummyRequest({
51+
socket: { encrypted: false },
52+
gotBucketNameFromHost: true,
53+
bucketName: 'test',
54+
});
55+
const responseMock = new HttpResponseMock();
56+
const routing = {
57+
redirectLocationHeader,
58+
protocol: 'https',
59+
hostName: 'test.scality.com',
60+
};
61+
routesUtils.redirectRequest(
62+
routing, objectKey, requestMock, responseMock,
63+
corsHeaders, new DummyRequestLogger(),
64+
);;
65+
assert.strictEqual(responseMock.statusCode, 301);
66+
assert.strictEqual(responseMock._body, null);
67+
assertCors(responseMock);
68+
assert.strictEqual(responseMock._headers.Location, 'https://test.scality.com');
69+
});
70+
71+
it('should redirect to relative url (path style bucket)', () => {
72+
const requestMock = new DummyRequest({
73+
socket: { encrypted: false },
74+
gotBucketNameFromHost: false,
75+
bucketName: 'test',
76+
});
4377
const responseMock = new HttpResponseMock();
4478
const routing = {
4579
redirectLocationHeader,
4680
justPath: true,
4781
replaceKeyWith: 'testing/redirect.html',
4882
};
4983
routesUtils.redirectRequest(
50-
routing, objectKey, encrypted, responseMock, hostHeader,
84+
routing, objectKey, requestMock, responseMock,
85+
corsHeaders, new DummyRequestLogger(),
86+
);
87+
assert.strictEqual(responseMock.statusCode, 301);
88+
assert.strictEqual(responseMock._body, null);
89+
assertCors(responseMock);
90+
assert.strictEqual(responseMock._headers.Location, '/test/testing/redirect.html');
91+
});
92+
93+
it('should redirect to relative url (dns style bucket)', () => {
94+
const requestMock = new DummyRequest({
95+
socket: { encrypted: false },
96+
gotBucketNameFromHost: true,
97+
bucketName: 'test',
98+
});
99+
const responseMock = new HttpResponseMock();
100+
const routing = {
101+
redirectLocationHeader,
102+
justPath: true,
103+
replaceKeyWith: 'testing/redirect.html',
104+
};
105+
routesUtils.redirectRequest(
106+
routing, objectKey, requestMock, responseMock,
51107
corsHeaders, new DummyRequestLogger(),
52108
);
53109
assert.strictEqual(responseMock.statusCode, 301);
@@ -56,7 +112,35 @@ describe('routesUtils.redirectRequest', () => {
56112
assert.strictEqual(responseMock._headers.Location, '/testing/redirect.html');
57113
});
58114

59-
it('should redirect to root /', () => {
115+
it('should redirect to root / (path style bucket)', () => {
116+
const requestMock = new DummyRequest({
117+
socket: { encrypted: false },
118+
gotBucketNameFromHost: false,
119+
bucketName: 'test',
120+
});
121+
const responseMock = new HttpResponseMock();
122+
const routing = {
123+
redirectLocationHeader,
124+
justPath: true,
125+
// cloudserver removes the /, arsenal puts it back
126+
replaceKeyWith: '',
127+
};
128+
routesUtils.redirectRequest(
129+
routing, objectKey, requestMock, responseMock,
130+
corsHeaders, new DummyRequestLogger(),
131+
);
132+
assert.strictEqual(responseMock.statusCode, 301);
133+
assert.strictEqual(responseMock._body, null);
134+
assertCors(responseMock);
135+
assert.strictEqual(responseMock._headers.Location, '/test');
136+
});
137+
138+
it('should redirect to root / (dns style bucket)', () => {
139+
const requestMock = new DummyRequest({
140+
socket: { encrypted: false },
141+
gotBucketNameFromHost: true,
142+
bucketName: 'test',
143+
});
60144
const responseMock = new HttpResponseMock();
61145
const routing = {
62146
redirectLocationHeader,
@@ -65,7 +149,7 @@ describe('routesUtils.redirectRequest', () => {
65149
replaceKeyWith: '',
66150
};
67151
routesUtils.redirectRequest(
68-
routing, objectKey, encrypted, responseMock, hostHeader,
152+
routing, objectKey, requestMock, responseMock,
69153
corsHeaders, new DummyRequestLogger(),
70154
);
71155
assert.strictEqual(responseMock.statusCode, 301);

0 commit comments

Comments
 (0)