Fixes nginxinc#164 (nginxinc#167)

This PR fixes the issue nginxinc#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 <[email protected]> * 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 <[email protected]>
elJosho · Oct 25, 2023 · 83d7612 · 83d7612
1 parent fea842c
commit 83d7612
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 29 deletions.
diff --git 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
@@ -331,7 +331,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
@@ -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
@@ -0,0 +1 @@
+<h1>This is an index page of the d directory</h1>
diff --git 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
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		<h1>This is an index page of the d directory</h1>