fix(pkg): treat a successful response with no body as an error - #7010
Merged
Conversation
An agent pointed at something that answers `GET /info` with `200` and a zero-length body segfaults before it connects. resty decodes into the `**models.Info` given to SetResult, and with nothing to unmarshal it leaves the pointer nil and reports no error; ErrorFromResponse reads only the status, so the caller gets `(nil, nil)`. probeServerInfo stored that nil, Setup carried on, and Authorize dereferenced it while building its log fields. This is not exotic: the gateway's site block matches on SHELLHUB_DOMAIN, and a request arriving with any other Host falls through to Caddy's default, which answers 200 with no body rather than a 404. Any reverse proxy fronting the wrong vhost, load-balancer health stub or captive portal does the same. The operator sees a stack trace instead of "cannot reach the server", and anyone able to answer that request keeps the agent down for as long as they keep replying. Every method here decoded into a pointer this way, so the guard goes at the seam they share rather than at /info alone: requireBody turns a nil result into ErrEmptyResponse, and each returns through it. It asserts that a body arrived, which is all it does -- resty has already decoded. AuthDevice is the one that mattered as much as GetInfo, since Authorize dereferences its response on the very next lines. probeServerInfo now checks the error before it stores, which is what it should have done regardless of what the client returns. Reported-by: Edu0x01 <https://github.com/Edu0x01>
The site block matches on SHELLHUB_DOMAIN, so a request arriving with any other Host fell through to Caddy's own default, which answers 200 with an empty body. An agent pointed at the wrong vhost reads that as ShellHub's /info and gets no server info and no error. A reverse proxy fronting the wrong site, a load-balancer health stub and a captive portal all reply the same way, and none of them should look like a healthy ShellHub. The fallback matches every name, so it has to be the last route on port 80 -- and it is, because the Caddyfile adapter sorts by host specificity and leaves healthcheck.internal and the site ahead of it. That is an ordering property the template cannot show, so the test asserts it on the adapted JSON instead. What the fallback does shadow is automatic HTTPS's own redirect. Caddy writes one rule per name it manages a certificate for, and a single blanket rule for the rest at the end of the list. A deployment serving SHELLHUB_TLS_CERT_FILE manages no certificate for its domain, so that blanket rule was the only redirect it had, and the fallback swallowed it: http://<domain> answered 404 instead of redirecting. Hence the explicit redirect block -- the name gets a rule of its own, and neither redirect depends on the other. Checked against a running Caddy in four shapes (supplied certificate, automatic SSL, plain HTTP, and web endpoints on an internal certificate): the site redirects or proxies, a tunnel subdomain redirects, healthcheck.internal answers, and an unknown Host gets 404. Reported-by: Edu0x01 <https://github.com/Edu0x01>
|
Claude finished @otavio's task in 1m 48s —— View job Code Review CompleteReviewed 10 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is. A few notes on why the change holds up:
Tests cover the new error path on both To request another review round, comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
An agent whose
SHELLHUB_SERVER_ADDRESSpoints at something that answersGET /infowith200and an empty body crashed on a nil dereference before it connected. The API client now reports that as an error, and the gateway no longer produces such a reply in the first place.Why
restydecodes into the**models.Infohanded toSetResult. With a success status and a zero-length body there is nothing to unmarshal, so the pointer stays nil and no error is raised;ErrorFromResponsereads only the status, soGetInforeturned(nil, nil).probeServerInfostored that nil,Setupcarried on, andAuthorizedereferenceda.serverInfo.Endpoints.SSHwhile building its log fields — a segfault, exit 2, and under a supervisor a restart loop.The reply is easy to arrive at by accident. The gateway's site block matches on
SHELLHUB_DOMAIN, so a request with any other Host fell through to Caddy's own default, which answers200with an empty body rather than a 404. A reverse proxy fronting the wrong vhost, a load-balancer health stub or a captive portal all do the same. What the operator got was a stack trace instead of "cannot reach the server".Changes
pkg/api/client:requireBodyturns a nil decode target into the newErrEmptyResponse, and every method that decodes into a pointer returns through it. The guard sits at the seam the nine methods share rather than at/infoalone —AuthDevicehad the identical hazard, andAuthorizedereferences its response on the very next lines.ListDevicesis untouched: a slice result cannot nil-deref.agent/pkg/agentd:probeServerInfochecks the error before it stores, instead of storing first and returning the error unwrapped.gateway: a fallback site answers 404 for a Host the proxy does not serve, so a misdirected agent gets an unambiguous signal.gateway: an explicithttp://<domain>redirect when TLS is on. The fallback matches every name, which shadows automatic HTTPS's blanket redirect — and a deployment servingSHELLHUB_TLS_CERT_FILEmanages no certificate for its domain, so that blanket rule was the only redirect it had. Without this block,http://<domain>would answer 404 instead of redirecting.Testing
The fallback's ordering is the part worth probing. It matches every Host, so anything after it is unreachable;
TestTheFallbackIsTheLastRouteOnPortEightyasserts that on the adapted JSON across every configuration shape, because the template cannot show it.TestMain_smokenow sends an unserved Host to the real binary and expects 404.Checked by hand against a running Caddy in four shapes — supplied certificate, automatic SSL, plain HTTP, and web endpoints on an internal certificate. In each: the site redirects (308) or proxies, a tunnel subdomain redirects,
healthcheck.internalanswers 200, and an unknown Host gets 404. The supplied-certificate shape is the one that regressed on the fallback alone, and the one to re-check if the redirect block is ever touched.Thanks to @Edu0x01, who reported the crash and its trigger.