Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite DocumentReference URLs to the export base url, not the default base URL #72

Open
jtmarmon opened this issue Apr 17, 2024 · 3 comments

Comments

@jtmarmon
Copy link

Attachment URLs currently point at the configured "base URL" of the FHIR server:

// Rewrite urls in DocumentReference resources. Only url props
// that begin with `/files/` will be converted to absolute HTTP
// URLs to allow the client to directly download bigger files
if (row.resource_json.resourceType == "DocumentReference") {
const url = getPath(row.resource_json, "content.0.attachment.url");
if (url && url.search(/\/attachments\/.*/) === 0) {
row.resource_json.content[0].attachment.url = buildUrlPath(
baseUrl,
base64url.encode(JSON.stringify({
err : sim.err || "",
secure: !!sim.secure
})),
"fhir",
url
);
}
}
}

However, in practice the real Base URL for this server can change based on the parameters one inputs into https://bulk-data.smarthealthit.org/

This means that the attachment URLs are pointing at a different FHIR Base URL than the FHIR server that you initiate export from. For example, this is the URL generated from the website:
https://bulk-data.smarthealthit.org/eyJlcnIiOiIiLCJwYWdlIjoxMDAwMDAsImR1ciI6MTAsInRsdCI6MTUsIm0iOjEsInN0dSI6MywiZGVsIjowfQ/fhir
and this is the URL that the attachments point at:
https://bulk-data.smarthealthit.org/eyJlcnIiOiIiLCJzZWN1cmUiOnRydWV9/fhir/attachments/DICOM.jpg

Because these looks to a naive piece of code like different FHIR servers, it's generally not safe to send your SMART authentication token to this server. Many FHIR servers host their images on external image servers like S3, and you wouldn't want to give external servers your token, so our code specifically does not send an authentication token to attachment URLs that aren't hosted on the same FHIR server.

Would it be possible to dynamically rewrite this URL to point to the same URL as the one the export came from, rather than the default configured one?

@jtmarmon
Copy link
Author

Just to add a little extra color - obviously the domain host is the same, but it doesn't seem safe to rely on that as many sites host their FHIR servers at subpaths of a domain like /foo/bar/fhir where there are other servers hosted on that domain. We wouldn't want to accidentally send our FHIR credentials to a marketing site that happens to be on the same domain and have it show up in the logs there.

@vlad-ignatov
Copy link
Contributor

Thank you for reporting this. It is a good catch! Unfortunately, I don't think it can be fixed without loosing functionality.

  • First and foremost, this is just a demo project used to demonstrate what could be done with bulk data using fake patient data. It must NEVER be used in production, therefore security is not a real concern.
  • The problem is that we are trying to overload the spec with additional functionality and a little bit of magic. The urls look like https://bulk-data.smarthealthit.org/sim/fhir/fhir-path.... The sim segment contains what I would describe as encoded behavior instructions. For example, it might be telling the server to throw an error when you download that attachment. For that reason the sim segment cannot always be the same as what you see in the export url.

Again, you can borrow ideas from this project, or use it for demos, or test your client against it, but please don't build anything on top of it because it is just a tool to simulate the behavior of the FHIR servers. In fact, it was created back when Bulk Data was only an idea, to show how would it feel if the EHR vendors decide to support it some day...

@jtmarmon
Copy link
Author

jtmarmon commented Sep 5, 2024

Thanks for the reply Vlad! To be clear, we definitely don't use it in production, of course. It's a super helpful tool for us for testing and development. I'm not pointing out any security issue with the app itself, moreso that to use it you have to apply logic that is insecure (ie allowing your client to send your FHIR server token to a different server).

Totally fine not to patch as I understand this is just meant for testing, just wanted to point it out. We've special cased this server in our authentication logic to get around it.

That being said, one simple way to solve this would be to not require authentication for the /attachments endpoints. Many FHIR servers in the wild have their attachment URLs pointing at presigned S3 URLs that don't require authentication. Removing the need for auth would mirror that pattern and prevent encouraging those developing against this app from unintentionally writing insecure authentication logic into their app.

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

No branches or pull requests

2 participants