smart-on-fhir / bulk-data-server

Other
47 stars 15 forks source link

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

Open jtmarmon opened 7 months ago

jtmarmon commented 7 months ago

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

https://github.com/smart-on-fhir/bulk-data-server/blob/0cea99318a7a7d153083ef3b8156143c1bd2bcb4/transforms/dbRowTranslator.ts#L101-L119

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 commented 7 months ago

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 commented 3 months ago

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

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 commented 2 months ago

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.