From 9b27d129297449390ae0ebebaf99770c52f8a72f Mon Sep 17 00:00:00 2001 From: dylanhuff-at-coder Date: Thu, 6 Aug 2026 11:03:47 -0400 Subject: [PATCH] chore: forbid direct response body JSON decode in codersdk (#27859) Add a ruleguard rule forbidding direct `json.NewDecoder(res.Body).Decode(...)` on `*http.Response` in codersdk packages, so new typed endpoints use `codersdk.ReadBodyAsJSON` and keep returning structured errors for non-JSON bodies. The rule matches both the chained call form and decoders assigned to a variable first. Intentional raw-body paths carry documented `//nolint:gocritic` exceptions: the 16 agent-direct HTTP decodes in `workspacesdk/agentconn.go` route through a single `decodeAgentJSON` helper (agent-direct over tailnet, so `ReadBodyAsJSON`'s reverse proxy/SSO error guidance does not apply), and the Azure IMDS attested-document decode in `agentsdk/azure.go` keeps an inline exception. The two `UseNumber` decoders in `licenses.go` are migrated to a new `codersdk.ReadBodyAsJSONUseNumber`, so `coder licenses add/list` also return structured errors for non-JSON bodies instead of `invalid character '<' looking for beginning of value`. Note for local verification: golangci-lint caches results, so run `golangci-lint cache clean` after modifying `scripts/rules.go` or the rule may silently not fire. Final PR of the stack on #27804, #27857, and #27858. Refs #27044. Stack plan Inventory (full-tree audit): 280 migratable call sites across 47 files; 17 excluded (16 agent-direct HTTP sites in `workspacesdk/agentconn.go`, 1 Azure IMDS decode in `agentsdk/azure.go`). 1. **#27857** `refactor(codersdk): use ReadBodyAsJSON in typed endpoints`: mechanical migration of all sites except `chats.go` (224 sites, 46 files). 2. **#27858** `refactor(codersdk): use shared error helpers in chat endpoints`: migrate the 56 `chats.go` sites and consolidate the duplicated `readRawBodyAsError`/`newResponseError` helpers onto the shared `client.go` error path, with regression tests for the 409 usage-limit flow. 3. **#27859** `chore: forbid direct response body JSON decode in codersdk`: ruleguard rule with documented exceptions for the intentional raw-body paths, plus `ReadBodyAsJSONUseNumber` for the `licenses.go` decoders. Reviewed and updated by Coder Agents on behalf of @dylanhuff-at-coder. --- codersdk/agentsdk/azure.go | 2 +- codersdk/client.go | 23 +++++++++++++++- codersdk/client_internal_test.go | 18 +++++++++++++ codersdk/licenses.go | 8 ++---- codersdk/workspacesdk/agentconn.go | 43 +++++++++++++++++++----------- scripts/rules.go | 29 ++++++++++++++++++++ 6 files changed, 99 insertions(+), 24 deletions(-) diff --git a/codersdk/agentsdk/azure.go b/codersdk/agentsdk/azure.go index 269246d2c6..e5e3b9e7d5 100644 --- a/codersdk/agentsdk/azure.go +++ b/codersdk/agentsdk/azure.go @@ -48,7 +48,7 @@ func (a *AzureSessionTokenExchanger) exchange(ctx context.Context) (Authenticate defer res.Body.Close() var token AzureInstanceIdentityToken - err = json.NewDecoder(res.Body).Decode(&token) + err = json.NewDecoder(res.Body).Decode(&token) //nolint:gocritic // Azure IMDS attested document response, not the Coder API. if err != nil { return AuthenticateResponse{}, err } diff --git a/codersdk/client.go b/codersdk/client.go index 90e0fcf6ec..afa54bdd7c 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -519,6 +519,23 @@ const htmlResponseHelper = "Ensure the Coder URL is correct and that any reverse // API endpoints never serve HTML. The caller remains responsible for // closing the response body. func ReadBodyAsJSON(res *http.Response, v any) error { + return decodeBodyAsJSON(res, v, nil) +} + +// ReadBodyAsJSONUseNumber behaves like ReadBodyAsJSON but decodes JSON +// numbers into json.Number instead of float64, preserving integer +// precision for callers that re-serialize or type-assert numeric +// claims, such as license JWT claims. +func ReadBodyAsJSONUseNumber(res *http.Response, v any) error { + return decodeBodyAsJSON(res, v, func(dec *json.Decoder) { + dec.UseNumber() + }) +} + +// decodeBodyAsJSON decodes the response body as JSON into v. When +// configure is non-nil it is called with the decoder before decoding, +// allowing callers to set options such as UseNumber. +func decodeBodyAsJSON(res *http.Response, v any, configure func(*json.Decoder)) error { if res == nil || res.Body == nil { return xerrors.New("no response body to decode") } @@ -530,7 +547,11 @@ func ReadBodyAsJSON(res *http.Response, v any) error { body := &responseBodyReader{Reader: res.Body} prefix := &bodyPrefixWriter{} - err := json.NewDecoder(io.TeeReader(body, prefix)).Decode(v) + dec := json.NewDecoder(io.TeeReader(body, prefix)) + if configure != nil { + configure(dec) + } + err := dec.Decode(v) switch { case err == nil: return nil diff --git a/codersdk/client_internal_test.go b/codersdk/client_internal_test.go index 3dde597779..23941eda98 100644 --- a/codersdk/client_internal_test.go +++ b/codersdk/client_internal_test.go @@ -640,6 +640,24 @@ func Test_ReadBodyAsJSON(t *testing.T) { require.ErrorContains(t, err, "read response body") require.NotContains(t, err.Error(), "invalid API response") }) + + //nolint:bodyclose // The response is constructed, not from a client. + t.Run("UseNumber", func(t *testing.T) { + t.Parallel() + + var v map[string]any + res := newResponse(http.StatusOK, jsonCT, `{"exp":1750000000}`) + require.NoError(t, ReadBodyAsJSONUseNumber(res, &v)) + num, ok := v["exp"].(json.Number) + require.True(t, ok, "expected json.Number, got %T", v["exp"]) + require.Equal(t, "1750000000", num.String()) + + // ReadBodyAsJSON decodes the same body's numbers as float64. + res = newResponse(http.StatusOK, jsonCT, `{"exp":1750000000}`) + require.NoError(t, ReadBodyAsJSON(res, &v)) + _, ok = v["exp"].(float64) + require.True(t, ok, "expected float64, got %T", v["exp"]) + }) } func assertSDKError(t *testing.T, err error) *Error { diff --git a/codersdk/licenses.go b/codersdk/licenses.go index a5f2853b85..24a8366b62 100644 --- a/codersdk/licenses.go +++ b/codersdk/licenses.go @@ -106,9 +106,7 @@ func (c *Client) AddLicense(ctx context.Context, r AddLicenseRequest) (License, return License{}, ReadBodyAsError(res) } var l License - d := json.NewDecoder(res.Body) - d.UseNumber() - return l, d.Decode(&l) + return l, ReadBodyAsJSONUseNumber(res, &l) } func (c *Client) Licenses(ctx context.Context) ([]License, error) { @@ -121,9 +119,7 @@ func (c *Client) Licenses(ctx context.Context) ([]License, error) { return nil, ReadBodyAsError(res) } var licenses []License - d := json.NewDecoder(res.Body) - d.UseNumber() - return licenses, d.Decode(&licenses) + return licenses, ReadBodyAsJSONUseNumber(res, &licenses) } func (c *Client) DeleteLicense(ctx context.Context, id int32) error { diff --git a/codersdk/workspacesdk/agentconn.go b/codersdk/workspacesdk/agentconn.go index e86ad80192..f63aebfa72 100644 --- a/codersdk/workspacesdk/agentconn.go +++ b/codersdk/workspacesdk/agentconn.go @@ -404,7 +404,7 @@ func (c *agentConn) ListeningPorts(ctx context.Context) (codersdk.WorkspaceAgent } var resp codersdk.WorkspaceAgentListeningPortsResponse - return resp, json.NewDecoder(res.Body).Decode(&resp) + return resp, decodeAgentJSON(res, &resp) } // Netcheck returns a network check report from the workspace agent. @@ -421,7 +421,7 @@ func (c *agentConn) Netcheck(ctx context.Context) (healthsdk.AgentNetcheckReport } var resp healthsdk.AgentNetcheckReport - return resp, json.NewDecoder(res.Body).Decode(&resp) + return resp, decodeAgentJSON(res, &resp) } // DebugMagicsock makes a request to the workspace agent's magicsock debug endpoint. @@ -607,7 +607,7 @@ func (c *agentConn) ListContainers(ctx context.Context) (codersdk.WorkspaceAgent return codersdk.WorkspaceAgentListContainersResponse{}, codersdk.ReadBodyAsError(res) } var resp codersdk.WorkspaceAgentListContainersResponse - return resp, json.NewDecoder(res.Body).Decode(&resp) + return resp, decodeAgentJSON(res, &resp) } func (c *agentConn) WatchContainers(ctx context.Context, logger slog.Logger) (<-chan codersdk.WorkspaceAgentListContainersResponse, io.Closer, error) { @@ -820,7 +820,7 @@ func (c *agentConn) ExecuteDesktopAction(ctx context.Context, action DesktopActi } var result DesktopActionResponse - if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + if err := decodeAgentJSON(resp, &result); err != nil { return DesktopActionResponse{}, xerrors.Errorf("decode action response: %w", err) } return result, nil @@ -898,7 +898,7 @@ func (c *agentConn) RecreateDevcontainer(ctx context.Context, devcontainerID str return codersdk.Response{}, codersdk.ReadBodyAsError(res) } var m codersdk.Response - if err := json.NewDecoder(res.Body).Decode(&m); err != nil { + if err := decodeAgentJSON(res, &m); err != nil { return codersdk.Response{}, xerrors.Errorf("decode response body: %w", err) } return m, nil @@ -1017,7 +1017,7 @@ func (c *agentConn) LS(ctx context.Context, path string, req LSRequest) (LSRespo } var m LSResponse - if err := json.NewDecoder(res.Body).Decode(&m); err != nil { + if err := decodeAgentJSON(res, &m); err != nil { return LSResponse{}, xerrors.Errorf("decode response body: %w", err) } return m, nil @@ -1046,7 +1046,7 @@ func (c *agentConn) ResolvePath(ctx context.Context, path string) (string, error } var m ResolvePathResponse - if err := json.NewDecoder(res.Body).Decode(&m); err != nil { + if err := decodeAgentJSON(res, &m); err != nil { return "", xerrors.Errorf("decode response body: %w", err) } return m.ResolvedPath, nil @@ -1076,7 +1076,7 @@ func (c *agentConn) ReadFileLines(ctx context.Context, path string, offset, limi } var resp ReadFileLinesResponse - if err := json.NewDecoder(res.Body).Decode(&resp); err != nil { + if err := decodeAgentJSON(res, &resp); err != nil { return ReadFileLinesResponse{}, xerrors.Errorf("decode response: %w", err) } return resp, nil @@ -1127,7 +1127,7 @@ func (c *agentConn) WriteFile(ctx context.Context, path string, reader io.Reader } var m codersdk.Response - if err := json.NewDecoder(res.Body).Decode(&m); err != nil { + if err := decodeAgentJSON(res, &m); err != nil { return xerrors.Errorf("decode response body: %w", err) } return nil @@ -1277,7 +1277,7 @@ func (c *agentConn) StartProcess(ctx context.Context, req StartProcessRequest) ( return StartProcessResponse{}, codersdk.ReadBodyAsError(res) } var resp StartProcessResponse - return resp, json.NewDecoder(res.Body).Decode(&resp) + return resp, decodeAgentJSON(res, &resp) } // ListProcesses returns information about tracked processes on the agent. @@ -1293,7 +1293,7 @@ func (c *agentConn) ListProcesses(ctx context.Context) (ListProcessesResponse, e return ListProcessesResponse{}, codersdk.ReadBodyAsError(res) } var resp ListProcessesResponse - return resp, json.NewDecoder(res.Body).Decode(&resp) + return resp, decodeAgentJSON(res, &resp) } // ContextConfig returns the resolved context configuration from @@ -1310,7 +1310,7 @@ func (c *agentConn) ContextConfig(ctx context.Context) (ContextConfigResponse, e return ContextConfigResponse{}, codersdk.ReadBodyAsError(res) } var resp ContextConfigResponse - return resp, json.NewDecoder(res.Body).Decode(&resp) + return resp, decodeAgentJSON(res, &resp) } // CallMCPTool proxies a tool call to an MCP server running in @@ -1327,7 +1327,7 @@ func (c *agentConn) CallMCPTool(ctx context.Context, req CallMCPToolRequest) (Ca return CallMCPToolResponse{}, codersdk.ReadBodyAsError(res) } var resp CallMCPToolResponse - return resp, json.NewDecoder(res.Body).Decode(&resp) + return resp, decodeAgentJSON(res, &resp) } // ProcessOutput returns the output of a tracked process on the agent. @@ -1347,7 +1347,7 @@ func (c *agentConn) ProcessOutput(ctx context.Context, id string, opts *ProcessO return ProcessOutputResponse{}, codersdk.ReadBodyAsError(res) } var resp ProcessOutputResponse - return resp, json.NewDecoder(res.Body).Decode(&resp) + return resp, decodeAgentJSON(res, &resp) } // SignalProcess sends a signal to a tracked process on the agent. @@ -1363,7 +1363,7 @@ func (c *agentConn) SignalProcess(ctx context.Context, id string, signal string) return codersdk.ReadBodyAsError(res) } var m codersdk.Response - if err := json.NewDecoder(res.Body).Decode(&m); err != nil { + if err := decodeAgentJSON(res, &m); err != nil { return xerrors.Errorf("decode response body: %w", err) } return nil @@ -1386,7 +1386,7 @@ func (c *agentConn) EditFiles(ctx context.Context, edits FileEditRequest) (FileE } var resp FileEditResponse - if err := json.NewDecoder(res.Body).Decode(&resp); err != nil { + if err := decodeAgentJSON(res, &resp); err != nil { return FileEditResponse{}, xerrors.Errorf("decode response body: %w", err) } return resp, nil @@ -1445,6 +1445,17 @@ func (c *agentConn) apiRequest(ctx context.Context, method, path string, body in return c.apiClient(ctx).Do(req) } +// decodeAgentJSON decodes an agent-direct HTTP response body. Agent +// endpoints are served by the workspace agent over tailnet, not the +// Coder control plane, so no reverse proxy or SSO portal can intercept +// the response and codersdk.ReadBodyAsJSON's proxy/SSO-oriented error +// guidance would be misleading here. +// +//nolint:gocritic // See doc comment. +func decodeAgentJSON(res *http.Response, v any) error { + return json.NewDecoder(res.Body).Decode(v) +} + // apiClient returns an HTTP client that can be used to make // requests to the workspace agent's HTTP API server. The client is // scoped to a single request: its transport cancels in-flight dials diff --git a/scripts/rules.go b/scripts/rules.go index 327c21dcd7..bf89712a6d 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -529,3 +529,32 @@ func netAddrNil(m dsl.Matcher) { m.Match("$_.RemoteAddr().Network()").Report("RemoteAddr() may return nil and segfault if you call Network()") m.Match("$_.LocalAddr().Network()").Report("LocalAddr() may return nil and segfault if you call Network()") } + +// codersdkResponseBodyDecode ensures that codersdk typed endpoint methods +// decode HTTP response bodies through codersdk.ReadBodyAsJSON, which +// returns a structured *codersdk.Error when an intermediary such as a +// reverse proxy or SSO portal responds with HTML, an empty body, or other +// non-JSON content. Responses that intentionally bypass the Coder API +// error contract (agent-direct HTTP over tailnet, cloud metadata +// services) suppress this rule with a nolint:gocritic comment explaining +// why. +// +// Both the chained call form and decoders assigned to a variable first +// are matched. +// +//nolint:unused,deadcode,varnamelen +func codersdkResponseBodyDecode(m dsl.Matcher) { + m.Import("encoding/json") + m.Import("net/http") + m.Match( + `json.NewDecoder($res.Body).Decode($_)`, + `$_ := json.NewDecoder($res.Body)`, + `$_ = json.NewDecoder($res.Body)`, + ). + Where( + (m["res"].Type.Is("*http.Response") || m["res"].Type.Is("http.Response")) && + m.File().PkgPath.Matches(`github.com/coder/coder/v2/codersdk`) && + !m.File().Name.Matches(`_test\.go$`), + ). + Report("Use codersdk.ReadBodyAsJSON to decode typed API responses so non-JSON bodies produce a structured error. For responses that are intentionally not Coder API JSON, add a nolint:gocritic comment explaining why.") +}