feat: S3 credential resolution via APOS_S3_SECRET or AWS default chain#256
feat: S3 credential resolution via APOS_S3_SECRET or AWS default chain#256
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors S3 storage setup in the uploadfs module by introducing a centralized Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
🔍 Vulnerabilities of
|
| digest | sha256:dc84cab89f5072faf6a38c45155443e7784c48f27585834c1727cd06d8ca2e4e |
| vulnerabilities | |
| platform | linux/amd64 |
| size | 173 MB |
| packages | 971 |
📦 Base Image node:24-alpine
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@website/modules/`@apostrophecms/uploadfs/index.js:
- Around line 18-21: The current check uses aposS3Secret !== undefined which
will treat an empty string as valid and configure uploadfsOptions.secret and
attempt getEnv('APOS_S3_KEY'); change the guard to require a non-empty value
(e.g., a truthy string) before setting uploadfsOptions.secret and calling
getEnv('APOS_S3_KEY') so empty APOS_S3_SECRET does not trigger credential
configuration; locate the conditional around aposS3Secret and update it to
verify the value is not empty.
- Line 11: The `https` property is being set to the raw string returned by
getEnv('APOS_S3_HTTPS'), which yields `"true"`/`"false"` and will be truthy even
when `"false"`; change the assignment so the value is converted to a real
boolean before being passed to uploadfs (e.g., replace the direct getEnv call
for the `https` property with a boolean conversion that treats case-insensitive
"true" as true and anything else as false, or call a shared helper like
parseBoolean/getEnvBoolean), referencing the `https` property and the
getEnv('APOS_S3_HTTPS') usage to locate where to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dcf9d6fa-b1d9-4f82-bf22-dbc6c86e8cf7
📒 Files selected for processing (1)
website/modules/@apostrophecms/uploadfs/index.js
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@website/modules/`@apostrophecms/uploadfs/index.js:
- Line 12: The https property currently sets https: aposS3Https === 'true' which
is case-sensitive and treats 'TRUE', 'True', or '1' as false; update the check
for aposS3Https (the variable used to set the https option in
website/modules/@apostrophecms/uploadfs/index.js) to perform a case-insensitive
and/or multi-value truthy check (e.g. normalize to string and compare to 'true'
case-insensitively, or check against a set like ['1','true','yes']) so that
common truthy representations are accepted.
- Around line 19-22: The current logic sets uploadfsOptions.secret when
aposS3Secret is present but grabs the key via getEnv('APOS_S3_KEY') separately,
which yields a confusing `"APOS_S3_KEY" is not defined` message when the secret
is set but the key is missing; modify the block that checks aposS3Secret to
validate both credentials together: when aposS3Secret is truthy, first attempt
to read APOS_S3_KEY (via getEnv('APOS_S3_KEY')) and if it's missing throw or log
a clear error that both APOS_S3_SECRET and APOS_S3_KEY are required together,
otherwise set uploadfsOptions.secret = aposS3Secret and uploadfsOptions.key =
the fetched key so the failure message indicates the paired requirement (refer
to aposS3Secret, uploadfsOptions.secret, uploadfsOptions.key, and
getEnv('APOS_S3_KEY')).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70e7b82d-5dac-460a-9b8a-bc0cdefccae4
📒 Files selected for processing (1)
website/modules/@apostrophecms/uploadfs/index.js
feat: S3 credential resolution via APOS_S3_SECRET or AWS default chain
Refactors S3 credential configuration in the uploadfs module by introducing a centralized uploadfsOptions object populated from environment variables. It sets HTTPS flag, CDN settings, bucket/region/endpoint/style, and conditionally adds secret and key when APOS_S3_SECRET is present. Credentials otherwise rely on the AWS default credential chain via omission of inline keys for uploads.