fix(frontend): scope archived pod-log reads to the authorized namespace

[herikwebb]

Summary

The ml-pipeline-ui pod-logs handler authorizes access against the caller-supplied podnamespace, but the archive fallback (getPodLogsStreamFromArchive) derives the object-store key entirely from the caller-supplied podname/createdat via the configured keyFormat. The shipped default keyFormat is not namespace-scoped, so in multi-user mode the namespace access check does not constrain which archived log is returned, and an authenticated tenant could read another tenant's archived pod logs from the shared bucket.

Change

In createPodLogsMinioRequestConfig, fail closed when auth is enabled:

• Require a non-empty authorized namespace and require the keyFormat to embed {{workflow.namespace}}, so the resolved key stays confined to the authorized namespace prefix. If the key template is not namespace-scoped, refuse the archive read instead of serving an unscoped key.
• Reject any .. path segment in the resolved key so a caller-controlled podname cannot climb out of the namespace prefix (the existing regex allows . and /).

Behavior is unchanged when auth is disabled (single-user / standalone).

Tests

Adds frontend/server unit tests for the namespace-scoped key, both fail-closed paths, and the .. rejection. workflow-helper vitest suite passes; tsc --noEmit and Prettier are clean on the changed files.

Tracking / disclosure

Tracking issue: #103

Per kubeflow/pipelines SECURITY.md, coordinated disclosure should go through the upstream private channel (a GitHub Security Advisory on kubeflow/pipelines or ksc@kubeflow.org); this fork PR is the review/tracking record and is not for merge here.

---

Generated by Claude Code

[github-actions]

Automated PR Review

Model: gpt-5.5
Verdict: CHANGES_REQUESTED

Changed files:

frontend/server/handlers/pod-logs.ts
frontend/server/workflow-helper.test.ts
frontend/server/workflow-helper.ts

Review result:

Automated review findings

High — frontend/server/workflow-helper.ts

Namespace scoping check only requires {{workflow.namespace}} to appear, not that it forms an unambiguous namespace prefix/path segment.

The new multi-user guard rejects key formats that omit {{workflow.namespace}}, but a key format can still pass while not being safely scoped to the authorized namespace. For example, formats that concatenate namespace with another attacker-controlled field or use a non-boundary delimiter can collide with another namespace’s archived log key:

artifacts/{{workflow.namespace}}-{{pod.name}}
artifacts/{{workflow.namespace}}{{pod.name}}

Because podName is supplied by the request and interpolated into the key, a tenant in namespace foo may be able to construct keys that overlap with logs under namespace-like prefixes such as foo-bar, depending on the configured keyFormat.

The comment says the key must stay confined to the authorized prefix, but key.includes('{{workflow.namespace}}') does not enforce a prefix boundary. Consider requiring the namespace placeholder to be a complete path segment, e.g. matching (^|/){{workflow.namespace}}(/|$), and rejecting key formats where it is adjacent to other dynamic/static characters. Add tests for adjacent/concatenated namespace templates.

[herikwebb]

Thanks — good catch. Fixed in f3c65fe.

includes('{{workflow.namespace}}') was too weak; a keyFormat placing the namespace tag adjacent to the caller-controlled {{pod.name}} (e.g. {{workflow.namespace}}{{pod.name}} or {{workflow.namespace}}-{{pod.name}}) would pass while letting a tenant in foo reach keys under foobar/foo-bar. The guard now requires {{workflow.namespace}} to be a complete /-delimited path segment, matching (^|/){{workflow.namespace}}(/|$).

Added tests for the concatenated and delimiter-adjacent templates (both rejected) and for a trailing namespace segment (accepted). workflow-helper vitest (18 tests), tsc --noEmit, and Prettier are all clean.

---

Generated by Claude Code

[github-actions]

Automated PR Review

Model: gpt-5.5
Verdict: CHANGES_REQUESTED

Changed files:

frontend/server/handlers/pod-logs.ts
frontend/server/workflow-helper.test.ts
frontend/server/workflow-helper.ts

Review result:

Automated review findings

• Severity: High
  Files: frontend/server/handlers/pod-logs.ts, frontend/server/workflow-helper.ts
  Issue: Namespace scoping is enforced only for the final configured archive fallback, not for the workflow-status archive path.
  Rationale: In multi-user mode the handler now passes authEnabled into createPodLogsMinioRequestConfig, which rejects key formats that are not scoped by {{workflow.namespace}}. However, composePodLogsStreamHandler still tries getPodLogsStreamFromWorkflow before that fallback. getPodLogsStreamFromWorkflow trusts the main-logs artifact s3.key from workflow status and reads it using the server/shared object-store credentials, without validating that the key is confined to the authorized namespace. If a tenant can influence the workflow artifact key or artifact repository configuration for their namespace, they can potentially point the workflow-status log artifact at another namespace’s archived log object and bypass the new fail-closed check entirely.
  Suggested fix: Apply equivalent namespace-scoping validation to workflow-derived logKey when authEnabled is true, or pass authEnabled through and refuse workflow-status log retrieval unless the resolved key is confined to the authorized namespace. Add a regression test covering the handler/workflow-status fallback path with an unscoped or cross-namespace artifact key.

