feat: query string dynamic app config

[Ouwen]

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

[netlify[bot]]

✅ 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.

[netlify[bot]]

✅ 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

[Ouwen]

@sedghi @swederik @wayfarer3130 This might be related to #2867

[codecov[bot]]

Codecov Report

Merging #2925 (d467c6f) into v3-stable (9f1e813) will increase coverage by 14.19%. The diff coverage is n/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.

[swederik]

That is great and I'm sure @wayfarer3130 will love this. Thanks!

[Ouwen]

Force push to rebase

[wayfarer3130]

@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

[Ouwen]

hi folks, just wanted to follow up on this PR!

[sedghi]

Can you rebase this and we can then merge?

[Ouwen]

@sedghi rebased and should be ready for merge

[sedghi]

@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?

[Ouwen]

hmmm that's strange that it pops up the black screen. Maybe the example is outdated somehow

[cupidchan]

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:

1. 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.
2. 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
3. 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.

[wayfarer3130]

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:

1. 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.
2. 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
3. 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.

[cupidchan]

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?

[anarnoli]

@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.

[Ouwen]

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.

[cupidchan]

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.

[anarnoli]

I do agree with @Ouwen thoughts and @cupidchan's suggestion for flag can be a good option. @wayfarer3130 do you agree with this logic?

[Ouwen]

@anarnoli, let me know if the above suits your needs (required rebase)

[anarnoli]

@wayfarer3130 @sedghi Could you please take a look at this PR to merge?

[sedghi]

@Ouwen @cupidchan The v3-stable has moved forward, you need to resolve the conflicts please

[anarnoli]

Suggested changes by @wayfarer3130 covered in new PR https://github.com/OHIF/Viewers/pull/3391 to avoid conflict with v3-stable branch.