From ddb616edefd075a84f21e3c71bbc08701cf94cf1 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Mon, 18 Sep 2023 14:15:05 -0700 Subject: [PATCH] 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 * 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 --- common/etc/nginx/include/s3gateway.js | 37 ++++++++----------- .../etc/nginx/templates/default.conf.template | 2 +- test.sh | 5 +++ test/data/bucket-1/test/index.html | 1 + test/integration/test_api.sh | 26 +++++++++---- 5 files changed, 42 insertions(+), 29 deletions(-) create mode 100644 test/data/bucket-1/test/index.html diff --git a/common/etc/nginx/include/s3gateway.js b/common/etc/nginx/include/s3gateway.js index dda556cc..2ca140ec 100644 --- a/common/etc/nginx/include/s3gateway.js +++ b/common/etc/nginx/include/s3gateway.js @@ -322,9 +322,9 @@ function redirectToS3(r) { if (isDirectoryListing && (r.method === 'GET' || r.method === 'HEAD')) { r.internalRedirect("@s3PreListing"); - } else if ( PROVIDE_INDEX_PAGE == true ) { + } else if (PROVIDE_INDEX_PAGE === true) { r.internalRedirect("@s3"); - } else if ( !ALLOW_LISTING && !PROVIDE_INDEX_PAGE && uriPath == "/" ) { + } else if (!ALLOW_LISTING && !PROVIDE_INDEX_PAGE && uriPath === "/") { r.internalRedirect("@error404"); } else { r.internalRedirect("@s3"); @@ -333,8 +333,12 @@ function redirectToS3(r) { function trailslashControl(r) { if (APPEND_SLASH) { + // For the purposes of understanding whether this is a directory, + // consider the uri without query params or anchors + const path = r.variables.uri_path.split(/[?#]/)[0]; + const hasExtension = /\/[^.\/]+\.[^.]+$/; - if (!hasExtension.test(r.variables.uri_path) && !_isDirectory(r.variables.uri_path)){ + if (!hasExtension.test(path) && !_isDirectory(path)){ return r.internalRedirect("@trailslash"); } } @@ -353,22 +357,20 @@ async function loadContent(r) { r.internalRedirect("@s3Directory"); return; } - const url = s3uri(r); + const uri = s3uri(r); let reply = await ngx.fetch( - `http://127.0.0.1:80${url}` + `http://127.0.0.1:80${uri}` ); - if (reply.status == 200) { - // found index.html, so redirect to it - r.internalRedirect(r.variables.request_uri + INDEX_PAGE); - } else if (reply.status == 404) { - // else just list the contents of the directory + if (reply.status === 200) { + utils.debug_log(r, `Found index file, redirecting to: ${uri}`); + r.internalRedirect(uri); + } else if (reply.status === 404) { + // As there was no index file found, just list the contents of the directory r.internalRedirect("@s3Directory"); } else { r.internalRedirect("@error500"); } - - return; } /** @@ -449,16 +451,9 @@ function _escapeURIPath(uri) { * @private */ function _isDirectory(path) { - if (path === undefined) { - return false; - } - const len = path.length; - - if (len < 1) { - return false; - } + if (!path) return false; - return path.charAt(len - 1) === '/'; + return path.slice(-1) === '/'; } /** diff --git a/common/etc/nginx/templates/default.conf.template b/common/etc/nginx/templates/default.conf.template index c78ffc46..7873a190 100644 --- a/common/etc/nginx/templates/default.conf.template +++ b/common/etc/nginx/templates/default.conf.template @@ -326,7 +326,7 @@ server { location @trailslash { # 302 to request without slashes - rewrite ^ $scheme://$http_host$request_uri/ redirect; + rewrite ^ $scheme://$http_host$uri/$is_args$query_string redirect; } # Provide a hint to the client on 405 errors of the acceptable request methods diff --git a/test.sh b/test.sh index d90f9ce7..d2be720e 100755 --- a/test.sh +++ b/test.sh @@ -416,6 +416,11 @@ integration_test 2 0 1 0 compose stop nginx-s3-gateway # Restart with new config +p "Testing API with AWS Signature V2 and allow directory listing on and append slash and allow index" +integration_test 2 1 1 1 + +compose stop nginx-s3-gateway # Restart with new config + p "Test API with AWS Signature V4 and allow directory listing off" integration_test 4 0 0 0 diff --git a/test/data/bucket-1/test/index.html b/test/data/bucket-1/test/index.html new file mode 100644 index 00000000..da85dd89 --- /dev/null +++ b/test/data/bucket-1/test/index.html @@ -0,0 +1 @@ +

This is an index page of the d directory

\ No newline at end of file diff --git a/test/integration/test_api.sh b/test/integration/test_api.sh index ec49f99f..5153233c 100644 --- a/test/integration/test_api.sh +++ b/test/integration/test_api.sh @@ -63,6 +63,7 @@ if ! [ -x "${curl_cmd}" ]; then e "required dependency not found: curl not found in the path or not executable" exit ${no_dep_exit_code} fi +curl_cmd="${curl_cmd} --connect-timeout 3 --max-time 30 --no-progress-meter" # Allow for MacOS which does not support "md5sum" # but has "md5 -r" which can be substituted @@ -104,11 +105,11 @@ assertHttpRequestEquals() { if [ "${method}" = "HEAD" ]; then expected_response_code="$3" - actual_response_code="$(${curl_cmd} -s -o /dev/null -w '%{http_code}' --head "${uri}" ${extra_arg})" + actual_response_code="$(${curl_cmd} -o /dev/null -w '%{http_code}' --head "${uri}" ${extra_arg})" if [ "${expected_response_code}" != "${actual_response_code}" ]; then e "Response code didn't match expectation. Request [${method} ${uri}] Expected [${expected_response_code}] Actual [${actual_response_code}]" - e "curl command: ${curl_cmd} -s -o /dev/null -w '%{http_code}' --head '${uri}' ${extra_arg}" + e "curl command: ${curl_cmd} -o /dev/null -w '%{http_code}' --head '${uri}' ${extra_arg}" exit ${test_fail_exit_code} fi elif [ "${method}" = "GET" ]; then @@ -118,21 +119,21 @@ assertHttpRequestEquals() { checksum_output="$(${checksum_cmd} "${body_data_path}")" expected_checksum="${checksum_output:0:${checksum_length}}" - curl_checksum_output="$(${curl_cmd} -s -X "${method}" "${uri}" ${extra_arg} | ${checksum_cmd})" + curl_checksum_output="$(${curl_cmd} -X "${method}" "${uri}" ${extra_arg} | ${checksum_cmd})" s3_file_checksum="${curl_checksum_output:0:${checksum_length}}" if [ "${expected_checksum}" != "${s3_file_checksum}" ]; then e "Checksum doesn't match expectation. Request [${method} ${uri}] Expected [${expected_checksum}] Actual [${s3_file_checksum}]" - e "curl command: ${curl_cmd} -s -X '${method}' '${uri}' ${extra_arg} | ${checksum_cmd}" + e "curl command: ${curl_cmd} -X '${method}' '${uri}' ${extra_arg} | ${checksum_cmd}" exit ${test_fail_exit_code} fi else expected_response_code="$3" - actual_response_code="$(${curl_cmd} -s -o /dev/null -w '%{http_code}' "${uri}" ${extra_arg})" + actual_response_code="$(${curl_cmd} -o /dev/null -w '%{http_code}' "${uri}" ${extra_arg})" if [ "${expected_response_code}" != "${actual_response_code}" ]; then e "Response code didn't match expectation. Request [${method} ${uri}] Expected [${expected_response_code}] Actual [${actual_response_code}]" - e "curl command: ${curl_cmd} -s -o /dev/null -w '%{http_code}' '${uri}' ${extra_arg}" + e "curl command: ${curl_cmd} -o /dev/null -w '%{http_code}' '${uri}' ${extra_arg}" exit ${test_fail_exit_code} fi fi @@ -288,6 +289,15 @@ assertHttpRequestEquals "GET" "/statichost/noindexdir/multipledir/" "data/bucket assertHttpRequestEquals "GET" "/statichost" "data/bucket-1/statichost/index.html" assertHttpRequestEquals "GET" "/statichost/noindexdir/multipledir" "data/bucket-1/statichost/noindexdir/multipledir/index.html" fi + + if [ "${allow_directory_list}" == "1" ]; then + if [ "$append_slash" == "1" ]; then + assertHttpRequestEquals "GET" "test" "200" + assertHttpRequestEquals "GET" "test/" "200" + assertHttpRequestEquals "GET" "test?foo=bar" "200" + assertHttpRequestEquals "GET" "test/?foo=bar" "200" + fi + fi fi if [ "${allow_directory_list}" == "1" ]; then @@ -299,7 +309,9 @@ if [ "${allow_directory_list}" == "1" ]; then assertHttpRequestEquals "GET" "%D1%81%D0%B8%D1%81%D1%82%D0%B5%D0%BC%D1%8B/" "200" assertHttpRequestEquals "GET" "системы/" "200" if [ "$append_slash" == "1" ]; then - assertHttpRequestEquals "GET" "b" "302" + if [ "${index_page}" == "0" ]; then + assertHttpRequestEquals "GET" "b" "302" + fi else assertHttpRequestEquals "GET" "b" "404" fi