Target create reads the destination URL via r.FormValue, so a query-string credential lands in the access log and Sentry #160
Reference in New Issue
Block a user
Delete Branch "%!s()"
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?
Found by the independent review of #126, which was masking the same credential on the render path. Not introduced by that PR.
internal/handlers/source_management.go:1002reads the destination withr.FormValue("url").r.FormValuefalls back to the URL query string when the field is absent from the POST body.internal/middleware/middleware.go:123logs"url", r.URL.String()unmasked, and the same URL reaches the Sentry request context.So:
creates the target AND writes the full credential into the access log and to Sentry — off-host. This is the same credential class that #113, #118 and #115 were each filed to contain; those closed the render, delivery-error and log-line paths, and this is the remaining ingress path.
Milestoned 1.0.0 on that basis: the release has treated "the webhook URL must not come to rest anywhere an operator did not intend" as a blocking property three times already, and a partial fix of a credential-exposure class is the kind of gap that reads as closed when it is not. It requires an authenticated request to trigger, but the consequence is a live third-party credential written to log storage and shipped to an external error tracker.
Definition of done
r.PostFormValue— so a query-string value cannot populate a target's configuration. Auditsource_management.gofor every otherr.FormValuecall and convert each one whose value is secret-bearing or security-relevant; state in the PR which you converted and which you deliberately left.?url=<secret>with an empty body asserts the access-log line contains no path segment of that URL, and asserts the target was NOT created from the query value.Second, cheaper item found in the same review
database.Targetcarriesjson:"config", and the/api/v1route group exists but is empty.TargetViewis the masking barrier for the HTML path only — the first handler that marshals the model directly will serialise the raw config blob, credential included. Addjson:"-"to that field now, while it costs one line and no behaviour, rather than after an API handler exists. If any code genuinely needs to marshal it, say so in the PR instead.Implementation requirements
next, PR based onnext, single commit, title ending(closes #N).TODO.md(see #112).make checkplus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host.