Viewers
Viewers copied to clipboard
feat: query string dynamic app config
This PR allows for the app config to reference a json over network. This allows for fast swapping of data source configuration, OIDC, etc, without recompiling the application.
If the configUrl
query string is passed, the worklist and modes will load from the referenced json
rather than the default .env config
If there is no configUrl
path provided, the default behaviour is used and there should not be any
deviation from current user experience.
Example usage:
http://localhost:3000/?configUrl=http://localhost:3000/config/example.json
The following will load the default.js settings, but with 6 web workers instead of 3 (check window.config
) to verify.
PR Checklist
- [x] Brief description of changes
- [x] Links to any relevant issues
- [ ] Required status checks are passing
- [x] User cases if changes impact the user's experience
- [x]
@mention
a maintainer to request a review
Deploy Preview for ohif-platform-docs ready!
Name | Link |
---|---|
Latest commit | 0332dc9db62ca50946f9ce7bd8d4df3053b02b9b |
Latest deploy log | https://app.netlify.com/sites/ohif-platform-docs/deploys/644100c046df09000837d50e |
Deploy Preview | https://deploy-preview-2925--ohif-platform-docs.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
Deploy Preview for ohif-platform-viewer canceled.
Name | Link |
---|---|
Latest commit | 0332dc9db62ca50946f9ce7bd8d4df3053b02b9b |
Latest deploy log | https://app.netlify.com/sites/ohif-platform-viewer/deploys/644100c092c6b40008dd5687 |
@sedghi @swederik @wayfarer3130 This might be related to #2867
Codecov Report
Merging #2925 (d467c6f) into v3-stable (9f1e813) will increase coverage by
14.19%
. The diff coverage isn/a
.
:exclamation: Current head d467c6f differs from pull request most recent head 0332dc9. Consider uploading reports for the commit 0332dc9 to get more accurate results
@@ Coverage Diff @@
## v3-stable #2925 +/- ##
==============================================
+ Coverage 25.15% 39.35% +14.19%
==============================================
Files 119 97 -22
Lines 2862 2071 -791
Branches 555 430 -125
==============================================
+ Hits 720 815 +95
+ Misses 1856 1024 -832
+ Partials 286 232 -54
see 61 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
Legend - Click here to learn more
Ξ = absolute <relative> (impact)
,ΓΈ = not affected
,? = missing data
Powered by Codecov. Last update 226244a...0332dc9. Read the comment docs.
That is great and I'm sure @wayfarer3130 will love this. Thanks!
Force push to rebase
@sedghi @swederik @wayfarer3130 This might be related to #2867
No, it isn't related to 2867 - that PR allows loading of UMD JavaScript modules for modes and extensions, whereas this one just allows hot-swapping the config file
hi folks, just wanted to follow up on this PR!
Can you rebase this and we can then merge?
@sedghi rebased and should be ready for merge
@Ouwen I tried http://localhost:3000/?configUrl=http://localhost:3000/config/example.json
and it shows a black screen. I don't think it is expected right?
hmmm that's strange that it pops up the black screen. Maybe the example is outdated somehow
We are trying to contribute to this PR by adding prefixConfigUrl
which @wayfarer3130 brought up. However, after a second thought, we agree with @Ouwen that cross-origin = 'same-origin'
is the proper approach to handle cross-site scripting concern because of the following reason:
- If a hacker comes in, this new dynamic server feature will only allow him to change which server OHIF will connect to. Assuming, the server belongs to him, there is not much "security concern" since we are not leaking the data from the intended DICOM server.
- If the hacker is really that knowledgeable to change the setting and steal the data from the other DICOM servers, he should also be able to fake the
prefixConfigUrl
- Such knowledgeable hacker should also be able to steal the data from the original DICOM server from OHIF, even without this PR.
Therefore, I propose to let this PR in, with the documentation enhancement to explain how to set cross-origin = 'same-origin'
properly.
We are trying to contribute to this PR by adding
prefixConfigUrl
which @wayfarer3130 brought up. However, after a second thought, we agree with @Ouwen thatcross-origin = 'same-origin'
is the proper approach to handle cross-site scripting concern because of the following reason:
- If a hacker comes in, this new dynamic server feature will only allow him to change which server OHIF will connect to. Assuming, the server belongs to him, there is not much "security concern" since we are not leaking the data from the intended DICOM server.
- If the hacker is really that knowledgeable to change the setting and steal the data from the other DICOM servers, he should also be able to fake the
prefixConfigUrl
- Such knowledgeable hacker should also be able to steal the data from the original DICOM server from OHIF, even without this PR.
Therefore, I propose to let this PR in, with the documentation enhancement to explain how to set
cross-origin = 'same-origin'
properly.
No, that isn't the concern I have - the issue is that if the configuration specifies a valid/standard back end for the retrieve part, but specifies something like WADO URL retrieves for the retrieve part or perhaps for STOW, then one might end up retrieving valid information and storing to something not permitted. I know you only parse hte JSON, but there are places where that JSON gets used in a way that might leak PHI.
Thanks for your additional information @wayfarer3130! Let me try to understand and please correct me if I am wrong.
Supposed there is really WADO URL/STOW being set using this PR @Ouwen developed, I understand that there may be many places that JSON is used. However, how is this different than the configuration without this PR? I mean in the current set up, user can also configure a WADO and leaking PHI can still be the resulted just like people using this PR to set the server dynamically, right?
@wayfarer3130 Are you suggesting to put a regex filter for configUrl
so that it always return valid config json? Can you please help to understand more on it? @cupidchan has some valid point and your input awaiting.
I think the worry is that someone would use:
mydomain.com/?configUrl=https://mymaliciouslink.com/cookies_get.js
A regex filter would prevent the above. However, it would not prevent the following:
mydomain.com/?configUrl=$crazybytestobufferoverflowintoaneval(mycode)=>cookies_get.js
A malicious user would have to craft javascript which can escape the fetch call context (maybe with buffer overflow?). This would require proper cross-origin
settings to prevent PII data from being set out.
The regex whitelist adds some protection, but ultimately the cross-origin should probably be same-site to be safest.
Now, I see your point now @Ouwen! If I may rephrase, having the regex enhancement will NOT 100% prevent the hacker to inject malicious script. In order to prevent the true attack, we need to set the same-site
appropriately, right? If that's the case, then I suggest to put that in the document to explain this to OHIF users, instead of adding a regex check to prevent the incident partially. Another way is to add a flag (default to false) to enable this feature, so that users must be well aware they turn on this feature.
I do agree with @Ouwen thoughts and @cupidchan's suggestion for flag can be a good option. @wayfarer3130 do you agree with this logic?
@anarnoli, let me know if the above suits your needs (required rebase)
@wayfarer3130 @sedghi Could you please take a look at this PR to merge?
@Ouwen @cupidchan The v3-stable has moved forward, you need to resolve the conflicts please
Suggested changes by @wayfarer3130 covered in new PR https://github.com/OHIF/Viewers/pull/3391 to avoid conflict with v3-stable
branch.