feat: Add customization URL parameter#5992
Conversation
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
sedghi
left a comment
sedghi
left a comment
There was a problem hiding this comment.
I’m not fully convinced the added value of this PR justifies the new security surface yet. This introduces URL-driven runtime JavaScript loading via ?customization=, which means a shared link can change viewer behavior and, depending on deployment config, potentially load executable code into the same browser context as OHIF. The validation does block obvious arbitrary URLs and path traversal, so this is not an immediate “any URL can execute code” issue. But the security boundary becomes the configured customization prefix and whoever can publish files there. If that directory/CDN is writable by the wrong party, this could become XSS-equivalent: token/session access, DICOM metadata exposure, UI manipulation, report tampering, or authenticated API abuse from the victim’s browser.
My current view is that this level of runtime configurability may be better kept in downstream forks or deployment-specific builds, where the deploying team can own the threat model and hosting controls explicitly. I’m not sure it should become a default upstream capability.
If the CDN is writable by the wrong party, it doesn't matter what you do, they can replace the entire OHIF source control. At that point you are completely open. What about adding a user configuration option to specifically and manually add customization prefixes rather than allowing it to be done via customization? That way we can default to one customization deploy somewhere that we control for the demonstration deployments, making note that is intended for demo purposes only, and same-host http /customization/ prefix path options so that we can deploy with a fixed deployment? It is clear the advisory board wants SOMETHING that allows dynamic loading. I agree it needs to be controlled, but it also has to be external to the build process of OHIF, otherwise we will never meet the goals of allowing OHIF to be customized by non-developers. Some other things we could consider:
That value of this is clearly extremely high given how many people on the meeting wanted something better. The only question becomes how to make it reasonably safe. It isn't fully safe, but neither is OHIF in the current configuration. |
|
@sedghi I think we should change the direction of this PR to avoid that for the first version. What I would propose instead is a narrower model:
One nuance I want to preserve is that a customization should still be able to add existing build-approved modes/plugins/extensions to the active load list, even if they were not part of the original default load list. In other words, the customization can choose from a build-time-approved set, but cannot expand that set at runtime. The lifecycle would be something like:
So the runtime customization can change which already-bundled capabilities become active, but it cannot cause the viewer to fetch or execute new code that was not already included by the build. That does make this less flexible than the original PR, but I think it gives us a much cleaner security boundary:
I also agree that a full remote plugin ecosystem is a separate design problem. Eventually we may want a trusted plugin catalog, immutable versions, hashes/signatures, provenance, and tighter policy controls. But I do not think this PR needs to solve that yet. For this PR, I think the safer target is: runtime customization can activate/configure build-approved capabilities, but cannot load arbitrary runtime JavaScript. Would that address your main concern if I rework the PR in that direction? |
|
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:
📝 WalkthroughWalkthroughThe PR adds URL-driven customization module loading to ChangesURL-driven customization module loading
Sequence Diagram(s)sequenceDiagram
participant AppInit as appInit.js
participant CustomizationService
participant parseCustomizationParams
participant validateCustomizationRequests
participant resolveCustomizationUrl
participant importFn as dynamic import()
participant setCustomizations
AppInit->>CustomizationService: init(extensionManager)
AppInit->>CustomizationService: applyWindowUrlCustomizations()
CustomizationService->>parseCustomizationParams: window.location.search → string[]
parseCustomizationParams-->>CustomizationService: raw names
CustomizationService->>validateCustomizationRequests: raws + policy → ValidationResult
validateCustomizationRequests-->>CustomizationService: valid[], rejected[]
loop depth-first per valid entry
CustomizationService->>resolveCustomizationUrl: ValidatedCustomization → URL
resolveCustomizationUrl-->>CustomizationService: absolute URL
CustomizationService->>importFn: import(URL)
importFn-->>CustomizationService: module
CustomizationService->>CustomizationService: getUrlCustomizationModulePayload(module)
CustomizationService->>CustomizationService: recurse requires[]
end
CustomizationService->>setCustomizations: apply loaded global payloads
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
tests/Customization.spec.ts (2)
6-8: ⚡ Quick winExercise the chaining entrypoint in this E2E path.
Using
veterinaryOverlayskips validation of theveterinary -> requires -> veterinaryOverlayflow introduced with this sample module pair.Suggested diff
await visitStudyOptions(page, studyInstanceUID, { - customization: 'veterinaryOverlay', + customization: 'veterinary', });🤖 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 `@tests/Customization.spec.ts` around lines 6 - 8, The test is directly using 'veterinaryOverlay' as the customization parameter in the visitStudyOptions call, which bypasses the chaining entrypoint validation. Change the customization parameter from 'veterinaryOverlay' to 'veterinary' so that the dependency chain flow (veterinary -> requires -> veterinaryOverlay) is properly exercised and validated in this E2E test path.
16-21: ⚡ Quick winUse auto-waiting text assertions to reduce E2E flake risk.
textContent()is a point-in-time read. Prefer locator text expectations so Playwright waits for final rendered content.Suggested diff
- const patientNameOverlayText = (await patientNameOverlayItem.textContent())?.trim() ?? ''; - const patientNameValue = patientNameOverlayText.replace(/^Patient\s*/, ''); - - expect(patientNameValue.length).toBeGreaterThan(0); - expect(patientNameValue).not.toContain('[object Object]'); - expect(patientNameValue).toMatch(/horse/i); + await expect(patientNameOverlayItem).not.toContainText('[object Object]'); + await expect(patientNameOverlayItem).toContainText(/horse/i);🤖 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 `@tests/Customization.spec.ts` around lines 16 - 21, The test is using textContent() which performs a point-in-time read and can cause flakiness in E2E tests. Replace the textContent extraction approach with Playwright's auto-waiting locator text expectations. Instead of extracting the text from patientNameOverlayItem and performing assertions on the extracted value, use the locator's built-in matchers like toHaveText() or toContainText() that automatically wait for the final rendered content before asserting, eliminating the need to manually extract and trim the text value.tests/utils/visitStudy.ts (1)
3-8: ⚡ Quick winSupport multi-value
customizationparams in test navigation helper.
visitStudyOptionscurrently only accepts a single customization value. Acceptingstring | string[]makes this helper reusable for multi-customization URL scenarios.Suggested diff
type VisitStudyOptions = { mode?: string; delay?: number; datasources?: string; - customization?: string; + customization?: string | string[]; }; @@ const params = new URLSearchParams({ StudyInstanceUIDs: studyInstanceUID }); - if (customization) { - params.set('customization', customization); - } + const customizationValues = Array.isArray(customization) + ? customization + : customization + ? [customization] + : []; + for (const value of customizationValues) { + params.append('customization', value); + }Also applies to: 31-34
🤖 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 `@tests/utils/visitStudy.ts` around lines 3 - 8, The `customization` property in the `VisitStudyOptions` type currently only accepts a single string value, but should support multiple customization parameters. Change the type definition of the `customization` field from `string` to `string | string[]` in the `VisitStudyOptions` type. Additionally, update the code that consumes this parameter (referenced at lines 31-34) to handle both single string and array of strings values appropriately when constructing URLs or passing them to the navigation logic.platform/app/src/utils/preserveQueryParameters.test.ts (1)
31-41: ⚡ Quick winAdd an overlap regression case for base + custom preserve keys.
This suite should also assert behavior when customization-provided keys include an existing base key and the current URL has values for that key (to prevent double-append regressions).
Suggested test addition
it('uses customization service values for multi-key preservation', () => { const customizationService = { getValue: jest.fn().mockReturnValue(['customization', 'customizationAlt']), }; const current = new URLSearchParams(); + current.append('customization', 'a'); current.append('customizationAlt', 'c'); const out = new URLSearchParams(); preserveQueryParameters(out, customizationService, current); + expect(out.getAll('customization')).toEqual(['a']); expect(out.getAll('customizationAlt')).toEqual(['c']); expect(customizationService.getValue).toHaveBeenCalled(); });🤖 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 `@platform/app/src/utils/preserveQueryParameters.test.ts` around lines 31 - 41, Add a new test case after the existing 'uses customization service values for multi-key preservation' test to cover the overlap regression scenario. The test should verify that when a query parameter key exists in both the base preserved keys and the keys returned by the customization service's getValue method, and the current URL contains a value for that overlapping key, the preserveQueryParameters function appends that value only once to the output URLSearchParams without duplication. Set up the test with a customization service that returns keys including an existing base key, populate the current URLSearchParams with a value for that overlapping key, call preserveQueryParameters, and then assert that the output contains only a single copy of that value.platform/core/src/services/CustomizationService/CustomizationService.requires.test.ts (1)
9-184: ⚡ Quick winAdd a regression test for same normalized request under different prefix mappings.
Current coverage doesn’t assert behavior when
/default/Ais requested across calls with differentpolicy.prefixes.default. A focused test here would lock in correct cache semantics.🧪 Suggested test case
+ it('does not reuse cache when the same normalized name resolves to a different base URL', async () => { + const importFn = jest.fn(async (url: string) => ({ + customizations: { global: { marker: { value: url } } }, + })); + + await service.requires(['A'], { + policy: { prefixes: { default: './customizations-v1/' } }, + importFn, + }); + await service.requires(['A'], { + policy: { prefixes: { default: './customizations-v2/' } }, + importFn, + }); + + expect(importFn).toHaveBeenCalledTimes(2); + });🤖 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 `@platform/core/src/services/CustomizationService/CustomizationService.requires.test.ts` around lines 9 - 184, Add a new test case to verify cache semantics when the same module name is requested with different policy prefix mappings. Create a test that calls service.requires with a module like /default/A first with one policy.prefixes.default configuration, then calls it again with a different policy.prefixes.default value, and assert that the service correctly handles the cache state (either reusing or reloading appropriately based on whether the normalized paths are truly the same). This ensures the caching logic in the requires method doesn't incorrectly share cached modules across different prefix contexts.platform/core/src/services/CustomizationService/validate.test.ts (1)
17-22: ⚡ Quick winAdd an interleaved mixed-case ordering regression test
Given the parser is case-insensitive and order-sensitive, add a case like
Customization=a&customization=b&Customization=cto lock expected output order.🤖 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 `@platform/core/src/services/CustomizationService/validate.test.ts` around lines 17 - 22, Add a new test case to the matches the parameter key case-insensitively test block that creates a URLSearchParams with interleaved mixed-case variations of the same parameter (such as Customization=a, customization=b, Customization=c) and verify that parseCustomizationParams returns them in the order they appear regardless of case differences. This ensures the parser is truly order-sensitive while being case-insensitive.
🤖 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 `@platform/app/src/utils/preserveQueryParameters.ts`:
- Around line 25-32: The getPreserveKeys function returns an array by combining
preserveKeys and customKeys without deduplication, which can result in duplicate
keys being passed to preserveQueryParameters. Modify the return statement to
remove duplicates from the combined array before returning it, ensuring that if
a key exists in both preserveKeys and customKeys, it only appears once in the
final result.
In `@platform/core/src/services/CustomizationService/CustomizationService.ts`:
- Around line 370-373: The cache key for _urlCustomizationLoaded and
_urlCustomizationPending only uses request.normalized, which can cause different
modules to be incorrectly reused when different policy.prefixes are provided.
You need to modify the cache key generation to include policy information (such
as policy.prefixes) in addition to request.normalized to ensure each unique
combination of normalized name and policy prefix gets its own cache entry. Apply
this cache key change consistently across all locations where
_urlCustomizationLoaded or _urlCustomizationPending are accessed (the if
statement starting at line 370, the get call around lines 389-390, the
assignment around lines 407-413, and the pending check around lines 478-479).
- Around line 199-210: The catch block in applyWindowUrlCustomizations currently
logs all errors as warnings regardless of configuration, but when strict
customization policy is enabled, errors should cause the method to fail
(rethrow) rather than silently continue. Modify the catch block to check if
strict mode is enabled, and if so, rethrow the error after logging; otherwise,
keep the current warning behavior for non-strict mode.
In `@platform/core/src/services/CustomizationService/resolve.test.ts`:
- Around line 22-30: The beforeAll hook in this test suite mutates
window.location but never restores it after tests complete, which causes state
to leak into subsequent tests. Save the original window.location descriptor
before the beforeAll hook modifies it, then add an afterAll hook that uses
Object.defineProperty to restore window.location back to its original state
using the saved descriptor. This ensures the global state is cleaned up after
this test suite runs.
In `@platform/core/src/services/CustomizationService/validate.ts`:
- Around line 23-37: The current implementation using new Set(params.keys()) to
deduplicate keys loses the original ordering of customization values when keys
with different casing are interleaved in the query parameters. To fix this,
replace the Set-based deduplication approach with direct iteration through the
original params entries in their natural order. Instead of creating a
deduplicated set and then calling getAll for each unique key, iterate through
params' entries directly while still performing case-insensitive key matching,
ensuring all values are collected in the exact order they appear in the source
params object.
---
Nitpick comments:
In `@platform/app/src/utils/preserveQueryParameters.test.ts`:
- Around line 31-41: Add a new test case after the existing 'uses customization
service values for multi-key preservation' test to cover the overlap regression
scenario. The test should verify that when a query parameter key exists in both
the base preserved keys and the keys returned by the customization service's
getValue method, and the current URL contains a value for that overlapping key,
the preserveQueryParameters function appends that value only once to the output
URLSearchParams without duplication. Set up the test with a customization
service that returns keys including an existing base key, populate the current
URLSearchParams with a value for that overlapping key, call
preserveQueryParameters, and then assert that the output contains only a single
copy of that value.
In
`@platform/core/src/services/CustomizationService/CustomizationService.requires.test.ts`:
- Around line 9-184: Add a new test case to verify cache semantics when the same
module name is requested with different policy prefix mappings. Create a test
that calls service.requires with a module like /default/A first with one
policy.prefixes.default configuration, then calls it again with a different
policy.prefixes.default value, and assert that the service correctly handles the
cache state (either reusing or reloading appropriately based on whether the
normalized paths are truly the same). This ensures the caching logic in the
requires method doesn't incorrectly share cached modules across different prefix
contexts.
In `@platform/core/src/services/CustomizationService/validate.test.ts`:
- Around line 17-22: Add a new test case to the matches the parameter key
case-insensitively test block that creates a URLSearchParams with interleaved
mixed-case variations of the same parameter (such as Customization=a,
customization=b, Customization=c) and verify that parseCustomizationParams
returns them in the order they appear regardless of case differences. This
ensures the parser is truly order-sensitive while being case-insensitive.
In `@tests/Customization.spec.ts`:
- Around line 6-8: The test is directly using 'veterinaryOverlay' as the
customization parameter in the visitStudyOptions call, which bypasses the
chaining entrypoint validation. Change the customization parameter from
'veterinaryOverlay' to 'veterinary' so that the dependency chain flow
(veterinary -> requires -> veterinaryOverlay) is properly exercised and
validated in this E2E test path.
- Around line 16-21: The test is using textContent() which performs a
point-in-time read and can cause flakiness in E2E tests. Replace the textContent
extraction approach with Playwright's auto-waiting locator text expectations.
Instead of extracting the text from patientNameOverlayItem and performing
assertions on the extracted value, use the locator's built-in matchers like
toHaveText() or toContainText() that automatically wait for the final rendered
content before asserting, eliminating the need to manually extract and trim the
text value.
In `@tests/utils/visitStudy.ts`:
- Around line 3-8: The `customization` property in the `VisitStudyOptions` type
currently only accepts a single string value, but should support multiple
customization parameters. Change the type definition of the `customization`
field from `string` to `string | string[]` in the `VisitStudyOptions` type.
Additionally, update the code that consumes this parameter (referenced at lines
31-34) to handle both single string and array of strings values appropriately
when constructing URLs or passing them to the navigation logic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 93677f12-e870-453e-83ef-ddd105467388
📒 Files selected for processing (29)
extensions/cornerstone/src/Viewport/Overlays/CustomizableViewportOverlay.tsxextensions/default/src/ViewerLayout/ViewerHeader.tsxextensions/default/src/customizations/overlayItemCustomization.tsxplatform/app/public/customizations/veterinary.jsplatform/app/public/customizations/veterinaryOverlay.jsplatform/app/src/App.tsxplatform/app/src/appInit.jsplatform/app/src/utils/preserveQueryParameters.test.tsplatform/app/src/utils/preserveQueryParameters.tsplatform/core/src/services/CustomizationService/CustomizationService.init.test.tsplatform/core/src/services/CustomizationService/CustomizationService.requires.test.tsplatform/core/src/services/CustomizationService/CustomizationService.tsplatform/core/src/services/CustomizationService/customizationUrl.tsplatform/core/src/services/CustomizationService/customizationUrlDefaults.tsplatform/core/src/services/CustomizationService/customizationUrlTypes.tsplatform/core/src/services/CustomizationService/getUrlCustomizationModulePayload.tsplatform/core/src/services/CustomizationService/resolve.test.tsplatform/core/src/services/CustomizationService/resolve.tsplatform/core/src/services/CustomizationService/validate.test.tsplatform/core/src/services/CustomizationService/validate.tsplatform/core/src/services/MultiMonitorService.tsplatform/core/src/services/index.tsplatform/core/src/utils/formatValue.jsplatform/core/src/utils/index.tsplatform/docs/docs/platform/services/customization-service/customizationService.mdplatform/docs/docs/platform/services/customization-service/specificCustomizations.mdtests/Customization.spec.tstests/utils/index.tstests/utils/visitStudy.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@platform/core/src/services/CustomizationService/customizationUrlDefaults.ts`:
- Around line 11-13: Update the documentation comment in
customizationUrlDefaults.ts to reflect that the warn-and-skip behavior is only
applied in non-strict mode, and that strict mode (when policy.strict === true)
should throw on invalid customization query entries, failed imports, resolve
errors, and modules with no customization payload. Then locate the validation
and load call sites within the customizationUrlDefaults logic and implement
conditional behavior based on policy.strict: throw errors when strict mode is
enabled, and warn-and-skip when strict mode is disabled to restore the
configurable strict mode behavior that was originally supported.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2a4d5d1-9ce3-4874-92e4-78459d71d506
📒 Files selected for processing (4)
platform/core/src/services/CustomizationService/CustomizationService.requires.test.tsplatform/core/src/services/CustomizationService/CustomizationService.tsplatform/core/src/services/CustomizationService/customizationUrlDefaults.tsplatform/docs/docs/platform/services/customization-service/specificCustomizations.md
✅ Files skipped from review due to trivial changes (1)
- platform/docs/docs/platform/services/customization-service/specificCustomizations.md
🚧 Files skipped from review as they are similar to previous changes (1)
- platform/core/src/services/CustomizationService/CustomizationService.ts
|
@sedghi - as requested, I've added additional examples, described in the testing block above, as well as in the documentation internally. |
3 participants
Context
In order to allow custom versions of OHIF to be defined/added without having to rebuild OHIF, it is necessary to have a customization framework that can load dynamic modules. This has been added as a customization= parameter.
Changes & Results
Added a customization handler for the customization= parameter
Added a requires= export in the loaded global customizations to allow customizations to depend on other ones, eg veterinary depends on veterinaryOverlay
Add an example customization to test with, the start of a veterinary example.
URL customization examples (
?customization=)This PR ships a set of runtime-loadable customization modules under
platform/app/public/customizations/to demonstrate the?customization=framework. Each is a plain ES module whoseglobalpayload is applied as a global customization at bootstrap (using immutability-helper commands, so it extends the built-in defaults rather than replacing them). Append&customization=<name>to any viewer URL to try one — e.g./viewer?StudyInstanceUIDs=<uid>&customization=smoothRotate.?customization=smoothRotatectPresetsmeasurementLabelsveterinaryOverlayveterinaryrequiresdependency chain (pulls inveterinaryOverlay)smoothRotate— add a toolbar buttonAdds a Smooth Rotate button to the More Tools menu that activates the cornerstone
PlanarRotatetool (drag to rotate the image to any angle, unlike the fixed-90° Rotate Right).This is enabled by a supporting change: the basic/longitudinal viewers no longer hard-code their toolbar — the cornerstone extension now registers it as customizations (
cornerstone.toolbarButtons= button defs,cornerstone.toolbarSections= the section→ids layout), and the modes reference those by name inonModeEnter. The module$pushes a button definition and its id into theMoreToolssection.PlanarRotateis registered as an available tool by default. Because the defaults register at default scope and the URL module applies at global scope, the push extends the built-in toolbar instead of clobbering it.ctPresets— site-specific window/level presets$merges a customCTpreset list intocornerstone.windowLevelPresets. The new presets appear in the window-level menu for CT; presets for other modalities (PT, etc.) are preserved because only theCTkey is replaced.measurementLabels— predefined measurement labelsSets
measurementLabelswithlabelOnMeasure: trueand anexclusiveitem list, so completing a measurement prompts the user to pick a label (Head/Shoulder/Knee/Toe) instead of free-typing.veterinaryOverlay— replace the viewport overlayReplaces
viewportOverlay.topLeft/viewportOverlay.topRightwith a veterinary-style demographics layout (species/breed), demonstrating overlay customization.veterinary— dependency chainsA near-empty module that declares
requires: ['veterinaryOverlay'], showing how one module can pull in another so layered packs (base → shared → site-specific) load in order.Security notes
/prefix/name, full URLs and path traversal (including percent-encoded forms like%2e%2e) are rejected, and the resolved URL is verified to stay within the configured prefix directory.Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
Release Notes
New Features
?customization=veterinaryOverlay).Improvements
Documentation
Tests
Refactor