fix(frontend): enforce domain allowlist on s3/minio artifact endpoint#98
Open
herikwebb wants to merge 13 commits into
Open
fix(frontend): enforce domain allowlist on s3/minio artifact endpoint#98herikwebb wants to merge 13 commits into
herikwebb wants to merge 13 commits into
Conversation
Signed-off-by: Herik Webb <herik@shardplate.local>
Signed-off-by: Herik Webb <herik@shardplate.local>
Add a pull request watch policy so that any agent (including scheduled routines) subscribes to a PR immediately after opening it, handing it to the persistent watcher for review-feedback and CI follow-up until the PR is merged or closed. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
…tter Codify that every commit must be authored, committed, and signed off by the human submitter rather than an agent identity. Automated environments default the git identity to the agent (e.g. Claude <noreply@anthropic.com>), so a bare 'git commit -s' signs off as the agent and breaks the DCO requirement. Document setting the repo-local identity to the submitter, verifying author and sign-off before pushing, and reusing the submitter identity for follow-up commits on a PR they already authored. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
gpt-5.5 is a reasoning model whose reasoning tokens count against max_output_tokens, so the previous 3000 budget was intermittently exhausted before any review text was produced, failing the review job with 'OpenAI API response did not include review text'. Raise the default to 16000 and make it overridable via the OPENAI_MAX_OUTPUT_TOKENS repository variable. Also export the variable in review-pr.sh so the python child process actually inherits it — previously it was set without export, so the child always used its own hardcoded default regardless of the shell value. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
Raising the token budget means a review can exceed GitHub's 65536-character comment body limit, which would make 'gh pr comment' fail after a successful model call. Truncate the body (with margin) and append a notice when it is too large instead of failing to post the review. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
`resolveFilePathOnVolume` in `frontend/server/utils.ts` composed the final filesystem path with `path.join(volumeMountPath, filePathInVolume)`, which collapses `..` segments. `getArtifactsHandler` in `frontend/server/handlers/artifacts.ts` validates the `bucket` query param with `isAllowedResourceName` but only length-caps the `key` param and hands it to `getVolumeArtifactsHandler` → `findFileOnPodVolume` → `resolveFilePathOnVolume` unchanged. Combined with a well-known volume name mounted on the ml-pipeline-ui pod (for example `config-volume` from `manifests/kustomize/base/pipeline/ml-pipeline-ui-deployment.yaml`), an authenticated Kubeflow user could pass `?source=volume&bucket=config-volume&key=../../var/run/secrets/kubernetes.io/serviceaccount/token` through the artifacts auth middleware — which only checks that they can access their own namespace — and stream the ml-pipeline-ui pod's service account token (or any other file the pod's filesystem contains). Normalize the joined path with `path.resolve` and require that it stays under the resolved `volumeMountPath` (equal to it or under `volumeMountPath + '/'`). This blocks both `../` escapes when no `subPath` is set and `subPath + '/../../'` escapes when `subPath` is configured. Add two `resolveFilePathOnVolume` regression tests covering both variants. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
…eads The lexical path.resolve containment check in resolveFilePathOnVolume stops '..' traversal but not a symlink inside the mounted volume pointing outside it (e.g. 'link -> /etc/passwd'), which fs.createReadStream would follow at read time. Add resolveRealPathWithinMount, which resolves the real (symlink-free) path immediately before the read and re-verifies it stays within the volume mount, minimizing the TOCTOU window by returning the fully resolved path to open. findFileOnPodVolume now also returns the resolved volume mount path so the read site can perform this check; the extra tuple element is backward compatible with existing two-element destructures. getVolumeArtifactsHandler calls the resolver before stat/createReadStream and reads the returned safe path. When the path cannot be resolved (missing file) the original path is returned so existing not-found behavior is preserved. Extract a shared isPathInsideMount helper used by both the lexical and the realpath containment checks. Add resolveRealPathWithinMount tests covering an in-mount file, a symlink escaping the mount, and an unresolvable path. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
The previous fix resolved the real path and re-checked containment, but the handler then reopened that path with stat()/createReadStream(). An attacker able to write the volume could swap the file for a symlink between the check and the open, and both calls follow symlinks, so the read could still escape. Replace resolveRealPathWithinMount with openRealFileWithinMount, which opens the file once, verifies the opened descriptor's real target (via /proc/self/fd on Linux, falling back to path resolution elsewhere) is inside the mount, and returns the handle. getVolumeArtifactsHandler streams from that same handle instead of reopening by name, so validation and read act on one fd and cannot be raced. Open errors (missing file) still propagate to preserve the existing not-found behavior. Update tests to cover the open-and-validate handle, symlink rejection, and missing-file propagation. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
Match the notebook-intelligence PR reviewer: - Trigger on pull_request instead of pull_request_target and check out the PR head directly (fork PRs get a read-only token and no secrets, so running the diff-only reviewer on the checked-out head is safe and simpler). - Emit a machine-readable VERDICT (APPROVE / CHANGES_REQUESTED) and fail the check on a non-APPROVE verdict, so a green 'review' check means the reviewer signed off. This is what an automated promotion can key off. - Add an 'override-review-gate' label escape hatch that relaxes only the exit status; the review still runs and comments. - Use a three-dot (merge-base) diff range with a two-dot fallback on shallow clones, so the reviewer sees what the PR introduces on merge. Keeps the fork's OPENAI_MAX_OUTPUT_TOKENS=16000 override: gpt-5.5 is a reasoning model and 3000 was easily exhausted, producing empty responses that would now fail the review gate. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
Add a workflow_dispatch action + scripts/promote-upstream.sh that opens a cross-fork PR from a chosen fork branch into kubeflow/pipelines, using a human PAT (secrets.UPSTREAM_PAT) since the default GITHUB_TOKEN cannot write upstream. It resolves the upstream default branch, refuses empty promotions, guards against leaking fork-only automation files into the upstream diff, and by default requires the branch's fork PR review gate to be green before promoting (override with require_review_pass=false). Change detection uses GitHub's server-side compare API so it is independent of local clone depth. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
The ml-pipeline-ui artifact endpoint accepts an object-store endpoint via the `providerInfo` query parameter. The http/https artifact sources are constrained by the ALLOWED_ARTIFACT_DOMAIN_REGEX allowlist, but the s3/minio provider endpoint was never checked, so a request could direct the server's outbound object-store fetch at an arbitrary host and port and have the response streamed back — a server-side request forgery reachable without authentication in the default standalone deployment. Subject the provider-supplied endpoint to the same isAllowedDomain check already applied to http/https sources, rejecting endpoints outside the configured allowlist before any client is constructed. The default regex remains permissive, so existing deployments are unaffected; operators who restrict the allowlist are now protected consistently across all artifact sources. Adds a regression test covering the rejected case.
herikwebb
added a commit
that referenced
this pull request
Jul 21, 2026
Automated PR ReviewModel: Changed files: Review result: No automated findings were found for the changed code. Residual risks: the effectiveness of the new SSRF guard still depends on the behavior and operator configuration of |
… SSRF guard Extend the artifact endpoint allowlist regression tests per review feedback: add a minio-source rejection case (the guard covers source=minio as well as s3), and an allowed-endpoint case asserting a restrictive allowlist still permits the operator's own object store and constructs the client. Closes the coverage gap noted in the automated review of the s3/minio SSRF fix.
Owner
Author
|
Thanks for the review. Addressed the test-coverage residual risk in e37831f: added a The other residual note — effectiveness depending on Generated by Claude Code |
Automated PR ReviewModel: Changed files: Review result: No automated findings were found in the changed code. Residual risks: the effectiveness of the SSRF guard depends on the existing |
herikwebb
force-pushed
the
master
branch
5 times, most recently
from
July 26, 2026 13:08
8e96fb2 to
a68c3e7
Compare
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
1 participant
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.
Summary
Closes the SSRF path tracked in #97. The
ml-pipeline-ui/artifacts/gethandler accepts an object-store endpoint via theproviderInfoquery parameter forsource=s3andsource=minio. Thehttp/httpssources are gated byisAllowedDomain(url, ALLOWED_ARTIFACT_DOMAIN_REGEX), but the s3/minio provider endpoint was never checked, so a request could direct the server's outbound object-store fetch at an arbitraryhost:port(including in-cluster services and169.254.169.254) and have the response streamed back — a full-read server-side request forgery, unauthenticated in the default standalone deployment.This subjects the provider-supplied endpoint to the same
isAllowedDomaincheck already applied to http/https sources, rejecting endpoints outside the configured allowlist with400before any client is constructed.Why this is safe / minimal
ALLOWED_ARTIFACT_DOMAIN_REGEX(^.*$) remains permissive, so configured deployments are unaffected; operators who restrict the allowlist are now protected consistently across every artifact source rather than having the s3/minio path silently bypass it.frontend/server/handlers/artifacts.ts(the guard) plus a regression test. No refactors, no dependency changes.Testing
frontend/server:tsc --noEmitclean, Prettier clean,artifact-get/artifacts/artifact-authsuites pass (79 tests).Follow-up
Per
kubeflow/pipelinesSECURITY.md, upstream disclosure should be coordinated privately (GitHub Security Advisory onkubeflow/pipelinesorksc@kubeflow.org). This PR targets the fork's syncedmaster; once reviewed, the change is intended to be promoted upstream via that private channel.Tracking issue: #97
Generated by Claude Code