This is a draft PR adding a REST/JSON gateway on top of the existing RPC framework. The design is sound: IDL annotations flow through the generator into HTTPBinding structs embedded in each Method, and RegisterREST uses those structs to mount routes on a plain http.ServeMux. I read through all of the new code carefully.
What works well
The Args precedence order (body → query → path) is the right call — path params canonically win over anything else. The coerceQueryValue switch is clear and covers the expected scalar kinds. The two-step round-trip (json.Marshal(fields) → json.Unmarshal(data, v)) is a little roundabout, but it's correct and lets the generated UnmarshalJSON on args structs do its own work without a custom parser. The generator changes are clean — httpBinding, httpPathParams, and httpQueryParams are small, focused, and easy to follow, and the test that compares generated output against the golden testdata/rest.go gives confidence the codegen is stable.
Concrete concerns
No auth enforcement in restHandler (rest.go, lines 41–60). The existing RPC transport refuses unauthenticated calls to non-Public methods. restHandler dispatches directly to m.Handler without checking m.Public or call.IsAuthenticated(). If a method is non-public and no auth middleware wraps the mux, the REST gateway will answer calls that the QUIC transport would reject. The doc comment says "a future auth middleware" will handle this, but that's an open security gap at the point of mounting. At minimum the code should enforce m.Public here, and ideally return HTTP 401/403 for non-public methods missing an identity, so callers get the same semantics over both transports.
Path prefix joining can produce double slashes (generator.go, httpBinding function):
bare = strings.TrimRight(i.HTTP.Prefix, "/") + "/" + strings.TrimLeft(bare, "/")
When the interface prefix is /api/v1 and the method path already starts with /, you get /api/v1/apps. Fine. But if the method path has no leading slash (which the IDL files here don't), the logic is still correct. The real risk is the opposite: if someone writes a prefix without a trailing slash and a method path without a leading slash, the middle "/" handles it. This is robust. Not a bug, just worth knowing.
restCall.Results captures only the last assignment (rest.go, line 187–189). If a handler stores intermediate results before the final state (unusual but possible), earlier writes are silently dropped. This matches the QUIC path's semantics so it's consistent, but worth documenting if this ever supports streaming results.
json.Marshal(call.results) after w.Header().Set but before w.WriteHeader (rest.go, lines 46–60). The Content-Type header is set at line 46, then the marshal happens. If json.Marshal fails at line 54, http.Error is called — but http.Error sets its own Content-Type and writes a status. That works fine in practice because WriteHeader hasn't been called yet at that point, so the second Content-Type will win. However, if marshal succeeds, w.WriteHeader(http.StatusOK) is explicit at line 59. Calling w.Header().Set at line 46 before w.WriteHeader is correct, but a reader has to trace the two paths carefully to confirm it. Moving the Content-Type assignment to just before the write would make the success path unambiguous.
Silent body-decode error (rest.go, lines 82–86). Ignoring JSON decode errors for a body-binding (Body == "*") is intentional and the comment explains it. That's a reasonable choice for an "all fields optional" schema. But a malformed Content-Type: application/json body that sends {"name": } (invalid JSON) is silently treated as "no body", and the args will be empty rather than rejected. If the IDL has any mandatory-feeling fields, callers get a confusing empty-args invocation rather than a 400. Consider returning 400 when the body is non-empty and fails to parse, at least when the Content-Type is application/json.
The security point (item 1) is the only thing I'd call a merge blocker for a non-draft. Since this is still a draft, it's appropriate to raise these now rather than after the PR graduates. Everything else is refinement.
restHandler dispatches directly to m.Handler without checking m.Public or whether the caller is authenticated. The RPC transport enforces auth on non-public methods; the REST gateway currently does not. If this mux is mounted without an external auth middleware, non-public methods will be callable unauthenticated. At minimum, add a check like if !m.Public && !call.IsAuthenticated() { writeRESTError(w, ErrUnauthorized); return } before invoking the handler.Body == "*", a malformed JSON body (e.g. {"name": }) is silently ignored and args are left empty. For callers sending Content-Type: application/json, consider returning HTTP 400 when the body is non-empty and fails to decode, so they learn about the bad payload rather than receiving a confusing empty-args response.Verdict: caveats
{
"owner": "mirendev",
"repo": "runtime",
"number": 978,
"verdict": "caveats",
"event": "comment",
"summary": "This is a draft PR adding a REST/JSON gateway on top of the existing RPC framework. The design is sound: IDL annotations flow through the generator into `HTTPBinding` structs embedded in each `Method`, and `RegisterREST` uses those structs to mount routes on a plain `http.ServeMux`. I read through all of the new code carefully.\n\n**What works well**\n\nThe `Args` precedence order (body → query → path) is the right call — path params canonically win over anything else. The `coerceQueryValue` switch is clear and covers the expected scalar kinds. The two-step round-trip (`json.Marshal(fields)` → `json.Unmarshal(data, v)`) is a little roundabout, but it's correct and lets the generated `UnmarshalJSON` on args structs do its own work without a custom parser. The generator changes are clean — `httpBinding`, `httpPathParams`, and `httpQueryParams` are small, focused, and easy to follow, and the test that compares generated output against the golden `testdata/rest.go` gives confidence the codegen is stable.\n\n**Concrete concerns**\n\n1. **No auth enforcement in `restHandler`** (`rest.go`, lines 41–60). The existing RPC transport refuses unauthenticated calls to non-`Public` methods. `restHandler` dispatches directly to `m.Handler` without checking `m.Public` or `call.IsAuthenticated()`. If a method is non-public and no auth middleware wraps the mux, the REST gateway will answer calls that the QUIC transport would reject. The doc comment says \"a future auth middleware\" will handle this, but that's an open security gap at the point of mounting. At minimum the code should enforce `m.Public` here, and ideally return HTTP 401/403 for non-public methods missing an identity, so callers get the same semantics over both transports.\n\n2. **Path prefix joining can produce double slashes** (`generator.go`, `httpBinding` function):\n\n ```go\n bare = strings.TrimRight(i.HTTP.Prefix, \"/\") + \"/\" + strings.TrimLeft(bare, \"/\")\n ```\n\n When the interface prefix is `/api/v1` and the method path already starts with `/`, you get `/api/v1/apps`. Fine. But if the method path has no leading slash (which the IDL files here don't), the logic is still correct. The real risk is the opposite: if someone writes a prefix *without* a trailing slash and a method path *without* a leading slash, the middle `\"/\"` handles it. This is robust. Not a bug, just worth knowing.\n\n3. **`restCall.Results` captures only the last assignment** (`rest.go`, line 187–189). If a handler stores intermediate results before the final state (unusual but possible), earlier writes are silently dropped. This matches the QUIC path's semantics so it's consistent, but worth documenting if this ever supports streaming results.\n\n4. **`json.Marshal(call.results)` after `w.Header().Set` but before `w.WriteHeader`** (`rest.go`, lines 46–60). The `Content-Type` header is set at line 46, then the marshal happens. If `json.Marshal` fails at line 54, `http.Error` is called — but `http.Error` sets its own `Content-Type` and writes a status. That works fine in practice because `WriteHeader` hasn't been called yet at that point, so the second `Content-Type` will win. However, if marshal succeeds, `w.WriteHeader(http.StatusOK)` is explicit at line 59. Calling `w.Header().Set` at line 46 before `w.WriteHeader` is correct, but a reader has to trace the two paths carefully to confirm it. Moving the `Content-Type` assignment to just before the write would make the success path unambiguous.\n\n5. **Silent body-decode error** (`rest.go`, lines 82–86). Ignoring JSON decode errors for a body-binding (`Body == \"*\"`) is intentional and the comment explains it. That's a reasonable choice for an \"all fields optional\" schema. But a malformed `Content-Type: application/json` body that sends `{\"name\": }` (invalid JSON) is silently treated as \"no body\", and the args will be empty rather than rejected. If the IDL has any mandatory-feeling fields, callers get a confusing empty-args invocation rather than a 400. Consider returning 400 when the body is non-empty and fails to parse, at least when the `Content-Type` is `application/json`.\n\nThe security point (item 1) is the only thing I'd call a merge blocker for a non-draft. Since this is still a draft, it's appropriate to raise these now rather than after the PR graduates. Everything else is refinement.",
"comments": [
{
"path": "pkg/rpc/rest.go",
"line": 41,
"side": "RIGHT",
"body": "**Auth gap**: `restHandler` dispatches directly to `m.Handler` without checking `m.Public` or whether the caller is authenticated. The RPC transport enforces auth on non-public methods; the REST gateway currently does not. If this mux is mounted without an external auth middleware, non-public methods will be callable unauthenticated. At minimum, add a check like `if !m.Public \u0026\u0026 !call.IsAuthenticated() { writeRESTError(w, ErrUnauthorized); return }` before invoking the handler.",
"ai_prompt": "In pkg/rpc/rest.go inside the restHandler closure, before the call to m.Handler (around line 41), add an authentication check. If the method is not public (m.Public == false) and the caller is not authenticated (call.IsAuthenticated() == false), call writeRESTError(w, ErrUnauthorized) and return early. This mirrors the auth enforcement the QUIC transport already does for non-public methods."
},
{
"path": "pkg/rpc/rest.go",
"line": 83,
"side": "RIGHT",
"body": "**Silent body parse failure**: when `Body == \"*\"`, a malformed JSON body (e.g. `{\"name\": }`) is silently ignored and args are left empty. For callers sending `Content-Type: application/json`, consider returning HTTP 400 when the body is non-empty and fails to decode, so they learn about the bad payload rather than receiving a confusing empty-args response.",
"ai_prompt": "In pkg/rpc/rest.go in the restCall.Args method around line 82-86, when c.binding.Body == \"*\" and c.r.Body is non-nil, the JSON decode error is currently silently discarded. Change this so that if the request Content-Type is application/json and the body is non-empty but fails to decode into the fields map, the function signals an error to the caller. One approach: check r.Header.Get(\"Content-Type\") and r.ContentLength != 0, and if the decode fails, write an HTTP 400 error via the response writer. Because Args doesn't have access to the ResponseWriter directly, consider having Args return an error or having restHandler check the decode result before proceeding."
}
],
"posted_to_pr": true,
"draft": true
}