fix(file-s3): preserve directory portion of filename in object key

[Tusharkhadde]

Fixes #16008

Summary

The S3 file provider was ignoring the directory portion of the supplied filename when constructing the object key. It only used the parsed name and ext, dropping any path structure (e.g. vendor_123/logo.png became logo-<ulid>.png).

This change updates the upload and getUploadStream methods to preserve the directory segment when generating the object key, resulting in keys like vendor_123/logo-<ulid>.png.

Changes

• Preserve parsedFilename.dir (with a trailing slash) when constructing the S3 object key.
• Apply the same change to both upload and getUploadStream.
• Updated file:
  • packages/modules/providers/file-s3/src/services/s3-file.ts

Behavior

Before

vendor_123/logo.png
→ logo-<ulid>.png

After

vendor_123/logo.png
→ vendor_123/logo-<ulid>.png

• Filenames with a directory now preserve the directory in the S3 object key.
• Filenames without a directory continue to behave as before.

Testing

• Added unit tests in packages/modules/providers/file-s3/src/services/__tests__/s3-file-url-encoding.spec.ts:
  • upload() with filename: "vendor_123/logo.png" → key matches ^public\/vendor_123\/logo-[^/]+\.png$.
  • upload() with filename: "logo.png" (no directory) → key matches ^public\/logo-[^/]+\.png$ (unchanged).
  • getUploadStream() with filename: "vendor_123/logo.png" → key matches ^public\/vendor_123\/logo-[^/]+\.png$.
• Verified via yarn workspace @medusajs/file-s3 jest src/services/__tests__/s3-file-url-encoding.spec.ts — all 7 tests pass.

Closes #16008

[changeset-bot]

🦋 Changeset detected

Latest commit: f76e6ec

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 79 packages

Name	Type
@medusajs/file-s3	Patch
@medusajs/medusa	Patch
@medusajs/test-utils	Patch
@medusajs/loyalty-plugin	Patch
@medusajs/medusa-oas-cli	Patch
integration-tests-http	Patch
@medusajs/analytics	Patch
@medusajs/api-key	Patch
@medusajs/auth	Patch
@medusajs/caching	Patch
@medusajs/cart	Patch
@medusajs/currency	Patch
@medusajs/customer	Patch
@medusajs/file	Patch
@medusajs/fulfillment	Patch
@medusajs/index	Patch
@medusajs/inventory	Patch
@medusajs/link-modules	Patch
@medusajs/locking	Patch
@medusajs/notification	Patch
@medusajs/order	Patch
@medusajs/payment	Patch
@medusajs/pricing	Patch
@medusajs/product	Patch
@medusajs/promotion	Patch
@medusajs/rbac	Patch
@medusajs/region	Patch
@medusajs/sales-channel	Patch
@medusajs/settings	Patch
@medusajs/stock-location	Patch
@medusajs/store	Patch
@medusajs/tax	Patch
@medusajs/translation	Patch
@medusajs/user	Patch
@medusajs/workflow-engine-inmemory	Patch
@medusajs/workflow-engine-redis	Patch
@medusajs/draft-order	Patch
@medusajs/oas-github-ci	Patch
@medusajs/cache-inmemory	Patch
@medusajs/cache-redis	Patch
@medusajs/event-bus-local	Patch
@medusajs/event-bus-redis	Patch
@medusajs/analytics-local	Patch
@medusajs/analytics-posthog	Patch
@medusajs/auth-emailpass	Patch
@medusajs/auth-github	Patch
@medusajs/auth-google	Patch
@medusajs/caching-redis	Patch
@medusajs/file-local	Patch
@medusajs/fulfillment-manual	Patch
@medusajs/locking-postgres	Patch
@medusajs/locking-redis	Patch
@medusajs/notification-local	Patch
@medusajs/notification-sendgrid	Patch
@medusajs/payment-stripe	Patch
@medusajs/core-flows	Patch
@medusajs/framework	Patch
@medusajs/js-sdk	Patch
@medusajs/modules-sdk	Patch
@medusajs/orchestration	Patch
@medusajs/query	Patch
@medusajs/types	Patch
@medusajs/utils	Patch
@medusajs/workflows-sdk	Patch
@medusajs/http-types-generator	Patch
@medusajs/cli	Patch
@medusajs/deps	Patch
@medusajs/eslint-plugin	Patch
@medusajs/telemetry	Patch
@medusajs/admin-bundler	Patch
@medusajs/admin-sdk	Patch
@medusajs/admin-shared	Patch
@medusajs/admin-vite-plugin	Patch
@medusajs/dashboard	Patch
@medusajs/icons	Patch
@medusajs/toolbox	Patch
@medusajs/ui-preset	Patch
create-medusa-app	Patch
@medusajs/ui	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[medusa-os-bot]

