Target credentials leak into the per-webhook event databases via GORM association upsert #206
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?
Two related exposures of the same secret.
targets.configis stored in plaintext inwebhooker.db. For a Slack target thewebhookUrlIS the bearer credential; for anhttptarget the URL can embed userinfo. The UI masks these on screen (#113, #115, #118) but the storage is clear.Worse, they do not stay in that one file. GORM's association upsert on the retry path copies the whole target row,
configincluded, into the PER-WEBHOOK event database, with an emptywebhook_id. Confirmed:returns live destination URLs. So anything that backs up, ships, or hands over an event DB — and event DBs are the thing most likely to be copied around, since they hold the payload history — ships the credentials with it.
Definition of done:
Omit(clause.Associations)or a scoped session), so only delivery and event rows are written theretargetsrows already written into existing event DBsevents-*.dbcontains notargetsrowswebhooker.dbusing the existing session-key material or a dedicated key, OR, if that is judged out of scope for 1.0, that decision is recorded on this issue and the exposure documented in the README backup sectionThe first three bullets are the blocking part; the encryption question can be split out if it is large.
Scope for 1.0: the first three bullets only — stop the association upsert writing
targetsrows into the per-webhook event DBs, sweep the rows already written into existing event DBs, and test that a delivery plus a retry leaves notargetsrows there.Encryption at rest is explicitly OUT of this unit and deferred past 1.0, per the escape hatch in the definition of done. It needs a key-management story (key source, rotation, re-wrap migration) and getting it wrong loses every target config; the file is trusted-operator-only in the meantime. Tracked at #212, and the exposure is documented in the README backup section by #210.
Implementation notes: the sweep must be idempotent and must not fail startup on an event DB that has no
targetstable at all. Deleting rows is correct here — those rows are junk with an emptywebhook_id, not history; delivery history carries no FK to them.