Skip to content

caddyhttp: return 413 for oversized body placeholders#7692

Open
cyphercodes wants to merge 1 commit into
caddyserver:masterfrom
cyphercodes:fix-request-body-placeholder-max-size-7691
Open

caddyhttp: return 413 for oversized body placeholders#7692
cyphercodes wants to merge 1 commit into
caddyserver:masterfrom
cyphercodes:fix-request-body-placeholder-max-size-7691

Conversation

@cyphercodes
Copy link
Copy Markdown
Contributor

Fixes #7691

Summary:

  • read request body placeholders with error handling instead of ignoring read errors
  • convert http.MaxBytesError from request_body max_size into an HTTP 413 handler error
  • propagate placeholder errors through templates and vars/vars_regexp matchers
  • add regression coverage for oversized {http.request.body} usage

Tests:

  • go test ./modules/caddyhttp ./modules/caddyhttp/templates -count=1
  • go test ./caddytest/integration -run 'TestRespondWithJSON|TestRequestBodyPlaceholderRespectsMaxSize' -count=1
  • git diff --check

Assistance Disclosure

This PR was implemented with assistance from Hermes Agent (OpenAI Codex/GPT-5.5). The changes were reviewed and verified with the targeted tests listed above.

@francislavoie
Copy link
Copy Markdown
Member

I agree with the concept, but the implementation might have too many side effects. Changing how errors for all placeholders are handled has far-reaching effects. Needs deeper analysis.

@WeidiDeng
Copy link
Copy Markdown
Member

I think we can at limit record if the body is truncated and if the body is being limited by max bytes.

Request body placeholder was introduced for debugging, and it's also possible for users to want to view the truncated body while knowing it's truncated and limited by the handler.

In general, placeholders give users the freedom to manage caddy's behaviour. We can start just by setting the two aforementioned placeholders.

@chris-morgan, do you think this approach is ok, or do you prefer an implicit 413 status code in this case?

@chris-morgan
Copy link
Copy Markdown

I don’t know about how it gets implemented (you’ve got various different considerations because of things like reverse proxying), but I think a normal person will expect (a) that http.request.body will contain the entire body; and (b) that a body exceeding the specified max_size will always fail. Especially when it tells you up front that it exceeds it.

So I’m a little bit surprised to see it being patched in placeholders, though perhaps the broader “content-length is greater than max_size, 413 immediately” could be treated as a breaking change.

But really, I’m just a weird user who’s having fun stretching Caddy in unusual directions. You can definitely just raise an eyebrow at my shenanigans and say things are good enough and you prefer things to be broken in this way rather than messing with other things, though I’d hope for a couple of minor documentation tweaks in that case.

@steadytao
Copy link
Copy Markdown
Member

I think the issue is real and Chris’ expectation is reasonable but I agree with Francis that this PR changes too much generic placeholder behaviour to be comfortable merging as-is.

{http.request.body} and {http.request.body_base64} should not silently return a truncated body. The documentation says reads past request_body max_size return 413 and returning a prefix as though it were the complete body is a bad failure mode especially in vars_regexp or templates where users may make decisions from that value. I also agree with Weidi that explicit state would be useful if we want to preserve access to the truncated prefix, for example placeholders indicating that the body hit the limit or was truncated. But that should be explicit; the default {http.request.body} should either mean the complete body or fail.

I would prefer a narrower PR here; handle http.MaxBytesError specifically in the request-body placeholder path, document the behaviour and add targeted tests for templates and vars_regexp. I would avoid introducing a rule like allowing placeholder values to be errors and downstream consumers must simply propagate them in this PR because that has a far wider blast radius than the bug being fixed. The separate idea of rejecting immediately when Content-Length > max_size is also worth discussing but I would not bundle it into this fix because it changes admission behaviour beyond placeholder reads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Placeholder http.request.body truncates at max_size bytes, rather than producing a 413 error

5 participants