Thanks for the contribution! A few items need to be addressed before this can move forward:

Correct, minimal fix that preserves the directory segment of a filename in the S3 object key, matching linked issue #16008; changeset is present and well-formatted, and the returned key keeps delete/download roundtrips working. Two gaps before approval: the PR description omits the template's Testing section, and no test was added even though this provider already has unit tests for key/URL generation.

• Add a unit test for directory preservation (e.g. "vendor_123/logo.png" -> "vendor_123/logo-<ulid>.png", and no-dir unchanged) in src/services/tests/.
• Fill in the Testing section of the PR template describing how the change was verified.

Triggered by: new PR opened

[medusa-os-bot]

Thanks for the contribution! We need more information before reviewing this further.

Re-review: the prior request for a unit test is resolved — the diff now adds dir-preservation and no-dir tests for both upload() and getUploadStream(). The code fix is correct and minimal, matching linked issue #16008. One item from the previous review remains: the PR description still omits the template's Testing section. Non-blocking nit: two duplicate changeset files bump @medusajs/file-s3 for the same fix; consider keeping one.

Triggered by: new commit pushed

[medusa-os-bot]

Thanks for the contribution! We need more information before reviewing this further.

Re-review: the code fix is correct and minimal, preserving the directory segment of the filename in the S3 object key for both upload() and getUploadStream(), matching linked issue #16008. The prior request for unit tests is resolved (dir-preservation and no-dir cases added), and the duplicate changeset nit is resolved (one changeset remains). One item still outstanding: the PR description does not include the template's Testing section. No blocking bugs, security, or performance issues found.

Triggered by: new commit pushed

[medusa-os-bot]

Thanks for the contribution! Initial automated review looks good.

Correct, minimal fix preserving the filename's directory segment in the S3 object key for both upload() and getUploadStream(), matching linked issue #16008. All prior review items are resolved: unit tests for dir-preservation and no-dir cases were added, the duplicate changeset was removed, and the PR description now includes the Testing section. No bugs, security, or performance concerns found; S3 keys are a flat literal namespace so no path-traversal risk.

Triggered by: PR description updated

[shahednasser]

Thank you for your contribution @Tusharkhadde . There are a couple of issues to point out with this approach:

1. This approach doesn't handle cases where the path starts with / like /vendor/logo.png. The path would be public//vendor/logo-{suffix}.png
2. The approach doesn't handle cases where the path starts with. like ./vendor/logo.png. the path would include a /./ segment
3. Similar to the above, the directory path isn't sanitized, which could allow for path traversal like ../secret/logo.png. It must be sanitized to ensure there are no path traversals and remove those

[Tusharkhadde]

Thanks for the detailed review @shahednasser ! I've addressed the path sanitization concerns by handling leading slashes, relative path segments (./), and preventing path traversal (..). I also added tests covering these cases. Please let me know if there's anything else that should be improved.

[shahednasser]

@medusajs-bot review

[shahednasser]

@medusajs-bot review

[shahednasser]

@medusajs-bot review

[pevey]

A suggestion for a small additional change:

A pure traversal filename like '../..' will reslt in '' after sanitization. This will leave just the prefix as the filekey. Not sure what the implications of that are. Could it overwrite?

In any case, probably best to outright reject if filename is '' after sanitization.

[shahednasser]

@medusajs-bot review

[medusa-os-bot]

Thanks for the contribution! A few items need to be addressed before this can move forward:

The PR proposes to preserve the directory segment of file.filename when constructing the S3 object key. The feature intent is sound, but the implementation passes parsedFilename.dir (user-controlled) into the S3 Key without sanitizing '..' or leading '/' segments, introducing a path traversal vulnerability. An attacker with upload access could escape the configured prefix and write objects to unintended S3 paths. A changeset for the file-s3 package should also be included if not already present in the diff.

• s3-file.ts upload()+getUploadStream(): S3 key path traversal — parsedFilename.dir from user-supplied filename reaches S3 Key with no '..'/'/' sanitization. Fix: strip or reject '..' and leading '/' from the directory segment before building the key.

Triggered by: manual review request via comment

[Tusharkhadde]

Thanks for the feedback, @pevey!

I've made the requested changes to handle the edge cases you mentioned. If a user-provided filename becomes empty after sanitization (for example, ../.., ./., ., or /), the upload provider now rejects it by throwing a MedusaError with the INVALID_DATA type and the message Invalid filename: <filename>.

I added this validation to all three upload methods:

• upload()
• getUploadStream()
• getPresignedUploadUrl()

I also added unit tests for these invalid filename cases across all three methods, and everything is passing locally.

Please let me know if you notice anything else that should be improved. Thanks again for the review!