Skip to content

Commit ddb616e

Browse files
dekobon4141done
dekobon
and
4141done
authored
Fixes #164 (#167)
This PR fixes the issue #164 ## Expectations This solution assumes that the following configuration options are set to true * `APPEND_SLASH_FOR_POSSIBLE_DIRECTORY` * `PROVIDE_INDEX_PAGE` * `ALLOW_DIRECTORY_LIST` Given a folder `test` **with** an `index.html` present in the directory, the `index.html` should be served for: * `/test` * `/test/` * `/test?foo=bar` * `/test/?foo=bar` Given a folder `test` **WITHOUT** an `index.html` present in the directory, files in the directory should be listed for: * `/test` * `/test/` * `/test?foo=bar` * `/test/?foo=bar` ## Notes * The `@trailslash` was rewriting to `$request_uri` with a trailing slash on the end. In the case of `/test?foo=bar` we'd wind up with `/test?foo=bar/` which when combined with the other changes led to a rewrite loop * Changed the check for directory or file to consider the path without anchor or querystring * Added yet another integration test suite and shuffled around the conditionals that maybe make the tests even more confusing - but do cover this case. Co-authored-by: Javier Evans <j.evans@f5.com> * make the isDirectory check simpler and more inclusive of error states * Allow useful output from curl and enable timeouts This change adds three new flags when using curl to hit endpoints as part of integration tests: --connect-timeout 3 --max-time 30 --no-progress-meter --------- Co-authored-by: Javier Evans <j.evans@f5.com>
1 parent 7e35275 commit ddb616e

File tree

5 files changed

+42
-29
lines changed

5 files changed

+42
-29
lines changed

‎common/etc/nginx/include/s3gateway.js‎

