refactor(webui): Rename S3Manager to StreamFilesS3Manager; namespace stream-output AWS env vars.#2195
Merged
junhaoliao merged 10 commits intomainfrom May 7, 2026
Merged
Conversation
…stream-output AWS env vars.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds explicit AWS credential support to the S3Manager component by introducing optional access key environment variables, extending the S3Manager constructor to accept credentials, renaming the Fastify decorator from ChangesS3 Credentials Configuration & Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/webui/server/src/plugins/app/S3Manager/index.ts`:
- Around line 85-92: The S3 client config silently omits credentials when only
one of accessKeyId/secretAccessKey is provided, causing unintended SDK
credential fallback; inside the S3Manager initialization (where s3ClientConfig
and variables accessKeyId, secretAccessKey, profile are used) add a validation
that if exactly one of accessKeyId or secretAccessKey is set you immediately
throw or return an error (fail fast) with a clear message about the partial
credential configuration, otherwise continue to set credentials when both are
present and keep the existing profile/region handling; reference s3ClientConfig,
accessKeyId, secretAccessKey, and profile when adding the guard so the check is
colocated with the current config assembly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f7ae897-3b39-4319-8e8b-6c8cc64413c5
📒 Files selected for processing (6)
components/webui/server/.envcomponents/webui/server/src/plugins/app/S3Manager/index.tscomponents/webui/server/src/plugins/external/env.tscomponents/webui/server/src/routes/api/stream-files/index.tstools/deployment/package-helm/templates/webui-deployment.yamltools/deployment/package/docker-compose-all.yaml
Comment on lines
+85
to
+92
| const s3ClientConfig: S3ClientConfig = { | ||
| ...((accessKeyId && secretAccessKey) ? | ||
| {credentials: {accessKeyId, secretAccessKey}} : | ||
| {}), | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| ...((null !== profile) && {profile}), | ||
| region, | ||
| }; |
Contributor
There was a problem hiding this comment.
Fail fast on partial credential config to avoid unintended identity fallback.
At Line 86, when only one of the two CLP_STREAM_OUTPUT_* values is set, credentials are silently omitted and SDK default credential resolution is used. That can generate pre-signed URLs with the wrong IAM identity and mask misconfiguration.
Proposed fix
const {
CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID: accessKeyId,
CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY: secretAccessKey,
} = fastify.config;
+ const hasAccessKeyId = "" !== accessKeyId;
+ const hasSecretAccessKey = "" !== secretAccessKey;
+ if (hasAccessKeyId !== hasSecretAccessKey) {
+ throw new Error(
+ "Both CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and " +
+ "CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY must be set together."
+ );
+ }
+
const s3ClientConfig: S3ClientConfig = {
- ...((accessKeyId && secretAccessKey) ?
+ ...((hasAccessKeyId && hasSecretAccessKey) ?
{credentials: {accessKeyId, secretAccessKey}} :
{}),
// eslint-disable-next-line `@typescript-eslint/no-unnecessary-condition`
...((null !== profile) && {profile}),
region,
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/webui/server/src/plugins/app/S3Manager/index.ts` around lines 85 -
92, The S3 client config silently omits credentials when only one of
accessKeyId/secretAccessKey is provided, causing unintended SDK credential
fallback; inside the S3Manager initialization (where s3ClientConfig and
variables accessKeyId, secretAccessKey, profile are used) add a validation that
if exactly one of accessKeyId or secretAccessKey is set you immediately throw or
return an error (fail fast) with a clear message about the partial credential
configuration, otherwise continue to set credentials when both are present and
keep the existing profile/region handling; reference s3ClientConfig,
accessKeyId, secretAccessKey, and profile when adding the guard so the check is
colocated with the current config assembly.
junhaoliao
commented
Apr 10, 2026
- Revert S3Manager class to original constructor (region, profile) with eager S3Client init; add optional credentials parameter. - Revert class docstring (not stream-files-specific). - controller.py already uses CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID (no changes needed).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/webui/server/src/plugins/app/S3Manager/index.ts (1)
84-97:⚠️ Potential issue | 🟠 MajorFail fast on partial stream-output credential config.
If only one of
CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID/CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEYis set, this silently falls back to the SDK default credential chain and can sign URLs with the wrong identity. Reject the partial config before deciding whichS3Managerconstructor path to take.Proposed fix
const { CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID: accessKeyId, CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY: secretAccessKey, } = fastify.config; + + const hasAccessKeyId = "" !== accessKeyId; + const hasSecretAccessKey = "" !== secretAccessKey; + if (hasAccessKeyId !== hasSecretAccessKey) { + throw new Error( + "Both CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and " + + "CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY must be set together." + ); + } fastify.log.info( {region, profile}, "Initializing StreamFilesS3Manager" ); fastify.decorate( "StreamFilesS3Manager", - (accessKeyId && secretAccessKey) ? + (hasAccessKeyId && hasSecretAccessKey) ? new S3Manager(region, profile, {accessKeyId, secretAccessKey}) : new S3Manager(region, profile) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/webui/server/src/plugins/app/S3Manager/index.ts` around lines 84 - 97, Detect and reject partial stream-output AWS credentials before constructing the S3Manager: check CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY (fastify.config) and if exactly one is set, throw or fail fast with a clear error (do not fall back to SDK defaults); only call new S3Manager(region, profile, {accessKeyId, secretAccessKey}) when both are present, otherwise call new S3Manager(region, profile) when neither is set. Update the initialization of the decorated "StreamFilesS3Manager" to perform this validation and error out on partial config to avoid accidental credential mixing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/webui/server/src/plugins/app/S3Manager/index.ts`:
- Around line 84-97: Detect and reject partial stream-output AWS credentials
before constructing the S3Manager: check CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and
CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY (fastify.config) and if exactly one is
set, throw or fail fast with a clear error (do not fall back to SDK defaults);
only call new S3Manager(region, profile, {accessKeyId, secretAccessKey}) when
both are present, otherwise call new S3Manager(region, profile) when neither is
set. Update the initialization of the decorated "StreamFilesS3Manager" to
perform this validation and error out on partial config to avoid accidental
credential mixing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8af4f02e-f66f-4d9c-9473-6e73d37431f1
📒 Files selected for processing (1)
components/webui/server/src/plugins/app/S3Manager/index.ts
junhaoliao
commented
Apr 10, 2026
junhaoliao
commented
Apr 10, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/webui/server/src/plugins/app/S3Manager/index.ts (1)
90-92:⚠️ Potential issue | 🟠 MajorFail fast on partial stream-output credentials (still unresolved).
Line 90 still treats partial config as “no credentials”, which can silently fall back to SDK default identity and generate URLs with the wrong IAM principal.
Proposed fix
- const credentials = (accessKeyId && secretAccessKey) ? - {accessKeyId, secretAccessKey} : - null; + const hasAccessKeyId = "" !== accessKeyId; + const hasSecretAccessKey = "" !== secretAccessKey; + if (hasAccessKeyId !== hasSecretAccessKey) { + throw new Error( + "Both CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and " + + "CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY must be set together." + ); + } + const credentials = (hasAccessKeyId && hasSecretAccessKey) ? + {accessKeyId, secretAccessKey} : + null;#!/bin/bash set -euo pipefail file="components/webui/server/src/plugins/app/S3Manager/index.ts" # Show the credential assembly block under review. nl -ba "$file" | sed -n '80,98p' # Verify whether a paired-credentials guard already exists. rg -n 'must be set together|hasAccessKeyId|hasSecretAccessKey' "$file" || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/webui/server/src/plugins/app/S3Manager/index.ts` around lines 90 - 92, The current credential assembly sets credentials to null when only one of accessKeyId or secretAccessKey is provided, which allows silent fallback to SDK defaults; update the logic around the credentials variable so it validates paired credentials: if both accessKeyId and secretAccessKey are present use {accessKeyId, secretAccessKey}, if neither are present keep null, but if exactly one is present throw an explicit error (or return a rejected Promise) with a clear message indicating both accessKeyId and secretAccessKey must be set together; locate the const credentials assignment and add the guard using accessKeyId and secretAccessKey to enforce the fail-fast behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/webui/server/src/plugins/app/S3Manager/index.ts`:
- Around line 90-92: The current credential assembly sets credentials to null
when only one of accessKeyId or secretAccessKey is provided, which allows silent
fallback to SDK defaults; update the logic around the credentials variable so it
validates paired credentials: if both accessKeyId and secretAccessKey are
present use {accessKeyId, secretAccessKey}, if neither are present keep null,
but if exactly one is present throw an explicit error (or return a rejected
Promise) with a clear message indicating both accessKeyId and secretAccessKey
must be set together; locate the const credentials assignment and add the guard
using accessKeyId and secretAccessKey to enforce the fail-fast behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d620cb9-45d8-48b1-bee8-af1914bd8e6d
📒 Files selected for processing (2)
components/webui/server/src/plugins/app/S3Manager/index.tstools/deployment/package-helm/Chart.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tools/deployment/package-helm/templates/webui-deployment.yaml`:
- Around line 64-67: Replace the direct env value injections for
CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY
with Kubernetes Secret references: change the env entries in
webui-deployment.yaml so each uses valueFrom.secretKeyRef with name pulled from
a Helm value (e.g. {{ .s3_config.kubernetes_secret_name | quote }}) and the
appropriate key (e.g. access_key_id and secret_access_key), ensuring the
template expects/uses .s3_config.kubernetes_secret_name (or similar) rather than
embedding raw credentials.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05fc6106-0804-4927-8068-4b5fba5bc21d
📒 Files selected for processing (1)
tools/deployment/package-helm/templates/webui-deployment.yaml
Comment on lines
+64
to
67
| - name: "CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID" | ||
| value: {{ .s3_config.aws_authentication.credentials.access_key_id | quote }} | ||
| - name: "AWS_SECRET_ACCESS_KEY" | ||
| - name: "CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY" | ||
| value: {{ .s3_config.aws_authentication.credentials.secret_access_key | quote }} |
Contributor
There was a problem hiding this comment.
Use secretKeyRef for AWS credentials instead of plain value injection.
Line 65 and Line 67 currently place credential material directly into the Deployment env spec. This weakens secret handling (manifest exposure, Helm history exposure, broader read surface). Please source these vars from a Kubernetes Secret.
🔐 Suggested direction (Helm template)
- - name: "CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID"
- value: {{ .s3_config.aws_authentication.credentials.access_key_id | quote }}
- - name: "CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY"
- value: {{ .s3_config.aws_authentication.credentials.secret_access_key | quote }}
+ - name: "CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID"
+ valueFrom:
+ secretKeyRef:
+ name: {{ include "clp.fullname" $ }}-stream-output-s3-credentials
+ key: "access_key_id"
+ - name: "CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY"
+ valueFrom:
+ secretKeyRef:
+ name: {{ include "clp.fullname" $ }}-stream-output-s3-credentials
+ key: "secret_access_key"🤖 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 `@tools/deployment/package-helm/templates/webui-deployment.yaml` around lines
64 - 67, Replace the direct env value injections for
CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY
with Kubernetes Secret references: change the env entries in
webui-deployment.yaml so each uses valueFrom.secretKeyRef with name pulled from
a Helm value (e.g. {{ .s3_config.kubernetes_secret_name | quote }}) and the
appropriate key (e.g. access_key_id and secret_access_key), ensuring the
template expects/uses .s3_config.kubernetes_secret_name (or similar) rather than
embedding raw credentials.
hoophalab
previously approved these changes
May 7, 2026
Member
Author
|
@hoophalab i updated the chart version in 09d5f5c |
hoophalab
approved these changes
May 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Extracted from the WIP prototype in #2169.
Renames the existing
S3ManagerFastify decorator toStreamFilesS3Managerto clarify its role(serving pre-signed URLs for stream files) and prepare for a second
LogsInputS3Managerdecoratorin a follow-up PR. Also namespaces the AWS credential env vars from the generic
AWS_ACCESS_KEY_ID/
AWS_SECRET_ACCESS_KEYtoCLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID/CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEYso they don't collide with futureCLP_LOGS_INPUT_AWS_*vars.
Key changes:
credentials: Nullable<AwsCredentialIdentity>parameter to theconstructor so explicit credentials can be passed (required because the AWS SDK default credential
chain no longer finds credentials under the renamed env var names). Settings values are now cast
to
Nullable<string>, removing the need for eslint-disable comments.FastifyInstance.S3Manager→FastifyInstance.StreamFilesS3Manager(interface + decorator + all references in
stream-filesroute).CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_IDandCLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEYwith empty defaults.AWS_ACCESS_KEY_ID→CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID(same for secret key) inside the webui service.
conditional block.
0.2.1-dev.7.Checklist
breaking change.
Validation performed
Scenario 1: TypeScript build
Task: Verify the renamed decorator and updated imports compile without errors.
Command:
Output:
Scenario 2: ESLint
Task: Verify no new lint violations are introduced.
Command:
Output:
Scenario 3: Verify decorator rename is complete
Task: Confirm no stale references to the old decorator name remain.
Command:
grep -rn '"S3Manager"' components/webui/server/src/Output:
Explanation: All references to the old
S3Managerdecorator have been updated toStreamFilesS3Manager.Summary by CodeRabbit
Release Notes
New Features
Chores