caddyhttp: Fix fallback for the error handler chain

[francislavoie]

This is mainly a problem with the default behaviour of the Caddyfile's handle_errors routes, but it's kinda tricky to solve well. I went with an approach that involves a smidge of magic which might not really be desirable.

See https://caddy.community/t/problem-with-basicauth-handle-errors/12243/9 for context.

So we do already have a default fallback for error routes, i.e. errorEmptyHandler in caddyhttp.go which is always configured as the last handler in the chain (implicitly, not visible in the config).

The problem is that when subroutes routes come into play with "terminal": true, then this fallback handler will never be reached. This is the case when the Caddyfile generates a config which has a host matcher from a site block (which is most of the time) when the user configured handle_errors to handle specific errors (such as 502s or 404s to serve HTML pages for those, etc). If other errors, like basicauth's 401s are emitted in that case, then the result is that the default of HTTP status 200 will be served instead of the 401, which breaks basicauth completely.

The fix I went with is to make the Caddyfile adapter append special error handlers inside of the handle_errors subroutes which throw error -1, which server.go then picks up, and seeing -1 responds with the original error code of 401 instead. The -1 thing is the aforementioned magic.

At first, I had this implemented with static_response setting the StatusCode to {http.error.status_code}, but it didn't feel right to use a placeholder because it's inherently slightly less efficient, and it wasn't 100% correct because non-handler errors wouldn't be handled as 500s properly I think (because if it's not a HandlerError, then http.error.status_code doesn't exist, so it would maybe try to write an the placeholder replacement result of an empty string as 0 for the status code).

Edit: The fix I went with in the end (after realizing some mistaken assumptions above) is to just make the routes fall back to errorEmptyHandler instead of the non-error empty handler, if Terminal is true, making the routes error-aware. Ultimately this was probably just an oversight when errors was implemented at some point in the early betas of v2.

[francislavoie]

@mholt I thought of another way we could fix this which I just want to write down now so I don't forget -- we could change the subroute handlers to check if they're in the error handler chain (same way that file_server does this check on the request context) and if it's terminal, use the default fallback error handler as "next" instead of stopping. Should do the trick I think.

[francislavoie]

Okay yeah, this is better. Just need to make routes.go be error-chain aware and it's fixed. Updated my comment up top as well to reflect the change.

I didn't want to squash here cause I think it's still interesting to see what my original implementation looked like for possibly future reference.

[mholt]

Are my eyes deceiving me or is this now a 4-line patch 👀

[mholt]

Hard approve. This is WAY better than the previous implementation. 😄 💯