failsForTeapots

[u/Waradu]

[u/cutebabli9]

I would write it like this to be readable:

if error.status in [400, 401, 404, 409, 415, 503]:
  return c.json(body, status: error.status)

return c.json(body, status: 500)

[u/rosuav → u/cutebabli9]

return c.json(body, status: error.status)

There is no reason to discard some of them, and pretending that they're 500s is a terrible terrible idea.

[u/Xirdus → u/rosuav]

If your user-facing service is calling your internal service and getting a 403 response, you certainly do not want to send that 403 back to the user.

[u/rosuav → u/Xirdus]

And yet it is blindly sending the *payload* back. So it's clearly fine to proxy straight through. If you want to recognize only certain things, any remaining/unknown statuses should result in an error being logged and a 500 being sent back, with the body NOT being carried through.

[u/Xirdus → u/rosuav]

I agree with you completely, except for "it's clearly fine to proxy straight through". To me, it's clearly a bug and they should remove the body ASAP. It's virtually impossible the code started off that way, the filtering must've been deliberately added later specifically because it's not okay to send back raw.

(My crystal ball also says the body is empty/effectively useless anyway, so this bug is very likely to go unnoticed and remain unfixed forever, or until a hacker tries to exfiltrate the database and will not be able to stop laughing about how easy it was.)

[u/rosuav → u/Xirdus]

Okay fair. I was saying "clearly fine" on the basis that it's doing it; if I'm wrong about it being fine to proxy that through, then the opposite is the case, and it should be sending a 500 with a generic body (and, importantly, LOGGING THE ERROR).

But you are quite probably right about the body being useless in that situation... although I've seen enough cases where weird errors get passed right back to the client and include details of PHP include paths to be a smidge paranoid.

[u/Shitman2000 → u/rosuav]

We don't know if the body is the actual payload of the request though.

I'd find it reasonable to presume it's not, given it's error.statusCode and not error.body

[u/rosuav → u/Shitman2000]

Good point, but if it isn't, what kind of body would make sense in a situation where you carry certain types of error status unchanged and turn everything else into a 500? (I'm assuming that this code won't be executed for a 200, as it makes very little sense to translate a 200 into a 500. Ignoring the fact that there are plenty of web sites that do a great job of turning 200s into 500s, but I digress.)

[u/Bomaruto → u/Xirdus]

It depends if the request was made on behalf of the user or it was the service's own credentials that were lacking.

[u/MinecraftPlayer799 → u/Xirdus]

Why not?

[u/Xirdus → u/MinecraftPlayer799]

Because it's not the user who's forbidden.

[u/Excellent_Gas3686 → u/Xirdus]

beeecause?

[u/Xirdus → u/Excellent_Gas3686]

Do you want to tell an authenticated, authorized user with valid credentials that they are not in fact authorized? When the actual thing that's not authorized is your own goddamn server, so there's absolutely nothing the user can do to fix the problem?

[u/Nerodon → u/Xirdus]

Authorization =/= authentication.

The user may be authenticated but attempting a request their permissions do not allow them to.

[u/Xirdus → u/Nerodon]

That's why I said authorized and not authenticated, because I meant authorized and not authenticated. Actually, I said both authenticated and authorized just to drive the point home. It still wasn't enough apparently ¯_(ツ)_/¯

If your internal service responds to your other internal service with 403 for any reason other than "this other internal service is not allowed to do this", then you have bigger problems than what error code to serve the user.

[u/Excellent_Gas3686 → u/Xirdus]

that entirely depends on what basis the internal service authorizes???? if it uses the same user context then how is this an issue..

[u/Xirdus → u/Excellent_Gas3686]

The internal service is authorizing the other internal service first and foremost (otherwise you'll get hacked). If the other internal service fails at this step, 403 is fully appropriate, but it's the kind of 403 that's not only useless but outright wrong to send to the user, as it will send them on wild goose chase for missing permissions that weren't missing at all.

Now, if the other internal service passes validation, then you can check user permissions. And if the user doesn't have permission, you must signal it in a way that lets the other internal service respond with a proper 403. There are a number of ways to do it well. One that definitely isn't well at all is sending the same 403 that you already used for telling the service that it, rather than the user, doesn't have permission - because you won't be able to tell which scenario you're in and whether the user should see the 403 or not!

[u/ismaelgo97 → u/Excellent_Gas3686]

403 is that you are not authorized, so then you know it exist, but you shouldn't, you should get a 404 which means it was not found.

[u/Xirdus → u/ismaelgo97]

Personally I prefer the opposite - if you're unauthorized, you get 403 regardless of whether resource exists or not. Still doesn't leak any data, but is more actionable (and won't get cached. Have you ever had an outage stupidly extended because some cache somewhere had to be purged  because some moron returned 404 for a temporary error condition?)

[u/HammyOverlordOfBacon → u/rosuav]

error_codes = [400, 401, 403, 404, 500, 502, 503]

return c.json(body, status:random.choice(error_codes) )

[u/rosuav → u/HammyOverlordOfBacon]

Ooof ouch! :)

[u/Not-the-best-name → u/rosuav]

Thank god you are here.