fix(frontend): sanitize pipeline code_source_url before rendering as link#100
Open
herikwebb wants to merge 16 commits into
Open
fix(frontend): sanitize pipeline code_source_url before rendering as link#100herikwebb wants to merge 16 commits into
herikwebb wants to merge 16 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>
promote-upstream.sh used GitHub's compare API only and never rebased, so a fix branch cut from the fork's default branch dragged the fork-only automation (pr-review.yml, this script, AGENTS.md, ...) into the upstream compare and tripped the fork-only guard — the same branch could not serve both the fork review PR and the upstream promotion. Add an optional FORK_BASE input: when set (e.g. master), replay only the commits the branch adds over that base onto the current upstream tip, push the result as a script-managed derived branch, and promote that. The review gate still keys off the original fork PR branch. Blank FORK_BASE preserves the previous promote-as-is behavior.
The derive step now cherry-picks with --signoff so the promoted commits carry a Signed-off-by trailer for the submitter identity, satisfying kubeflow/pipelines' DCO requirement without a manual re-sign. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
With more than one fix commit GitHub falls back to the branch name for the PR title and an empty description. Default the promotion PR title/body from the primary (first) fix commit instead of the branch tip, list the included commits, and append the sign-off / title-convention checklist. Add a body input to the workflow so it can be overridden. No squashing: upstream squash-merges anyway, so the PR title/body is what becomes the merge commit. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
…link The pipeline version "Version source" link renders a version's code_source_url straight into an anchor href in both PipelineVersionCard and PipelineDetailsV1. That value is stored verbatim from the pipeline / pipeline-version upload endpoints with no scheme validation, so a non-http(s) scheme (for example a javascript: URI) supplied at upload time is rendered as a clickable link and executes in the ml-pipeline-ui origin when a user opens the pipeline summary and clicks the link — stored cross-site scripting reachable by any user who can upload a pipeline version. Add a shared sanitizeExternalHref helper that returns a URL only when it uses the http/https scheme, use it at both render sites, and render the link only when a safe URL remains. This closes the injection at the sink and protects already-stored values, mirroring the existing scheme allowlist used for artifact links. Includes unit tests for the helper. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
Automated PR ReviewModel: Changed files: Review result: No automated findings were found in the changed code. Residual risks: |
Update the PipelineVersionCard fixture to carry a real https source URL so the "Version source" link renders and is asserted to point at that URL, and add a case verifying that an unsafe (javascript:) code_source_url renders no link at all. Locks in the scheme allowlist added to sanitizeExternalHref. Signed-off-by: Herik Webb <herikwebb@users.noreply.github.com>
Automated PR ReviewModel: Changed files: Review result: No automated findings were found. Residual risks: the URL allowlist is intentionally narrow ( |
herikwebb
force-pushed
the
master
branch
3 times, most recently
from
July 25, 2026 13:06
675041e to
8e96fb2
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.
What
The pipeline version "Version source" link renders a version's
code_source_urldirectly into an anchorhrefin bothPipelineVersionCardandPipelineDetailsV1. That value is stored verbatim from the pipeline / pipeline-version upload endpoints with no scheme validation, so a non-http(s)scheme supplied at upload time is rendered as a clickable link that runs in theml-pipeline-uiorigin when a user opens the pipeline summary and clicks it — a stored cross-site scripting issue reachable by any user who can upload a pipeline version.Change
sanitizeExternalHrefhelper (frontend/src/lib/Utils.tsx) that returns a URL only when it uses thehttp/httpsscheme.Testing
prettier --checkpasses on the changed files.typecheck/ Vitest were not run in this environment (nonode_modules); the change is a small, self-contained helper plus two render-site guards with new unit-test coverage.Tracking issue: #99
Coordinated disclosure: upstream
kubeflow/pipelinesrequests private reporting (GitHub Security Advisory /ksc@kubeflow.org); this fork PR is the review record and the upstream report will go through that private channel.Generated by Claude Code