Send RxFill to EHR only after NCPDP REMS interactions.#150
Send RxFill to EHR only after NCPDP REMS interactions.#150plarocque4 wants to merge 2 commits intodevfrom
Conversation
| @@ -275,52 +302,8 @@ router.patch('/api/updateRx/:id/pickedUp', async (req, res) => { | |||
| return; | |||
| } | |||
|
|
|||
| const rxFill = buildRxFill(newRx); | |||
| console.log('Sending RxFill per NCPDP workflow'); | |||
|
|
|||
| const config = getConfig(); | |||
|
|
|||
| if (config.useIntermediary) { | |||
| // Send to intermediary - it will forward to both EHR and REMS Admin | |||
| const endpoint = getRxFillEndpoint(); | |||
| console.log(`Sending RxFill to intermediary: ${endpoint}`); | |||
| await axios.post(endpoint, rxFill, { | |||
| headers: { 'Content-Type': 'application/xml' } | |||
| }); | |||
| } else { | |||
| // Send to EHR | |||
| try { | |||
| const ehrStatus = await axios.post(env.EHR_RXFILL_URL, rxFill, { | |||
| headers: { | |||
| Accept: 'application/xml', | |||
| 'Content-Type': 'application/xml' | |||
| } | |||
| }); | |||
| console.log('Sent RxFill to EHR, received status:', ehrStatus.data); | |||
| } catch (ehrError) { | |||
| console.log('Failed to send RxFill to EHR:', ehrError.message); | |||
| } | |||
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
In general, the problem is fixed by introducing a rate-limiting middleware for routes that perform expensive operations (database / network calls). In an Express app, the typical solution is to use a well-known middleware like express-rate-limit, configure sensible limits (e.g., max requests per IP per time window), and apply it either globally to the router or to specific sensitive routes.
For this file, the least intrusive, most effective fix is to import express-rate-limit, define a limiter tailored for doctor order update operations, and apply it specifically to the /api/updateRx/:id PATCH route (and optionally similar DB-heavy routes if desired). This avoids changing any existing functionality of the handler itself while satisfying the requirement that the route be rate-limited. Concretely:
- Add
import rateLimit from 'express-rate-limit';near the top ofbackend/src/routes/doctorOrders.jswithout altering existing imports. - Define a limiter constant, e.g.,
const updateRxLimiter = rateLimit({ windowMs: 15 * 60 * 1000, max: 100 });after the router/middleware setup. - Change the route declaration from
router.patch('/api/updateRx/:id', async (req, res) => { ... })torouter.patch('/api/updateRx/:id', updateRxLimiter, async (req, res) => { ... }), so each client IP can only hit this endpoint a bounded number of times per window.
This single limiter on the route will cover all three database accesses within the handler because the middleware runs before the handler executes.
| @@ -17,6 +17,7 @@ | ||
| import { NewRx } from '../database/schemas/newRx.js'; | ||
| import { medicationRequestToRemsAdmins } from '../database/data.js'; | ||
| import { getConfig, updateConfig, getNCPDPEndpoint, getRxFillEndpoint } from '../lib/pharmacyConfig.js'; | ||
| import rateLimit from 'express-rate-limit'; | ||
|
|
||
| bpx(bodyParser); | ||
| router.use( | ||
| @@ -30,6 +31,11 @@ | ||
| ); | ||
| router.use(bodyParser.urlencoded({ extended: false })); | ||
|
|
||
| const updateRxLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 updateRx requests per windowMs | ||
| }); | ||
|
|
||
| const XML2JS_OPTS = { | ||
| explicitArray: false, | ||
| trim: true, | ||
| @@ -152,7 +158,7 @@ | ||
| * Route: 'doctorOrders/api/updateRx/:id' | ||
| * Description : 'Updates prescription based on mongo id, sends NCPDP REMSRequest for authorization' | ||
| */ | ||
| router.patch('/api/updateRx/:id', async (req, res) => { | ||
| router.patch('/api/updateRx/:id', updateRxLimiter, async (req, res) => { | ||
| try { | ||
| const order = await doctorOrder.findById(req.params.id).exec(); | ||
| console.log('Found doctor order by id! --- ', order); |
| @@ -13,7 +13,8 @@ | ||
| "mongoose": "^8.9.5", | ||
| "var": "^0.4.0", | ||
| "web-vitals": "^2.1.4", | ||
| "xml2js": "^0.6.0" | ||
| "xml2js": "^0.6.0", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/chai": "^4.3.4", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
backend/src/routes/doctorOrders.js
Outdated
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
Generally, to fix this kind of SSRF risk you must ensure that any configuration affecting outbound URLs cannot be set to arbitrary values by untrusted input. This can be done by (a) validating user-provided configuration against strict rules (e.g., allow only https, disallow private IPs/hosts, optional allow-list) and/or (b) preventing certain sensitive fields (like endpoint base URLs) from being set via user-facing APIs at all.
For this codebase, the least intrusive, functionality-preserving fix is to harden updateConfig in backend/src/lib/pharmacyConfig.js so that only a controlled subset of fields can be updated, and only with safe values. Specifically:
- Define a small allow-list of configurable keys; for example, allow boolean
useIntermediarybut block arbitrary updates to URL fields fromreq.body. - Optionally add format validation for any URL fields you really do need to update at runtime (e.g., require
https://and a host that is not localhost or a private IP). Because we can’t assume other project parts, the safest is to block URL changes from the API completely and rely on environment variables for these. - Implement this by having
updateConfigconstruct a sanitizedsafeConfigfromnewConfig, copying only allowed keys, and ignoring the rest.
Concretely:
- Edit
backend/src/lib/pharmacyConfig.js, lines 17–21, to:- Introduce a list of allowed keys (e.g., only
useIntermediary). - Build a
sanitizedConfigobject with only those keys and with minimal type coercion (e.g., convert"true"/"false"strings to booleans). - Merge
sanitizedConfigintoconfiginstead of mergingnewConfigdirectly.
- Introduce a list of allowed keys (e.g., only
- Leave
doctorOrders.js’ssendRxFillfunction unchanged; it still callsgetRxFillEndpoint(), but now that endpoint can no longer be redirected arbitrarily by untrusted configuration updates.
This preserves existing behavior for safe fields (like toggling useIntermediary via the API) while preventing a user from changing intermediaryUrl, remsAdminUrl, or ehrUrl to arbitrary attacker-controlled hosts.
| @@ -15,7 +15,26 @@ | ||
|
|
||
|
|
||
| export function updateConfig(newConfig) { | ||
| config = { ...config, ...newConfig }; | ||
| // Only allow specific, safe fields to be updated from external input. | ||
| // URL fields (intermediaryUrl, remsAdminUrl, ehrUrl) remain controlled by environment/configuration, | ||
| // not by the API, to avoid SSRF risks. | ||
| const sanitizedConfig = {}; | ||
|
|
||
| if (Object.prototype.hasOwnProperty.call(newConfig, 'useIntermediary')) { | ||
| // Coerce to boolean where possible | ||
| const value = newConfig.useIntermediary; | ||
| if (typeof value === 'boolean') { | ||
| sanitizedConfig.useIntermediary = value; | ||
| } else if (typeof value === 'string') { | ||
| if (value.toLowerCase() === 'true') { | ||
| sanitizedConfig.useIntermediary = true; | ||
| } else if (value.toLowerCase() === 'false') { | ||
| sanitizedConfig.useIntermediary = false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| config = { ...config, ...sanitizedConfig }; | ||
| console.log('Configuration updated:', config); | ||
| return { ...config }; | ||
| } |
backend/src/routes/doctorOrders.js
Outdated
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
In general, to fix SSRF caused by user‑controlled configuration, you must prevent untrusted clients from setting arbitrary URLs used in outbound HTTP calls. That can be done by: (1) restricting which configuration fields can be updated at runtime, (2) validating URL fields carefully (scheme, host allow‑list, no localhost/metadata IPs, etc.), or (3) removing dynamic configuration of such URLs entirely and sourcing them only from trusted environment variables or static config.
The best minimal fix here, without changing intended functionality, is to validate and sanitize the newConfig object in updateConfig before merging it into config. Specifically: ensure useIntermediary is just a boolean; for each URL field (intermediaryUrl, remsAdminUrl, ehrUrl) either ignore updates from the client or strictly validate them so they cannot point to internal/loopback/metadata addresses, and require http or https scheme and a normal hostname. To keep behavior flexible but safe, we can: (a) only allow updates to a small subset of fields from /api/config; and (b) for URLs, parse them with Node’s standard URL class, reject invalid URLs or ones with disallowed hosts, and possibly restrict host to an allow‑list pattern (e.g., only specific domains). Since we must not assume external code, we will implement a small internal validator in backend/src/lib/pharmacyConfig.js and call it inside updateConfig to produce a sanitized merged config object.
Concretely:
- In
backend/src/lib/pharmacyConfig.js, add a helper likesanitizeConfigUpdate(newConfig)that:- Creates a copy of the existing
config. - If
newConfig.useIntermediaryis present, coerces it to a boolean if it’s"true"/"false"or a boolean. - Option A (safest and simplest): completely ignore client updates to
intermediaryUrl,remsAdminUrl, andehrUrlso they remain environment‑defined and not attacker‑controlled. - Option B (more flexible, but more logic): if we choose to allow URL updates, parse them with
new URL(value), ensure protocol ishttp:orhttps:, and ensure the hostname is notlocalhost, not in127.0.0.0/8, not::1, and not in the 169.254.169.254 metadata pattern or private ranges. To keep the patch small and robust, I’ll use Option A and simply refuse to override the URL fields fromreq.body, which fully breaks the taint flow togetRxFillEndpoint().
- Creates a copy of the existing
- Change
updateConfig(newConfig)to callsanitizeConfigUpdate(newConfig), assign the sanitized result toconfig, and return a copy.
No changes are needed in doctorOrders.js beyond this, because once config.intermediaryUrl (and the other URLs) can’t be set from req.body, getRxFillEndpoint() no longer depends on untrusted input and the SSRF sink is neutralized.
| @@ -7,15 +7,48 @@ | ||
| ehrUrl: process.env.EHR_NCPDP_URL | ||
| }; | ||
|
|
||
| /** | ||
| * Sanitize incoming configuration updates so that untrusted clients | ||
| * cannot override sensitive URL fields used for outbound requests. | ||
| */ | ||
| function sanitizeConfigUpdate(newConfig) { | ||
| if (!newConfig || typeof newConfig !== 'object') { | ||
| return { ...config }; | ||
| } | ||
|
|
||
| const updated = { ...config }; | ||
|
|
||
| // Only allow updating useIntermediary; coerce typical string forms to boolean. | ||
| if (Object.prototype.hasOwnProperty.call(newConfig, 'useIntermediary')) { | ||
| const val = newConfig.useIntermediary; | ||
| if (typeof val === 'boolean') { | ||
| updated.useIntermediary = val; | ||
| } else if (typeof val === 'string') { | ||
| if (val.toLowerCase() === 'true') { | ||
| updated.useIntermediary = true; | ||
| } else if (val.toLowerCase() === 'false') { | ||
| updated.useIntermediary = false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Ignore any attempts to update URL fields from the request body to | ||
| // avoid server-side request forgery through dynamic configuration. | ||
| // - intermediaryUrl | ||
| // - remsAdminUrl | ||
| // - ehrUrl | ||
|
|
||
| return updated; | ||
| } | ||
|
|
||
|
|
||
| export function getConfig() { | ||
| return { ...config }; | ||
| } | ||
|
|
||
|
|
||
| export function updateConfig(newConfig) { | ||
| config = { ...config, ...newConfig }; | ||
| config = sanitizeConfigUpdate(newConfig); | ||
| console.log('Configuration updated:', config); | ||
| return { ...config }; | ||
| } |
Describe your changes
Send RxFill to EHR only after NCPDP REMS interactions.
This has corresponding Pull Requests in rems-intermediary, request-generator, test-ehr.
Issue ticket number and Jira link
869
Checklist before requesting a review
devnot main (the only exception to this is releases fromdevand hotfix branches)Checklist for conducting a review
Workflow
Owner of the Pull Request will be responsible for merge after all requirements are met, including approval from at least one reviewer. Additional changes made after a review will dismiss any approvals and require re-review of the additional updates. Auto merging can be enabled below if additional changes are likely not to be needed. The bot will auto assign reviewers to your Pull Request for you.