Fix windows nested files generating bad urls

lib/remote-storage.js

@@ -24,22 +24,6 @@ const { codes, logAndThrow } = require('./StorageError')
 
 const fileExtensionPattern = /\*\.[0-9a-zA-Z]+$/
 
-// /**
-//  * Joins url path parts
-//  * @param {...string} args url parts
-//  * @returns {string}
-//  */
-function urlJoin (...args) {
-  let start = ''
-  if (args[0] &&
-      args[0].startsWith('/')) {
-    start = '/'
-  }
-  return start + args.map(a => a && a.replace(/(^\/|\/$)/g, ''))
-    .filter(a => a) // remove empty strings / nulls
-    .join('/')
-}
-
 module.exports = class RemoteStorage {
   /**
    * @param  {object} creds
@@ -153,7 +137,7 @@ module.exports = class RemoteStorage {
     const cacheControlString = this._getCacheControlConfig(mimeType, appConfig.app)
     const uploadParams = {
       Bucket: this.bucket,
-      Key: urlJoin(prefix, path.basename(file)),
+      Key: this._urlJoin(prefix, path.basename(file)),
       Body: content
     }
     // if we found it in the global config, we will use it ( for now )
@@ -293,7 +277,7 @@ module.exports = class RemoteStorage {
         // base directory returns ".", ignore that.
         prefixDirectory = prefixDirectory === '.' ? '' : prefixDirectory
         // newPrefix is now the initial prefix plus the files relative directory path.
-        const newPrefix = urlJoin(prefix, prefixDirectory)
+        const newPrefix = this._urlJoin(prefix, prefixDirectory)
         const s3Res = await this.uploadFile(f, newPrefix, appConfig, dir)
         if (postFileUploadCallback) {
           postFileUploadCallback(f)
@@ -306,6 +290,41 @@ module.exports = class RemoteStorage {
     return allResults
   }
 
+  /**
+   * Joins file path parts into a URL-friendly S3 key
+   * Normalizes paths by:
+   * - Preserving leading slash if first part starts with '/'
+   * - Removing leading/trailing slashes from each part
+   * - Converting Windows backslashes to forward slashes
+   * - Removing double slashes
+   * - Filtering out empty/null parts
+   * @param {...string} args - Path parts to join
+   * @returns {string} Normalized S3 key path
+   */
+  _urlJoin (...args) {
+    // Preserve leading slash if first argument starts with one
+    const hasLeadingSlash = args[0] && args[0].startsWith('/')
+
+    // Normalize each path part:
+    // 1. Remove leading and trailing slashes (^\/ matches start, \/$ matches end)
+    // 2. Convert Windows backslashes to forward slashes
+    // 3. Filter out empty/null values
+    const normalizedParts = args
+      .map(part => {
+        if (!part) return null
+        // Remove leading/trailing slashes and convert backslashes to forward slashes
+        return part.replace(/(^\/|\/$)/g, '').replace(/\\/g, '/')
+      })
+      .filter(part => part) // Remove empty strings and nulls
+
+    // Join parts with '/' and remove any double slashes that may result
+    const joined = normalizedParts.join('/')
+    const withoutDoubleSlashes = joined.replace(/\/+/g, '/')
+
+    // Restore leading slash if original first part had one
+    return hasLeadingSlash ? '/' + withoutDoubleSlashes : withoutDoubleSlashes
+  }
+
   /**
     * Get cache control string based on mime type and config
     * @param {string|boolean} mimeType - string if valid mimeType or false for unknown files

test/lib/remote-storage.test.js

@@ -176,6 +176,269 @@ describe('RemoteStorage', () => {
     })
   })
 
+  describe('_urlJoin', () => {
+    let rs
+
+    beforeEach(() => {
+      rs = new RemoteStorage(global.fakeTVMResponse)
+    })
+
+    describe('basic path joining', () => {
+      test('joins simple path parts', () => {
+        expect(rs._urlJoin('prefix', 'file.js')).toBe('prefix/file.js')
+      })
+
+      test('joins multiple path parts', () => {
+        expect(rs._urlJoin('prefix', 'deep', 'nested', 'file.js')).toBe('prefix/deep/nested/file.js')
+      })
+
+      test('handles single part', () => {
+        expect(rs._urlJoin('file.js')).toBe('file.js')
+      })
+
+      test('handles empty string parts', () => {
+        expect(rs._urlJoin('prefix', '', 'file.js')).toBe('prefix/file.js')
+      })
+
+      test('handles null parts', () => {
+        expect(rs._urlJoin('prefix', null, 'file.js')).toBe('prefix/file.js')
+      })
+
+      test('handles undefined parts', () => {
+        expect(rs._urlJoin('prefix', undefined, 'file.js')).toBe('prefix/file.js')
+      })
+
+      test('handles all empty/null parts', () => {
+        expect(rs._urlJoin('', null, undefined)).toBe('')
+      })
+    })
+
+    describe('leading slash preservation', () => {
+      test('preserves leading slash from first argument', () => {
+        expect(rs._urlJoin('/prefix', 'file.js')).toBe('/prefix/file.js')
+      })
+
+      test('preserves leading slash with multiple parts', () => {
+        expect(rs._urlJoin('/prefix', 'deep', 'file.js')).toBe('/prefix/deep/file.js')
+      })
+
+      test('does not add leading slash when first part does not have one', () => {
+        expect(rs._urlJoin('prefix', 'file.js')).toBe('prefix/file.js')
+      })
+
+      test('handles leading slash with single part', () => {
+        expect(rs._urlJoin('/file.js')).toBe('/file.js')
+      })
+    })
+
+    describe('trailing slash removal', () => {
+      test('removes trailing slash from parts', () => {
+        expect(rs._urlJoin('prefix/', 'file.js')).toBe('prefix/file.js')
+      })
+
+      test('removes trailing slash from multiple parts', () => {
+        expect(rs._urlJoin('prefix/', 'deep/', 'file.js')).toBe('prefix/deep/file.js')
+      })
+
+      test('removes trailing slash from last part', () => {
+        expect(rs._urlJoin('prefix', 'file.js/')).toBe('prefix/file.js')
+      })
+
+      test('removes trailing slash while preserving leading slash', () => {
+        expect(rs._urlJoin('/prefix/', 'file.js')).toBe('/prefix/file.js')
+      })
+    })
+
+    describe('leading slash removal', () => {
+      test('removes leading slash from non-first parts', () => {
+        expect(rs._urlJoin('prefix', '/deep', 'file.js')).toBe('prefix/deep/file.js')
+      })
+
+      test('removes leading slash from middle parts', () => {
+        expect(rs._urlJoin('prefix', '/deep', '/nested', 'file.js')).toBe('prefix/deep/nested/file.js')
+      })
+
+      test('preserves leading slash only on first part', () => {
+        expect(rs._urlJoin('/prefix', '/deep', '/file.js')).toBe('/prefix/deep/file.js')
+      })
+    })
+
+    describe('Windows backslash conversion', () => {
+      test('converts Windows backslashes to forward slashes', () => {
+        expect(rs._urlJoin('prefix\\deep', 'file.js')).toBe('prefix/deep/file.js')
+      })
+
+      test('converts backslashes in single part', () => {
+        expect(rs._urlJoin('prefix\\deep\\file.js')).toBe('prefix/deep/file.js')
+      })
+
+      test('converts backslashes in multiple parts', () => {
+        expect(rs._urlJoin('prefix\\deep', 'nested\\file.js')).toBe('prefix/deep/nested/file.js')
+      })
+
+      test('converts backslashes while preserving leading slash', () => {
+        expect(rs._urlJoin('/prefix\\deep', 'file.js')).toBe('/prefix/deep/file.js')
+      })
+
+      test('converts mixed backslashes and forward slashes', () => {
+        expect(rs._urlJoin('prefix\\deep', 'nested/file.js')).toBe('prefix/deep/nested/file.js')
+      })
+
+      test('handles Windows-style path with backslashes', () => {
+        expect(rs._urlJoin('fakeprefix\\deep\\dir', 'index.js')).toBe('fakeprefix/deep/dir/index.js')
+      })
+    })
+
+    describe('double slash removal', () => {
+      test('removes double slashes from joined path', () => {
+        expect(rs._urlJoin('prefix', '', 'file.js')).toBe('prefix/file.js')
+      })
+
+      test('removes multiple consecutive slashes', () => {
+        expect(rs._urlJoin('prefix//deep', 'file.js')).toBe('prefix/deep/file.js')
+      })
+
+      test('removes triple slashes', () => {
+        expect(rs._urlJoin('prefix///deep', 'file.js')).toBe('prefix/deep/file.js')
+      })
+
+      test('removes double slashes in middle of path', () => {
+        expect(rs._urlJoin('prefix', '//deep', 'file.js')).toBe('prefix/deep/file.js')
+      })
+
+      test('handles double slashes with leading slash', () => {
+        expect(rs._urlJoin('/prefix', '//deep', 'file.js')).toBe('/prefix/deep/file.js')
+      })
+
+      test('removes double slashes created by empty parts', () => {
+        expect(rs._urlJoin('prefix', '', '', 'file.js')).toBe('prefix/file.js')
+      })
+    })
+
+    describe('edge cases with slashes', () => {
+      test('handles part that is just a slash', () => {
+        expect(rs._urlJoin('prefix', '/', 'file.js')).toBe('prefix/file.js')
+      })
+
+      test('handles part that is just backslash', () => {
+        expect(rs._urlJoin('prefix', '\\', 'file.js')).toBe('prefix/file.js')
+      })
+
+      test('handles multiple slashes-only parts', () => {
+        expect(rs._urlJoin('prefix', '/', '/', 'file.js')).toBe('prefix/file.js')
+      })
+
+      test('handles leading slash with empty parts', () => {
+        expect(rs._urlJoin('/prefix', '', 'file.js')).toBe('/prefix/file.js')
+      })
+    })
+
+    describe('real-world S3 key scenarios', () => {
+      test('creates S3 key from prefix and filename', () => {
+        expect(rs._urlJoin('fakeprefix', 'index.js')).toBe('fakeprefix/index.js')
+      })
+
+      test('creates S3 key with nested directories', () => {
+        expect(rs._urlJoin('fakeprefix', 'deep', 'dir', 'index.js')).toBe('fakeprefix/deep/dir/index.js')
+      })
+
+      test('handles Windows-style prefix with trailing backslash', () => {
+        expect(rs._urlJoin('fakeprefix\\deep\\dir\\', 'index.js')).toBe('fakeprefix/deep/dir/index.js')
+      })
+
+      test('handles prefix with leading slash', () => {
+        expect(rs._urlJoin('/fakeprefix', 'index.js')).toBe('/fakeprefix/index.js')
+      })
+
+      test('handles relative directory path', () => {
+        expect(rs._urlJoin('deep', 'index.js')).toBe('deep/index.js')
+      })
+
+      test('handles empty prefix', () => {
+        expect(rs._urlJoin('', 'index.js')).toBe('index.js')
+      })
+
+      test('handles prefix with mixed separators', () => {
+        expect(rs._urlJoin('prefix\\deep', 'nested/file.js')).toBe('prefix/deep/nested/file.js')
+      })
+    })
+
+    describe('special characters and edge cases', () => {
+      test('handles file with no extension', () => {
+        expect(rs._urlJoin('prefix', 'file')).toBe('prefix/file')
+      })
+
+      test('handles file with multiple dots', () => {
+        expect(rs._urlJoin('prefix', 'file.min.js')).toBe('prefix/file.min.js')
+      })
+
+      test('handles path with spaces', () => {
+        expect(rs._urlJoin('prefix', 'file name.js')).toBe('prefix/file name.js')
+      })
+
+      test('handles path with special characters', () => {
+        expect(rs._urlJoin('prefix', 'file-name_123.js')).toBe('prefix/file-name_123.js')
+      })
+
+      test('handles very long path', () => {
+        const longPath = Array(10).fill('deep').join('/')
+        expect(rs._urlJoin('prefix', longPath, 'file.js')).toBe(`prefix/${longPath}/file.js`)
+      })
+
+      test('handles path starting with dot', () => {
+        expect(rs._urlJoin('prefix', '.hidden', 'file.js')).toBe('prefix/.hidden/file.js')
+      })
+
+      test('handles path with dot-dot', () => {
+        expect(rs._urlJoin('prefix', '..', 'file.js')).toBe('prefix/../file.js')
+      })
+    })
+
+    describe('empty and null handling', () => {
+      test('returns empty string for no arguments', () => {
+        expect(rs._urlJoin()).toBe('')
+      })
+
+      test('returns empty string for single empty string', () => {
+        expect(rs._urlJoin('')).toBe('')
+      })
+
+      test('filters out all empty parts', () => {
+        expect(rs._urlJoin('', '', '')).toBe('')
+      })
+
+      test('handles mix of valid and empty parts', () => {
+        expect(rs._urlJoin('', 'prefix', '', 'file.js', '')).toBe('prefix/file.js')
+      })
+
+      test('handles null in middle', () => {
+        expect(rs._urlJoin('prefix', null, 'file.js')).toBe('prefix/file.js')
+      })
+
+      test('handles undefined in middle', () => {
+        expect(rs._urlJoin('prefix', undefined, 'file.js')).toBe('prefix/file.js')
+      })
+    })
+
+    describe('complex combinations', () => {
+      test('combines all normalization features', () => {
+        expect(rs._urlJoin('/prefix\\deep/', '//nested/', '\\file.js')).toBe('/prefix/deep/nested/file.js')
+      })
+
+      test('handles Windows path with leading slash', () => {
+        expect(rs._urlJoin('/prefix\\deep\\dir', 'index.js')).toBe('/prefix/deep/dir/index.js')
+      })
+
+      test('handles multiple trailing slashes', () => {
+        expect(rs._urlJoin('prefix///', 'deep///', 'file.js')).toBe('prefix/deep/file.js')
+      })
+
+      test('handles mixed separators and slashes', () => {
+        expect(rs._urlJoin('prefix\\deep', '/nested', '\\file.js')).toBe('prefix/deep/nested/file.js')
+      })
+    })
+  })
+
   test('folderExists missing prefix', async () => {
     const rs = new RemoteStorage(global.fakeTVMResponse)
     await expect(rs.folderExists()).rejects.toEqual(expect.objectContaining({ message: 'prefix must be a valid string' }))
@@ -191,6 +454,20 @@ describe('RemoteStorage', () => {
     await expect(rs.uploadFile()).rejects.toEqual(expect.objectContaining({ message: 'prefix must be a valid string' }))
   })
 
+  test('uploadFile with windows path', async () => {
+    const rs = new RemoteStorage(global.fakeTVMResponse)
+    // Create file using platform-agnostic paths
+    await global.addFakeFiles(vol, 'fakeDir', { 'deep/index.js': 'fake content' })
+    // Use platform-specific path for file reading (must match actual file system)
+    const filePath = path.join('fakeDir', 'deep', 'index.js')
+    // Test that Windows-style backslashes in prefix are normalized correctly
+    const prefixPath = 'fakeprefix\\deep\\dir\\'
+    await expect(rs.uploadFile(filePath, prefixPath, global.fakeConfig, 'fakeDir')).resolves.toBeUndefined()
+    expect(mockS3.putObject).toHaveBeenCalledWith(
+      expect.objectContaining({ Bucket: 'fake-bucket', Key: 'fakeprefix/deep/dir/index.js' })
+    )
+  })
+
   test('uploadDir missing prefix', async () => {
     const rs = new RemoteStorage(global.fakeTVMResponse)
     await expect(rs.uploadDir()).rejects.toEqual(expect.objectContaining({ message: 'prefix must be a valid string' }))
@@ -634,21 +911,6 @@ describe('RemoteStorage', () => {
     expect(putObjectCall.Metadata).not.toHaveProperty('adp-cache-control')
   })
 
-  test('uploadFile includes auditUserId in metadata when set', async () => {
-    global.addFakeFiles(vol, 'fakeDir', { 'index.js': 'fake content' })
-    const rs = new RemoteStorage(global.fakeTVMResponse)
-    const fakeConfig = { ...global.fakeConfig, auditUserId: 'test-user-123' }
-    await rs.uploadFile('fakeDir/index.js', 'fakeprefix', fakeConfig, 'fakeDir')
-    const body = Buffer.from('fake content', 'utf8')
-    const expected = {
-      Bucket: 'fake-bucket',
-      Key: 'fakeprefix/index.js',
-      Body: body,
-      ContentType: 'application/javascript'
-    }
-    expect(mockS3.putObject).toHaveBeenCalledWith(expect.objectContaining(expected))
-  })
-
   test('uploadFile does not set Metadata when responseHeaders is empty', async () => {
     global.addFakeFiles(vol, 'fakeDir', { 'index.js': 'fake content' })
     const rs = new RemoteStorage(global.fakeTVMResponse)