search

cactus / go-camo

A secure image proxy server
MIT License
254 stars 48 forks source link

proxy: Swap "content length exceeded" exception to be a HTTP 413 #31

Closed jacobbednarz closed 5 years ago

jacobbednarz commented 5 years ago

We've encountered a couple of cases where the content length of the remote was over the threshold and while the error description is helpful, the HTTP status code is not (currently a 404). Instead of returning the "not found" status, we can update it to be a HTTP 413 which is "request entity too large". This will allow us to differentiate resources that aren't really there and ones that are oversized.

dropwhile commented 5 years ago

hmm. A HTTP 413 status specifies that the /request/ was too large. That doesn't seem appropriate here.

Perhaps a 403 might be a better option. 🤔

This would diverge from legacy camo though, which as I recall used 404 for every error (EDIT: code in question). I'm not sure how important it is to maintain this level of compatibility with legacy camo at this point though -- my guess is "not very".

jacobbednarz commented 5 years ago

I too flopped back and forth on this one too and the main goal was to get more insight into what was actually a 404 and what was another client error.

I'm happy either way if you think this is fine for now as I'm working on a change in our internal fork that will introduce omitting some statsd style metrics instead of relying on the HTTP status for internal metrics. Is this something you'd be interested in reviewing in the form of a PR? If so, I can generalise it a little to account for a broader audience.

dropwhile commented 5 years ago

I too flopped back and forth on this one too and the main goal was to get more insight into what was actually a 404 and what was another client error.

After thinking about it a bit more, I would be fine with a 403 for "response too large".

change in our internal fork that will introduce omitting some statsd style metrics instead of relying on the HTTP status for internal metrics. Is this something you'd be interested in reviewing in the form of a PR? If so, I can generalise it a little to account for a broader audience.

I would certainly be interested in taking a look.

jacobbednarz commented 5 years ago

Let's close this one off and I'll open a PR for exposing some more statistics via statsd-ish interface when I have something that will be configurable enough for others to use too.