fix(s3): trim whitespace in bucket name + creds so uploads can't break on a stray space by NiallJoeMaher · Pull Request #1338 · codu-code/codu

NiallJoeMaher · 2026-06-17T14:31:07Z

Problem

Uploads were totally broken on production. Root cause: a trailing space in the prod S3_BUCKET_NAME env var. Every presigned PUT targeted the invalid bucket name "codu.uploads ", so S3 returned 400 InvalidBucketName.

The space is invisible in the Vercel UI, and S3_BUCKET_NAME / ACCESS_KEY / SECRET_KEY aren't in config/env.js validation — so the bad value failed silently at runtime instead of at boot. The bucket, CORS, public-read policy and IAM creds were all verified healthy; only the env value was bad.

Fix

Trim these values at the point of use so stray whitespace on any of them can't break uploads:

getPresignedUrl.ts → Bucket: process.env.S3_BUCKET_NAME?.trim()
utils/s3helpers.ts → trim ACCESS_KEY / SECRET_KEY before they reach the SigV4 credential

Test

server/common/getPresignedUrl.test.ts — fails without the trim (URL came out as .../codu.uploads%20/... with X-Amz-Credential=...%20), passes with it. Full unit suite green.

Note

This is defence-in-depth — the live fix is also to retype the prod env value with no whitespace and redeploy. Follow-up worth considering: add the three S3 vars to config/env.js so bad values fail at boot.

🤖 Generated with Claude Code

…k on a stray space A trailing space in the prod S3_BUCKET_NAME env var made every presigned PUT target the invalid bucket "codu.uploads " -> S3 returned 400 InvalidBucketName, taking uploads down entirely. The whitespace is invisible in the Vercel UI, and S3_BUCKET_NAME/ACCESS_KEY/SECRET_KEY aren't in config/env.js validation, so the bad value failed silently at runtime instead of at boot. Trim these values at the point of use (getPresignedUrl bucket name, s3 client credentials) so stray whitespace on any of them can't break signing or the PUT. Adds a regression test that fails without the trim (URL came out as .../codu.uploads%20/...) and passes with it. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

vercel · 2026-06-17T14:31:14Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
codu	Ready	Preview, Comment	Jun 17, 2026 2:37pm

coderabbitai · 2026-06-17T14:31:28Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c61fe2db-a4d8-4f98-947a-55504ab80819

📥 Commits

Reviewing files that changed from the base of the PR and between 40af115 and 510b821.

📒 Files selected for processing (2)

server/common/getPresignedUrl.test.ts
server/common/getPresignedUrl.ts

💤 Files with no reviewable changes (1)

server/common/getPresignedUrl.ts

🚧 Files skipped from review as they are similar to previous changes (1)

server/common/getPresignedUrl.test.ts

Walkthrough

Whitespace trimming is added to ACCESS_KEY and SECRET_KEY environment variables in s3Client construction, and to S3_BUCKET_NAME in PutObjectCommand inside getPresignedUrl. A Vitest suite is added to verify both trims via dynamically imported module behavior.

Changes

S3 env var whitespace hardening

Layer / File(s)	Summary
Credential and bucket name trimming `utils/s3helpers.ts`, `server/common/getPresignedUrl.ts`	`s3Client` trims `ACCESS_KEY` and `SECRET_KEY` before evaluating credential presence and constructing the `credentials` object. `getPresignedUrl` trims `S3_BUCKET_NAME` before passing it to `PutObjectCommand`, with inline comments explaining the reason.
Regression tests `server/common/getPresignedUrl.test.ts`	New Vitest suite sets whitespace-padded env vars, dynamically imports `getPresignedUrl` in a `beforeAll` hook, and asserts the presigned URL contains no URL-encoded spaces in the bucket path or `X-Amz-Credential` query parameter. Cleanup removes modified env vars in `afterAll`.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A sneaky space hid in the key,
The bucket rejected with glee.
I trimmed left and right,
Now S3 signs just right—
No %20 shall trouble thee! ✂️

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: trimming whitespace from S3 bucket name and credentials to prevent upload failures caused by stray spaces.
Description check	✅ Passed	The description is comprehensive and well-structured, covering the problem, root cause, fix details, and testing approach. However, the 'Fixes #(issue)' field is missing, and 'Breaking changes' and 'Screenshots' sections are not included per the template.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/s3-env-whitespace-hardening

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@fix

- restore process.env in afterAll so the suite doesn't lean on neighbours - remove the misleading `// @fix TS ERROR` comment (no actual error) Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

server/common/getPresignedUrl.test.ts (1)

8-8: ⚡ Quick win

Replace any with proper typing.

Using any disables type checking for getPresignedUrl. Consider using the actual type from the import or typeof:

let getPresignedUrl: typeof import("`@/server/common/getPresignedUrl`").getPresignedUrl;

Or simply:

