Pre-fix: buildSSHEnv built a minimal explicit env list with just
SHITHUB_USER_ID/USERNAME/REPO_ID/REPO_FULL_NAME/PROTOCOL/REMOTE_IP/
REQUEST_ID/PATH + the safe.directory GIT_CONFIG_* triple.
When git-receive-pack forked the pre-receive hook ('shithubd hook
pre-receive', via the shim in internal/git/hooks/install.go), the
hook called config.Load(nil) and exited with 'DB URL not set'
because SHITHUB_DATABASE_URL wasn't in the env. User saw:
remote: shithub: server error: DB URL not set
remote: shithubd: DB URL not set
! [remote rejected] trunk -> trunk (pre-receive hook declined)
The HTTPS-git path didn't hit this because shithubd-web's systemd
unit sources /etc/shithub/web.env via EnvironmentFile= and
receive-pack inherits the env directly.
Fix: base the SSH env on os.Environ() so the SHITHUB_* config keys
the git-shell-commands wrapper sourced from web.env propagate
through receive-pack into the hook. Push-event metadata
(SHITHUB_USER_ID/REPO_ID/...) is appended after the inheritance
so any stale value in the parent env is shadowed by the dispatcher's
real value (last-wins on duplicate env keys).
PATH no longer needs an explicit copy; it's part of os.Environ().
Tests:
- TestBuildSSHEnv_InheritsParentEnv pins SHITHUB_DATABASE_URL +
SHITHUB_STORAGE__REPOS_ROOT propagation.
- TestBuildSSHEnv_ExplicitOverridesInheritance pins last-wins
semantics on the SHITHUB_REQUEST_ID collision case.
Both run without a DB so they fire on every CI invocation, not
just the dbtest-equipped ones.
scripts/lint-unused.sh fails the build when any non-test Go file
under internal/ or cmd/ carries 'var _ = symbol' (the dead-code
shim shape). Allows the legitimate 'var _ Type = (*X)(nil)'
interface-assertion pattern by anchoring the regex to '= [A-Za-z]'
without a type name in between.
Wired into:
- Makefile 'lint-unused' target + the 'ci' alias
- .github/workflows/ci.yml as a dedicated step (the rest of the
bash lints are still local-only; lint-unused gets first-class CI
treatment because the audit caught regrowth twice in 3 days).
Pre-fix: 8 sites carried 'var _ = symbol' shims with comments like
'silence unused-import warnings during refactors'. These were lying:
in every case the import was already used elsewhere in the file or
the symbol was unreachable. Audit 2026-05-08 flagged 3 occurrences;
audit 2026-05-10 found 8 — the pattern grew because nothing failed
CI on it.
Sites cleaned:
- internal/auth/totp/recovery.go (base32.StdEncoding)
- internal/web/middleware/pat.go (pgx.ErrNoRows)
- internal/web/handlers/orgs/teams.go (pgx.ErrNoRows + errors.New)
- internal/web/handlers/profile/profile.go (context.Background)
- internal/web/handlers/api/api.go (context.Background)
- internal/web/handlers/repo/code.go (pgtype.Int8{})
- internal/web/handlers/repo/redirect.go (usersdb.New)
Build still passes after each shim drop because the imports are
genuinely live; the shims were cargo-cult.
Pre-fix: storage.RepoFS.InitBare ran 'git init --bare' without
--shared=group, so objects/ wound up 0755 with no group-write.
shithubd-web (runs as 'shithub' user) created repos; SSH-git
dispatched git-receive-pack as the 'git' user (in the 'shithub'
group). 'git' had read-execute on objects/ but not write, so push
failed with 'unable to create temporary object directory'.
git-upload-pack worked because read was sufficient.
Fix at the source:
- InitBare now runs 'git init --bare --shared=group --initial-branch=trunk'.
Persists core.sharedRepository=group in config; produces 2775
dirs (group write + setgid) and 0664 files. Parent dir gets 2750
so the setgid propagates from byte zero.
- CloneBareShared (fork path) prepends '-c core.sharedRepository=group'
so the cloned repo carries the contract. NB: 'git clone --shared'
alone is the alternates flag, NOT the perms flag — same word, two
meanings.
- RepairSharedPerms backfills existing repos: sets the config flag,
walks the tree, chmods g+w on files and g+w+s on dirs. Idempotent.
- 'shithubd storage repair-shared-perms' subcommand walks every
<prefix>/<owner>/<name>.git under storage.repos_root and applies
the repair. One-time use after deploying this binary on shithub-
prod (the live droplet has 1 repo created pre-fix that needs it).
Tests:
- TestInitBare_SharedGroupContract: asserts core.sharedRepository
config value + group-write bit on objects/.
- TestRepairSharedPerms_FixesPreFixRepo: builds a deliberately
pre-fix repo, calls Repair, asserts post-conditions match the
contract InitBare produces from byte zero.
Closes nothing yet — operator runs the new subcommand on the
droplet after deploy. Audit script (deploy/audit/check-droplet-
drift.sh) will pick up the binary swap and reflect drift if not.
H7: threadAction now loads the issue by id, asserts kind matches
the URL kind (issue vs pr), then runs policy.IsVisibleTo on the
parent repo before upserting notification_threads. Pre-fix any
logged-in user could pollute the table with rows for private or
non-existent issues.
L4: dropped the raw `r.Header.Get("Referer")` redirect; route
through notificationReturnPath which already validates the
return_to form value via safeNotificationReturnPath.
L5: kind check moved before VerifyUnsubscribe so unknown kinds
fast-fail without a constant-time HMAC compare.
L7: 5 silent `_ = h.d.Render.RenderPage(...)` callsites in
orgs/orgs.go and orgs/teams.go now log on render error so a
broken template surfaces instead of returning a blank 200.
H8: pre-fix repoForceArchive flipped is_archived as a toggle —
clicking on an already-archived repo silently un-archived it with
audit row ActionAdminRepoForceArchived (the name lied on the
unarchive path). Now two routes:
- POST /admin/repos/{id}/archive → repoForceArchive (idempotent)
- POST /admin/repos/{id}/unarchive → repoForceUnarchive
Each emits its own audit action (ActionAdminRepoForceArchived /
ActionAdminRepoForceUnarchived). repo_view.html branches on
IsArchived to render the correct button.
M2: inline raw SQL in admin handlers replaced with sqlc-generated
queries:
- admin/repos.repoForceArchive → reposdb.ArchiveRepo
- admin/repos.repoForceUnarchive → reposdb.UnarchiveRepo (existed)
- admin/repos.repoForceDelete → reposdb.AdminForceDeleteRepo (new)
- admin/users.userUnsuspend → usersdb.UnsuspendUser (new)
- orgs/teams.canSeeTeam → orgsdb.IsTeamMember (new)
- orgs/teams.filterSecretTeams → orgsdb.IsTeamMember (same query)
M3: drop the wasted ListTeamMembers call in canSeeTeam — the
result was thrown into _ before the actual EXISTS check ran.
M6 (partial): admin/repos.go switches http.Error → renderer for
all error paths in the touched handlers.
Strict health-probe tooling (Kubernetes-style HEAD-only liveness,
some monitoring services) issues HEAD requests. chi only registers
the verbs you ask for, so the GET-only registration returned 405
on HEAD. DO uptime tolerated this; not every consumer will.
r.Head registers a separate handler entry; same handler function so
behavior matches GET. Regression test added.
Pre-fix: handlers.go:172 hardcoded CSRFConfig{Secure: false} with
comment 'S37 enables under TLS'. The session cookie correctly
honored cfg.Secure via server.go's session-store wiring; the CSRF
cookie did not. Caddy 301-redirects http→https in practice, but
the cookie was still settable/readable over plaintext if anyone
crafted an http:// link.
Post-fix:
- handlers.Deps gains CookieSecure: bool.
- server.go threads cfg.Session.Secure (already true on prod TLS
deployment per inventory). Tests and dev keep the false default.
- CSRFConfig.Secure now mirrors deps.CookieSecure.
Pre-fix: webhook.Create / Update called only validateURL (scheme
check) and ssrf.Validate (scheme + port). Loopback / RFC1918 /
CGNAT / multicast hosts were rejected only at delivery time inside
dialContext — admin could persist a hook with http://localhost or
http://192.168.1.1 and only see failures on the deliveries page
after the first attempt. Disallowed ports were caught (port 9090
fails Validate); IPs were not.
Post-fix:
- ssrf.ValidateWithResolve runs Validate plus a DNS lookup +
IsForbiddenIP check on every resolved IP. IP literals are
matched directly without DNS. AllowedHosts and
AllowPrivateNetworks behave the same as in dialContext.
- webhook.Create + Update call ValidateWithResolve. The plain
Validate is left in place as the cheap syntactic gate.
- dialContext keeps re-resolving as defense in depth (DNS
rebinding) — the validate-resolve check is *not* a substitute.
ssrf_create_test pins the table directly: 127.0.0.1, [::1],
192.168.1.1, 10.0.0.1, 172.16.0.1, port 9090 — all rejected.
A public host on a default port still passes.
Pre-fix: webhook create/update overloaded ActionRepoCreated with
a meta.action discriminator. Webhook delete/toggle/ping/redeliver
were not audited at all — admin had no forensic record of who
disabled, deleted, or replayed a hook.
Post-fix:
- New audit actions: ActionWebhook{Created,Updated,Deleted,
ActiveSet,ActiveUnset,Pinged,Redelivered}.
- All 7 webhook handlers now audit, all attributed via
viewer.AuditActor so impersonation trails carry.
- Bonus: ActionAdminRepoForceUnarchived added in preparation for
SR2 H8 (split repoForceArchive into archive/unarchive).
Pre-fix: S30/S31 wired org/team handlers to call orgs.IsOwner
directly without going through policy.Can. A suspended user listed
as an org owner could still invite, change roles, remove members,
accept invitations, create teams, add team members, and grant
team→repo access — bypassing the suspension gate every other
mutation surface enforces. Same shape as SR1 C1, new surface.
Lighter fix per SR2 sprint plan: short-circuit on viewer.IsSuspended
at every point of entry. Heavier fix (real policy.Can actions for
ActionOrgInvite/ActionTeamMemberAdd/...) is correct architecturally
but is a larger refactor; left as forward-link.
Gated:
- requireOrgOwner — covers teamCreate, teamMemberAddRemove,
teamRepoGrant via the existing helper.
- invite — direct IsOwner caller, gated inline.
- memberMutate — direct IsOwner caller, gated inline (covers
changeRole + removeMember).
- invitationAction — accept/decline now denies suspended users.
Read-only IsOwner callers (teamsList, teamView, canSeeTeam,
filterSecretTeams) intentionally NOT gated — suspension blocks
writes, not reads (consistent with SR1).
Pre-fix: userResetPassword minted a token, persisted password_resets,
audited ActionAdminUserPasswordReset, then `_ = tokEnc` discarded
the token. admin.Deps had no email.Sender at all, so the comment
'surfaced via the email path' was false from the start. The audit
row claimed 'sent'; the user never received anything.
Post-fix:
- admin.Deps adds Email + Branding fields, mirroring auth.Deps.
- admin_wiring.buildAdminHandlers takes the same email.Sender
the auth surface uses (server.go threads pickEmailSender(cfg)
through both paths).
- userResetPassword calls email.ResetMessage + sender.Send. The
audit row now carries email_sent: bool plus email_error: string
on failure, so a stuck mailbox is visible in /admin/audit instead
of silently misleading.
- L6: server.go now passes version.Version into buildAdminHandlers
instead of the literal 'dev', so /admin/system reflects the
ld-flag-stamped build version.
admin_deps_test pins the field contract — removing Email or
Branding breaks compile.
Substitutes (viewer.ID, raw_meta) with viewer.AuditActor(raw_meta)
at every non-admin Audit.Record callsite in repo/settings_*.go and
repo/webhooks.go. During an impersonated session the row is now
attributed to the real admin and the impersonated user_id lands in
meta — uniform with the existing /admin/* trail.
admin/helpers.recordAdminAction also moves to AuditActor for one
canonical substitution point.
18 handler sites switched from policy.UserActor(viewer.ID,
viewer.Username, viewer.IsSuspended, false) to viewer.PolicyActor().
Includes profile, search, and repo (issues/PRs/lifecycle/social/
fork/labels/repo home) surfaces.
Pre-migration these all hardcoded IsSiteAdmin=false and ignored
viewer.ImpersonatedUserID. Post-migration impersonation is a
construct-time concern and admin-read-private gets a 200 instead
of 404.
PAT-bearing API handlers (api/checks.go, api/stars.go) and the
SSH/HTTPS-git paths keep plain UserActor() — those gates reject
suspended at credential check, and impersonation is impossible via
non-cookie auth, so the false literals are correct by construction.
Adds the canonical web-layer Actor constructor that propagates
IsSuspended, IsSiteAdmin, Impersonating, ImpersonateWriteOK from
the request-bound CurrentUser. Plain UserActor() left intact for
SSH/HTTP-git/worker callers that don't have a CurrentUser handy.
CurrentUser gains PolicyActor() and AuditActor(meta) helpers so
non-admin handlers can build actors and record audit rows with
uniform impersonation handling (SR2 H2).
actor_test pins:
- IsSuspended propagates (regression for SR1 C1)
- IsSiteAdmin propagates (regression for SR2 C2)
- Impersonating fires when ImpersonatedUserID != 0 (SR2 C1)
- ImpersonateWriteOK is honored (read-only-by-default guarantee)
- IsSiteAdmin stays false on the impersonated identity