Lines changed: 16 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -322,9 +322,9 @@ function redirectToS3(r) {
322322

323323
if (isDirectoryListing && (r.method === 'GET' || r.method === 'HEAD')) {
324324
r.internalRedirect("@s3PreListing");
325-
} else if ( PROVIDE_INDEX_PAGE == true ) {
325+
} else if (PROVIDE_INDEX_PAGE === true) {
326326
r.internalRedirect("@s3");
327-
} else if ( !ALLOW_LISTING && !PROVIDE_INDEX_PAGE && uriPath == "/" ) {
327+
} else if (!ALLOW_LISTING && !PROVIDE_INDEX_PAGE && uriPath === "/") {
328328
r.internalRedirect("@error404");
329329
} else {
330330
r.internalRedirect("@s3");
@@ -333,8 +333,12 @@ function redirectToS3(r) {
333333

334334
function trailslashControl(r) {
335335
if (APPEND_SLASH) {
336+
// For the purposes of understanding whether this is a directory,
337+
// consider the uri without query params or anchors
338+
const path = r.variables.uri_path.split(/[?#]/)[0];
339+
336340
const hasExtension = /\/[^.\/]+\.[^.]+$/;
337-
if (!hasExtension.test(r.variables.uri_path) && !_isDirectory(r.variables.uri_path)){
341+
if (!hasExtension.test(path) && !_isDirectory(path)){
338342
return r.internalRedirect("@trailslash");
339343
}
340344
}
@@ -353,22 +357,20 @@ async function loadContent(r) {
353357
r.internalRedirect("@s3Directory");
354358
return;
355359
}
356-
const url = s3uri(r);
360+
const uri = s3uri(r);
357361
let reply = await ngx.fetch(
358-
`http://127.0.0.1:80${url}`
362+
`http://127.0.0.1:80${uri}`
359363
);
360364

361-
if (reply.status == 200) {
362-
// found index.html, so redirect to it
363-
r.internalRedirect(r.variables.request_uri + INDEX_PAGE);
364-
} else if (reply.status == 404) {
365-
// else just list the contents of the directory
365+
if (reply.status === 200) {
366+
utils.debug_log(r, `Found index file, redirecting to: ${uri}`);
367+
r.internalRedirect(uri);
368+
} else if (reply.status === 404) {
369+
// As there was no index file found, just list the contents of the directory
366370
r.internalRedirect("@s3Directory");
367371
} else {
368372
r.internalRedirect("@error500");
369373
}
370-
371-
return;
372374
}
373375

374376
/**
@@ -449,16 +451,9 @@ function _escapeURIPath(uri) {
449451
* @private
450452
*/
451453
function _isDirectory(path) {
452-
if (path === undefined) {
453-
return false;
454-
}
455-
const len = path.length;
456-
457-
if (len < 1) {
458-
return false;
459-
}
454+
if (!path) return false;
460455

461-
return path.charAt(len - 1) === '/';
456+
return path.slice(-1) === '/';
462457
}
463458

464459
/**

‎common/etc/nginx/templates/default.conf.template‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ server {
326326

327327
location @trailslash {
328328
# 302 to request without slashes
329-
rewrite ^ $scheme://$http_host$request_uri/ redirect;
329+
rewrite ^ $scheme://$http_host$uri/$is_args$query_string redirect;
330330
}
331331

332332
# Provide a hint to the client on 405 errors of the acceptable request methods

‎test.sh‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,11 @@ integration_test 2 0 1 0
416416

417417
compose stop nginx-s3-gateway # Restart with new config
418418

419+
p "Testing API with AWS Signature V2 and allow directory listing on and append slash and allow index"
420+
integration_test 2 1 1 1
421+
422+
compose stop nginx-s3-gateway # Restart with new config
423+
419424
p "Test API with AWS Signature V4 and allow directory listing off"
420425
integration_test 4 0 0 0
421426

‎test/data/bucket-1/test/index.html‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
<h1>This is an index page of the d directory</h1>

‎test/integration/test_api.sh‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ if ! [ -x "${curl_cmd}" ]; then
6363
e "required dependency not found: curl not found in the path or not executable"
6464
exit ${no_dep_exit_code}
6565
fi
66+
curl_cmd="${curl_cmd} --connect-timeout 3 --max-time 30 --no-progress-meter"
6667

6768
# Allow for MacOS which does not support "md5sum"
6869
# but has "md5 -r" which can be substituted
@@ -104,11 +105,11 @@ assertHttpRequestEquals() {
104105

105106
if [ "${method}" = "HEAD" ]; then
106107
expected_response_code="$3"
107-
actual_response_code="$(${curl_cmd} -s -o /dev/null -w '%{http_code}' --head "${uri}" ${extra_arg})"
108+
actual_response_code="$(${curl_cmd} -o /dev/null -w '%{http_code}' --head "${uri}" ${extra_arg})"
108109

109110
if [ "${expected_response_code}" != "${actual_response_code}" ]; then
110111
e "Response code didn't match expectation. Request [${method} ${uri}] Expected [${expected_response_code}] Actual [${actual_response_code}]"
111-
e "curl command: ${curl_cmd} -s -o /dev/null -w '%{http_code}' --head '${uri}' ${extra_arg}"
112+
e "curl command: ${curl_cmd} -o /dev/null -w '%{http_code}' --head '${uri}' ${extra_arg}"
112113
exit ${test_fail_exit_code}
113114
fi
114115
elif [ "${method}" = "GET" ]; then
@@ -118,21 +119,21 @@ assertHttpRequestEquals() {
118119
checksum_output="$(${checksum_cmd} "${body_data_path}")"
119120
expected_checksum="${checksum_output:0:${checksum_length}}"
120121

121-
curl_checksum_output="$(${curl_cmd} -s -X "${method}" "${uri}" ${extra_arg} | ${checksum_cmd})"
122+
curl_checksum_output="$(${curl_cmd} -X "${method}" "${uri}" ${extra_arg} | ${checksum_cmd})"
122123
s3_file_checksum="${curl_checksum_output:0:${checksum_length}}"
123124

124125
if [ "${expected_checksum}" != "${s3_file_checksum}" ]; then
125126
e "Checksum doesn't match expectation. Request [${method} ${uri}] Expected [${expected_checksum}] Actual [${s3_file_checksum}]"
126-
e "curl command: ${curl_cmd} -s -X '${method}' '${uri}' ${extra_arg} | ${checksum_cmd}"
127+
e "curl command: ${curl_cmd} -X '${method}' '${uri}' ${extra_arg} | ${checksum_cmd}"
127128
exit ${test_fail_exit_code}
128129
fi
129130
else
130131
expected_response_code="$3"
131-
actual_response_code="$(${curl_cmd} -s -o /dev/null -w '%{http_code}' "${uri}" ${extra_arg})"
132+
actual_response_code="$(${curl_cmd} -o /dev/null -w '%{http_code}' "${uri}" ${extra_arg})"
132133

133134
if [ "${expected_response_code}" != "${actual_response_code}" ]; then
134135
e "Response code didn't match expectation. Request [${method} ${uri}] Expected [${expected_response_code}] Actual [${actual_response_code}]"
135-
e "curl command: ${curl_cmd} -s -o /dev/null -w '%{http_code}' '${uri}' ${extra_arg}"
136+
e "curl command: ${curl_cmd} -o /dev/null -w '%{http_code}' '${uri}' ${extra_arg}"
136137
exit ${test_fail_exit_code}
137138
fi
138139
fi
@@ -288,6 +289,15 @@ assertHttpRequestEquals "GET" "/statichost/noindexdir/multipledir/" "data/bucket
288289
assertHttpRequestEquals "GET" "/statichost" "data/bucket-1/statichost/index.html"
289290
assertHttpRequestEquals "GET" "/statichost/noindexdir/multipledir" "data/bucket-1/statichost/noindexdir/multipledir/index.html"
290291
fi
292+
293+
if [ "${allow_directory_list}" == "1" ]; then
294+
if [ "$append_slash" == "1" ]; then
295+
assertHttpRequestEquals "GET" "test" "200"
296+
assertHttpRequestEquals "GET" "test/" "200"
297+
assertHttpRequestEquals "GET" "test?foo=bar" "200"
298+
assertHttpRequestEquals "GET" "test/?foo=bar" "200"
299+
fi
300+
fi
291301
fi
292302

293303
if [ "${allow_directory_list}" == "1" ]; then
@@ -299,7 +309,9 @@ if [ "${allow_directory_list}" == "1" ]; then
299309
assertHttpRequestEquals "GET" "%D1%81%D0%B8%D1%81%D1%82%D0%B5%D0%BC%D1%8B/" "200"
300310
assertHttpRequestEquals "GET" "системы/" "200"
301311
if [ "$append_slash" == "1" ]; then
302-
assertHttpRequestEquals "GET" "b" "302"
312+
if [ "${index_page}" == "0" ]; then
313+
assertHttpRequestEquals "GET" "b" "302"
314+
fi
303315
else
304316
assertHttpRequestEquals "GET" "b" "404"
305317
fi

0 commit comments

Comments
 (0)