let getPresignedUrl: (
  fileType: string,
  fileSize: number,
  config: { kind: string; userId?: string }
) => Promise<string>;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/common/getPresignedUrl.test.ts` at line 8, The `getPresignedUrl`
variable is declared with the `any` type, which disables TypeScript's type
checking. Replace the `any` type annotation with a proper type by either using
`typeof` to reference the actual function type from the getPresignedUrl module
import, or by explicitly defining the function signature with its parameters
(fileType: string, fileSize: number, config object) and return type
(Promise<string>). This will restore type safety and catch potential type errors
during development.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@server/common/getPresignedUrl.test.ts`:
- Around line 10-16: Add an afterAll hook to restore the environment variables
that are modified during test setup. Store the original values of
process.env.ACCESS_KEY, process.env.SECRET_KEY, and process.env.S3_BUCKET_NAME
before the beforeAll hook modifies them, then restore those original values (or
delete the variables if they were not originally set) in the new afterAll hook
to prevent test pollution.

---

Nitpick comments:
In `@server/common/getPresignedUrl.test.ts`:
- Line 8: The `getPresignedUrl` variable is declared with the `any` type, which
disables TypeScript's type checking. Replace the `any` type annotation with a
proper type by either using `typeof` to reference the actual function type from
the getPresignedUrl module import, or by explicitly defining the function
signature with its parameters (fileType: string, fileSize: number, config
object) and return type (Promise<string>). This will restore type safety and
catch potential type errors during development.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 9213f097-192a-4635-96ee-4c3e79a5b862

📥 Commits

Reviewing files that changed from the base of the PR and between 80bbc3d and 40af115.

📒 Files selected for processing (3)

server/common/getPresignedUrl.test.ts
server/common/getPresignedUrl.ts
utils/s3helpers.ts

coderabbitai · 2026-06-17T14:35:54Z

+  beforeAll(async () => {
+    // Whitespace-padded credentials, set before import so the s3 client (built
+    // at module load) sees them.
+    process.env.ACCESS_KEY = "AKIATESTKEY1234567890 ";
+    process.env.SECRET_KEY = "secretsecretsecretsecretsecretsecret1234\n";
+    ({ getPresignedUrl } = await import("@/server/common/getPresignedUrl"));
+  });


🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add cleanup to restore environment variables.

The beforeAll hook modifies process.env.ACCESS_KEY, process.env.SECRET_KEY, and the tests modify process.env.S3_BUCKET_NAME, but these changes are never reverted. This can pollute other tests if Vitest runs them in the same process or if other tests depend on these variables.

🧹 Add cleanup with afterAll

describe("getPresignedUrl whitespace hardening", () => { // eslint-disable-next-line `@typescript-eslint/no-explicit-any` let getPresignedUrl: any; + const originalEnv = { + ACCESS_KEY: process.env.ACCESS_KEY, + SECRET_KEY: process.env.SECRET_KEY, + S3_BUCKET_NAME: process.env.S3_BUCKET_NAME, + }; beforeAll(async () => { // Whitespace-padded credentials, set before import so the s3 client (built // at module load) sees them. process.env.ACCESS_KEY = "AKIATESTKEY1234567890 "; process.env.SECRET_KEY = "secretsecretsecretsecretsecretsecret1234\n"; ({ getPresignedUrl } = await import("`@/server/common/getPresignedUrl`")); }); + + afterAll(() => { + process.env.ACCESS_KEY = originalEnv.ACCESS_KEY; + process.env.SECRET_KEY = originalEnv.SECRET_KEY; + process.env.S3_BUCKET_NAME = originalEnv.S3_BUCKET_NAME; + });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/common/getPresignedUrl.test.ts` around lines 10 - 16, Add an afterAll hook to restore the environment variables that are modified during test setup. Store the original values of process.env.ACCESS_KEY, process.env.SECRET_KEY, and process.env.S3_BUCKET_NAME before the beforeAll hook modifies them, then restore those original values (or delete the variables if they were not originally set) in the new afterAll hook to prevent test pollution.

NiallJoeMaher requested a review from a team as a code owner June 17, 2026 14:31

NiallJoeMaher had a problem deploying to production June 17, 2026 14:32 — with GitHub Actions Error

vercel Bot deployed to Preview June 17, 2026 14:33 View deployment

chore(review): self-contained test cleanup + drop stale @fix comment

510b821

- restore process.env in afterAll so the suite doesn't lean on neighbours - remove the misleading `// @fix TS ERROR` comment (no actual error) Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

coderabbitai Bot reviewed Jun 17, 2026

View reviewed changes

NiallJoeMaher had a problem deploying to production June 17, 2026 14:36 — with GitHub Actions Error

vercel Bot deployed to Preview June 17, 2026 14:37 View deployment

NiallJoeMaher merged commit e8922ee into develop Jun 17, 2026
5 of 6 checks passed

NiallJoeMaher deleted the fix/s3-env-whitespace-hardening branch June 17, 2026 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(s3): trim whitespace in bucket name + creds so uploads can't break on a stray space#1338

fix(s3): trim whitespace in bucket name + creds so uploads can't break on a stray space#1338
NiallJoeMaher merged 2 commits into
developfrom
fix/s3-env-whitespace-hardening

NiallJoeMaher commented Jun 17, 2026

Uh oh!

vercel Bot commented Jun 17, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 17, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jun 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

NiallJoeMaher commented Jun 17, 2026

Problem

Fix

Test

Note

Uh oh!

vercel Bot commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel Bot commented Jun 17, 2026 •

edited

Loading

coderabbitai Bot commented Jun 17, 2026 •

edited

Loading