search

getodk / central-backend

Node.js based backend for ODK Central
https://docs.getodk.org/central-intro/
Apache License 2.0
50 stars 72 forks source link

Backend crash when opening hostile-named submission detail #1157

Open issa-tseng opened 2 weeks ago

issa-tseng commented 2 weeks ago
  1. Go to here: https://staging.getodk.cloud/#/projects/1/forms/'%3D%2B%2F*-451%25%2F%25/submissions
  2. Open the submission detail page of any of the strangely named submissions
  3. Server crash, user sees a 404
Problem [Error]: Could not find the resource you were looking for.
    at Object.result [as notFound] (lib/util/problem.js:45:31)
    at filterFields (lib/formats/odata.js:610:48)
    at lib/formats/odata.js:660:94
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
lognaturel commented 2 weeks ago

It didn't ring a bell when you mentioned it but now I'm pretty sure this was @lindsay-stevens pushing the limits with pyodk. I think we discussed with @matthew-white and decided that we didn't need to support non-uuid instance ids. If that sounds right to everyone, I think we can close this!

lognaturel commented 2 weeks ago

Well, could we surface the error and not do anything instead of crashing? Maybe that's not any easier than just supporting that case?

alxndrsn commented 2 weeks ago

we didn't need to support non-uuid instance ids

Should submissions with non-uuid instanceId values be rejected when initially submitted?

issa-tseng commented 2 weeks ago

i'm not sure how the user input was inserted, but it feels like the hard crash should be addressed at least. bad input shouldn't crash the server, that's a DOS.

alxndrsn commented 2 weeks ago

@issa-tseng where are you seeing the server crashing? I'm seeing the 404, but having trouble recreating the crash.

Recreated by running 20 requests in parallel:

{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}{"message":"Could not find the resource you were looking for.","code":404.1}<html>
<head><title>502 Bad Gateway</title></head>
<body>
<center><h1>502 Bad Gateway</h1></center>
<hr><center>nginx</center>
</body>
</html>
<html>
<head><title>502 Bad Gateway</title></head>
<body>
<center><h1>502 Bad Gateway</h1></center>
<hr><center>nginx</center>
</body>
</html>
<html>
<head><title>502 Bad Gateway</title></head>
<body>
<center><h1>502 Bad Gateway</h1></center>
<hr><center>nginx</center>
</body>
</html>
alxndrsn commented 2 weeks ago

It looks like the crash is caused by an unhandled error thrown by the synchronous call to selectFields() on line 52:

https://github.com/getodk/central-backend/blob/7afef66d7eb6c0e9844b7f3f67082a8e575835d0/lib/resources/odata.js#L49-L55

issa-tseng commented 2 weeks ago

the crash happens without any parallel reqs for me. my guess is you need it because docker is restarting the server faster than you can see. i click the link once my server crashes 100% of the time.

lognaturel commented 2 weeks ago

I want to be clear that I agree the crash should be prevented so glad to see #1158! I was also initially being fooled by the Docker restart.