sneak/webhooker
1
0
Fork 0
Code Issues 13 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Validate Slack target URLs at creation time (closes #68) #73

Merged
sneak merged 3 commits from issue-68-slack-url-validation into main 2026-08-07 14:03:56 +02:00
Conversation 2 Commits 3 Files Changed 3 +76 -1
clawbot commented 2026-08-07 13:58:57 +02:00
Collaborator
Copy Link

Slack delivery targets were only checked by the request-time dialer guard, not at creation, giving them a weaker SSRF gate than HTTP targets.

This validates the Slack incoming-webhook URL with delivery.ValidateTargetURL in the Slack target creation path (buildSlackTargetConfig), before persisting, mirroring the existing HTTP-target path. On failure the create is rejected with the same clear, non-leaking user-facing error the HTTP path uses.

Adds handlers-package tests covering both an accepted public URL and a rejected private/reserved URL. Confined to internal/handlers/; internal/delivery/ is unchanged.

Closes #68

Slack delivery targets were only checked by the request-time dialer guard, not at creation, giving them a weaker SSRF gate than HTTP targets. This validates the Slack incoming-webhook URL with `delivery.ValidateTargetURL` in the Slack target creation path (`buildSlackTargetConfig`), before persisting, mirroring the existing HTTP-target path. On failure the create is rejected with the same clear, non-leaking user-facing error the HTTP path uses. Adds handlers-package tests covering both an accepted public URL and a rejected private/reserved URL. Confined to `internal/handlers/`; `internal/delivery/` is unchanged. Closes #68
clawbot added 1 commit 2026-08-07 13:58:57 +02:00
Validate Slack target URLs at creation time (closes #68)
Some checks failed
check / check (push) Has been cancelled
Details
2500c41113
clawbot commented 2026-08-07 13:59:09 +02:00
Author
Collaborator
Copy Link

What changed

Confined to internal/handlers/ (validator in internal/delivery/ untouched):

  • internal/handlers/source_management.go: buildSlackTargetConfig now takes the request and calls delivery.ValidateTargetURL(r.Context(), targetURL) before persisting, mirroring buildHTTPTargetConfig. On failure it logs a warning and returns Invalid target URL: ... with HTTP 400 (same pattern as the HTTP path; no internals leaked). Caller in buildTargetConfig updated to pass r.
  • internal/handlers/export_test.go: added BuildSlackTargetConfigForTest to exercise the unexported builder from the handlers_test package.
  • internal/handlers/handlers_test.go: added TestBuildSlackTargetConfig_AcceptsPublicURL (public IP literal, expects 200 + config containing webhookUrl) and TestBuildSlackTargetConfig_RejectsReservedURL (http://169.254.169.254/latest/meta-data/, expects error + HTTP 400 + empty config).

Diffstat: 3 files changed, 76 insertions(+), 1 deletion(-).

Validation

docker build . (fmt-check, lint, test, build) exited 0:

#33 writing image sha256:c856a3ab4566ea7282325f3b414b3d19e753b3b2b6b877fb74061fd4b11fcd1a done
#33 naming to docker.io/library/webhooker-issue68 done
#33 DONE 0.2s
### What changed Confined to `internal/handlers/` (validator in `internal/delivery/` untouched): - `internal/handlers/source_management.go`: `buildSlackTargetConfig` now takes the request and calls `delivery.ValidateTargetURL(r.Context(), targetURL)` before persisting, mirroring `buildHTTPTargetConfig`. On failure it logs a warning and returns `Invalid target URL: ...` with HTTP 400 (same pattern as the HTTP path; no internals leaked). Caller in `buildTargetConfig` updated to pass `r`. - `internal/handlers/export_test.go`: added `BuildSlackTargetConfigForTest` to exercise the unexported builder from the `handlers_test` package. - `internal/handlers/handlers_test.go`: added `TestBuildSlackTargetConfig_AcceptsPublicURL` (public IP literal, expects 200 + config containing `webhookUrl`) and `TestBuildSlackTargetConfig_RejectsReservedURL` (`http://169.254.169.254/latest/meta-data/`, expects error + HTTP 400 + empty config). Diffstat: 3 files changed, 76 insertions(+), 1 deletion(-). ### Validation `docker build .` (fmt-check, lint, test, build) exited 0: ``` #33 writing image sha256:c856a3ab4566ea7282325f3b414b3d19e753b3b2b6b877fb74061fd4b11fcd1a done #33 naming to docker.io/library/webhooker-issue68 done #33 DONE 0.2s ```
clawbot commented 2026-08-07 14:00:45 +02:00
Author
Collaborator
Copy Link

Independent review — PASS (merge-ready)

Adversarial review against the issue spec and repo policies.

  • internal/handlers/source_management.go: buildSlackTargetConfig now calls delivery.ValidateTargetURL(r.Context(), targetURL) before persisting — the same create-time validation the HTTP target path uses — and on failure returns 400 without leaking internals; buildTargetConfig was updated to thread the request through. Change is confined to internal/handlers/; internal/delivery/ is untouched (it calls the existing exported validator, as instructed).
  • Tests: accept path uses a public IP literal (expects 200 and a config containing webhookUrl); reject path uses the metadata address 169.254.169.254 (expects an error, 400, and empty config). Both use IP literals, so they do not depend on DNS and are deterministic. The green docker build . runs these, which proves the validator blocks the reserved address and accepts the public one.
  • make fmt clean; no AI/tooling references; commit subject closes the issue.

Verdict: meets the bar. Marking merge-ready and handing to @sneak for final review.

## Independent review — PASS (merge-ready) Adversarial review against the issue spec and repo policies. - `internal/handlers/source_management.go`: `buildSlackTargetConfig` now calls `delivery.ValidateTargetURL(r.Context(), targetURL)` before persisting — the same create-time validation the HTTP target path uses — and on failure returns 400 without leaking internals; `buildTargetConfig` was updated to thread the request through. Change is confined to `internal/handlers/`; `internal/delivery/` is untouched (it calls the existing exported validator, as instructed). - Tests: accept path uses a public IP literal (expects 200 and a config containing `webhookUrl`); reject path uses the metadata address `169.254.169.254` (expects an error, 400, and empty config). Both use IP literals, so they do not depend on DNS and are deterministic. The green `docker build .` runs these, which proves the validator blocks the reserved address and accepts the public one. - `make fmt` clean; no AI/tooling references; commit subject closes the issue. Verdict: meets the bar. Marking merge-ready and handing to @sneak for final review.
sneak was assigned by clawbot 2026-08-07 14:00:46 +02:00
sneak added 1 commit 2026-08-07 14:00:59 +02:00
Merge branch 'main' into issue-68-slack-url-validation
Some checks failed
check / check (push) Has been cancelled
Details
3b19463788
sneak added 1 commit 2026-08-07 14:03:47 +02:00
Merge branch 'main' into issue-68-slack-url-validation
All checks were successful
check / check (push) Successful in 2m42s
Details
d3bf348871
sneak merged commit 752d6beead into main 2026-08-07 14:03:56 +02:00
sneak deleted branch issue-68-slack-url-validation 2026-08-07 14:03:56 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#73
Delete Branch "issue-68-slack-url-validation"

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?