Repository navigation
Conversation
A request to a web endpoint's address is rewritten to /http/proxy and answered by the device's own HTTP service, or by the proxy refusing it. No API schema describes either answer. Strict validation, which the testcontainers stack runs, replaced the proxy's 403 for an unknown or expired address with a 500 saying the response does not match the schema. Cloud registers the route on the core router, so the path moves to pkg/webendpoints, where the skipper and cloud can both name it.
The site for *.<web endpoint domain> proxied every Host under that domain to the server. A
subdomain that is not an address, such as console.localhost when the web endpoint domain is the
instance's own, went through the server's address resolution untouched and reached any server
route. That includes /internal/metrics, which the main site refuses with 404.
The site now proxies only a Host whose first label is a 32 hex digit address, matched without
regard to case as the server matches it, and answers 404 for any other Host. vars_regexp reads
{http.request.host}, which carries no port.
otavio
changed the title
|
Claude finished @otavio's task in 6m 1s —— View job Code Review CompleteReviewed 11 files. 2 inline issues posted. Standards: 0 violations, 2 judgement calls. Spec: not run. Correctness: 0. StandardsThere are no hard violations. Every comment the branch adds is a doc comment on an exported declaration that states its contract. The one SpecThe Spec axis did not run. No issue in this repo is linked. The only reference is Additional notes
To request another review round, comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
WithAutoSSL adds docker-compose.autossl.yml and publishes HTTPS on a reserved port, so a test can reach what the gateway serves only over TLS. HTTPSPort reads empty without it, because .env sets SHELLHUB_HTTPS_PORT=443 for every stack.
9af16f5 to
41d48e6
Compare
|
/review |
|
Claude finished @otavio's task in 5m 48s —— View job Code Review CompleteReviewed 11 files. 2 inline issues posted. Standards: 1 violation, 1 judgement call. Spec: not run. Correctness: 0. StandardsThe violation: SpecThe Spec axis did not run. The PR closes no issue in this repo. Its only reference is Additional notes
To request another review round, comment |
41d48e6 to
1569026
Compare
|
/review |
|
Claude finished @otavio's task in 3m 33s —— View job Code Review CompleteReviewed 11 files. 2 inline issues posted. Standards: 0 violations, 2 judgement calls. Spec: not run. Correctness: 0. StandardsThere are no hard violations, and all four fixes from the previous round are in place. The more useful of the two judgement calls: SpecThe Spec axis did not run. The PR closes no issue in this repo. Its only reference is Additional notes
To request another review round, comment |
TestEnterpriseWebEndpoints, TestEnterpriseWebEndpointsDisabled and TestEnterpriseWebEndpointsTLS cover Domain 22 of shellhub-io/team#243 and the web endpoint items of Domain 27. They run on the enterprise stack, so the enterprise job runs them against the cloud branch of the same name. The device side is cmd/device-http, started in the gateway's network namespace like the agent, so the agent dials it on 127.0.0.1. It answers with its name and the Host, request URI and TLS state it received, and generates a self-signed certificate for TLS_DOMAIN when it starts. It binds both listeners before it logs "listening", which is the line the test waits for. A lookup renews an endpoint's TTL, so the expiry cases wait idle past the TTL and then send one request. Refs: shellhub-io/team#243
1569026 to
98546ba
Compare
|
/review |
|
Claude finished @otavio's task in 2m 1s —— View job Code Review CompleteReviewed 11 files on two axes: Standards (repo rules, Go conventions, tests, code smells) and Correctness (security and runtime defects). No new issues found. The code looks good as-is. The Spec axis did not run. The PR closes no issue in this repo, and its only reference, shellhub-io/team#243, is in a repo this run cannot read. |
A user who owns another namespace and an accepted device in it names the first namespace's endpoint, through the device route and by address. Both deletes must answer 404 and leave the endpoint answering. The lookup the case makes first caches the endpoint, which is the state a visit to its URL leaves behind. Refs: shellhub-io/team#243
|
/review |
|
Claude finished @otavio's task in 2m 21s —— View job Code Review CompleteI reviewed 12 files on two axes and found no new issues. The code looks good as-is.
The Spec axis did not run. The PR closes no issue in this repo. Its only reference is |
submitNamespaceDeletion returned once it clicked, before the DELETE answered, and the case then polled the owner's namespaces through a fresh login. A login that landed while the DELETE was in flight got a token naming the namespace, which the DELETE removed before the listing that followed. The server then cannot resolve the token's role and answers 401. expect.poll calls its callback outside the try that retries a failed match, so that 401 failed the case instead of being polled again, as it did in an e2e (cloud) run. The helper now waits for the DELETE's answer and returns it. The first attempt asserts it failed and the second that it succeeded. Deleting a namespace is synchronous, so once it has answered the namespaces can be read once, without polling.
|
/review |
|
Claude finished @otavio's task in 2m 16s —— View job Code Review CompleteReviewed 13 files. 1 inline issue posted. Standards: 1 violation, 0 judgement calls. Spec: not run. Correctness: 0. StandardsThe violation is in the newest commit, ce17f39, at SpecThe Spec axis did not run. The PR closes no issue in this repo. Its only reference is Additional notes
To request another review round, comment |
…ng refuses Deleting a namespace whose billing still holds an active subscription is refused with ErrBillingReportNamespaceDelete, whose code maps to 402 Payment Required. The admin delete route documents that answer; the user route did not, so a server validating its responses strictly replaced the 402 with a 500, and a client reading the spec had no 402 to expect.
The billing case asserted only that its first deletion failed and its second succeeded, which any error answer satisfies, the 401 of the login race fixed two commits back included. It now expects the 402 that billing's refusal answers, which the spec documents since the previous commit, and the 200.
|
/review |
|
Claude finished @otavio's task in 2m 32s —— View job Code Review CompleteReviewed 14 files on two axes and found no new issues. The code looks good as-is.
The Spec axis did not run. The PR closes no issue in this repo. Its only reference is |
This branch has not been deployed
1 participant
Summary
testcontainers cases for Domain 22 (Web Endpoints) of shellhub-io/team#243 and the web endpoint items of Domain 27 (Gateway), plus the fixes they turned up. Web endpoints are enterprise only, so the cases are
TestEnterprise*and run in thevalidate (tests - enterprise)job against the cloud branch of the same name, shellhub-io/cloud#2653. That PR carries the cloud fixes and must merge with this one.The worst fix is in cloud. After one request to a web endpoint, the server left the gateway's pooled upstream connection piped to that endpoint's device. Every later request on the web endpoint site then reached that device, whatever its address: another tenant's endpoint, a deleted or expired endpoint, an address no endpoint has. The second, also in cloud, let a user with an accepted device in one namespace delete another namespace's web endpoint through
DELETE /api/devices/:uid/tunnels/:address, once a lookup had cached it. The user chose to ship both as plain fixes rather than advisories.The device side is
tests/cmd/device-http, built likecmd/enrollment-webhookand started in the gateway's network namespace like the agent, so the agent dials it on127.0.0.1. It answers with its name and the Host, request URI and TLS state it received, on plain HTTP and on TLS with a self-signed certificate it makes at startup. Every case acts as the namespace owner, so shellhub-io/cloud#2642's tunnel permissions change nothing here.Lines for the issue, once merged
Domain 22, Web Endpoints:
Domain 27, Gateway, Web Endpoint Subdomain Routing:
Not duplicated from #7437 or #7429: neither touches web endpoints, tunnels or the gateway's web endpoint site.
Evidence
Before, with the cases written against master (the OpenAPI skipper fix applied, cloud and the gateway unchanged):
Deliberately broken stacks fail the remaining two:
TestEnterpriseWebEndpointsDisabledwith the feature on (200, 400 and 403 where 404 is expected) andTestEnterpriseWebEndpointsTLSwith the gateway choosing DNS-01 instead of internal TLS (remote error: tls: internal error).After, every case passes in the
testcompose runner:Review
Five Codex (
gpt-6.1-sol, high effort) rounds on the Standards and Spec axes, over both repos:On an early spike, before the routing bug was understood: doc contracts missing errors and context, a test doc claiming coverage it did not have yet, a weak suppression reason,
CreateWebEndpointrepeatingPostWebEndpoint, and the fixture logging "listening" before it bound its listeners. All accepted. Rejected: wrapping the helpers' errors withpkg/errors. Theenvironmenthelpers return raw errors throughout, and the fixture is a lonemain.gobuilt with no module to import from.Two correctness findings, both accepted:
LookupWebEndpointnow reads the endpoint again past the cache when its device is gone.Connectionheader named, droppedForwarded, and let the transport add gzip.Also accepted: incomplete doc contracts, a duplicated
AwaitDeviceOffline,HTTPSAddressreporting port 443 without HTTPS, and a routing unit test that did not prove the connection was reused. On the spec side, the renewal case did not require a request after the first expiry, and its TTLs left CI little slack.Spec clean. Four doc contracts still incomplete or wrong (
CreateTunnel,TunnelUpdateDeviceUID,WebEndpointLookup, andProxy's error handler discarding a write error). All fixed.Spec clean.
Proxy's contract did not coverhttp.ErrAbortHandler, whichReverseProxypanics with when copying an answer already started fails. Documented.Clean on both axes.
The automated review's first pass found two judgement calls here, both accepted and amended into the commits that wrote them:
deviceServicenow builds them (plain,secure), and the loopback host is one constant.HTTPSAddressbuilt its address onDockerCompose; it now lives onStacknext toBaseURLandSSHAddress.It also asked whether cloud saves an endpoint's last access before it answers, which the renewal case relies on. It does:
WebEndpointLookupwrites it before the proxy dials.billing.spec.tsfailed once ine2e (cloud)with a 401 that has nothing to do with web endpoints. The case read the owner's namespaces through a login that could land mid-deletion, andexpect.polldoes not retry a callback that throws. It now waits for the DELETE's answer first. Pinning the refusal's status turned up a spec gap: the user route'sdeleteNamespacedid not document the 402 billing's refusal answers, so the strict test stack had been turning it into a 500. The spec now documents it, as the admin route already did. The Stripe keys this spec needs are not available to this session locally, so thee2e (cloud)job is its run; ESLint, Prettier andtscpass on it.After the passes below came back clean, the two existing problems the review had set aside were fixed here as well, at the user's request: the cross-tenant delete (
another namespaceabove, red then green) and the proxy's anonymous allowance moving into cloud's allowlist.The second pass found two more, accepted: test cleanups now use
context.WithoutCancel(t.Context()), and therequire*Refusedhelpers stop on a wrong status. The third pass found two more, accepted:PostWebEndpointtakes a typed body, andunknownDeviceUIDnames the zero UID. The cloud PR's findings are listed there. All three web endpoint tests pass again against both amended branches.Merge Danger
Door: two-way
No migration and no wire format the agent or Connect reads. The agent's side of the HTTP proxy tunnel is unchanged.
Blast Radius: web endpoints
On instances with web endpoints on, a subdomain of the web endpoint domain that is not an address now gets the gateway's 404 instead of reaching the server. Nothing ShellHub serves lives there. The device sees one request per tunnel instead of a kept-alive stream, and the server keeps answering on the gateway connection. Instances with web endpoints off render the same Caddyfile as before. The OpenAPI skip matters only where response validation is on, which production leaves off by default.