From 808c8d2f1beecfe2cec1626fea7b701e9b6fbe02 Mon Sep 17 00:00:00 2001
From: Moshe Atlow <moshe@atlow.co.il>
Date: Wed, 27 Sep 2023 18:30:56 +0300
Subject: [PATCH] fix: redirect when missing trailing slash (#28)

---
 src/handlers/get.ts                         |  4 ++
 src/util.ts                                 | 18 ++++++---
 tests/e2e/directory.test.ts                 | 45 ++++++++++++++-------
 tests/e2e/test-data/expected-html/dist.html |  4 +-
 4 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/src/handlers/get.ts b/src/handlers/get.ts
index 3427881..0f4a9ca 100644
--- a/src/handlers/get.ts
+++ b/src/handlers/get.ts
@@ -2,6 +2,7 @@ import responses from '../responses';
 import {
   isCacheEnabled,
   isDirectoryPath,
+  hasTrailingSlash,
   mapUrlPathToBucketPath,
   parseUrl,
 } from '../util';
@@ -39,6 +40,9 @@ const getHandler: Handler = async (request, env, ctx, cache) => {
     // File not found since we should only be allowing
     //  file paths if directory listing is off
     return responses.FILE_NOT_FOUND(request);
+  } else if (isPathADirectory && !hasTrailingSlash(bucketPath)) {
+    url.pathname += '/';
+    return Response.redirect(url.toString(), 301);
   }
 
   const response: Response = isPathADirectory
diff --git a/src/util.ts b/src/util.ts
index 472ccb3..b7a7e86 100644
--- a/src/util.ts
+++ b/src/util.ts
@@ -99,6 +99,18 @@ export function mapBucketPathToUrlPath(
     : ['/' + bucketPath];
 }
 
+export function hasTrailingSlash(path: string): boolean {
+  return path[path.length - 1] === '/';
+}
+
+export function isExtensionless(path: string): boolean {
+  // `path.lastIndexOf('.') == -1` is a Node-specific
+  //  heuristic here. There aren't any files that don't
+  //  have file extensions, so, if there are no file extensions
+  //  specified in the url, treat it like a directory.
+  return path.lastIndexOf('.') === -1;
+}
+
 /**
  * Checks if a R2 path is for a directory or not.
  *  If a path ends in a `/` or there's no file
@@ -107,11 +119,7 @@ export function mapBucketPathToUrlPath(
  * @returns True if it's for a directory
  */
 export function isDirectoryPath(path: string): boolean {
-  // `path.lastIndexOf('.') == -1` is a Node-specific
-  //  heuristic here. There aren't any files that don't
-  //  have file extensions, so, if there are no file extensions
-  //  specified in the url, treat it like a directory.
-  return path[path.length - 1] == '/' || path.lastIndexOf('.') == -1;
+  return hasTrailingSlash(path) || isExtensionless(path);
 }
 
 /**
diff --git a/tests/e2e/directory.test.ts b/tests/e2e/directory.test.ts
index 39b730f..aa8eeb1 100644
--- a/tests/e2e/directory.test.ts
+++ b/tests/e2e/directory.test.ts
@@ -24,15 +24,16 @@ describe('Directory Tests (Restricted Directory Listing)', () => {
     url = await mf.ready;
   });
 
-  it('allows `/dist` and returns expected html', async () => {
-    const [res, expectedHtml] = await Promise.all([
-      mf.dispatchFetch(`${url}dist`),
+  it('redirects `/dist` to `/dist/` and returns expected html', async () => {
+    const [originalRes, expectedHtml] = await Promise.all([
+      mf.dispatchFetch(`${url}dist`, { redirect: 'manual' }),
       readFile('./tests/e2e/test-data/expected-html/dist.html', {
         encoding: 'utf-8',
       }),
     ]);
 
-    assert.strictEqual(res.status, 200);
+    assert.strictEqual(originalRes.status, 301);
+    const res = await mf.dispatchFetch(originalRes.headers.get('location')!);
 
     // Assert that the html matches what we're expecting
     //  to be returned. If this passes, we can assume
@@ -48,8 +49,12 @@ describe('Directory Tests (Restricted Directory Listing)', () => {
     assert.strictEqual(res.status, 200);
   });
 
-  it('allows `/download`', async () => {
-    const res = await mf.dispatchFetch(`${url}download`);
+  it('redirects `/download` to `/download/`', async () => {
+    const originalRes = await mf.dispatchFetch(`${url}download`, {
+      redirect: 'manual',
+    });
+    assert.strictEqual(originalRes.status, 301);
+    const res = await mf.dispatchFetch(originalRes.headers.get('location')!);
     assert.strictEqual(res.status, 200);
   });
 
@@ -58,8 +63,12 @@ describe('Directory Tests (Restricted Directory Listing)', () => {
     assert.strictEqual(res.status, 200);
   });
 
-  it('allows `/docs`', async () => {
-    const res = await mf.dispatchFetch(`${url}docs`);
+  it('redirects `/docs` to `/docs/`', async () => {
+    const originalRes = await mf.dispatchFetch(`${url}docs`, {
+      redirect: 'manual',
+    });
+    assert.strictEqual(originalRes.status, 301);
+    const res = await mf.dispatchFetch(originalRes.headers.get('location')!);
     assert.strictEqual(res.status, 200);
   });
 
@@ -68,8 +77,12 @@ describe('Directory Tests (Restricted Directory Listing)', () => {
     assert.strictEqual(res.status, 200);
   });
 
-  it('allows `/api`', async () => {
-    const res = await mf.dispatchFetch(`${url}api`);
+  it('redirects `/api` to `/api/`', async () => {
+    const originalRes = await mf.dispatchFetch(`${url}api`, {
+      redirect: 'manual',
+    });
+    assert.strictEqual(originalRes.status, 301);
+    const res = await mf.dispatchFetch(originalRes.headers.get('location')!);
     assert.strictEqual(res.status, 200);
   });
 
@@ -78,8 +91,12 @@ describe('Directory Tests (Restricted Directory Listing)', () => {
     assert.strictEqual(res.status, 200);
   });
 
-  it('allows `/metrics`', async () => {
-    const res = await mf.dispatchFetch(`${url}metrics`);
+  it('redirects `/metrics` to `/metrics/`', async () => {
+    const originalRes = await mf.dispatchFetch(`${url}metrics`, {
+      redirect: 'manual',
+    });
+    assert.strictEqual(originalRes.status, 301);
+    const res = await mf.dispatchFetch(originalRes.headers.get('location')!);
     assert.strictEqual(res.status, 200);
   });
 
@@ -92,7 +109,7 @@ describe('Directory Tests (Restricted Directory Listing)', () => {
     let res = await mf.dispatchFetch(url);
     assert.strictEqual(res.status, 401);
 
-    res = await mf.dispatchFetch(`${url}/asd`);
+    res = await mf.dispatchFetch(`${url}/asd/`);
     assert.strictEqual(res.status, 401);
 
     res = await mf.dispatchFetch(`${url}/asd/123/`);
@@ -100,7 +117,7 @@ describe('Directory Tests (Restricted Directory Listing)', () => {
   });
 
   it('returns 404 for unknown directory', async () => {
-    const res = await mf.dispatchFetch(`${url}/dist/asd123`);
+    const res = await mf.dispatchFetch(`${url}/dist/asd123/`);
     assert.strictEqual(res.status, 404);
 
     const body = await res.text();
diff --git a/tests/e2e/test-data/expected-html/dist.html b/tests/e2e/test-data/expected-html/dist.html
index 518ace1..ef89609 100644
--- a/tests/e2e/test-data/expected-html/dist.html
+++ b/tests/e2e/test-data/expected-html/dist.html
@@ -1,7 +1,7 @@
 <!DOCTYPE html>
       <html>
         <head>
-          <title>Index of /dist</title>
+          <title>Index of /dist/</title>
           <meta name="viewport" content="width=device-width, initial-scale=1.0">
           <meta charset="utf-8">
           <style type="text/css">
@@ -24,7 +24,7 @@
           </style>
         </head>
         <body>
-          <h1>Index of /dist</h1>
+          <h1>Index of /dist/</h1>
           <table>
             <tr><th>Filename</th><th>Modified</th><th>Size</th></tr>
             <tr>