Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change stores transformed and prefix-adjusted bulk data URIs before URI resolution. Tests cover absolute, server-relative, and relative URIs with different WADO roots and resolution settings. ChangesBulk data URI handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change makes bulk data URI transforms apply to absolute and server-relative URIs, and behavior without a transform is unchanged. No actionable merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 1 | ❓ 1❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue Resolution Provide a reproducible check, or code-level evidence, for the Orthanc + OHIF v3 setup in ✨ 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 |
…DataURIs fixBulkDataURI runs bulkDataURI.transform on a local copy of the URI, but writes the copy back to value.BulkDataURI only in the startsWith branch and in the branches that resolve relative paths. An absolute http(s) URI reaches none of them, so the transform result is dropped and retrieveBulkData requests the original URI. A /path URI with a relative wadoRoot loses the transform the same way. The relative-path check also read value.BulkDataURI instead of the local copy, so a transform that returns a server-relative /path was resolved against the series URL. Write the local copy back once after transform and startsWith/prefixWith, and use it in the relative-path check. The relative and /path branches overwrite the value as before. Add fixBulkDataURI.test.ts.
1aedd33 to
8aad036
Compare
This branch has not been deployed
1 participant
Context
bulkDataURI.transformin the DICOMweb data source config has no effect when the server returnsan absolute BulkDataURI. A user reported this in
#4256 (comment) and patched the code locally.
This PR does not close #4256, whose original Orthanc problem was a different one.
It shows up when the DICOMweb server runs behind a TLS reverse proxy and returns BulkDataURIs as
http://..., while the viewer is served over https. This config should upgrade the URIs:With this config, the request still goes to
http://..., the browser blocks it as mixed content,and dicomweb-client throws
Error: request failed.In
extensions/default/src/DicomWebDataSource/utils/fixBulkDataURI.ts,transformwrites into alocal
BulkDataURI. The function copies that back tovalue.BulkDataURIonly in thestartsWithbranch and in the branches that resolve relative paths. An absolute
http(s)URI reaches none ofthem, and
retrieveBulkDatareadsvalue.BulkDataURI, so it requests the untransformed URI. A/pathURI with a relativewadoRootloses its transform the same way. The relative-path checkalso reads
value.BulkDataURIinstead of the local, so when a transform turns an absolute orrelative URI into a
/path, the result gets appended to the series URL.transformcame in with#4182 without a write-back for these cases.
Changes & Results
fixBulkDataURIwrites the local URI back tovalue.BulkDataURIonce, aftertransformandstartsWith/prefixWith. The relative and/pathbranches overwrite it as before.fixBulkDataURI.test.tsnext to the source. There was no unit test for this function.BulkDataURI
http://pacs.example.com/bulk/1, wadoRoothttps://viewer.example.com/dicomweb:http:tohttps:http://pacs.example.com/bulk/1https://pacs.example.com/bulk/1/pacshttps://viewer.example.com/dicomweb/studies/1.2.3/series/4.5.6//pacs/bulk/1https://viewer.example.com/pacs/bulk/1Without a transform, results do not change for any input: relative URIs,
/pathURIs,startsWith/prefixWithandrelativeResolution. The new test covers each of them.Deployments that set
transformand receive absolute or/pathURIs will now see the transformapplied, where before it was ignored.
dicom-web.mdalready describestransformwithoutlimiting it to relative URIs, so I did not change the docs.
The microscopy viewport calls
fixBulkDataURIa second time throughcleanDenaturalizedDatasetwith
dataSource.getConfig(). For the DICOMweb data source that config is a JSON copy withouttransform, so the second call gives the same result as before.Testing
I ran everything below on Windows 11 with Node 24.21.0 and pnpm 11.5.2 (the
packageManagerversion, called as
corepack pnpm), with dependencies installed from the repo lockfile.The new test file, from the repo root:
pnpm exec jest --ci extensions/default/src/DicomWebDataSource/utils/fixBulkDataURI.test.tsThis branch:
Tests: 14 passed, 14 total.Same test file with
fixBulkDataURI.tsrestored from master 9ba15ec:Tests: 4 failed, 10 passed, 14 total. The 4 failures are the bugs this PR fixes:applies transform to an absolute URIreceiveshttp://pacs.example.com/bulk/1.resolves a transform result of /path against wadoRoot ..., withhttps://viewer.example.com/dicomweband with/dicomweb, receives.../dicomweb/studies/1.2.3/series/4.5.6//pacs/bulk/1.applies transform to a /path URI with a relative wadoRootreceives/bulk/1instead of/pacs/bulk/1.The other 10 cases (no config,
startsWith/prefixWith,/path, relative resolution) pass onmaster as well.
The
extensions/defaultsuite, the way CI runs it:This branch:
Test Suites: 22 passed, 22 total,Tests: 164 passed, 164 total. Master 9ba15ec:21 suites and 150 tests, all passed. The only difference is the new
fixBulkDataURI.test.ts(1 suite, 14 tests).
The other two steps of the CircleCI
UNIT_TESTSjob,pnpm run lint:compiler:ciandpnpm run compiler:coverage:ci, exit 0 and print the same numbers as on master.prettier --checkpasses on both changed files.eslint --config eslint.config.mjsreports0 problems in
fixBulkDataURI.tsand skips the test file, because the config ignores**/*.test.*.For a browser check with synthetic data I made two production builds with
pnpm run build: onefrom master 9ba15ec and one from master plus this commit. The second build also contained two other
local fixes of mine (overlay plane module, SR SOP class handler). They are not part of this PR and
do not touch
fixBulkDataURI. Both builds used the sameapp-config.js, with a DICOMweb datasource pointed at a local mock server on 127.0.0.1. The mock serves one synthetic study with an
Encapsulated PDF. Its
EncapsulatedDocumentBulkDataURI is absolute and names a host that doesnot resolve:
http://internal-pacs.invalid:8080/dicomweb/studies/.../bulkdata/00420011. Thetransform maps that host to the mock:
I opened the PDF series in headless Microsoft Edge 154.0.4258.37 and logged the network requests
over CDP:
GET http://internal-pacs.invalid:8080/dicomweb/studies/.../bulkdata/00420011failswith
net::ERR_NAME_NOT_RESOLVED. The console showsFailed to retrieve encapsulated document Error: request failed, and the viewport shows"Unable to retrieve this document".
GET http://127.0.0.1:18080/dicomweb/studies/.../bulkdata/00420011returns 200and the PDF renders in the viewport. No request goes to
internal-pacs.invalid.I did not set up the https reverse proxy case from Context. The browser check runs through the same
code path, an absolute URI whose transform result was dropped, with a host rewrite instead of a
scheme rewrite. To try it against a real server, add a
transformfor its absolute BulkDataURIs,open a study with an encapsulated PDF, and check the bulkdata request URL in the network tab.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit