Skip to content

Commit 030891b

Browse files
mastermanas805claude
mastermanas805
and
claude
authored
feat(github-app): P4.2 — App webhook → push-to-deploy + security hardening (#225)
* feat(github-app): P4.2 — App webhook → push-to-deploy + security hardening POST /webhooks/github: one App-level endpoint for ALL installations (vs the manual per-repo /webhooks/github/:webhook_id). Verifies the App HMAC, dedupes on X-GitHub-Delivery (Redis), and dispatches: `push` → match repo+branch to a connection linked to that installation+team → enqueue the same rate-limited redeploy the manual path uses; `installation` deleted/suspend/unsuspend → sync github_installations. Gated by GITHUB_APP_ENABLED (default OFF). Token-into-clone wiring is P4.2b. Built with parallel agents: internal/github/webhook.go (HMAC verify + push/installation parsers, 100%), internal/models/github_push_match.go (repo/branch + installation lookups, 100%), infra observability (separate repo). Security review fixes (adversarial pass): - HIGH-1: config.Load now fails closed — GITHUB_APP_ENABLED=true with an empty GITHUB_APP_WEBHOOK_SECRET (<16 chars) / missing key / missing id panics, instead of serving an HMAC verifiable with a public empty key. - HIGH-2: UpsertGitHubInstallation is WHERE-guarded so a callback can't rebind an installation owned by another team → ErrGitHubInstallationTeamConflict → 409 install_conflict (anti-hijack). Residual first-writer race + worker-dispatch guard (MED-2) documented as pre-flag-flip prerequisites in the runbook. - MED-1: webhook event metric label canonicalized to a bounded set before the HMAC check (pre-auth Prometheus cardinality DoS). - LOW-2: push `after` validated as 40-hex; response reports successfully-enqueued count, not matched (matched/enqueued split). Metrics instant_github_webhook_received_total / _pushdeploy_total (+ infra alerts/dashboard/catalog in the companion infra change, rule 25). New routes whitelisted in the openapi route-coverage gate (GitHub-facing, flag-gated; contract sync is P4.3). Coverage: 100% on the new github/models packages + the webhook handler arms (white-box + sqlmock + DB/Redis-gated). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(github-app): cover P4.2 config fail-closed + webhook error arms Close the patch-coverage gaps CI flagged on the P4.2 review fixes: - config.go 492/495 — the private-key-missing + app-id-missing fail-closed panic arms (HIGH-1), via two more mustPanic cases. - github_app_webhook.go 132-141/201-207 — the installation delete/suspend/ unsuspend slog.Warn arms, the generic enqueue-error arm, and the last-deploy-update warn arm, via sqlmock white-box tests (force each DB op to error; the real-DB integration path can't reach them). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 1c195e4 commit 030891b

20 files changed

Lines changed: 1950 additions & 5 deletions

‎internal/config/config.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -479,6 +479,22 @@ func Load() *Config {
479479
cfg.GitHubAppWebhookSecret = os.Getenv("GITHUB_APP_WEBHOOK_SECRET")
480480
cfg.GitHubAppClientID = os.Getenv("GITHUB_APP_CLIENT_ID")
481481
cfg.GitHubAppClientSecret = os.Getenv("GITHUB_APP_CLIENT_SECRET")
482+
// Fail-closed when the App is enabled: an empty GITHUB_APP_WEBHOOK_SECRET
483+
// makes the webhook HMAC verifiable with a publicly-known (empty) key — any
484+
// attacker could forge a valid X-Hub-Signature-256. Likewise an empty
485+
// private key / App ID can't mint tokens. Panic at Load (like JWT_SECRET)
486+
// rather than silently serve an auth-bypassing webhook. (Review HIGH-1.)
487+
if cfg.GitHubAppEnabled {
488+
if len(strings.TrimSpace(cfg.GitHubAppWebhookSecret)) < 16 {
489+
panic(&ErrMissingConfig{Key: "GITHUB_APP_WEBHOOK_SECRET (>=16 chars required when GITHUB_APP_ENABLED=true)"})
490+
}
491+
if strings.TrimSpace(cfg.GitHubAppPrivateKey) == "" {
492+
panic(&ErrMissingConfig{Key: "GITHUB_APP_PRIVATE_KEY (required when GITHUB_APP_ENABLED=true)"})
493+
}
494+
if strings.TrimSpace(cfg.GitHubAppID) == "" {
495+
panic(&ErrMissingConfig{Key: "GITHUB_APP_ID (required when GITHUB_APP_ENABLED=true)"})
496+
}
497+
}
482498

483499
if len(cfg.JWTSecret) < 32 {
484500
panic("JWT_SECRET must be at least 32 bytes")

‎internal/config/config_test.go‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -374,8 +374,18 @@ func TestLoad_DeploySourceGitEnabled(t *testing.T) {
374374
}
375375

376376
func TestLoad_GitHubAppEnabled(t *testing.T) {
377+
// When enabling the App, Load() fails closed unless the webhook secret +
378+
// private key + app id are present (review HIGH-1), so set them here.
379+
appSecrets := func(enabled string) map[string]string {
380+
return map[string]string{
381+
"GITHUB_APP_ENABLED": enabled,
382+
"GITHUB_APP_WEBHOOK_SECRET": "a-sufficiently-long-webhook-secret",
383+
"GITHUB_APP_PRIVATE_KEY": "-----BEGIN RSA PRIVATE KEY-----\nx\n-----END RSA PRIVATE KEY-----",
384+
"GITHUB_APP_ID": "12345",
385+
}
386+
}
377387
for _, val := range []string{"true", "1", "yes", "TRUE", " Yes "} {
378-
applyBaselineEnv(t, map[string]string{"GITHUB_APP_ENABLED": val})
388+
applyBaselineEnv(t, appSecrets(val))
379389
if !Load().GitHubAppEnabled {
380390
t.Errorf("GITHUB_APP_ENABLED=%q should enable", val)
381391
}
@@ -386,6 +396,28 @@ func TestLoad_GitHubAppEnabled(t *testing.T) {
386396
t.Errorf("GITHUB_APP_ENABLED=%q should stay disabled", val)
387397
}
388398
}
399+
400+
// Fail-closed: enabling without each required secret must panic, not
401+
// silently serve an HMAC-bypassable / token-less App.
402+
mustPanic := func(name string, env map[string]string) {
403+
defer func() {
404+
if recover() == nil {
405+
t.Errorf("%s: Load() must panic", name)
406+
}
407+
}()
408+
applyBaselineEnv(t, env)
409+
_ = Load()
410+
}
411+
mustPanic("no webhook secret", map[string]string{"GITHUB_APP_ENABLED": "true"})
412+
mustPanic("no private key", map[string]string{
413+
"GITHUB_APP_ENABLED": "true",
414+
"GITHUB_APP_WEBHOOK_SECRET": "a-sufficiently-long-webhook-secret",
415+
})
416+
mustPanic("no app id", map[string]string{
417+
"GITHUB_APP_ENABLED": "true",
418+
"GITHUB_APP_WEBHOOK_SECRET": "a-sufficiently-long-webhook-secret",
419+
"GITHUB_APP_PRIVATE_KEY": "-----BEGIN RSA PRIVATE KEY-----\nx\n-----END RSA PRIVATE KEY-----",
420+
})
389421
// the GITHUB_APP_* values are plumbed verbatim.
390422
applyBaselineEnv(t, map[string]string{
391423
"GITHUB_APP_ID": "12345",

‎internal/github/webhook.go‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
// Webhook verification and event parsing helpers for the InstaNode GitHub App
2+
// (P4.2). These functions are called by the HTTP handler that receives
3+
// X-GitHub-Event deliveries; they must be called before any payload processing
4+
// so an unverified payload is never acted on.
5+
package github
6+
7+
import (
8+
"crypto/hmac"
9+
"crypto/sha256"
10+
"encoding/hex"
11+
"encoding/json"
12+
"fmt"
13+
"strings"
14+
)
15+
16+
// VerifyWebhookSignature checks the X-Hub-Signature-256 header GitHub sends
17+
// with every delivery. It returns nil only when the HMAC-SHA256 computed from
18+
// body and secret matches the signature in signatureHeader; any other condition
19+
// (missing header, bad prefix, non-hex value, or mismatch) returns an error.
20+
// Comparison is constant-time (hmac.Equal) so this is safe against timing
21+
// attacks. Error messages never echo the secret or the expected signature.
22+
func VerifyWebhookSignature(body []byte, signatureHeader, secret string) error {
23+
if signatureHeader == "" {
24+
return fmt.Errorf("github webhook: missing X-Hub-Signature-256 header")
25+
}
26+
const prefix = "sha256="
27+
if !strings.HasPrefix(signatureHeader, prefix) {
28+
return fmt.Errorf("github webhook: signature header has wrong prefix (expected sha256=)")
29+
}
30+
hexSig := signatureHeader[len(prefix):]
31+
gotSig, err := hex.DecodeString(hexSig)
32+
if err != nil {
33+
return fmt.Errorf("github webhook: signature is not valid hex")
34+
}
35+
36+
mac := hmac.New(sha256.New, []byte(secret))
37+
mac.Write(body)
38+
wantSig := mac.Sum(nil)
39+
40+
if !hmac.Equal(wantSig, gotSig) {
41+
return fmt.Errorf("github webhook: signature mismatch")
42+
}
43+
return nil
44+
}
45+
46+
// pushEventWire is the minimal shape of a GitHub push event we need.
47+
type pushEventWire struct {
48+
Ref string `json:"ref"`
49+
After string `json:"after"`
50+
Repository struct {
51+
FullName string `json:"full_name"`
52+
} `json:"repository"`
53+
Installation struct {
54+
ID int64 `json:"id"`
55+
} `json:"installation"`
56+
}
57+
58+
// PushEvent holds the normalised fields from a GitHub push webhook payload.
59+
type PushEvent struct {
60+
// Repo is repository.full_name, e.g. "acme/my-app".
61+
Repo string
62+
// Ref is the full ref string, e.g. "refs/heads/main" or "refs/tags/v1.0".
63+
Ref string
64+
// HeadCommitSHA is the "after" field — the SHA of the head commit after the push.
65+
HeadCommitSHA string
66+
// InstallationID is installation.id from the delivery envelope.
67+
InstallationID int64
68+
}
69+
70+
// Branch returns the short branch name extracted from Ref (e.g. "main" from
71+
// "refs/heads/main"). It returns "" when Ref is not a branch ref (e.g. a tag
72+
// or an empty string).
73+
func (e *PushEvent) Branch() string {
74+
const branchPrefix = "refs/heads/"
75+
if strings.HasPrefix(e.Ref, branchPrefix) {
76+
return e.Ref[len(branchPrefix):]
77+
}
78+
return ""
79+
}
80+
81+
// ParsePushEvent decodes a GitHub push event payload. Only the fields InstaNode
82+
// needs for a build dispatch are populated; every other field is ignored.
83+
func ParsePushEvent(body []byte) (*PushEvent, error) {
84+
var w pushEventWire
85+
if err := json.Unmarshal(body, &w); err != nil {
86+
return nil, fmt.Errorf("github webhook: parse push event: %w", err)
87+
}
88+
return &PushEvent{
89+
Repo: w.Repository.FullName,
90+
Ref: w.Ref,
91+
HeadCommitSHA: w.After,
92+
InstallationID: w.Installation.ID,
93+
}, nil
94+
}
95+
96+
// installationEventWire is the minimal shape of a GitHub installation event.
97+
type installationEventWire struct {
98+
Action string `json:"action"`
99+
Installation struct {
100+
ID int64 `json:"id"`
101+
Account struct {
102+
Login string `json:"login"`
103+
} `json:"account"`
104+
} `json:"installation"`
105+
}
106+
107+
// InstallationEvent holds the normalised fields from a GitHub installation
108+
// webhook payload (created / deleted / suspend / unsuspend).
109+
type InstallationEvent struct {
110+
// Action is the lifecycle verb: "created", "deleted", "suspend", or "unsuspend".
111+
Action string
112+
// InstallationID is installation.id.
113+
InstallationID int64
114+
// AccountLogin is installation.account.login — the GitHub user or org that owns the install.
115+
AccountLogin string
116+
}
117+
118+
// ParseInstallationEvent decodes a GitHub installation event payload.
119+
func ParseInstallationEvent(body []byte) (*InstallationEvent, error) {
120+
var w installationEventWire
121+
if err := json.Unmarshal(body, &w); err != nil {
122+
return nil, fmt.Errorf("github webhook: parse installation event: %w", err)
123+
}
124+
return &InstallationEvent{
125+
Action: w.Action,
126+
InstallationID: w.Installation.ID,
127+
AccountLogin: w.Installation.Account.Login,
128+
}, nil
129+
}

0 commit comments

Comments
 (0)