Fixes the timing side-channel vulnerability in webhook secret lookup (issue #13).
Problem
FindAppByWebhookSecret() passed the user-supplied secret directly to a SQL WHERE webhook_secret = ? clause. SQL string comparison short-circuits on the first differing byte, enabling timing attacks that could reveal the secret character by character.
Solution
Store a SHA-256 hash of the webhook secret in a new webhook_secret_hash column. On webhook receipt, hash the incoming secret and look up by hash. Since SHA-256 produces fixed-length output with avalanche properties, the SQL comparison time is independent of the original secret.
Backfill on startup: Hashes existing plaintext secrets into the new column
FindAppByWebhookSecret(): Now queries by hash instead of plaintext
App creation: Computes and stores hash at insert time
Tests: New TestHashWebhookSecret, all existing tests updated and passing
The plaintext webhook_secret column is retained for display (webhook URL generation).
Note
golangci-lint is not available in the build environment. go vet passes cleanly.
## Summary
Fixes the timing side-channel vulnerability in webhook secret lookup (issue #13).
### Problem
`FindAppByWebhookSecret()` passed the user-supplied secret directly to a SQL `WHERE webhook_secret = ?` clause. SQL string comparison short-circuits on the first differing byte, enabling timing attacks that could reveal the secret character by character.
### Solution
Store a SHA-256 hash of the webhook secret in a new `webhook_secret_hash` column. On webhook receipt, hash the incoming secret and look up by hash. Since SHA-256 produces fixed-length output with avalanche properties, the SQL comparison time is independent of the original secret.
### Changes
- **Migration 005**: Adds `webhook_secret_hash` column
- **`database.HashWebhookSecret()`**: SHA-256 hashing helper
- **Backfill on startup**: Hashes existing plaintext secrets into the new column
- **`FindAppByWebhookSecret()`**: Now queries by hash instead of plaintext
- **App creation**: Computes and stores hash at insert time
- **Tests**: New `TestHashWebhookSecret`, all existing tests updated and passing
The plaintext `webhook_secret` column is retained for display (webhook URL generation).
### Note
`golangci-lint` is not available in the build environment. `go vet` passes cleanly.
sneak
was assigned by clawbot2026-02-15 23:07:53 +01:00
Store a SHA-256 hash of the webhook secret in a new webhook_secret_hash
column. FindAppByWebhookSecret now hashes the incoming secret and queries
by hash, eliminating the SQL string comparison timing side-channel.
- Add migration 005_add_webhook_secret_hash.sql
- Add database.HashWebhookSecret() helper
- Backfill existing secrets on startup
- Update App model to include WebhookSecretHash in all queries
- Update app creation to compute hash at insert time
- Add TestHashWebhookSecret unit test
- Update all test fixtures to set WebhookSecretHash
Closes#13
$ go test ./...
ok git.eeqj.de/sneak/upaas/internal/database 0.172s
ok git.eeqj.de/sneak/upaas/internal/handlers 0.674s
ok git.eeqj.de/sneak/upaas/internal/models 0.275s
ok git.eeqj.de/sneak/upaas/internal/service/app 0.884s
ok git.eeqj.de/sneak/upaas/internal/service/auth 0.768s
ok git.eeqj.de/sneak/upaas/internal/service/webhook 1.137s
ok git.eeqj.de/sneak/upaas/internal/ssh 0.329s
All packages pass. No failures.
### Test Output 1: Full test suite
```
$ go test ./...
ok git.eeqj.de/sneak/upaas/internal/database 0.172s
ok git.eeqj.de/sneak/upaas/internal/handlers 0.674s
ok git.eeqj.de/sneak/upaas/internal/models 0.275s
ok git.eeqj.de/sneak/upaas/internal/service/app 0.884s
ok git.eeqj.de/sneak/upaas/internal/service/auth 0.768s
ok git.eeqj.de/sneak/upaas/internal/service/webhook 1.137s
ok git.eeqj.de/sneak/upaas/internal/ssh 0.329s
```
All packages pass. No failures.
$ go test -v -run 'TestHashWebhookSecret|TestAppFindByWebhookSecret|TestGetAppByWebhookSecret' ./...
=== RUN TestHashWebhookSecret
--- PASS: TestHashWebhookSecret (0.00s)
PASS
ok git.eeqj.de/sneak/upaas/internal/database 0.177s
=== RUN TestAppFindByWebhookSecret
--- PASS: TestAppFindByWebhookSecret (0.01s)
PASS
ok git.eeqj.de/sneak/upaas/internal/models 0.354s
=== RUN TestGetAppByWebhookSecret
=== RUN TestGetAppByWebhookSecret/finds_app_by_webhook_secret
--- PASS: TestGetAppByWebhookSecret/finds_app_by_webhook_secret
=== RUN TestGetAppByWebhookSecret/returns_nil_for_invalid_secret
--- PASS: TestGetAppByWebhookSecret/returns_nil_for_invalid_secret
--- PASS: TestGetAppByWebhookSecret (0.04s)
PASS
ok git.eeqj.de/sneak/upaas/internal/service/app 1.052s
Linting
golangci-lint not available in build environment. go vet ./... passes cleanly with no warnings.
### Test Output 2: Targeted tests (verbose)
```
$ go test -v -run 'TestHashWebhookSecret|TestAppFindByWebhookSecret|TestGetAppByWebhookSecret' ./...
=== RUN TestHashWebhookSecret
--- PASS: TestHashWebhookSecret (0.00s)
PASS
ok git.eeqj.de/sneak/upaas/internal/database 0.177s
=== RUN TestAppFindByWebhookSecret
--- PASS: TestAppFindByWebhookSecret (0.01s)
PASS
ok git.eeqj.de/sneak/upaas/internal/models 0.354s
=== RUN TestGetAppByWebhookSecret
=== RUN TestGetAppByWebhookSecret/finds_app_by_webhook_secret
--- PASS: TestGetAppByWebhookSecret/finds_app_by_webhook_secret
=== RUN TestGetAppByWebhookSecret/returns_nil_for_invalid_secret
--- PASS: TestGetAppByWebhookSecret/returns_nil_for_invalid_secret
--- PASS: TestGetAppByWebhookSecret (0.04s)
PASS
ok git.eeqj.de/sneak/upaas/internal/service/app 1.052s
```
### Linting
`golangci-lint` not available in build environment. `go vet ./...` passes cleanly with no warnings.
Clean, well-structured fix for the timing side-channel in webhook secret lookup.
Security Analysis
SHA-256 is sufficient here. The goal is not password storage (where you'd want bcrypt/argon2) -- it's to eliminate timing side-channels in SQL string comparison. SHA-256 produces fixed-length output with avalanche properties, making the SQL WHERE webhook_secret_hash = ? comparison constant-time w.r.t. the original secret. The secrets are high-entropy UUIDs, so rainbow tables / brute-force are not practical concerns.
Migration and Backfill
Migration 005: DEFAULT '' ensures existing rows get a valid value; no data loss risk
backfillWebhookSecretHashes() correctly collects rows before updating (avoids cursor issues), queries only rows needing backfill, logs each update
Runs after migrations in connect(), so the column exists
Transition Period
Fail-closed: if an app somehow has no hash (crash between migration and backfill), FindAppByWebhookSecret simply will not match. New apps compute hash at insert time. Backfill covers existing apps on next startup.
Code Quality
HashWebhookSecret is clean, exported, pure function
appColumns and scan order updated consistently
Insert includes webhook_secret_hash; update deliberately omits it (secret does not change) -- correct
All test helpers set WebhookSecretHash via database.HashWebhookSecret()
No index on webhook_secret_hash -- fine at current scale, worth adding if apps table grows significantly
Plaintext webhook_secret retained for URL generation, documented in PR description
Ready to merge.
## Code Review: Hashed Webhook Secrets
**Verdict: MERGE-READY**
Clean, well-structured fix for the timing side-channel in webhook secret lookup.
### Security Analysis
**SHA-256 is sufficient here.** The goal is not password storage (where you'd want bcrypt/argon2) -- it's to eliminate timing side-channels in SQL string comparison. SHA-256 produces fixed-length output with avalanche properties, making the SQL `WHERE webhook_secret_hash = ?` comparison constant-time w.r.t. the original secret. The secrets are high-entropy UUIDs, so rainbow tables / brute-force are not practical concerns.
### Migration and Backfill
- Migration 005: `DEFAULT ''` ensures existing rows get a valid value; no data loss risk
- `backfillWebhookSecretHashes()` correctly collects rows before updating (avoids cursor issues), queries only rows needing backfill, logs each update
- Runs after migrations in `connect()`, so the column exists
### Transition Period
Fail-closed: if an app somehow has no hash (crash between migration and backfill), `FindAppByWebhookSecret` simply will not match. New apps compute hash at insert time. Backfill covers existing apps on next startup.
### Code Quality
- `HashWebhookSecret` is clean, exported, pure function
- `appColumns` and scan order updated consistently
- Insert includes `webhook_secret_hash`; update deliberately omits it (secret does not change) -- correct
- All test helpers set `WebhookSecretHash` via `database.HashWebhookSecret()`
### Tests
- `TestHashWebhookSecret`: determinism, uniqueness, known-value assertion
- `TestAppFindByWebhookSecret`: round-trip hash lookup
- All existing tests updated
### Minor Observations (non-blocking)
- No index on `webhook_secret_hash` -- fine at current scale, worth adding if apps table grows significantly
- Plaintext `webhook_secret` retained for URL generation, documented in PR description
Ready to merge.
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.
Summary
Fixes the timing side-channel vulnerability in webhook secret lookup (issue #13).
Problem
FindAppByWebhookSecret()passed the user-supplied secret directly to a SQLWHERE webhook_secret = ?clause. SQL string comparison short-circuits on the first differing byte, enabling timing attacks that could reveal the secret character by character.Solution
Store a SHA-256 hash of the webhook secret in a new
webhook_secret_hashcolumn. On webhook receipt, hash the incoming secret and look up by hash. Since SHA-256 produces fixed-length output with avalanche properties, the SQL comparison time is independent of the original secret.Changes
webhook_secret_hashcolumndatabase.HashWebhookSecret(): SHA-256 hashing helperFindAppByWebhookSecret(): Now queries by hash instead of plaintextTestHashWebhookSecret, all existing tests updated and passingThe plaintext
webhook_secretcolumn is retained for display (webhook URL generation).Note
golangci-lintis not available in the build environment.go vetpasses cleanly.Test Output 1: Full test suite
All packages pass. No failures.
Test Output 2: Targeted tests (verbose)
Linting
golangci-lintnot available in build environment.go vet ./...passes cleanly with no warnings.Code Review: Hashed Webhook Secrets
Verdict: MERGE-READY
Clean, well-structured fix for the timing side-channel in webhook secret lookup.
Security Analysis
SHA-256 is sufficient here. The goal is not password storage (where you'd want bcrypt/argon2) -- it's to eliminate timing side-channels in SQL string comparison. SHA-256 produces fixed-length output with avalanche properties, making the SQL
WHERE webhook_secret_hash = ?comparison constant-time w.r.t. the original secret. The secrets are high-entropy UUIDs, so rainbow tables / brute-force are not practical concerns.Migration and Backfill
DEFAULT ''ensures existing rows get a valid value; no data loss riskbackfillWebhookSecretHashes()correctly collects rows before updating (avoids cursor issues), queries only rows needing backfill, logs each updateconnect(), so the column existsTransition Period
Fail-closed: if an app somehow has no hash (crash between migration and backfill),
FindAppByWebhookSecretsimply will not match. New apps compute hash at insert time. Backfill covers existing apps on next startup.Code Quality
HashWebhookSecretis clean, exported, pure functionappColumnsand scan order updated consistentlywebhook_secret_hash; update deliberately omits it (secret does not change) -- correctWebhookSecretHashviadatabase.HashWebhookSecret()Tests
TestHashWebhookSecret: determinism, uniqueness, known-value assertionTestAppFindByWebhookSecret: round-trip hash lookupMinor Observations (non-blocking)
webhook_secret_hash-- fine at current scale, worth adding if apps table grows significantlywebhook_secretretained for URL generation, documented in PR descriptionReady to merge.