Add a target edit form and reachable header/timeout fields (closes #127) #229
Reference in New Issue
Block a user
Delete Branch "issue-127-target-edit-form"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #127, per the corrected scope in #127 (comment).
What was broken
Write-once configuration.
internal/server/routes.goregistered create, toggle and delete for targets but no edit route, so correcting a typo in a destination meant deleting the target and recreating it.Headers and timeout were worse than write-once.
HTTPTargetConfighas carriedHeadersandTimeoutand the delivery path has honoured both, butbuildURLTargetConfigonly ever wrote{"url":...}and no form offered either field. A destination needing anAuthorizationheader could not be configured through the UI at all.What changed
Both halves, one builder.
buildTargetConfignow takes atargetFormInputfilled from the request body, and both the create and the edit path hand it one. The destination check moved into a singlevalidateTargetURLhelper that both reach, so an edited destination is SSRF-validated exactly as a new one is. The guard's entry point (delivery.ValidateTargetURL) is untouched, so #204 lands on it unaffected.Create form. Gains a headers textarea and a timeout field for HTTP targets. With neither filled the stored config is byte-identical to what the form wrote before (
{"url":...}), pinned by a test, so no existing target's configuration is rewritten.Edit form.
GET/POST /source/{sourceID}/targets/{targetID}/editwithtemplates/target_edit.html, pre-filling name, destination, headers, timeout, retries or archive expiry according to the stored type. Both carry CSRF like every other form here.The masking exception, kept narrow. The pre-filled form is the one place the full destination and header values are shown; the operator cannot correct a value they cannot see. It is confined to
delivery.TargetConfigForm, reachable only from this page, on a route whose group suppliesRequireAuthandNoCache, behind the webhook's ownership check.delivery.TargetViewis unchanged, so #113, #115 and #118 hold everywhere else.Decisions worth a reviewer's attention
Host,Content-Length,Transfer-Encoding,Connection,User-Agent—applyRequestHeaderssets the UA after the configured headers). Storing a header that provably never reaches the wire would report a configuration that did not take effect. No error message ever quotes a header value: those are the credentials, and the message is rendered into a 400 body.delivery.MaxTargetTimeoutSeconds(300). A delivery attempt holds a worker for its whole duration. A value that is not a whole number in range is a 400, never a silent default.max_retriesabsent from a submission is not read as zero. The forms for types that do not retry omit the input; reading it unconditionally would silently disable retries.buildTargetConfigwrites its own response and is shared with create.Tests
internal/handlers/target_edit_test.go— the round trip the issue asks for (create, edit the destination, confirm the stored config changed and the new value was validated); headers and timeout round-tripping through create and edit; clearing them; the SSRF guard running on edit and leaving the stored config alone when it fires; every header and timeout rejection; the query-string ingress rule extended to the edit path; and 404 scoping for another webhook's target and another user's webhook.internal/delivery/target_headers_test.go— header parsing, formatting and round trip, the "no message quotes a value" rule, the timeout ceiling, andNewTargetConfigFormper target type including unreadable configs.These share one fx app per test function rather than one per case:
internal/handlersis already the slowest package in the suite and standing the app up is what a handler test mostly costs.Gate
make checkon the pushed SHA — green.Cache-defeated container build,
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .(#119) — both stages executed, noCACHEDlayer among them and no(cached)package line:The container build ran on this branch's tree before its final rebase onto
next; that rebase brought only unrelated commits, and themake checkabove was re-run green afterwards on the pushed SHA. Images from these runs were removed;docker ps -ashows nothing of mine, and no prune was run.One thing to flag rather than to review here: an earlier gate run at host load 141 failed on a data race in
internal/handlers/logbound_test.go— fx'stestutil.WriteSyncercallingt.Logffrom a start hook after the test goroutine had finished. Nothing here reaches it (a cleannextbuild passed, and this branch passed on re-run at lower load), so I have not filed it; say the word if you want it tracked.A target's configuration was write-once: `internal/server/routes.go` registered create, toggle and delete for targets but no edit route, so correcting a typo in a destination meant deleting the target and recreating it. Masking the stored value made that unrecoverable from the UI. Headers and timeout were worse than write-once. `HTTPTargetConfig` has carried `Headers` and `Timeout` and the delivery path has honoured both, but `buildURLTargetConfig` only ever wrote `{"url":...}` and no form offered either field, so a destination needing an `Authorization` header could not be configured through the UI at all. Both paths now build their configuration through `buildTargetConfig`, so an edited destination is SSRF-validated exactly as a new one is. The destination check lives in one helper that both reach, leaving the guard's entry point untouched. The edit form pre-fills the stored destination and header values in full. That is the one intentional exception to the masking rule, narrowed by the route it lives on: `RequireAuth`, `NoCache`, and the webhook's ownership check. `delivery.TargetView` is unchanged, so every other page still masks. A target's type stays fixed at creation: each type stores a different configuration shape and its delivery history is recorded against the row, so changing it is really a different target. Header and timeout input that could not be delivered as written is rejected rather than stored: a malformed line, an invalid name, a control character in a value, a repeated name, a header the delivery engine overwrites regardless, or a timeout that is not a whole number of seconds within the ceiling. Storing input that provably never reaches the wire would report a configuration that did not take effect.PASS. No SSRF bypass:
target.Confighas exactly two write sites (internal/handlers/source_management.go:1065,internal/handlers/target_edit.go:137), both go throughbuildTargetConfigand reachvalidateTargetURL; empty URL is a 400, not a keep-existing fallthrough; type comes from the stored row;internal/delivery/ssrf.gountouched, so #217 composes without duplication.Three non-blocking findings, for a follow-up rather than a rework:
internal/delivery/target_headers.go:59-71—isReservedTargetHeaderomitsTrailer. Go'sreqWriteExcludeHeader(net/http/request.go:99-105) dropsTrailerfrom the written request, soTrailer: xis accepted and stored but provably never reaches the wire — the exact failure mode the rejection list exists to prevent. Acceptable: addTrailerto the reserved list.internal/delivery/target_headers.go:113-124— the stated invariant "no error message ever quotes a header value" does not hold when the separator colon is missing and the value itself contains one:X-Auth-Token abc:defcuts at the first colon and the error rendersheader name must be a valid HTTP token: "X-Auth-Token abc"into the 400 body.TestParseTargetHeaders_ErrorsNeverQuoteAValuecoversX Bad Name: SECRET(secret after the colon) and the duplicate case, so it does not reach this. Exposure is limited to the operator's own submission echoed to their own browser — not logged, not stored, not sent to Sentry. Acceptable: do not quoterawName, or quote only up to the first whitespace.internal/delivery/engine.go:169— the delivery client sets noCheckRedirect, so Go follows up to 10 redirects. Go stripsAuthorization/Cookieon a cross-host redirect but forwards every other header, so an operator-setX-Api-Key/PRIVATE-TOKENfollows a destination's 302 to any other public host (dial-time SSRF guard still applies, so private targets stay blocked). Pre-existing code, but this PR is what first makes operator-set credential headers reachable, so the surface is created here. Acceptable:CheckRedirect: func(...) error { return http.ErrUseLastResponse }— webhook delivery has no reason to follow redirects.Disclosures:
assert.JSONEq(internal/handlers/target_edit_test.go:251) pins semantic, not byte, equality, so the PR body's "byte-identical" claim is not what the test asserts. Verified independently by construction:HTTPTargetConfigcarriesomitemptyonHeadersandTimeoutandSlackTargetConfigmarshals to{"webhookUrl":...}, so both are byte-identical to the maps they replace. No silent config migration.Redactoralready coversHTTPTargetConfig.Headersvalues for names matchingtoken|secret|key|auth|password|signatureplusAuthorization/Proxy-Authorization/Cookie, so this PR's headers are covered on the rendered-response path. Residual: an operator-set credential header whose NAME matches none of those renders unredacted if the remote echoes it — that is #219's disclosed limitation, not a defect here.check / check (push)status on84878cdispending("Waiting to run"), not green. Evidence below is my own run, per #119.internal/handlers/source_management.go; whichever lands second may need a trivial textual rebase.Judgement calls: type-immutable, reject-rather-than-store, 300s ceiling and
PostForm.Has("max_retries")all sound; none should block.parseNonNegativeIntis reached only behind theHasguard, so #221 is not made worse.Gate, run on
84878cdat host load 68.96 falling to 34.73 (docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain, exit 0): lint stage executed all 9 steps —make fmt-checkDONE 3.6s,golangci-lint config verifyDONE 0.4s,golangci-lint run"0 issues." DONE 55.7s; buildermake test(go test -v -race -timeout 90s ./...) DONE 81.5s, 15 packages ok with real durations,internal/handlers19.975s. Zero(cached)markers in the whole log; the singleCACHEDline is#6, the base-image FROM resolve, not a stage step. Neither #225 nor #230 surfaced. Image removed,docker ps -aempty, no prune run.Also verified: test-merges cleanly into current
next; one commit, title ends " (closes #127)", basenext;TODO.mduntouched; CSRF/MaxBodySize/NoCache/RequireAuthinherited from the/source/{sourceID}group;TargetConfigFormreachable only fromtarget_edit.go, never marshalled or logged;ownedTarget404s across webhooks and users; header injection impossible by construction (token-only names, control characters rejected in values) withTestDeliverHTTP_CustomTargetHeadersalready covering the stored-then-delivered path; no attribution trailers or vendor references; inclusive terminology clean.