Skip to content

fix(config url): Hardening fetch options. - #5985

Merged
jbocce merged 2 commits into
OHIF:masterfrom
jbocce:fix/hardening
Apr 29, 2026
Merged

fix(config url): Hardening fetch options.#5985
jbocce merged 2 commits into
OHIF:masterfrom
jbocce:fix/hardening

Conversation

@jbocce

@jbocce jbocce commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Context

Hardened fetch of DicomJSON and DicomWebProxy config url fetching.

Changes & Results

Use stricter fetch options.

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

System:
OS: Windows 11 10.0.26200
CPU: (20) x64 12th Gen Intel(R) Core(TM) i7-12700H
Memory: 3.86 GB / 31.68 GB
Binaries:
Node: 22.17.0 - C:\Users\joebo\AppData\Local\fnm_multishells\7300_1777381936598\node.EXE
Yarn: 1.22.22 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
npm: 11.6.2 - C:\Users\joebo\AppData\Local\fnm_multishells\7300_1777381936598\npm.CMD
bun: 1.2.23 - C:\Users\joebo.bun\bin\bun.EXE
Browsers:
Chrome: 147.0.7727.117
Edge: Chromium (146.0.3856.84)
Internet Explorer: 11.0.26100.8115

Greptile Summary

This PR simplifies fetchConfigJson to apply a single set of hardened fetch options (credentials: 'same-origin', redirect: 'error', referrerPolicy: 'no-referrer') to all config URL fetches instead of branching on isAuthenticated/isSameOrigin. Tests are updated accordingly.

  • The unified redirect: 'error' is a behavioural change: authenticated and same-origin requests previously used bare fetch() (i.e. redirect: 'follow'), so any deployment whose config URL involves a redirect on those paths will now fail.

Confidence Score: 4/5

Safe to merge with the awareness that redirect: 'error' now covers previously unrestricted paths.

One P2 finding: extending redirect: 'error' to authenticated/same-origin requests is an intentional hardening but could silently break deployments using redirecting config URLs. No P0/P1 issues found; tests are correctly updated.

extensions/default/src/utils/secureConfigFetch.js — review the redirect behaviour change for authenticated/same-origin paths.

Important Files Changed

Filename Overview
extensions/default/src/utils/secureConfigFetch.js Unified fetch options applied to all requests; redirect: 'error' now also covers authenticated/same-origin paths (previously redirect: 'follow'), which is a breaking change for redirecting config URLs. isAuthenticated/isSameOrigin still returned but unused in fetchConfigJson.
extensions/default/src/utils/secureConfigFetch.test.js Test cases correctly updated to match new unified fetch options with credentials: 'same-origin'; all three fetchConfigJson test paths now assert the hardened options object.

Comments Outside Diff (1)

  1. extensions/default/src/utils/secureConfigFetch.js, line 91-96 (link)

    P2 Dead return fields in resolveConfigFetchPolicy

    isAuthenticated (line 94) and isSameOrigin (line 95) are still computed and returned by resolveConfigFetchPolicy, but fetchConfigJson no longer destructures or uses them. These two fields in the returned object are now unused dead code. Consider removing them from the return value (the variables themselves are still needed internally for the origin-check guard at line 82).

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: extensions/default/src/utils/secureConfigFetch.js
    Line: 91-96
    
    Comment:
    **Dead return fields in `resolveConfigFetchPolicy`**
    
    `isAuthenticated` (line 94) and `isSameOrigin` (line 95) are still computed and returned by `resolveConfigFetchPolicy`, but `fetchConfigJson` no longer destructures or uses them. These two fields in the returned object are now unused dead code. Consider removing them from the return value (the variables themselves are still needed internally for the origin-check guard at line 82).
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/default/src/utils/secureConfigFetch.js
Line: 101-107

Comment:
**`redirect: 'error'` now blocks previously-allowed redirects**

Before this change, authenticated and same-origin requests used bare `fetch(url)` which defaults to `redirect: 'follow'`. Only unauthenticated cross-origin requests had `redirect: 'error'`. Now all requests reject on any redirect, including HTTP→HTTPS or path-normalizing redirects for same-origin config URLs. Any deployment that currently relies on a redirecting config URL in an authenticated or same-origin context will silently break with a network error after this change. If this tightening is intentional, it's worth documenting as a breaking change for operators.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (2): Last reviewed commit: "Update unit tests." | Re-trigger Greptile

@netlify

netlify Bot commented Apr 28, 2026

Copy link
Copy Markdown

Deploy Preview for ohif-dev canceled.

Name Link
🔨 Latest commit 366a816
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/69f1135de3abcc0008c3f779

Comment thread extensions/default/src/utils/secureConfigFetch.js
@jbocce
jbocce requested review from sedghi and wayfarer3130 April 28, 2026 20:24
@jbocce
jbocce merged commit 468e573 into OHIF:master Apr 29, 2026
8 checks passed
@jbocce
jbocce deleted the fix/hardening branch April 29, 2026 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants