lizhimins commented on PR #4639:
URL:
https://github.com/apache/rocketmq-dashboard/pull/4639#issuecomment-5761162552
Thanks — bounding an unguarded `io.ReadAll` is a reasonable instinct, and
the test you added does fail if the limit is removed.
**We are closing this because there is no path in trunk where Studio returns
an oversized body.** `rmqctl` talks to one endpoint: our own server,
authenticated with an HMAC signature via `AuthTransport`, and every response it
reads is a `types.ResultEnvelope` JSON that `client.go` decodes into a fixed
shape. Nothing on the server produces a response body anywhere near 64 MiB. The
description frames it the same way — "a misbehaving or compromised Studio
endpoint could…" — which is a hypothetical threat model rather than a defect a
user can hit. We do not want to carry client-side machinery for a scenario that
requires the server to already be broken or hostile.
The review also found four things that would need fixing before this could
land as hardening, listed so a future attempt starts from them:
1. **A real regression in error classification.** The size check sits before
the status-code branch in `client.go`, so an oversized 4xx/5xx would return a
bare `fmt.Errorf("studio response exceeds …")` instead of going through
`responseError`, which builds the `*APIError{StatusCode, Code, Message, Hint}`
that `rmqctl/cmd/error.go` maps into advice for the user. Either move the check
after the status branch or wrap the oversize case in the same error type. Your
test asserts only `strings.Contains(err.Error(), "exceeds")`, so it cannot
catch this — it makes no assertion about the error type.
2. **The cap does not follow the existing convention.** `mcp_http.go` uses
`const statusErrorBodyLimit = 4 * 1024` and injects the reader through a
parameter (`readMCPHTTPStatusError`). A mutable package-level `var
maxResponseBytes` that tests have to overwrite is a different pattern, and it
also makes the test unsafe to run in parallel with others in the package.
3. **Same-family coverage is missing.** If the goal is bounding memory, the
MCP side is still unbounded — `readMCPHTTPStatusError` caps only the error
body, not a successful MCP response. The description does not say why
`client.go` alone is the right scope.
4. **64 MiB is unsourced.** It is not a flag, not config, and not declared
in `docs/api-spec.md` as a server-side response limit, so a client-side
constant will drift from whatever the server actually enforces.
**What would change the answer:** a concrete case where a real Studio
deployment returns a body large enough to matter — for example a tool or export
path whose response we have not bounded server-side. If you find one, the
better fix is probably a server-side limit documented in `docs/api-spec.md`,
with the client cap derived from it rather than chosen independently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
