feat(instrument-repos): add GitHub-hosted instrument repositories#1383
Merged
Conversation
Admins can import instruments from a GitHub repository, sync to pull in new instruments, view a repo's instrument list, and delete repos. Group managers assign repos to groups and opt-in to individual instruments. Backend: - InstrumentRepo model + module/service/controller; instrument provenance tracked via Instrument.sourceRepoId/sourceRepoName - private-repo support via an AES-256-GCM encrypted access token - re-adding an existing repo re-syncs it (and updates a supplied token) instead of failing; repo name is no longer unique (url is the identity) - orphan reconciliation when a source repo is deleted - groups only auto-connect non-repo instruments on creation Frontend: - admin instrument-repos page (add/sync/view/delete) and repo assignment in admin groups; instrument preview in group management - repo mutations invalidate the instrument list; opt-out flag added to the axios error interceptor so callers can show a single specific error toast Tests: unit coverage for the repo service (crypto round-trip, secret stripping, delete guards, create/sync/dedup, orphan reconciliation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds full support for GitHub-backed instrument repositories. Includes new ChangesInstrument Repositories Feature
Sequence Diagram(s)sequenceDiagram
participant Admin
participant InstrumentReposPage
participant InstrumentReposService
participant GitHub
participant InstrumentsService
participant MongoDB
Admin->>InstrumentReposPage: Submit GitHub URL + PAT
InstrumentReposPage->>InstrumentReposService: POST /v1/instrument-repos
InstrumentReposService->>InstrumentReposService: parseRepoUrl, encryptToken(AES-256-GCM)
InstrumentReposService->>MongoDB: upsert InstrumentRepo by url
InstrumentReposService->>GitHub: GET tarball (main → master fallback)
GitHub-->>InstrumentReposService: tarball stream
InstrumentReposService->>InstrumentReposService: extract dirs, bundle instruments
InstrumentReposService->>InstrumentsService: create(bundledInstrument)
InstrumentsService-->>InstrumentReposService: instrumentId (or existing id on conflict)
InstrumentReposService->>MongoDB: tagInstruments(sourceRepoId, sourceRepoName)
InstrumentReposService->>MongoDB: reconcileOrphanedInstruments
InstrumentReposService-->>InstrumentReposPage: InstrumentRepo (accessToken stripped)
InstrumentReposPage->>InstrumentReposPage: invalidate instrument-repos + instrument-info cache
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/groups/groups.service.ts (1)
67-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the incoming name for duplicate checks in
updateById.Line 77 checks
group.nameinstead of the requesteddata.name, which can incorrectly throwConflictExceptionon rename attempts. Compare against the proposed name and skip when unchanged.Suggested fix
- const exists = typeof data.name === 'string' && (await this.groupModel.exists({ name: group.name })); + const nextName = typeof data.name === 'string' ? data.name : undefined; + const exists = + typeof nextName === 'string' && + nextName !== group.name && + (await this.groupModel.exists({ name: nextName })); if (exists) { - throw new ConflictException(`Group with name '${group.name}' already exists!`); + throw new ConflictException(`Group with name '${nextName}' already exists!`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/groups/groups.service.ts` around lines 67 - 80, The duplicate name check in the `updateById` method is comparing the current group's name against the database instead of the incoming requested name from `data.name`. This causes the method to incorrectly throw a ConflictException on rename attempts even when no actual conflict exists. Fix this by updating the exists check to use `data.name` instead of `group.name`, and add an additional condition to skip the duplicate check entirely when the incoming name matches the current group's name (to avoid false positives when no rename is occurring).
🧹 Nitpick comments (1)
apps/api/src/groups/__tests__/groups.service.spec.ts (1)
30-42: ⚡ Quick winAdd targeted assertions for the new repo-aware behavior.
Current tests don’t verify the new
instrumentModel.findMany({ where: { sourceRepoId: null } })path orinstrumentRepoIdsrelation updates. Add focused tests/assertions for both to lock in this feature behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/groups/__tests__/groups.service.spec.ts` around lines 30 - 42, The test suite for the groupsService.create method lacks assertions that verify the new repo-aware instrument behavior. Add targeted assertions within the existing test cases in the create describe block to verify that when creating a group, the service correctly calls instrumentModel.findMany with a filter for sourceRepoId: null and properly updates the instrumentRepoIds relation. Create one or more focused test cases that mock the instrumentModel.findMany response and assert that the returned instruments have their instrumentRepoIds relation updated with the newly created group's ID, ensuring the complete feature behavior is locked in by tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/auth/ability.factory.ts`:
- Line 30: The ability.can() call for GROUP_MANAGER read access to
InstrumentRepo on line 30 grants unconditional read permissions to all
repositories, which is inconsistent with the constrained manage access on Group
shown on line 28. Add a groupIds condition to the InstrumentRepo read permission
using the hasSome operator to restrict access only to the repositories assigned
to the user's groupIds, matching the pattern already used for the Group manage
constraint.
In `@apps/api/src/instrument-repos/instrument-repos.service.ts`:
- Line 225: The cleanup logic in importInstruments is deleting
path.dirname(repoDir) instead of the actual temporary directory created for this
import. When downloadAndExtractRepo returns tmpDir at line 225, the
importInstruments function should use that exact tmpDir for cleanup at lines
320-321 rather than computing the parent directory with path.dirname(repoDir).
Track and pass the tmpDir value through the import process and ensure cleanup
removes the exact tmpDir that was created, not its parent OS temp directory.
- Around line 214-215: The synchronous execFileSync calls for curl and tar
operations in the downloadAndExtractRepo method are blocking the Node.js event
loop and preventing other requests from being processed. Refactor
downloadAndExtractRepo to be an asynchronous method that delegates these
long-running operations (the curl and tar execFileSync calls) to a worker thread
or background queue, similar to the existing export-worker.js pattern used
elsewhere in the codebase. Update the importInstruments method and its callers
(create() and sync() request handlers) to properly await the refactored async
downloadAndExtractRepo method. This allows the event loop to remain responsive
and handle concurrent requests without blocking for the duration of the curl and
tar operations.
In `@apps/api/src/instruments/instruments.service.ts`:
- Around line 180-181: The buildInstrumentSourceMap() method is performing
full-table scans of all repo-sourced instruments with complete record data on
every findInfo call, which is inefficient. Modify buildInstrumentSourceMap() to
accept the requested instrument IDs as a parameter for filtering and only select
the necessary provenance fields instead of loading full records. Update the
calls to buildInstrumentSourceMap() at line 180 and at lines 260-274 to pass the
relevant instrument ID filters so the method performs scoped queries and returns
minimal data required for provenance information.
In `@apps/web/src/routes/_app/admin/groups/index.tsx`:
- Around line 65-72: The handleSave function closes the sheet by calling
setSelectedGroup(null) immediately after invoking
updateGroupReposMutation.mutate(), without waiting for the mutation to complete.
This causes the user to lose their selection context if the mutation fails. Move
the setSelectedGroup(null) call into the mutation's onSuccess callback so that
the sheet only closes after the save operation completes successfully. You can
configure this by adding an onSuccess handler when calling mutate or by using
the mutation options if they are already set up on the updateGroupReposMutation
definition.
- Around line 41-44: The onSuccess callback in the mutation handler only
invalidates the GROUPS_QUERY_KEY, but this mutation also updates group-to-repo
relationships. Add an additional queryClient.invalidateQueries call to also
invalidate the instrument-repos (or repo-related) cache alongside the existing
GROUPS_QUERY_KEY invalidation. This ensures that repo-management screens will
refetch and display the correct "in use" state after group assignments change.
In `@apps/web/src/routes/_app/admin/instrument-repos/index.tsx`:
- Around line 127-129: The warning message passed to the addNotification call in
the delete-guard logic is user-facing but lacks localization, unlike other UI
text in the application. Wrap the message string for `"${repo.name}" is assigned
to one or more groups. Remove it from those groups before deleting.` with the
appropriate translation function (such as the i18n hook or t() function used
elsewhere in the codebase) to ensure it is properly localized for different
languages.
- Around line 294-302: The structure violates HTML semantics by placing a header
`<div>` with className="grid grid-cols-[1fr_8rem_5rem] gap-x-6 font-bold" and an
`<hr>` element directly inside the `<ul>` tag. HTML `<ul>` elements should only
contain `<li>` children directly. Move the header `<div>` and `<hr>` outside and
above the `<ul>` opening tag (keeping them within the AnimatePresence wrapper if
needed), so that only the mapped instrument items remain as direct children of
the `<ul>`, wrapped appropriately in `<li>` elements.
In `@apps/web/src/routes/_app/group/manage.tsx`:
- Around line 122-131: The icon-only preview button (which contains only the
EyeIcon component and calls onPreview on click) lacks an accessible name, making
it inaccessible to screen readers. Add an aria-label attribute to the button
element with an appropriate descriptive label such as "Preview" to provide
screen reader users with context about the button's purpose.
- Around line 224-225: The selectedIds state on line 224 is only initialized
once from initialSelectedIds and won't update if the prop changes later, causing
stale data to be submitted. Add a useEffect hook that watches for changes to the
initialSelectedIds dependency and calls setSelectedIds with the new value
whenever initialSelectedIds changes, ensuring the component always has current
data from upstream group changes.
---
Outside diff comments:
In `@apps/api/src/groups/groups.service.ts`:
- Around line 67-80: The duplicate name check in the `updateById` method is
comparing the current group's name against the database instead of the incoming
requested name from `data.name`. This causes the method to incorrectly throw a
ConflictException on rename attempts even when no actual conflict exists. Fix
this by updating the exists check to use `data.name` instead of `group.name`,
and add an additional condition to skip the duplicate check entirely when the
incoming name matches the current group's name (to avoid false positives when no
rename is occurring).
---
Nitpick comments:
In `@apps/api/src/groups/__tests__/groups.service.spec.ts`:
- Around line 30-42: The test suite for the groupsService.create method lacks
assertions that verify the new repo-aware instrument behavior. Add targeted
assertions within the existing test cases in the create describe block to verify
that when creating a group, the service correctly calls instrumentModel.findMany
with a filter for sourceRepoId: null and properly updates the instrumentRepoIds
relation. Create one or more focused test cases that mock the
instrumentModel.findMany response and assert that the returned instruments have
their instrumentRepoIds relation updated with the newly created group's ID,
ensuring the complete feature behavior is locked in by tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a856d89c-81ba-428e-a499-e8164d48a595
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
apps/api/package.jsonapps/api/prisma/schema.prismaapps/api/src/auth/ability.factory.tsapps/api/src/groups/__tests__/groups.service.spec.tsapps/api/src/groups/dto/update-group.dto.tsapps/api/src/groups/groups.module.tsapps/api/src/groups/groups.service.tsapps/api/src/instrument-repos/__tests__/instrument-repos.service.spec.tsapps/api/src/instrument-repos/dto/create-instrument-repo.dto.tsapps/api/src/instrument-repos/instrument-repos.controller.tsapps/api/src/instrument-repos/instrument-repos.module.tsapps/api/src/instrument-repos/instrument-repos.service.tsapps/api/src/instruments/instruments.service.tsapps/api/src/main.tsapps/web/src/hooks/useCreateInstrumentRepoMutation.tsapps/web/src/hooks/useDeleteInstrumentRepoMutation.tsapps/web/src/hooks/useInstrumentReposQuery.tsapps/web/src/hooks/useNavItems.tsapps/web/src/hooks/useSyncInstrumentRepoMutation.tsapps/web/src/route-tree.tsapps/web/src/routes/_app/admin/groups/index.tsxapps/web/src/routes/_app/admin/instrument-repos/index.tsxapps/web/src/routes/_app/admin/users/index.tsxapps/web/src/routes/_app/datahub/index.tsxapps/web/src/routes/_app/group/manage.tsxapps/web/src/services/axios.tsapps/web/src/styles.cssapps/web/src/utils/error.tspackages/schemas/package.jsonpackages/schemas/src/core/core.tspackages/schemas/src/group/group.tspackages/schemas/src/instrument-repo/instrument-repo.tspackages/schemas/src/instrument/instrument.base.ts
💤 Files with no reviewable changes (1)
- apps/api/src/groups/groups.module.ts
CodeQL flagged `url.replace(/\/+$/, '')` as a polynomial regular expression on uncontrolled data. Replace the trailing-slash strip with a linear manual scan and trim the `.git` suffix via endsWith. Behavior is unchanged; add tests covering the normalization cases and a large many-trailing-slash input. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/.tanstack/tmp/ce07c9c8-8fde80335118f8a81687bb2fb01d6220 (1)
1-9: ⚡ Quick winRemove this generated temp route file from version control.
This duplicates the
/_app/group/manageroute in a temporary.tanstack/tmppath with placeholder UI, which can create route drift and maintenance confusion. Keep the canonical implementation only inapps/web/src/routes/_app/group/manage.tsxand ignore temp artifacts.Proposed cleanup
-import { createFileRoute } from '`@tanstack/react-router`' - -export const Route = createFileRoute('/_app/group/manage')({ - component: RouteComponent, -}) - -function RouteComponent() { - return <div>Hello "/_app/group/manage"!</div> -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/.tanstack/tmp/ce07c9c8-8fde80335118f8a81687bb2fb01d6220` around lines 1 - 9, Remove the temporary generated route file at apps/web/.tanstack/tmp/ce07c9c8-8fde80335118f8a81687bb2fb01d6220 from version control as it is a duplicate of the canonical route implementation in apps/web/src/routes/_app/group/manage.tsx. Delete this temporary file from the commit and ensure the .tanstack/tmp directory is listed in the project's .gitignore to prevent similar temp artifacts from being committed in the future.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/web/.tanstack/tmp/ce07c9c8-8fde80335118f8a81687bb2fb01d6220`:
- Around line 1-9: Remove the temporary generated route file at
apps/web/.tanstack/tmp/ce07c9c8-8fde80335118f8a81687bb2fb01d6220 from version
control as it is a duplicate of the canonical route implementation in
apps/web/src/routes/_app/group/manage.tsx. Delete this temporary file from the
commit and ensure the .tanstack/tmp directory is listed in the project's
.gitignore to prevent similar temp artifacts from being committed in the future.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f05c694b-3924-4e08-9167-ec09478035cc
📒 Files selected for processing (3)
apps/api/src/instrument-repos/__tests__/instrument-repos.service.spec.tsapps/api/src/instrument-repos/instrument-repos.service.tsapps/web/.tanstack/tmp/ce07c9c8-8fde80335118f8a81687bb2fb01d6220
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/instrument-repos/tests/instrument-repos.service.spec.ts
- apps/api/src/instrument-repos/instrument-repos.service.ts
API: - delete the exact temp dir after import (was removing path.dirname, i.e. the OS temp root, when the repo extracted without a single subdir) - run curl/tar via async execFile so imports don't block the event loop - scope buildInstrumentSourceMap to the requested instrument ids and select only provenance fields (avoids loading every bundle) - constrain GROUP_MANAGER read on InstrumentRepo to their own groups - groups.updateById: check the requested name (not the current one) and skip when unchanged, so renames no longer always conflict Web: - admin groups: close the edit sheet only after a successful save and invalidate the instrument-repos cache so "in use" state refreshes - localize the delete-guard warning - instrument list dialog: only <li> children inside <ul> (header/hr moved out) - add aria-label to the icon-only instrument preview button - resync group instrument selection when upstream group data changes Tests: add groups.service assertions (non-repo connect, repo relation, rename collision) and normalizeUrl cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Thanks for the review! Addressed all the CodeRabbit findings (and the earlier CodeQL alert) in 78cea14. Backend
Frontend
Tests: added CodeQL (ReDoS): On the one item I did not change — moving the import to a worker thread/queue: the async |
The `playwright install --with-deps` step has hung past the 6h job limit. Make CI resilient: - run lint + unit tests before the Playwright step so code feedback isn't blocked by a flaky browser download - cap the install with timeout-minutes: 10 so a hang fails fast - set DEBIAN_FRONTEND=noninteractive to avoid an apt prompt stalling - install only chromium + firefox (the only browsers the e2e suite uses) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/groups/groups.service.ts (1)
92-95:⚠️ Potential issue | 🔴 CriticalAdd authorization check before assigning
instrumentRepoIdsin group updates.GROUP_MANAGER users can currently assign repos because the service only validates Group update permission without checking InstrumentRepo manage capability. Lines 92–95 should enforce that only ADMIN-level callers can assign
instrumentRepoIds. Either:
- Filter
instrumentRepoIdsthrough ability before assignment, or- Reject updates containing
instrumentRepoIdsunlessability.can('manage', 'InstrumentRepo')is true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/groups/groups.service.ts` around lines 92 - 95, The group update logic in groups.service.ts at lines 92-95 (the instrumentRepos assignment block) is missing an authorization check to ensure only users with InstrumentRepo manage capability can assign instrumentRepoIds. Before assigning instrumentRepoIds through the set operation, validate that the caller has permission by checking ability.can('manage', 'InstrumentRepo'). If this check fails, either filter out the instrumentRepoIds from the update or reject the entire update request with an appropriate authorization error. This ensures GROUP_MANAGER users cannot escalate privileges by assigning repos they should not be able to manage.
🧹 Nitpick comments (1)
apps/api/src/groups/groups.service.ts (1)
26-28: ⚡ Quick winSelect only
idwhen fetching non-repo instruments for group creation.This query only feeds
connectIDs, so fetching full instrument records is unnecessary and can inflate memory/DB payload.Proposed diff
- const nonRepoInstruments = await this.instrumentModel.findMany({ - where: { sourceRepoId: null } - }); + const nonRepoInstruments = await this.instrumentModel.findMany({ + select: { id: true }, + where: { sourceRepoId: null } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/groups/groups.service.ts` around lines 26 - 28, The findMany query in the nonRepoInstruments assignment is fetching complete instrument records when only the IDs are needed for the subsequent connect operation. Optimize this query by adding a select option to the findMany call to retrieve only the id field from the instruments table, reducing unnecessary memory and database payload usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/api/src/groups/groups.service.ts`:
- Around line 92-95: The group update logic in groups.service.ts at lines 92-95
(the instrumentRepos assignment block) is missing an authorization check to
ensure only users with InstrumentRepo manage capability can assign
instrumentRepoIds. Before assigning instrumentRepoIds through the set operation,
validate that the caller has permission by checking ability.can('manage',
'InstrumentRepo'). If this check fails, either filter out the instrumentRepoIds
from the update or reject the entire update request with an appropriate
authorization error. This ensures GROUP_MANAGER users cannot escalate privileges
by assigning repos they should not be able to manage.
---
Nitpick comments:
In `@apps/api/src/groups/groups.service.ts`:
- Around line 26-28: The findMany query in the nonRepoInstruments assignment is
fetching complete instrument records when only the IDs are needed for the
subsequent connect operation. Optimize this query by adding a select option to
the findMany call to retrieve only the id field from the instruments table,
reducing unnecessary memory and database payload usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cef45596-2c2e-4633-9cbd-ca49d16c1007
📒 Files selected for processing (9)
.github/workflows/ci.yamlapps/api/src/auth/ability.factory.tsapps/api/src/groups/__tests__/groups.service.spec.tsapps/api/src/groups/groups.service.tsapps/api/src/instrument-repos/instrument-repos.service.tsapps/api/src/instruments/instruments.service.tsapps/web/src/routes/_app/admin/groups/index.tsxapps/web/src/routes/_app/admin/instrument-repos/index.tsxapps/web/src/routes/_app/group/manage.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/api/src/instruments/instruments.service.ts
- apps/web/src/routes/_app/group/manage.tsx
- apps/web/src/routes/_app/admin/instrument-repos/index.tsx
- apps/web/src/routes/_app/admin/groups/index.tsx
- apps/api/src/instrument-repos/instrument-repos.service.ts
The browser download hangs on a stalled CDN socket (Chromium completes, then the next download sits with no progress until the job/step timeout). Split OS deps from the browser download and wrap the download in a per-attempt `timeout 180` + 3x retry; Playwright skips already-downloaded browsers, so each retry resumes. apt deps stay in their own quick step. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The browser install hung at 100% under Node 24 (lts/krypton) — a known Playwright/Node 24 download issue fixed in newer releases. Bump to 1.60.0 (1.61.0 is blocked by the 7-day minimumReleaseAge policy). Also simplify the CI Playwright step back to a single capped install now that the version bump, not a retry hack, is the fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
joshunrau
requested changes
Jun 17, 2026
joshunrau
left a comment
joshunrau
left a comment
Collaborator
There was a problem hiding this comment.
looks great! A few minor comments. Also, French translations missing in quite a few places. Can you please just have the agent review and make sure everything is translted.
| ability.can('manage', 'Assignment', { groupId: { in: groupIds } }); | ||
| ability.can('manage', 'Group', { id: { in: groupIds } }); | ||
| ability.can('read', 'Instrument'); | ||
| ability.can('read', 'InstrumentRepo', { groupIds: { hasSome: groupIds } }); |
Collaborator
There was a problem hiding this comment.
Doesn't standard user also need to be able to read this?
Collaborator
There was a problem hiding this comment.
Otherwise, could cause problems. Did you verify everything works for standard users?
Collaborator
Author
There was a problem hiding this comment.
Verified — standard users don't need read InstrumentRepo. The provenance shown in the UI (sourceRepo) is denormalized onto each Instrument record and served via /instruments/info, which is gated only by read Instrument (standard users have that unconstrained). buildInstrumentSourceMap reads sourceRepoId/sourceRepoName straight off the Instrument — it never queries InstrumentRepo or checks that ability. The /instrument-repos endpoints are consumed solely by the two admin-only pages (useInstrumentReposQuery is imported only there), so standard users never hit them. I checked the manage-group / datahub / accessible-instruments flows and they work for standard users. Granting standard users repo read would just be over-permissive, so I left the ability as-is.
| curlArgs.push('-H', `Authorization: Bearer ${token}`); | ||
| } | ||
| curlArgs.push(tarballUrl, '-o', tarballPath); | ||
| await execFileAsync('curl', curlArgs, { timeout: 60_000 }); |
Collaborator
There was a problem hiding this comment.
do we know that curl is installed in the docker containers? I don't think it will be. Is there a Node-based way to do this?
Contributor
There was a problem hiding this comment.
There's definitely a node-based way to do this.
| onSubmit(result.data); | ||
| }; | ||
|
|
||
| return ( |
Collaborator
There was a problem hiding this comment.
is there a reason the libui form cannot be used here?
Collaborator
Author
There was a problem hiding this comment.
Intentional — this is a plain <form> rather than the libui Form because of autoComplete="off" plus the non-semantic field names, which are needed to stop browsers/password managers from autofilling the URL and access-token fields (there's a comment to that effect above the form). Happy to switch to libui Form if you'd prefer and we find another way to suppress autofill.
Contributor
There was a problem hiding this comment.
find another way to suppress autofill.
We can add that to libUI if its not already available.
API: - replace curl/tar with Node fetch + JSZip in downloadAndExtractRepo so imports no longer depend on curl/tar being present in the runtime container; add jszip dependency and a zip-slip-guarded extractor - seed the default DouglasNeuroInformatics/ODC_Instruments repo on fresh install (initApp), with graceful failure and a test-env skip Web: - translate the create/delete/sync instrument-repo mutation toasts and the admin groups repo-update error fallback via useTranslation Chore: - remove the accidentally-committed .tanstack/tmp route file and gitignore .tanstack Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The user-supplied repo URL flowed into the GitHub API request URL, which CodeQL flagged as server-side request forgery. parseGitHubUrl now requires the github.com host and validates owner/repo against a strict allowlist (rejecting @, :, ?, #, etc.), throwing BadRequestException otherwise, and the zipball URL encodes both components. Add tests for the rejected cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
4 participants
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.
Admins can import instruments from a GitHub repository, sync to pull in new instruments, view a repo's instrument list, and delete repos. Group managers assign repos to groups and opt-in to individual instruments.
Backend:
Frontend:
Tests: unit coverage for the repo service (crypto round-trip, secret stripping, delete guards, create/sync/dedup, orphan reconciliation).
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores