tenseleyflow/shithub / dc4dc18

+

+## PRO08 GA audit closure

+

+**Audit dates**: 2026-05-13 (run + remediation in a single sprint).

+

+**Scope**: PRO04 Stripe adapter + PRO05 entitlements Principal +

+PRO06 user-tier billing settings + PRO07 Pro v1 feature gates.

+

+**Methodology**: four parallel security/correctness audit agents

+ran read-only inspections against the PRO07 tip plus the deployed

+host (`shithub.sh`). Findings were triaged per-bug; HIGH and

+MEDIUM severity gaps were fixed inline on the `payments-pro/08-pro-ga-audit`

+branch with locking tests. LOW-severity items were either fixed or

+documented per the user directive "we don't want to take any chances

+with payments."

+

+### Findings + remediation status

+

+| # | Severity | Finding | Fix | Test |

+|---|---|---|---|---|

+| A1 | MED | `guardPriceKindMatch` bypassable on empty `Items` | refuse empty-items when prices configured | `TestBillingWebhookGuardRefusesEmptyItemsWhenPricesConfigured` |

+| A2 | HIGH | `(subject_kind, subject_id)` never written to receipts | `SetWebhookEventSubjectForPrincipal` after every resolve; `ListFailedWebhookEvents` operator query | `TestSetWebhookEventSubjectForPrincipalRecordsAuditTrail`, `TestListFailedWebhookEventsReturnsErroredAndStuckEntries` |

+| A3 | MED | Concurrent dup-replay TOCTOU race | session-scoped advisory lock keyed on event id | `TestBillingWebhookConcurrentReplayAppliesOnce` |

+| D1 | HIGH | Snapshot CTE wipes lock columns on past_due transitions | conditional preservation in `ApplySubscriptionSnapshot` + user analog | `TestApplySubscriptionSnapshotPreservesGraceLockOnPastDue` (+ recovery + user-side mirror) |

+| D2 | LOW | No `charge.refunded` handler | new dispatch + `MarkInvoiceRefunded` query + enum + UI surface | `TestBillingWebhookChargeRefundedMarksInvoiceRefunded` (+ unknown invoice + standalone refund) |

+| D3 | MED | Two subscriptions per customer silently overwrite | `guardSubscriptionOverwrite` rejects different-sub apply | `TestBillingWebhookGuardRefusesSecondSubscriptionForSameCustomer` |

+| D4 | MED | No stale-event detection (reverse-order corruption) | `last_event_at` column + `IsBillingEventStaleForPrincipal` + handler guard + post-apply touch | `TestBillingWebhookDropsStaleEvent` |

+| D5 | LOW | `customer.subscription.deleted` for unknown sub 5xx's | log + 200 no-op so Stripe stops retrying | `TestBillingWebhookSubscriptionDeletedForUnknownSubIsNoOp` |

+| C1 | HIGH | `require_signed_commits` unreachable from UI | form field + template checkbox + sqlc plumbing | covered by `TestSettingsBranches_UserKindEnforceFlipPreventForcePushBlocks` table |

+| C2 | HIGH | Advanced BP gate fires on wrong inputs | rewire predicate to fire on prevent_*, signing, AND status-checks | table test |

+| C3 | MED | Multi-reviewer deny copy indistinguishable | thread reviewer count → `required-reviewers-multi-upgrade(-pro)` codes | `TestSettingsBranches_UserKindEnforceFlipMultiReviewerBlocks` |

+| C4 | MED | Notice copy says "Team" / "organization" on user-tier denies | user-kind variants (`-pro` suffix) with `/settings/billing` href | `TestSettingsBranches_UserKindEnforceFlipRequiredReviewersBlocksSingle` (Pro copy) |

+| C5 | LOW | `profilePinsRemaining` hard-codes cap; under-counts for Pro | thread entitled cap into the function | covered by existing profile_test suite + no regression |

+

+Authorization audit (Agent B) found **no** cross-tenant data leak,

+no invoice-scoping bleed, and no deny-shape 500 reachable from

+production paths. Two `err→500` branches in the entitlement gate

+remain (in `RequirePrincipalFeature` and `evaluateBranchProtectionFeature`)

+but are defended against by the AFTER-INSERT seed triggers on

+both billing-state tables — `pgx.ErrNoRows` for a valid principal

+is dead code in production.

+

+### Pre-existing failures NOT introduced by PRO08

+

+These were noted across prior sprints and remain open:

+

+- `TestPrivateCollaborationExpansionEnforcesFreeLimitAndTeamUnlimited`

+- `TestPrivateCollaborationUsageCountsEffectivePrivateAccess`

+- `TestRepoPrivateVisibilityCountsRepoSpecificGrants`

+

+The fourth pre-existing failure

+(`TestSettingsBranchesAllowsDowngradedOrgToRemoveAdvancedSettings`)

+was fixed opportunistically while in the branch-protection cluster

+(seed fixture needed `AllowedPusherUserIds: []int64{}` to satisfy

+the NOT NULL constraint).

+

+### Go/no-go: GA

+

+**GO**, conditional on the operator completing the Stripe Dashboard

+checklist documented in `docs/internal/runbooks/stripe-billing.md`

+under "Subject resolution chain" and "Per-feature enforcement flags"

+sections. Production binary needs redeploy to pick up:

+

+- Migrations 0077 (`last_event_at`) + 0078 (`refunded` enum + column)

+- The 13 code fixes above

+

+Deploy steps:

+

+1. `ssh root@shithub.sh "sudo -u shithub /usr/local/bin/shithubd migrate up"` — applies 0077 + 0078

+2. Restart `shithubd-web.service` and `shithubd-worker.service`

+3. Verify with: `SELECT version_id FROM goose_db_version ORDER BY version_id DESC LIMIT 1` → expect `78`

+4. Configure Stripe env vars per the runbook (still in test mode until ratified)

+5. Run the smoke checklist in `runbooks/stripe-billing.md#smoke-test`

+6. Flip enforce flags per feature after 7-day report-only soak

+

+**No production customers exist today** (`SELECT plan, count(*) FROM users WHERE deleted_at IS NULL` → `free: 2`; same for orgs), so the deploy carries near-zero risk to existing customers — there are none on Pro/Team plans.