Skip to content

samples/restApi: buffer template render before flushing to response#126

Open
randomizedcoder wants to merge 2 commits into
NVIDIA:mainfrom
randomizedcoder:fix/printer-partial-write
Open

samples/restApi: buffer template render before flushing to response#126
randomizedcoder wants to merge 2 commits into
NVIDIA:mainfrom
randomizedcoder:fix/printer-partial-write

Conversation

@randomizedcoder

Copy link
Copy Markdown

⚠️ Depends on #124

This branch is based on the head of fix/g708-template-injection (PR #124), not on main directly. The diff against main includes #124's commit too. Please review #124 first; once it lands, this rebases onto main trivially. If you'd rather see them combined into one PR, happy to do that — say the word.

Hi 👋

While writing tests for #124 I noticed an existing wart in printer() and documented it in the test file with a comment calling it "out of scope for the G708 hardening." This PR is that follow-up.

The bug

printer() and processPrint() call Execute(resp, ...) directly:

if err := tmpl.Execute(resp, stats); err != nil {
    http.Error(resp, err.Error(), http.StatusInternalServerError)  // can't actually set 500
    log.Printf(...)
}

text/template writes the static template prefix to the output before evaluating the first {{action}}. On a ResponseWriter that prefix flush implicitly commits HTTP 200. If a later action then fails (stats missing a field, etc.), the http.Error() call cannot actually set status 500 — the headers are already on the wire — so the client silently gets 200 OK with a body containing both the partial template prefix and the error message.

You can see this happen in #124's existing test:

// out of scope for the G708 hardening:
"negative_unknown_field_appends_error_in_body", ...
http.StatusOK, "can't evaluate field Memory", "",

The StatusOK is the wart.

The fix

Execute into a bytes.Buffer; copy to resp only on success. On render failure the buffer is discarded and http.Error() writes a clean 500 with the template error message. No partial template output leaks to the client.

TDD

Drove the existing negative case to red by flipping its assertions:

-"negative_unknown_field_appends_error_in_body", ...
-http.StatusOK, "can't evaluate field Memory", "",
+"negative_unknown_field_returns_500_with_clean_body", ...
+http.StatusInternalServerError, "can't evaluate field Memory", "Memory(KB)",

The new notSubstr="Memory(KB)" is the strong assertion — it proves the partial template prefix never escapes the buffer.

Then implemented printer() and watched the case go green. All 18 cases pass.

Why this is low risk

  • Same public surface (printer/processPrint signatures unchanged from samples/restApi: pre-parse templates (gosec G708 hardening) #124).
  • Behavior change limited to the failure path — the success path is byte-for-byte identical (still copies the template output to the response).
  • All 9 sample binaries build cleanly.
  • processPrint() gets the same structural treatment for symmetry; the test suite can't trigger a processInfo render failure today, so the fix there is defensive (visible in the diff).

Test plan

  • go build ./samples/restApi/...
  • go test -v -count=1 ./samples/restApi/handlers/... — 18/18 pass
  • go vet ./samples/restApi/...
  • gofmt -l samples/restApi/handlers/ — empty
  • CI green

🤖 Generated with Claude Code

randomizedcoder and others added 2 commits May 24, 2026 16:29
…ction

printer() previously accepted `templ string` and parsed it inline, which
gosec correctly flags as a server-side template injection (G708): if any
future caller ever wires HTTP input into templ, that's RCE.

Refactor so every template is parsed once at package init into a
package-level *template.Template, and tighten printer's signature to
accept *template.Template. The function can no longer accept a free-form
string, so the taint source is closed at the type level. Same pattern
for processPrint + processInfoTmpl.

Adds table-driven tests under samples/restApi/handlers/utils_test.go
covering positive, negative, boundary, corner, and attacker categories.
The attacker cases verify that values containing {{ ... }} in stats
fields are rendered as data, NOT re-interpreted as template directives.

Drive-by: modernize DevicesUuids loop from `for i := uint(0); i < count; i++`
to `for i := range count` (Go 1.22+ rangeint).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: randomizedcoder <dave.seddon.ca@gmail.com>
printer() and processPrint() previously called Execute(resp, ...)
directly. text/template writes the static template prefix to the
output before evaluating the first action, which on the response
writer implicitly commits HTTP 200. If a later action then fails
(stats missing a field, etc.), the subsequent http.Error() in the
error branch could not actually set status 500 — the headers were
already on the wire — so clients silently got "200 OK" with a body
containing both the partial template prefix AND the error message.

Fix: Execute into a bytes.Buffer; copy to resp only on success.
On render failure the buffer is discarded and http.Error() writes
a clean 500 with the template error message. No partial template
output leaks to the client.

The pre-existing utils_test.go negative case was already pinning
this wart with a comment explicitly calling it "out of scope for
the G708 hardening". This PR flips that case to the correct
contract — wantCode=500, wantSubstr="can't evaluate field Memory",
notSubstr="Memory(KB)" (proves the partial prefix doesn't leak) —
and watching it go red drove the printer() rewrite (TDD).

processPrint() gets the same structural treatment for symmetry,
though the test suite cannot currently trigger a processInfo
render failure (template uses {{or ...}} defaults everywhere and
the signature locks the data type).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: randomizedcoder <dave.seddon.ca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant