Skip to content

refactor(webui): Rename S3Manager to StreamFilesS3Manager; namespace stream-output AWS env vars. #2195

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
junhaoliao wants to merge 5 commits into
base: main
Choose a base branch
from
Open

refactor(webui): Rename S3Manager to StreamFilesS3Manager; namespace stream-output AWS env vars. #2195

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce8123a
refactor(webui): Rename S3Manager to StreamFilesS3Manager; namespace …
junhaoliao Apr 10, 2026
fae8ce8
fixup: Address review feedback on PR #2195.
junhaoliao Apr 10, 2026
5c7bef4
fixup: Use Nullable<AwsCredentialIdentity> for credentials; bump char…
junhaoliao Apr 10, 2026
09d6dc6
fixup: Cast settings to Nullable<string>; remove eslint-disable.
junhaoliao Apr 10, 2026
065a864
Merge branch 'main' into webui-s3manager-refactor
junhaoliao Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions components/webui/server/.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,7 @@ PRESTO_SCHEMA=default
# Security
RATE_LIMIT=1000

# S3
CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID=
CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY=

41 changes: 29 additions & 12 deletions components/webui/server/src/plugins/app/S3Manager/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
GetObjectCommand,
S3Client,
S3ClientConfig,
} from "@aws-sdk/client-s3";
import {getSignedUrl} from "@aws-sdk/s3-request-presigner";
import {Nullable} from "@webui/common/utility-types";
Expand All @@ -11,20 +12,18 @@ import {PRE_SIGNED_URL_EXPIRY_TIME_SECONDS} from "./typings.js";


/**
* Class to manage Simple Storage Service (S3) objects.
* Class to manage Simple Storage Service (S3) objects for stream files.
*/
class S3Manager {
readonly #s3Client;
#s3Client: Nullable<S3Client> = null;

readonly #s3ClientConfig: S3ClientConfig;

/**
* @param region
* @param [profile]
* @param config
*/
constructor (region: string, profile: Nullable<string>) {
this.#s3Client = new S3Client({
region,
...((null !== profile) && {profile}),
});
constructor (config: S3ClientConfig) {
this.#s3ClientConfig = config;
}

/**
Expand All @@ -35,6 +34,10 @@ class S3Manager {
* @throws {Error} If a pre-signed URL couldn't be generated.
*/
async getPreSignedUrl (s3UriString: string): Promise<string> {
if (null === this.#s3Client) {
this.#s3Client = new S3Client(this.#s3ClientConfig);
}

const s3Uri = new URL(s3UriString);
const command = new GetObjectCommand({
Bucket: s3Uri.hostname,
Expand All @@ -60,7 +63,7 @@ class S3Manager {

declare module "fastify" {
interface FastifyInstance {
S3Manager?: S3Manager;
StreamFilesS3Manager?: S3Manager;
}
}

Expand All @@ -74,11 +77,25 @@ export default fp(
// values are not hardcoded.
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
if (null !== region && "" !== region) {
const {
CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID: accessKeyId,
CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY: secretAccessKey,
} = fastify.config;

const s3ClientConfig: S3ClientConfig = {
...((accessKeyId && secretAccessKey) ?
{credentials: {accessKeyId, secretAccessKey}} :
{}),
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
...((null !== profile) && {profile}),
region,
};
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


fastify.log.info(
{region, profile},
"Initializing S3Manager"
"Initializing StreamFilesS3Manager"
);
fastify.decorate("S3Manager", new S3Manager(region, profile));
fastify.decorate("StreamFilesS3Manager", new S3Manager(s3ClientConfig));
}
},
);
12 changes: 12 additions & 0 deletions components/webui/server/src/plugins/external/env.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ declare module "fastify" {
USER: string;
CLP_DB_USER: string;
CLP_DB_PASS: string;
CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID: string;
CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY: string;
PRESTO_CATALOG: string;
PRESTO_SCHEMA: string;
RATE_LIMIT: number;
Expand Down Expand Up @@ -56,6 +58,16 @@ const schema = {
type: "string",
},

// S3
CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID: {
type: "string",
default: "",
},
CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY: {
type: "string",
default: "",
},

// Presto
PRESTO_CATALOG: {
type: "string",
Expand Down
5 changes: 3 additions & 2 deletions components/webui/server/src/routes/api/stream-files/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,9 @@ const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
}
}

if (fastify.hasDecorator("S3Manager") && "undefined" !== typeof fastify.S3Manager) {
streamMetadata.path = await fastify.S3Manager.getPreSignedUrl(
if (fastify.hasDecorator("StreamFilesS3Manager") &&
"undefined" !== typeof fastify.StreamFilesS3Manager) {
streamMetadata.path = await fastify.StreamFilesS3Manager.getPreSignedUrl(
`s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}`
);
} else {
Expand Down
4 changes: 2 additions & 2 deletions tools/deployment/package-helm/templates/webui-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ spec:
value: {{ .Values.clpConfig.webui.rate_limit | quote }}
{{- with .Values.clpConfig.stream_output.storage }}
{{- if and (eq .type "s3") (eq .s3_config.aws_authentication.type "credentials") }}
- name: "AWS_ACCESS_KEY_ID"
- 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 }}
{{- end }}{{/* if and (eq .type "s3")
(eq .s3_config.aws_authentication.type "credentials") */}}
Expand Down
4 changes: 2 additions & 2 deletions tools/deployment/package/docker-compose-all.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,8 +366,8 @@ services:
<<: *service_defaults
hostname: "webui"
environment:
AWS_ACCESS_KEY_ID: "${CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID:-}"
AWS_SECRET_ACCESS_KEY: "${CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY:-}"
CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID: "${CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID:-}"
CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY: "${CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY:-}"
CLP_DB_PASS: "${CLP_DB_PASS:?Please set a value.}"
CLP_DB_USER: "${CLP_DB_USER:-clp-user}"
HOST: "0.0.0.0"
Expand Down
Loading