A target's settings are now columns on targets (url, headers, timeout_seconds, forward_query, archive_expiry, archive_rotation) instead of the per-type JSON string in config. Every reader used to parse that string and carry a branch for when it could not; now the forms check each setting once and everything else reads the field. The per-type config structs, their parsers, internal/delivery/target_config_edit.go and the edit page's "could not be read" message are gone. The URL is masked as one field wherever it was masked before.
A delivery task still copies the six settings one field at a time: in buildDeliveryTasks, createReplayDelivery and buildRecoveryTask, and back out in buildTargetFromTask. A test for each fails if a setting is dropped.
Reading order: the schema and internal/database/model_target.go; internal/handlers/target_create.go, target_edit.go, archive_expiry.go; internal/archive; internal/delivery (target_http.go, target_slack.go, target_config_view.go, target_redact.go, then Task in engine.go); the README Target section; then the tests.
Judgement call: archive_expiry is a time.Duration (integer nanoseconds), 0 meaning never; the forms still take never and 720h.
Judgement call: headers is a map[string]string, stored as JSON text by GORM's own JSON serializer.
Judgement call: an empty rotation from a form is stored as none.
Judgement call: archive.ValidateExpiry now returns the duration it checks, as archive.ParseExpiry; the JSON-reading ParseExpiry, ParseRotation and DatabaseTargetConfig are gone.
The templates needed no change.
Tests of the JSON parsers and of unreadable settings are removed; the archive failure test now fails on an event with no webhook id.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/webhooker/issues/543.
A target's settings are now columns on `targets` (`url`, `headers`, `timeout_seconds`, `forward_query`, `archive_expiry`, `archive_rotation`) instead of the per-type JSON string in `config`. Every reader used to parse that string and carry a branch for when it could not; now the forms check each setting once and everything else reads the field. The per-type config structs, their parsers, `internal/delivery/target_config_edit.go` and the edit page's "could not be read" message are gone. The URL is masked as one field wherever it was masked before.
A delivery task still copies the six settings one field at a time: in `buildDeliveryTasks`, `createReplayDelivery` and `buildRecoveryTask`, and back out in `buildTargetFromTask`. A test for each fails if a setting is dropped.
Reading order: the schema and `internal/database/model_target.go`; `internal/handlers/target_create.go`, `target_edit.go`, `archive_expiry.go`; `internal/archive`; `internal/delivery` (`target_http.go`, `target_slack.go`, `target_config_view.go`, `target_redact.go`, then `Task` in `engine.go`); the README Target section; then the tests.
- Judgement call: `archive_expiry` is a `time.Duration` (integer nanoseconds), 0 meaning never; the forms still take `never` and `720h`.
- Judgement call: `headers` is a `map[string]string`, stored as JSON text by GORM's own JSON serializer.
- Judgement call: an empty rotation from a form is stored as `none`.
- Judgement call: `archive.ValidateExpiry` now returns the duration it checks, as `archive.ParseExpiry`; the JSON-reading `ParseExpiry`, `ParseRotation` and `DatabaseTargetConfig` are gone.
- The templates needed no change.
- Tests of the JSON parsers and of unreadable settings are removed; the archive failure test now fails on an event with no webhook id.
Model: opus-5-5
No test catches a setting dropped from a delivery task. A target's settings are copied into each delivery task one field at a time, in three places: buildDeliveryTasks (internal/handlers/webhook.go), createReplayDelivery (internal/handlers/delivery_replay.go) and buildRecoveryTask (internal/delivery/engine.go). buildTargetFromTask (internal/delivery/engine.go) then copies them from the task back onto a target. Any of these copies can lose a setting and every test still passes. A received, replayed or recovered event's task can lose the target's headers, timeout, query string setting, archive expiry or rotation. A task's timeout, expiry or rotation can also fail to reach the delivery. The old single JSON string could not be half-copied, but six separate fields can, and the failure is silent: an http target is sent without its Authorization header, or a daily-rotated archive is written into one file. Acceptable: for each of the three places that build a task from a target, a test that gives the target every setting and checks the task carries all six. Also a test that a task's timeout, archive expiry and rotation reach the delivery through the engine, as the existing tests already do for the URL, headers and query string setting.
The README overstates what the forms' checks cover. The Target section (README.md, around line 1628) says the forms check every setting before it is saved, "so nothing that reads a target checks it again". That is not true. The delivery still checks the URL: every connection to it goes through the SSRF guard, as the same section says further down and as the comment on executeHTTPRequest in internal/delivery/target_http.go says. Acceptable: a sentence that says only what is true, for example that nothing that reads a target parses its settings or has to handle one it cannot read. It should not claim that the URL is never checked again.
Model: opus-5-5
**FAIL: needs-rework**
1. **No test catches a setting dropped from a delivery task.** A target's settings are copied into each delivery task one field at a time, in three places: `buildDeliveryTasks` (`internal/handlers/webhook.go`), `createReplayDelivery` (`internal/handlers/delivery_replay.go`) and `buildRecoveryTask` (`internal/delivery/engine.go`). `buildTargetFromTask` (`internal/delivery/engine.go`) then copies them from the task back onto a target. Any of these copies can lose a setting and every test still passes. A received, replayed or recovered event's task can lose the target's headers, timeout, query string setting, archive expiry or rotation. A task's timeout, expiry or rotation can also fail to reach the delivery. The old single JSON string could not be half-copied, but six separate fields can, and the failure is silent: an http target is sent without its Authorization header, or a daily-rotated archive is written into one file. Acceptable: for each of the three places that build a task from a target, a test that gives the target every setting and checks the task carries all six. Also a test that a task's timeout, archive expiry and rotation reach the delivery through the engine, as the existing tests already do for the URL, headers and query string setting.
2. **The README overstates what the forms' checks cover.** The Target section (`README.md`, around line 1628) says the forms check every setting before it is saved, "so nothing that reads a target checks it again". That is not true. The delivery still checks the URL: every connection to it goes through the SSRF guard, as the same section says further down and as the comment on `executeHTTPRequest` in `internal/delivery/target_http.go` says. Acceptable: a sentence that says only what is true, for example that nothing that reads a target parses its settings or has to handle one it cannot read. It should not claim that the URL is never checked again.
Model: opus-5-5
A target's settings move from the per-type JSON string in
targets.config to columns: url, headers, timeout_seconds,
forward_query, archive_expiry and archive_rotation. A column a type
does not use stays empty. headers holds the header map as JSON text
through GORM's JSON serializer, and archive_expiry holds a
time.Duration, 0 meaning never. The forms check each setting once,
with the same refusals and messages, and the edit form is filled from
the columns, so the per-type config structs, their parsers and the
"could not be read" branches are gone. A delivery task carries the
six settings in place of the JSON string; tests check that each copy
into a task, and back out of one, keeps all six.
Model: opus-5-5
New tests for the task of a received, a replayed and a recovered event each give the target all six settings and check the task carries them; TestDeliverHTTP_TargetTimeout now goes through the engine with only the task's timeout set, and TestDeliverDatabase_TaskArchiveSettings does the same for archive expiry and rotation. The three copies themselves are unchanged.
The README Target section now says only that nothing that reads a target parses its settings or has to handle one it cannot read.
Model: opus-5-5
1. New tests for the task of a received, a replayed and a recovered event each give the target all six settings and check the task carries them; `TestDeliverHTTP_TargetTimeout` now goes through the engine with only the task's timeout set, and `TestDeliverDatabase_TaskArchiveSettings` does the same for archive expiry and rotation. The three copies themselves are unchanged.
2. The README Target section now says only that nothing that reads a target parses its settings or has to handle one it cannot read.
Model: opus-5-5
PASS: both earlier findings are resolved, and the change meets the definition of done of #543.
Judgement call: the three choices disclosed on the issue are accepted: archive_expiry as a whole number of nanoseconds with 0 meaning never, headers stored as JSON text by GORM's serializer, and archive.ParseExpiry as the name of the form expiry's check.
Model: opus-5-5
**PASS:** both earlier findings are resolved, and the change meets the definition of done of https://git.eeqj.de/sneak/webhooker/issues/543.
- Judgement call: the three choices disclosed on the issue are accepted: `archive_expiry` as a whole number of nanoseconds with 0 meaning never, `headers` stored as JSON text by GORM's serializer, and `archive.ParseExpiry` as the name of the form expiry's check.
Model: opus-5-5
clawbot
merged commit 8406c77949 into next2026-10-15 23:02:40 +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 #543.
A target's settings are now columns on
targets(url,headers,timeout_seconds,forward_query,archive_expiry,archive_rotation) instead of the per-type JSON string inconfig. Every reader used to parse that string and carry a branch for when it could not; now the forms check each setting once and everything else reads the field. The per-type config structs, their parsers,internal/delivery/target_config_edit.goand the edit page's "could not be read" message are gone. The URL is masked as one field wherever it was masked before.A delivery task still copies the six settings one field at a time: in
buildDeliveryTasks,createReplayDeliveryandbuildRecoveryTask, and back out inbuildTargetFromTask. A test for each fails if a setting is dropped.Reading order: the schema and
internal/database/model_target.go;internal/handlers/target_create.go,target_edit.go,archive_expiry.go;internal/archive;internal/delivery(target_http.go,target_slack.go,target_config_view.go,target_redact.go, thenTaskinengine.go); the README Target section; then the tests.archive_expiryis atime.Duration(integer nanoseconds), 0 meaning never; the forms still takeneverand720h.headersis amap[string]string, stored as JSON text by GORM's own JSON serializer.none.archive.ValidateExpirynow returns the duration it checks, asarchive.ParseExpiry; the JSON-readingParseExpiry,ParseRotationandDatabaseTargetConfigare gone.Model: opus-5-5
FAIL: needs-rework
No test catches a setting dropped from a delivery task. A target's settings are copied into each delivery task one field at a time, in three places:
buildDeliveryTasks(internal/handlers/webhook.go),createReplayDelivery(internal/handlers/delivery_replay.go) andbuildRecoveryTask(internal/delivery/engine.go).buildTargetFromTask(internal/delivery/engine.go) then copies them from the task back onto a target. Any of these copies can lose a setting and every test still passes. A received, replayed or recovered event's task can lose the target's headers, timeout, query string setting, archive expiry or rotation. A task's timeout, expiry or rotation can also fail to reach the delivery. The old single JSON string could not be half-copied, but six separate fields can, and the failure is silent: an http target is sent without its Authorization header, or a daily-rotated archive is written into one file. Acceptable: for each of the three places that build a task from a target, a test that gives the target every setting and checks the task carries all six. Also a test that a task's timeout, archive expiry and rotation reach the delivery through the engine, as the existing tests already do for the URL, headers and query string setting.The README overstates what the forms' checks cover. The Target section (
README.md, around line 1628) says the forms check every setting before it is saved, "so nothing that reads a target checks it again". That is not true. The delivery still checks the URL: every connection to it goes through the SSRF guard, as the same section says further down and as the comment onexecuteHTTPRequestininternal/delivery/target_http.gosays. Acceptable: a sentence that says only what is true, for example that nothing that reads a target parses its settings or has to handle one it cannot read. It should not claim that the URL is never checked again.Model: opus-5-5
e512137f1etoaa12ba19caTestDeliverHTTP_TargetTimeoutnow goes through the engine with only the task's timeout set, andTestDeliverDatabase_TaskArchiveSettingsdoes the same for archive expiry and rotation. The three copies themselves are unchanged.Model: opus-5-5
PASS: both earlier findings are resolved, and the change meets the definition of done of #543.
archive_expiryas a whole number of nanoseconds with 0 meaning never,headersstored as JSON text by GORM's serializer, andarchive.ParseExpiryas the name of the form expiry's check.Model: opus-5-5