fix: enhance S3 configuration with region fallback and improved logging#40182
fix: enhance S3 configuration with region fallback and improved logging#40182abbiekuma wants to merge 2 commits intoRocketChat:developfrom
Conversation
- Trimmed whitespace from S3 region and bucket URL settings. - Added fallback to 'us-east-1' for custom endpoints if region is not set. - Introduced logging for fallback scenario. - Updated settings to include description for S3 region.
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughTrimmed S3 settings values and changed region resolution: use the trimmed Changes
Sequence Diagram(s)sequenceDiagram
participant AdminSettings
participant ConfigLoader
participant SystemLogger
participant S3Client
AdminSettings->>ConfigLoader: provide FileUpload_S3_Region, FileUpload_S3_BucketURL
ConfigLoader->>ConfigLoader: trim Region and BucketURL
alt trimmed Region non-empty
ConfigLoader->>S3Client: set region = trimmed Region, endpoint = trimmed BucketURL
else trimmed Region empty and trimmed BucketURL present
ConfigLoader->>SystemLogger: info("using us-east-1 for custom endpoint")
ConfigLoader->>S3Client: set region = "us-east-1", endpoint = trimmed BucketURL
else neither present
ConfigLoader->>S3Client: set region = trimmed Region (empty) / default behavior
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
abbiekuma
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/settings/file-upload.ts (1)
160-167:⚠️ Potential issue | 🟡 MinorDefault region now applies to all AWS S3 setups, not just custom endpoints.
Changing the package default from
''to'us-east-1'affects fresh installs using real AWS S3 as well, not only custom endpoints (R2/MinIO). For AWS users whose bucket is in another region, the client will now be initialized withus-east-1instead of being left unset. This should be transparently handled byfollowRegionRedirects: trueinAmazonS3.ts, but it's a subtle behavior change worth confirming — in particular, the fallback/logging branch inAmazonS3.ts(lines 95-102) is now effectively dead for fresh installs becausetrimmedRegionwill be'us-east-1'by default and theelse if (trimmedBucketURL)branch won't be reached unless the admin explicitly clears the region field.If the intent is to only fall back for custom endpoints, consider keeping the package default as
''and relying solely on the runtime fallback inAmazonS3.ts. Otherwise, the current approach is fine but the info log will rarely fire.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/settings/file-upload.ts` around lines 160 - 167, The change sets the FileUpload_S3_Region default to 'us-east-1', which causes trimmedRegion to be non-empty and prevents the custom-endpoint fallback in AmazonS3.ts from running; either revert the package default for FileUpload_S3_Region back to '' so real AWS installs without a chosen region still trigger the runtime fallback in AmazonS3 (keep symbol FileUpload_S3_Region), or if you want 'us-east-1' as the package default, update AmazonS3.ts logic (the trimmedRegion / trimmedBucketURL checks and the followRegionRedirects behavior) to treat the default value as unset for the purpose of triggering the custom-endpoint fallback and logging (adjust the code paths referencing trimmedRegion, trimmedBucketURL, and followRegionRedirects accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/server/settings/file-upload.ts`:
- Around line 160-167: The change sets the FileUpload_S3_Region default to
'us-east-1', which causes trimmedRegion to be non-empty and prevents the
custom-endpoint fallback in AmazonS3.ts from running; either revert the package
default for FileUpload_S3_Region back to '' so real AWS installs without a
chosen region still trigger the runtime fallback in AmazonS3 (keep symbol
FileUpload_S3_Region), or if you want 'us-east-1' as the package default, update
AmazonS3.ts logic (the trimmedRegion / trimmedBucketURL checks and the
followRegionRedirects behavior) to treat the default value as unset for the
purpose of triggering the custom-endpoint fallback and logging (adjust the code
paths referencing trimmedRegion, trimmedBucketURL, and followRegionRedirects
accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c08dd38d-ca46-4390-b43f-0530d6a1f663
📒 Files selected for processing (3)
apps/meteor/app/file-upload/server/config/AmazonS3.tsapps/meteor/server/settings/file-upload.tspackages/i18n/src/locales/en.i18n.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/settings/file-upload.tsapps/meteor/app/file-upload/server/config/AmazonS3.ts
🧠 Learnings (5)
📚 Learning: 2026-04-10T22:42:05.539Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 40075
File: apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx:69-71
Timestamp: 2026-04-10T22:42:05.539Z
Learning: In `apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx`, the submit handler converts an empty/whitespace-only description to `undefined` (`description?.trim() || undefined`) intentionally. All downstream image-rendering components (`AttachmentImage`, `ImagePreview`, `ImageItem`, `ImageGallery`) default `undefined` alt to `''`, so the `<img alt="">` attribute is always present. Do not flag this `undefined` conversion as a bug preventing alt text from being cleared.
Applied to files:
apps/meteor/server/settings/file-upload.tsapps/meteor/app/file-upload/server/config/AmazonS3.ts
📚 Learning: 2026-03-12T17:12:49.121Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:49.121Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
Applied to files:
apps/meteor/server/settings/file-upload.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/settings/file-upload.tsapps/meteor/app/file-upload/server/config/AmazonS3.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/settings/file-upload.tsapps/meteor/app/file-upload/server/config/AmazonS3.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.json
🔇 Additional comments (2)
packages/i18n/src/locales/en.i18n.json (1)
2333-2333: Looks good — clear and correctly scoped i18n guidance.The new description is concise and gives the exact expected values for MinIO, Cloudflare R2, and Amazon S3, matching the PR intent.
Based on learnings: new translation keys should be added to
packages/i18n/src/locales/en.i18n.jsononly.apps/meteor/app/file-upload/server/config/AmazonS3.ts (1)
94-102: The fallback branch is reachable for existing installations.The new default
'us-east-1'applies only to new settings; theadd()method preserves existing stored values without modification. Existing installations with an empty-stringFileUpload_S3_Regionwill continue to havetrimmedRegion = ''(falsy), making theelse if (trimmedBucketURL)fallback reachable as intended. This provides the upgrade safety net you intended.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/settings/file-upload.ts">
<violation number="1" location="apps/meteor/server/settings/file-upload.ts:160">
P2: Removing the default S3 region can leave standard AWS S3 configs without any resolved region, causing AWS SDK v3 request/signing failures when the region setting is left blank.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
abbiekuma
force-pushed
the
fix/issue-40098
branch
from
cd50070 to
89a2ddc
Compare
…ucket URLs - Modified the S3 region setting to accept an empty value for custom bucket URLs. - Updated the description for the S3 region to clarify usage for different storage options.
abbiekuma
changed the title
I've reverted the package default for FileUpload_S3_Region to ''. Runtime handling (including the region fallback when using a custom Bucket URL) stays in AmazonS3.ts. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
Summary
After Rocket.Chat switched the S3 integration to AWS SDK v3, uploads to custom S3-compatible endpoints (notably Cloudflare R2) started failing with “Region is missing” because the client always needs a region for signing, but when Region was left empty the app did not pass a region into the S3 client (and AWS_DEFAULT_REGION did not help in this setup), whereas SDK v2 had been more forgiving; the practical workaround was to set R2’s region to auto.
Proposed changes
issue-40098-after.mov
Issue
Closes #40098
Steps to test or reproduce
Summary by CodeRabbit
New Features
Bug Fixes