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

link urls are wrong when originalRequestUriHeader contains unencoded pipe #1907

Closed
lmsurpre opened this issue Feb 4, 2021 · 2 comments · Fixed by #1949
Closed

link urls are wrong when originalRequestUriHeader contains unencoded pipe #1907

lmsurpre opened this issue Feb 4, 2021 · 2 comments · Fixed by #1949
Assignees
Labels
bug Something isn't working

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Feb 4, 2021

Describe the bug
We are fronting the FHIR server with a reverse proxy and setting an originalRequestUriHeader to the frontend URI that was requested to the gateway.

On some requests, the search bundle response links come back with links that match our front-end url as expected. For example:
https://frontend-host/Observation?_count=10&code=57698-3&_page=10

But for other requests, its coming back with the backend server url which is not what we want:
https://backend-host/fhir-server/api/v4/Observation?_count=10&code=http://loinc.org|57698-3&_page=10

To Reproduce
Steps to reproduce the behavior:

  1. Configure IBM FHIR Server with an originalRequestUriHeaderName
  2. Make a request to the server and set this header to a value with an unescaped |
  3. Note that the URL that comes back has the backend-url and not the desired front-end URL

Expected behavior
All generated links should have the frontend-url in them, not the backend-url

Additional context
Specifically, its failing on URI requestUri = new URI(requestUriString); in UriBuilder.toSearchSelfUri.
If the originalRequestUriHeader value was properly encoded, it would work as expected. However, we should be more robust in our handling of cases where it is not. Furthermore, we don't even use the query portion of the originalRequestUriHeader (just the hostname and path), and so we should be able to avoid most encoding issues just by stripping off the query portion.

@lmsurpre lmsurpre added the bug Something isn't working label Feb 4, 2021
@lmsurpre lmsurpre changed the title link urls are wrong when originalRequestUriHeader contains unencoded chars link urls are wrong when originalRequestUriHeader contains unencoded pipe Feb 4, 2021
@lmsurpre lmsurpre added this to the Sprint 2021-02 milestone Feb 4, 2021
lmsurpre added a commit that referenced this issue Feb 4, 2021
…ng it

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre self-assigned this Feb 4, 2021
@d0roppe
Copy link
Collaborator

d0roppe commented Feb 15, 2021

I tested a few scenarios where this X-FHIR-FORWARDED-URI header produces really odd behavior
1907.xlsx
here is a file with the 4 odd behaviors all of these seem to be wrong. all data is mock data

@lmsurpre
Copy link
Member Author

lmsurpre commented Feb 16, 2021

I confirm that the problem in the description of this issue still exists. I did address it in one place (as tested in our updated UriBuilderTest), but I missed the fact that we also try to parse the incoming header value within FHIRRestServletFilter.

As for the strange behavior on the fullUrl values of a search response bundle, I would consider that a separate (but related) issue.
The originalRequestUri header value must be set appropriate for the incoming request, otherwise you're going to get strange results like this.
However, we have pretty limited documentation around what an "appropriate" header value would be, and absolutely no documentation wrt what the behavior will be (nor what it should be) for inappropriate header values.

Here is the extent of the current documentation:

This optional config parameter is provided for cases where the server is deployed behind a reverse proxy that overwrites the host and/or path portions of the original request.

Additionally @d0roppe and I exchanged ideas for ways to improve on the design of this feature. Because our only use case for this is to override the FHIR Base URL, we feel that it would be simpler (and with fewer edge cases) to deprecate the originalRequestUriHeaderName parameter and replace it with either:
A. a new parameter named fhirBaseUrlHeaderName, where instead of parsing the value of this header into a URI, we just always use its full text value as the baseUrl of the FHIR server for the scope of the request; and/or
B. a new parameter named fhirBaseUrl which configures the server to use a specific baseUrl for all requests (or at least all requests for a single tenant).

Such a design change could be larger than we'd like at this point and so I propose we debate that in a separate issue.

For this issue, I will:

  1. submit a pull request that fixes the specific case mentioned in the issue's description (an originalRequestUri header value that includes unescaped chars like | in the query string); and
  2. add some documentation to the FHIRUsersGuide to indicate how to appropriately set the header

lmsurpre added a commit that referenced this issue Feb 16, 2021
Previously I updated our UriBuilder for this case (and introduced tests
to ensure it is properly functioning). However, I missed the fact that
FHIRREstServletFilter parses the originalRequestUri header value into a
URI and, when this fails, it simply reverts to the actual URL of the
request.

Also performed some minor cleanup.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Feb 16, 2021
Previously I updated our UriBuilder for this case (and introduced tests
to ensure it is properly functioning). However, I missed the fact that
FHIRREstServletFilter parses the originalRequestUri header value into a
URI and, when this fails, it simply reverts to the actual URL of the
request.

Also performed some minor cleanup.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Feb 16, 2021
Previously I updated our UriBuilder for this case (and introduced tests
to ensure it is properly functioning). However, I missed the fact that
FHIRREstServletFilter parses the originalRequestUri header value into a
URI and, when this fails, it simply reverts to the actual URL of the
request.

Also performed some minor cleanup.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Feb 16, 2021
Previously I updated our UriBuilder for this case (and introduced tests
to ensure it is properly functioning). However, I missed the fact that
FHIRREstServletFilter parses the originalRequestUri header value into a
URI and, when this fails, it simply reverts to the actual URL of the
request.

Also performed some minor cleanup.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Feb 17, 2021
Previously I updated our UriBuilder for this case (and introduced tests
to ensure it is properly functioning). However, I missed the fact that
FHIRREstServletFilter parses the originalRequestUri header value into a
URI and, when this fails, it simply reverts to the actual URL of the
request.

Also performed some minor cleanup.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Feb 17, 2021
Previously I updated our UriBuilder for this case (and introduced tests
to ensure it is properly functioning). However, I missed the fact that
FHIRREstServletFilter parses the originalRequestUri header value into a
URI and, when this fails, it simply reverts to the actual URL of the
request.

Also performed some minor cleanup.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Feb 18, 2021
issues #1907 and #1964 - strip the query from originalRequestUri before parsing and fix baseUrl for whole-system queries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants