Write-once configuration.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.
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. A destination needing an Authorization header could not be configured through the UI at all.
What changed
Both halves, one builder.buildTargetConfig now takes a targetFormInput filled from the request body, and both the create and the edit path hand it one. The destination check moved into a single validateTargetURL helper 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}/edit with templates/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 supplies RequireAuth and NoCache, behind the webhook's ownership check. delivery.TargetView is unchanged, so #113, #115 and #118 hold everywhere else.
Decisions worth a reviewer's attention
A target's type is not editable. Each type stores a different configuration shape and its delivery history is recorded against the row, so changing it is really a different target. The form shows the type as text, and the stored type decides which builder runs.
Unusable header input is rejected, not stored. A malformed line, an invalid name, a control character in a value, a repeated name, or a header the delivery path overwrites regardless (Host, Content-Length, Transfer-Encoding, Connection, User-Agent — applyRequestHeaders sets 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.
Timeout is bounded at 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_retries absent 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.
Rejections are plain 400s, matching the target create path rather than the webhook edit form's re-render, because buildTargetConfig writes 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, and NewTargetConfigForm per target type including unreadable configs.
These share one fx app per test function rather than one per case: internal/handlers is already the slowest package in the suite and standing the app up is what a handler test mostly costs.
Gate
make check on the pushed SHA — green.
ok sneak.berlin/go/webhooker/internal/delivery 5.650s
ok sneak.berlin/go/webhooker/internal/handlers 37.726s
ok sneak.berlin/go/webhooker/internal/server 5.610s
#11 67.69 0 issues.
Cache-defeated container build, docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . (#119) — both stages executed, no CACHED layer among them and no (cached) package line:
#22 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#22 75.12 0 issues.
#22 DONE 77.4s
#35 [builder 9/11] RUN make test
#35 ... ok sneak.berlin/go/webhooker/internal/delivery 5.807s
#35 ... ok sneak.berlin/go/webhooker/internal/handlers 43.791s
#35 ... ok sneak.berlin/go/webhooker/internal/server 4.020s
#35 DONE 116.3s
The container build ran on this branch's tree before its final rebase onto next; that rebase brought only unrelated commits, and the make check above was re-run green afterwards on the pushed SHA. Images from these runs were removed; docker ps -a shows 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's testutil.WriteSyncer calling t.Logf from a start hook after the test goroutine had finished. Nothing here reaches it (a clean next build 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.
Closes https://git.eeqj.de/sneak/webhooker/issues/127, per the corrected scope in https://git.eeqj.de/sneak/webhooker/issues/127#issuecomment-66684.
## What was broken
**Write-once configuration.** `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.
**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. A destination needing an `Authorization` header could not be configured through the UI at all.
## What changed
**Both halves, one builder.** `buildTargetConfig` now takes a `targetFormInput` filled from the request body, and both the create and the edit path hand it one. The destination check moved into a single `validateTargetURL` helper 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 https://git.eeqj.de/sneak/webhooker/issues/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}/edit` with `templates/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 supplies `RequireAuth` and `NoCache`, behind the webhook's ownership check. `delivery.TargetView` is unchanged, so https://git.eeqj.de/sneak/webhooker/issues/113, https://git.eeqj.de/sneak/webhooker/issues/115 and https://git.eeqj.de/sneak/webhooker/issues/118 hold everywhere else.
## Decisions worth a reviewer's attention
- **A target's type is not editable.** Each type stores a different configuration shape and its delivery history is recorded against the row, so changing it is really a different target. The form shows the type as text, and the stored type decides which builder runs.
- **Unusable header input is rejected, not stored.** A malformed line, an invalid name, a control character in a value, a repeated name, or a header the delivery path overwrites regardless (`Host`, `Content-Length`, `Transfer-Encoding`, `Connection`, `User-Agent` — `applyRequestHeaders` sets 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.
- **Timeout is bounded** at `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_retries` absent 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.
- **Rejections are plain 400s**, matching the target create path rather than the webhook edit form's re-render, because `buildTargetConfig` writes 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, and `NewTargetConfigForm` per target type including unreadable configs.
These share one fx app per test function rather than one per case: `internal/handlers` is already the slowest package in the suite and standing the app up is what a handler test mostly costs.
## Gate
`make check` on the pushed SHA — green.
```
ok sneak.berlin/go/webhooker/internal/delivery 5.650s
ok sneak.berlin/go/webhooker/internal/handlers 37.726s
ok sneak.berlin/go/webhooker/internal/server 5.610s
#11 67.69 0 issues.
```
Cache-defeated container build, `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` (https://git.eeqj.de/sneak/webhooker/issues/119) — both stages executed, no `CACHED` layer among them and no `(cached)` package line:
```
#22 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#22 75.12 0 issues.
#22 DONE 77.4s
```
```
#35 [builder 9/11] RUN make test
#35 ... ok sneak.berlin/go/webhooker/internal/delivery 5.807s
#35 ... ok sneak.berlin/go/webhooker/internal/handlers 43.791s
#35 ... ok sneak.berlin/go/webhooker/internal/server 4.020s
#35 DONE 116.3s
```
The container build ran on this branch's tree before its final rebase onto `next`; that rebase brought only unrelated commits, and the `make check` above was re-run green afterwards on the pushed SHA. Images from these runs were removed; `docker ps -a` shows 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's `testutil.WriteSyncer` calling `t.Logf` from a start hook after the test goroutine had finished. Nothing here reaches it (a clean `next` build 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.
clawbot
self-assigned this 2026-08-20 07:14:16 +02:00
PASS. No SSRF bypass: target.Config has exactly two write sites (internal/handlers/source_management.go:1065, internal/handlers/target_edit.go:137), both go through buildTargetConfig and reach validateTargetURL; empty URL is a 400, not a keep-existing fallthrough; type comes from the stored row; internal/delivery/ssrf.go untouched, so #217 composes without duplication.
Three non-blocking findings, for a follow-up rather than a rework:
internal/delivery/target_headers.go:59-71 — isReservedTargetHeader omits Trailer. Go's reqWriteExcludeHeader (net/http/request.go:99-105) drops Trailer from the written request, so Trailer: x is accepted and stored but provably never reaches the wire — the exact failure mode the rejection list exists to prevent. Acceptable: add Trailer to 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:def cuts at the first colon and the error renders header name must be a valid HTTP token: "X-Auth-Token abc" into the 400 body. TestParseTargetHeaders_ErrorsNeverQuoteAValue covers X 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 quote rawName, or quote only up to the first whitespace.
internal/delivery/engine.go:169 — the delivery client sets no CheckRedirect, so Go follows up to 10 redirects. Go strips Authorization/Cookie on a cross-host redirect but forwards every other header, so an operator-set X-Api-Key / PRIVATE-TOKEN follows 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: HTTPTargetConfig carries omitempty on Headers and Timeout and SlackTargetConfig marshals to {"webhookUrl":...}, so both are byte-identical to the maps they replace. No silent config migration.
Composition with #219: its Redactor already covers HTTPTargetConfig.Headers values for names matching token|secret|key|auth|password|signature plus Authorization/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.
The check / check (push) status on 84878cd is pending ("Waiting to run"), not green. Evidence below is my own run, per #119.
README's Target section does not document the new operator-visible constraints (300s ceiling, the five rejected header names). Minor, given how precisely that section documents everything else.
Both #217 and #219 also touch 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. parseNonNegativeInt is reached only behind the Has guard, so #221 is not made worse.
Gate, run on 84878cd at 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-check DONE 3.6s, golangci-lint config verify DONE 0.4s, golangci-lint run "0 issues." DONE 55.7s; builder make test (go test -v -race -timeout 90s ./...) DONE 81.5s, 15 packages ok with real durations, internal/handlers 19.975s. Zero (cached) markers in the whole log; the single CACHED line is #6, the base-image FROM resolve, not a stage step. Neither #225 nor #230 surfaced. Image removed, docker ps -a empty, no prune run.
Also verified: test-merges cleanly into current next; one commit, title ends " (closes#127)", base next; TODO.md untouched; CSRF/MaxBodySize/NoCache/RequireAuth inherited from the /source/{sourceID} group; TargetConfigForm reachable only from target_edit.go, never marshalled or logged; ownedTarget 404s across webhooks and users; header injection impossible by construction (token-only names, control characters rejected in values) with TestDeliverHTTP_CustomTargetHeaders already covering the stored-then-delivered path; no attribution trailers or vendor references; inclusive terminology clean.
PASS. No SSRF bypass: `target.Config` has exactly two write sites (`internal/handlers/source_management.go:1065`, `internal/handlers/target_edit.go:137`), both go through `buildTargetConfig` and reach `validateTargetURL`; empty URL is a 400, not a keep-existing fallthrough; type comes from the stored row; `internal/delivery/ssrf.go` untouched, so https://git.eeqj.de/sneak/webhooker/pulls/217 composes without duplication.
Three non-blocking findings, for a follow-up rather than a rework:
1. `internal/delivery/target_headers.go:59-71` — `isReservedTargetHeader` omits `Trailer`. Go's `reqWriteExcludeHeader` (`net/http/request.go:99-105`) drops `Trailer` from the written request, so `Trailer: x` is accepted and stored but provably never reaches the wire — the exact failure mode the rejection list exists to prevent. Acceptable: add `Trailer` to the reserved list.
2. `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:def` cuts at the first colon and the error renders `header name must be a valid HTTP token: "X-Auth-Token abc"` into the 400 body. `TestParseTargetHeaders_ErrorsNeverQuoteAValue` covers `X 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 quote `rawName`, or quote only up to the first whitespace.
3. `internal/delivery/engine.go:169` — the delivery client sets no `CheckRedirect`, so Go follows up to 10 redirects. Go strips `Authorization`/`Cookie` on a cross-host redirect but forwards every other header, so an operator-set `X-Api-Key` / `PRIVATE-TOKEN` follows 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: `HTTPTargetConfig` carries `omitempty` on `Headers` and `Timeout` and `SlackTargetConfig` marshals to `{"webhookUrl":...}`, so both are byte-identical to the maps they replace. No silent config migration.
- Composition with https://git.eeqj.de/sneak/webhooker/pulls/219: its `Redactor` already covers `HTTPTargetConfig.Headers` values for names matching `token|secret|key|auth|password|signature` plus `Authorization`/`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.
- The `check / check (push)` status on `84878cd` is `pending` ("Waiting to run"), not green. Evidence below is my own run, per https://git.eeqj.de/sneak/webhooker/issues/119.
- README's Target section does not document the new operator-visible constraints (300s ceiling, the five rejected header names). Minor, given how precisely that section documents everything else.
- Both https://git.eeqj.de/sneak/webhooker/pulls/217 and https://git.eeqj.de/sneak/webhooker/pulls/219 also touch `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. `parseNonNegativeInt` is reached only behind the `Has` guard, so https://git.eeqj.de/sneak/webhooker/issues/221 is not made worse.
Gate, run on `84878cd` at 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-check` DONE 3.6s, `golangci-lint config verify` DONE 0.4s, `golangci-lint run` "0 issues." DONE 55.7s; builder `make test` (`go test -v -race -timeout 90s ./...`) DONE 81.5s, 15 packages ok with real durations, `internal/handlers` 19.975s. Zero `(cached)` markers in the whole log; the single `CACHED` line is `#6`, the base-image FROM resolve, not a stage step. Neither https://git.eeqj.de/sneak/webhooker/issues/225 nor https://git.eeqj.de/sneak/webhooker/issues/230 surfaced. Image removed, `docker ps -a` empty, no prune run.
Also verified: test-merges cleanly into current `next`; one commit, title ends " (closes #127)", base `next`; `TODO.md` untouched; CSRF/`MaxBodySize`/`NoCache`/`RequireAuth` inherited from the `/source/{sourceID}` group; `TargetConfigForm` reachable only from `target_edit.go`, never marshalled or logged; `ownedTarget` 404s across webhooks and users; header injection impossible by construction (token-only names, control characters rejected in values) with `TestDeliverHTTP_CustomTargetHeaders` already covering the stored-then-delivered path; no attribution trailers or vendor references; inclusive terminology clean.
clawbot
merged commit aba02bc509 into next2026-08-20 07:24:13 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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.