[herikwebb]

Good catch — fixed in 7b2133d.

You're right that composePodLogsStreamHandler tries getPodLogsStreamFromWorkflow before the configured archive fallback, and that path read the main-logs s3.key from the workflow status with the shared object-store credentials without confining it to the authorized namespace. A tenant able to shape their namespace's artifact-repositories keyFormat could point the recorded key at another namespace's object and bypass the fallback guard.

authEnabled is now threaded into getPodLogsMinioRequestConfigfromWorkflow, which in multi-user mode requires the workflow-recorded log key to contain the authorized namespace as a complete /-delimited path segment (and rejects ..), mirroring the archive-fallback check. The handler binds authEnabled where it composes the workflow-status stream. Added regression tests for an unscoped/cross-namespace key (rejected) and a namespace-scoped key (accepted). 20 workflow-helper tests pass; tsc --noEmit and Prettier are clean.

---

Generated by Claude Code

[github-actions]

Automated PR Review

Model: gpt-5.5
Verdict: CHANGES_REQUESTED

Changed files:

frontend/server/handlers/pod-logs.ts
frontend/server/workflow-helper.test.ts
frontend/server/workflow-helper.ts

Review result:

Automated findings

High — frontend/server/workflow-helper.ts

The new multi-user confinement check for workflow-recorded log keys only verifies that the authorized namespace appears anywhere as a /-delimited segment:

const segments = logKey.split('/');
if (!namespace || !segments.includes(namespace) || segments.includes('..')) { ... }

This does not prove the object key is actually scoped to that namespace. A malicious or compromised workflow/artifact repository configuration could point main-logs at another namespace’s object whose key happens to contain the authorized namespace as another segment, such as a workflow name or pod-derived segment. The same weak “namespace can be any segment” assumption is also codified for configured archive key formats, including a test that accepts the namespace as a trailing segment.

Because this path reads from the object store using shared/server credentials, the check should validate a deterministic namespace-scoped prefix or otherwise recompute/verify the key from a trusted template where {{workflow.namespace}} appears before any user-controlled fields. Merely requiring segments.includes(namespace) is not sufficient isolation for multi-user mode.

[herikwebb]

Agreed — presence-as-a-segment doesn't prove scoping. Fixed in f9c2651.

Added a shared namespaceScopedKeyPrefix(keyFormat, namespace) helper that returns a deterministic prefix only when {{workflow.namespace}} is a complete /-delimited segment ahead of every caller-influenced field ({{pod.name}}, {{workflow.name}}, {{workflow.creationTimestamp.*}}), and null otherwise. Both paths now use it:

• createPodLogsMinioRequestConfig fails closed unless the config keyFormat yields such a prefix.
• getPodLogsMinioRequestConfigfromWorkflow now receives the trusted operator keyFormat (from ARGO_KEYFORMAT, not the tenant-influenced per-namespace configmap) and requires the workflow-recorded key to startWith the derived <static>/<namespace>/ prefix — so a tenant-shaped artifact key that merely contains the namespace as a later/coincidental segment is rejected.

Test updates: a template placing a caller field before the namespace tag (including a trailing-segment namespace) is now rejected; added coverage for a bounded prefix segment, the workflow-status prefix match, and fail-closed when the trusted keyFormat isn't namespace-scoped. 22 tests pass; tsc --noEmit and Prettier clean.

Note on the workflow-status path: it now validates against the operator's global ARGO_KEYFORMAT. Deployments whose per-namespace archive layout diverges from the global keyFormat will fail closed on that path (the configured archive fallback still applies) — a deliberate, security-correct tradeoff, flagged for maintainer input.

---

Generated by Claude Code

[github-actions]

Automated PR Review

Model: gpt-5.5
Verdict: APPROVE

Changed files:

frontend/server/handlers/pod-logs.ts
frontend/server/workflow-helper.test.ts
frontend/server/workflow-helper.ts

Review result:

No automated findings were found in the changed code.

Residual risks: the new namespace-scoping checks are security-sensitive and depend on the configured keyFormat accurately representing the archive layout; deployments with custom artifact repository behavior should be validated in integration tests to ensure archived logs remain accessible only as intended.