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.

Sign up for GitHub

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

Jump to bottom

Location URI is wrong when resource id begins with resource type name #2965

Closed
lmsurpre opened this issue Nov 9, 2021 · 1 comment
Closed

Location URI is wrong when resource id begins with resource type name #2965

lmsurpre opened this issue Nov 9, 2021 · 1 comment
Assignees
@lmsurpre
Labels
bug Something isn't working
Milestone
Sprint 2021-15

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Nov 9, 2021

Describe the bug
A clear and concise description of what the bug is.

Environment
main

To Reproduce
Steps to reproduce the behavior:

  1. issue a PUT to {{base}}/Patient/{{id}} with an id like "Patient1"
  2. note the location header in the response

The resource is created successfully, but the location header in the response has an extra /Patient:
https://localhost:9443/fhir-server/api/v4/Patient/Patient/Patient1/_history/1

Expected behavior
The location header should point to the proper absolute url of the created resource

Additional context
When we try to construct the baseUrl from the request, we mistake the /Patient1 for the /Patient segment of the path.
@lmsurpre lmsurpre added the bug Something isn't working label Nov 9, 2021
@lmsurpre lmsurpre mentioned this issue Nov 9, 2021
Closed
@lmsurpre lmsurpre self-assigned this Nov 9, 2021
lmsurpre added a commit that referenced this issue Nov 9, 2021
@lmsurpre
issue #2965 - use the first instance of /resourceType and not the last
b565fe0 
when inferring the baseUrl from the request URL.

This should be relatively safe because the match is case-sensitive.
However, the logic would break if the intended baseUrl of the server
actually contains a path segment that overlaps with a resource name
(e.g. https://example.com/PatientAPI ).

Alternatives would be to either
A. rely solely on a configured baseUrl; or
B. do more processing of the URL to ensure we're stripping a path and
not a hostname

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre added this to the Sprint 2021-15 milestone Nov 9, 2021
lmsurpre added a commit that referenced this issue Nov 10, 2021
@lmsurpre
issue #2965 - use the first instance of /resourceType and not the last
a8f7179 
when inferring the baseUrl from the request URL.

This should be relatively safe because the match is case-sensitive.
However, the logic would break if the intended baseUrl of the server
actually contains a path segment that overlaps with a resource name
(e.g. https://example.com/PatientAPI ).

Alternatives would be to either
A. rely solely on a configured baseUrl; or
B. do more processing of the URL to ensure we're stripping a path and
not a hostname

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Nov 10, 2021
@lmsurpre
issue #2965 - modify getRequestBaseUri logic
8e17122 
Previously, we looked for the last instnace of `/[resourceType]` in the
path and assumed that was the first thing after the baseUrl. However, if
a resourceId happened to match the resource type, this proved erroneous.

The updated logic looks for the first instance of `/[resourceType]/` in
the path instead.
If that doesn't exist, then we fall back to the old approach of using
the last index of `/[resourceType]` (now that we know it should be safe
to do so).

This updated logic could break if someone insisted on including a
baseUrl that includes a path segment that matches `/[resourceType]/`
(e.g. https://example.com/my/Patient/api/ ) but I think that is an
acceptable risk because that would be very dumb to do.

Alternatives would be to either
A. rely solely on a configured baseUrl; or
B. do more processing of the URL to ensure we're stripping a path and
not a hostname

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Nov 10, 2021
@lmsurpre
issue #2965 - document known limitation for custom base urls
760db8e 
and add a test to OriginalRequestRewriteServerTest for the exact
examples used in the docs

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Nov 10, 2021
@lmsurpre
issue #2965 - document known limitation for custom base urls
3a27bb2 
and add a test to OriginalRequestRewriteServerTest for the exact
examples used in the docs

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Nov 10, 2021
@lmsurpre
issue #2965 - modify getRequestBaseUri logic (#2966)
44b0d80 
* issue #2965 - modify getRequestBaseUri logic

Previously, we looked for the last instnace of `/[resourceType]` in the
path and assumed that was the first thing after the baseUrl. However, if
a resourceId happened to match the resource type, this proved erroneous.

The updated logic looks for the first instance of `/[resourceType]/` in
the path instead.
If that doesn't exist, then we fall back to the old approach of using
the last index of `/[resourceType]` (now that we know it should be safe
to do so).

This updated logic could break if someone insisted on including a
baseUrl that includes a path segment that matches `/[resourceType]/`
(e.g. https://example.com/my/Patient/api/ ) but I think that is an
acceptable risk because that would be very dumb to do.

Alternatives would be to either
A. rely solely on a configured baseUrl; or
B. do more processing of the URL to ensure we're stripping a path and
not a hostname

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* issue #2965 - document known limitation for custom base urls

and add a test to OriginalRequestRewriteServerTest for the exact
examples used in the docs

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@d0roppe
Copy link
Collaborator

d0roppe commented Nov 12, 2021

Verified this no longer happens. Verified with 3 different resource types.

@d0roppe d0roppe closed this as completed Nov 12, 2021
@lmsurpre lmsurpre mentioned this issue Mar 24, 2022
Closed
lmsurpre added a commit that referenced this issue Mar 24, 2022
@lmsurpre
issue #3501 - apply fix for #2965 more universally
93ea3f7 
We had duplicate logic in two classes:
* FHIRResource.getRequestBaseUri
* FHIRRestHelper.getRequestBaseUri

For #2965, we fixed the version in FHIRResource but missed the one in
FHIRRestHelper.

Now I combined the two implementations into FHIRRestHelper, made it
static, and removed the version from FHIRResource.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre mentioned this issue Mar 24, 2022
Merged
lmsurpre added a commit that referenced this issue Mar 24, 2022
@lmsurpre
issue #3501 - apply fix for #2965 more universally
c7af108 
We had duplicate logic in two classes:
* FHIRResource.getRequestBaseUri
* FHIRRestHelper.getRequestBaseUri

For #2965, we fixed the version in FHIRResource but missed the one in
FHIRRestHelper.

Now I combined the two implementations into FHIRRestHelper, made it
static, and removed the version from FHIRResource.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 24, 2022
@lmsurpre
issue #3501 - apply fix for #2965 more universally
5f97f94 
We had duplicate logic in two classes:
* FHIRResource.getRequestBaseUri
* FHIRRestHelper.getRequestBaseUri

For #2965, we fixed the version in FHIRResource but missed the one in
FHIRRestHelper.

Now I combined the two implementations into FHIRRestHelper, made it
static, and removed the version from FHIRResource.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 25, 2022
@lmsurpre
issue #3501 - apply fix for #2965 more universally
43a14de 
We had duplicate logic in two classes:
* FHIRResource.getRequestBaseUri
* FHIRRestHelper.getRequestBaseUri

For #2965, we fixed the version in FHIRResource but missed the one in
FHIRRestHelper.

Now I combined the two implementations into FHIRRestHelper, made it
static, and removed the version from FHIRResource.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 30, 2022
@lmsurpre
Merge pull request #3506 from IBM/issue-3501
8907021 
issue #3501 - apply fix for #2965 more universally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lmsurpre lmsurpre

Labels
bug Something isn't working
Projects
None yet
Milestone
Sprint 2021-15
Development

No branches or pull requests
2 participants
@lmsurpre @d0roppe