sneak/upaas
1
0
Fork 0
Code Issues 21 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

Use hashed webhook secrets for constant-time comparison (closes #13) #15

Merged
sneak merged 2 commits from :fix/issue-13 into main 2026-02-16 05:55:46 +01:00
Conversation 3 Commits 2 Files Changed 7 +117 -27
clawbot commented 2026-02-15 23:07:53 +01:00
Collaborator
Copy Link

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.

## 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 clawbot 2026-02-15 23:07:53 +01:00
clawbot added 1 commit 2026-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
clawbot commented 2026-02-15 23:08:02 +01:00
Author
Collaborator
Copy Link

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.

### 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.
clawbot commented 2026-02-15 23:08:10 +01:00
Author
Collaborator
Copy Link

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.

### 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.
clawbot commented 2026-02-15 23:18:25 +01:00
Author
Collaborator
Copy Link

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.

## 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.
sneak added 1 commit 2026-02-16 05:55:18 +01:00
sneak merged commit 076442923c into main 2026-02-16 05:55:46 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
notplanned

   
question

More information is needed

  
wontfix

This won't be fixed

No Label
bug
duplicate
enhancement
help wanted
invalid
notplanned
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
sneak
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sneak/upaas#15
No description provided.
Delete Branch ":fix/issue-13"

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?