hapifhir / org.hl7.fhir.validator-wrapper

CLI, Desktop GUI, and Standalone Server for the FHIR Validator
Apache License 2.0
28 stars 15 forks source link

CORS config preventing validation #177

Open jmandel opened 4 months ago

jmandel commented 4 months ago

It seems like CORS support for validation is intended, from https://github.com/hapifhir/org.hl7.fhir.validator-wrapper/blob/d830da327b48c5a84c3b337bee14cbeddb3539d3/src/jvmMain/kotlin/Module.kt#L66-L78

But currently Access-Control-Allow-Origin response headers omit POST.

Example reproduction:

$ curl -vvv 'https://validator.fhir.org/validate'   -X 'OPTIONS'   -H 'Accept: */*'   -H 'Accept-Language: en-US,en;q=0.9'   -H 'Access-Control-Request-Headers: content-type'   -H 'Access-Control-Request-Method: POST'   -H 'Cache-Control: no-cache'   -H 'Connection: keep-alive'   -H 'Origin: http://localhost:5173'   -H 'Pragma: no-cache'   -H 'Referer: http://localhost:5173/'   -H 'Sec-Fetch-Dest: empty'   -H 'Sec-Fetch-Mode: cors'   -H 'Sec-Fetch-Site: cross-site'   -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/124.0.0.0 Safari/537.36'

Result:

< Access-Control-Allow-Origin: http://localhost:5173
< Access-Control-Allow-Credentials: true
< Access-Control-Allow-Methods: DELETE, OPTIONS, PATCH, PUT
< Access-Control-Allow-Headers: Access-Control-Allow-Origin, Authorization, Content-Type
< Access-Control-Max-Age: 86400

Expected:

< Access-Control-Allow-Methods: DELETE, OPTIONS, PATCH, POST, PUT

Adding allowMethod(HttpMethod.Post) to the CORS config block does not help (and shouldn't according to the docs, because POST is in the default config).

The config looks ok to me.

Looks like we're using a 2 year old version of ktor though.

dotasek commented 4 months ago

@jmandel if the config is correct, maybe I should take a look at the nginx config on the server to see if it's messing this up. I can't think of an immediately easy way to check cross origin with a local build, but I'm sure there's something.

And yes, this is an older version of ktor. The entire project needs a full update, but this is a bit of a hairy process. See these notes:

https://github.com/hapifhir/org.hl7.fhir.validator-wrapper/blob/d830da327b48c5a84c3b337bee14cbeddb3539d3/gradle.properties#L12

jmandel commented 4 months ago

No, my curl reproduction shows the problem (no POST in the allowed methods list) even locally, against a server launched via grandle run without any nginx in the mix. YMMV, so worth double-checking this result.

dotasek commented 4 months ago

OK, after a re-read and some exploration, I don't think this is an issue:

https://stackoverflow.com/questions/71409753/do-browsers-block-post-requests-if-post-isn-t-in-the-access-control-allow-method

GET, HEAD, or POST do not appear in Access-Control-Allow-Methods because there is no requirement that they are explicitly listed.

jmandel commented 4 months ago

Thanks, that's a good reference! Still my local web app example fails with cors errors. Let me explore whether there is some other reason.

jmandel commented 4 months ago

OK, so one issue that's making this challenging to debug is that the server appears to restart (or ... something, returning 503s for several seconds) when an invalid request is submitted.

(Additionally, the server doesn't return CORS headers with 5xx messages, so the client can't see error details.)

Is this restart issue known behavior?

jmandel commented 4 months ago

Ooh, even more pertinent: sending a pre-flight OPTIONS request, which is automatic/required behavior for CORS, triggers the server to restart, which causes the subsequent POST to fail.

Reproduction using just OPTIONS; the server is "down" by the time the 2nd request is sent:

curl -vvv 'https://validator.fhir.org/validate' \
  -X 'OPTIONS' \
  -H 'Access-Control-Request-Headers: content-type' \
  -H 'Access-Control-Request-Method: POST' \
  -H 'Origin: http://localhost:5173' \
  -H 'Pragma: no-cache' \
  -H 'Referer: http://localhost:5173/' \
  -H 'Sec-Fetch-Dest: empty' \
  -H 'Sec-Fetch-Mode: cors' \
  -H 'Sec-Fetch-Site: cross-site'; \
curl -vvv 'https://validator.fhir.org/validate' \
  -X 'OPTIONS' \
  -H 'Access-Control-Request-Headers: content-type' \
  -H 'Access-Control-Request-Method: POST' \
  -H 'Origin: http://localhost:5173' \
  -H 'Pragma: no-cache' \
  -H 'Referer: http://localhost:5173/' \
  -H 'Sec-Fetch-Dest: empty' \
  -H 'Sec-Fetch-Mode: cors' \
  -H 'Sec-Fetch-Site: cross-site'

You'll see a valid required followed by a 503.

dotasek commented 4 months ago

I will take a look. One thing this could be is that I set up some limits to connections that are present on the server to prevent abuse.

Two things to try:

Does this behaviour replicate with any OPTIONS request, maybe a totally stripped down version of what you sent?

Can you try this, but point to localhost, where it doesn't live behind NGINX (and might leave a very clear log)?

When I get a moment, I will try this against the server myself.

jmandel commented 4 months ago

I can't get the "validate" operation to return successfully when I run locally, so all my testing has been against the deployed server.

And yes, you can strip the params and reproduce just with:

 curl -X OPTIONS 'https://validator.fhir.org/validate'
 curl -X OPTIONS 'https://validator.fhir.org/validate'
dotasek commented 4 months ago

I can now get that test to pass with a change to the nginx config. There was rate-limiting in place, but I hadn't set the burst parameter, so it would only allow 6 validation queries per minute... so OPTIONS requests and GETs back-to-back were happening under the 10 second wait required by rate limiting.

jmandel commented 4 months ago

Okay. Are these limits applied per...IP? If so that's going to cause issues during conferences etc when many independent users are in the same room.

Could we address limitations by scaling the backend, rather than cutting off requests?

dotasek commented 3 months ago

That's actually a good question; this was configured from nginx's documentation on rate limiting: https://blog.nginx.org/blog/rate-limiting-nginx

A read of this, plus the documentation on the 'limit_req' module (https://nginx.org/en/docs/http/ngx_http_limit_req_module.html), indicates it is indeed the IP that gets used a key in default and most documented configurations.

I think, considering the resource intensive nature of the validator, that we do need some form of rate limiting, even if we scale the backend.

This somewhat ancient doc identifies our exact issue and suggests a workaround: https://serverfault.com/questions/704572/nginx-1-6-2-limit-req-zone-is-there-a-key-that-identifies-unique-users

jmandel commented 3 months ago

I don't think we want to require cookies in the context of cors at least.

It would be good only to kick in hard limits only if/when servers are actually struggling to keep up with load. I'd suggest We might ease up the restrictions and keep an eye on things. Then the logs may be revealing, especially around connectathons.

Curious @grahamegrieve what approach you take to rate limiting in the context of your terminology server.

dotasek commented 3 months ago

I wonder how much access nginx has to the underlying machine... the number of connections doesn't concern me as much as available system resources.