Conversation
✅ Deploy Preview for ohif-dev canceled.
|
Viewers
|
||||||||||||||||||||||||||||
| Project |
Viewers
|
| Branch Review |
feat/build-using-tarball
|
| Run status |
|
| Run duration | 02m 18s |
| Commit |
|
| Committer | Bill Wallace |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
37
|
| View all changes introduced in this branch ↗︎ | |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| { | ||
| "cornerstonejs": { | ||
| "path": "@cornerstonejs", | ||
| "url": "https://github.com/cornerstonejs/cornerstone3D.git", | ||
| "branch": "feat/publish-tarballs", | ||
| "directory": "cornerstone3D" | ||
| } | ||
| } |
There was a problem hiding this comment.
Feature branch hardcoded in externals.json shipped to master
libs/externals.json is committed with a specific, in-progress feature branch (feat/publish-tarballs) from the CS3D repo. This file is read by scripts/checkout-libs-for-netlify.mjs to clone the CS3D repo into libs/@cornerstonejs on every CI/Netlify build.
Once this PR is merged to master, every standard build of OHIF will clone that feature branch. If the branch is later deleted or rebased, all master builds will break with a git clone failure.
If externals.json is intended to be a transient per-integration-branch artifact (like cs3d-integration.json), it should either be reset to an empty object {} before merge, or checkout-libs-for-netlify.mjs should treat a missing/empty file as a no-op (which it already does) and the committed file should reflect that state.
| { | ||
| "mode": "integration-only", | ||
| "cs3dPr": 2648, | ||
| "releaseTag": "cs3d-pr-2648-d5bfc4c", | ||
| "cs3dRepo": "cornerstonejs/cornerstone3D" | ||
| } |
There was a problem hiding this comment.
Integration metadata for a specific CS3D PR is checked in to the base branch
This file records integration state for CS3D PR #2648 with a specific release tag (cs3d-pr-2648-d5bfc4c). Per the CS3D_INTEGRATION.md documentation, this file is written by the automation and is expected to be a per-integration-branch artifact.
If this file is merged to master, verify-cs3d-integration-diff.mjs will fail on the next automation run because the metadata will describe a stale integration. More critically, all the @cornerstonejs/* deps in package.json and the lockfiles still point to tarballs from that CS3D PR, meaning master would depend on a non-published pre-release build of cornerstone3D permanently.
This file (along with the tarball URL changes in all package.json files) should be reverted to the stable semver versions before merging to master.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: "20" | ||
| - name: Block merge if package.json has CS3D tarball refs | ||
| run: node .github/scripts/check-no-cs3d-tarball-deps.mjs |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
In general, the fix is to explicitly add a permissions block to the workflow or job so that the GITHUB_TOKEN has only the minimal required rights. For this job, it only needs to read repository contents to run the Node script on checked-out code, so contents: read is sufficient.
The best minimal change without altering existing behavior is to add a job-level permissions section under jobs.check (indented to align with runs-on), setting contents: read. actions/checkout works with read-only contents permissions for typical use cases, and no other steps require broader scopes. No imports or additional methods are needed; this is a pure YAML configuration change within .github/workflows/block-merge-cs3d-tarball-deps.yml.
Concretely: in .github/workflows/block-merge-cs3d-tarball-deps.yml, under jobs:, inside the check: job, insert:
permissions:
contents: readat the same indentation depth as runs-on, just before or after runs-on.
| @@ -12,6 +12,8 @@ | ||
| jobs: | ||
| check: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 |
1 participant
This PR changes the build scripts to allow pushing the results of a CS3D build into an OHIF build.
It currently links with:
cornerstonejs/cornerstone3D#2648
to demonstrate this capability, but will need to be pushed/created to finalize this work.
There is no functional change, with only a single logging change in StackViewport used to test the deploy preview.
See the README.md files for details in this and in the CS3D PR.
Context
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Greptile Summary
This PR introduces an automated OHIF ↔ CS3D integration pipeline that consumes tarball release assets from CS3D GitHub releases and rewrites
@cornerstonejs/*dependency references across the monorepo to point at those tarballs. It also adds local development tooling (worktree setup, lib linking, Netlify checkout) and a GitHub Actions workflow that creates/updates integration branches and PRs from incomingrepository_dispatchevents.Key changes and concerns:
.github/workflows/ohif-cs3d-integration.yml): TheCS3D_TRUSTED_REPOenvironment variable passed to the security-verification step is sourced directly fromgithub.event.client_payload.source_repository— the same untrusted value used to populate the dependency URLs. This makes the trust check circular and bypassable by anyone who can send arepository_dispatchevent..github/scripts/update-cs3d-deps-from-assets.mjs): The TSV asset-list input path has two problems: a dead-code ternary that always produces0, and an inverted header-detection condition that either silently drops the first asset or adds the header row text as an asset name, causing all rewriting to be skipped in the--assets <tsv-file>usage path..github/cs3d-integration.json,libs/externals.json, all modifiedpackage.jsonfiles, and lockfiles): These files record integration state for CS3D PR feat: Add headers to prevent preflight requests #2648, pin all@cornerstonejs/*packages to non-published tarball URLs, and reference an in-progress feature branch inexternals.json. The PR description notes this is intentional for demonstration purposes and must not be merged tomasterin its current form.scripts/worktree-cs3d.mjs,scripts/link-cornerstone-libs.mjs, etc.) are well-structured;link-cornerstone-libs.mjscovers only a hardcoded set ofnode_modulesroots and may miss extension-level hoisted installs.Confidence Score: 1/5
.github/workflows/ohif-cs3d-integration.yml(security issue),.github/scripts/update-cs3d-deps-from-assets.mjs(logic bug),.github/cs3d-integration.jsonandlibs/externals.json(should not be merged to master), all modifiedpackage.jsonand lockfiles (tarball URLs must be reverted to semver before merging).Important Files Changed
Sequence Diagram
sequenceDiagram participant CS3D as CS3D Repo participant GHA as GitHub Actions<br/>(ohif-cs3d-integration.yml) participant Script1 as update-cs3d-deps-<br/>from-assets.mjs participant Script2 as verify-cs3d-<br/>integration-diff.mjs participant OHIF as OHIF Repo<br/>(bot/cs3d-pr-N branch) CS3D->>GHA: repository_dispatch<br/>{event: cs3d_integration_requested,<br/>payload: {release_tag, source_repository,<br/>cs3d_pr_number, mode}} GHA->>GHA: checkout default branch<br/>create/reuse bot/cs3d-pr-N GHA->>Script1: node update-cs3d-deps-from-assets.mjs<br/>--release-tag --repo (from payload) Script1->>CS3D: GET /releases/tags/{release_tag}<br/>(GitHub API, Bearer token) CS3D-->>Script1: Release asset list (tgz filenames) Script1->>Script1: Map asset names → @cornerstonejs/* packages<br/>Rewrite package.json deps to tarball URLs<br/>Write .github/cs3d-integration.json Script1-->>GHA: Updated package.json files GHA->>GHA: yarn install (regenerate lockfile) GHA->>Script2: node verify-cs3d-integration-diff.mjs<br/>CS3D_TRUSTED_REPO=payload.source_repository ⚠️ Script2->>Script2: Check only allowed files changed<br/>Check @cornerstonejs URLs point to trusted repo Script2-->>GHA: Pass / Fail GHA->>OHIF: git commit + push bot/cs3d-pr-N GHA->>OHIF: gh pr create/update PRComments Outside Diff (3)
.github/workflows/ohif-cs3d-integration.yml, line 612-613 (link)Circular trust:
CS3D_TRUSTED_REPOsourced from the untrusted payloadBoth the dependency-update step and the verification step receive
source_repositoryfromgithub.event.client_payload. The update script writes tarball URLs using that repo, then the verify script trusts URLs from that same repo (CS3D_TRUSTED_REPO: ${{ github.event.client_payload.source_repository }}).A malicious actor who can send a
repository_dispatchevent to this repo (anyone with write access) could setsource_repository: "attacker/evil-packages"and the workflow would:@cornerstonejs/*deps to point athttps://github.com/attacker/evil-packages/releases/...The fix is to hard-code the trusted repository or source it from a repository variable/secret, not from the payload:
The same fix applies to the
merged-updatejob (line ~700)..github/scripts/update-cs3d-deps-from-assets.mjs, line 219-224 (link)Inverted TSV header condition drops first asset or adds header text as asset name
There are two bugs in the TSV-input parsing path:
idxis always0– the ternaryfirst.includes('\t') ? 0 : 0evaluates to0in both branches; the condition has no effect.Inverted header guard –
lines.slice(1)already skips the first line (treating it as a header). The subsequent check tries to addlines[0]back if it doesn't start withcornerstonejs-. The logic is backwards:lines[0]IS a header (e.g."name"): condition istrue, so the header text"name"is pushed intonamesas an asset → no matching package is ever found, all assets are silently ignored.lines[0]IS the first asset (no header, e.g."cornerstonejs-core-4.18.2.tgz"): condition isfalse, so that first asset is silently dropped.The condition should be inverted:
scripts/install-cs3d-from-pr.mjs, line 1813-1828 (link)Pagination cap of 100 releases may miss the target release on active repos
fetchLatestReleaseForPrfetches only?per_page=100releases without any pagination. On a busy repo, if more than 100 releases exist and the target PR's release is older than the 100 most recent ones, the script will silently report "No release found" even though the release exists.Consider adding a note in the error message, or implement pagination using a
Link: <...>; rel="next"header approach if this is expected to be used on repos with many releases.Last reviewed commit: 3e39ee5