Operator runs 'shithubd storage repair-shared-perms' as root over
SSH; the bare repos are owned by shithub:shithub. git 2.35+'s
dubious-ownership protection early-exits with the misleading
'fatal: not in a git directory' before it reads core.sharedRepository.
Same env trick the SSH dispatcher uses (cmd/shithubd/ssh_dispatch
injects GIT_CONFIG_COUNT=1 + safe.directory=* for the cross-user
git-receive-pack/upload-pack invocations). The path is verified
contained-in-root above the env injection, so '*' here is safe by
construction — every iteration of this loop targets a path we
already validated.
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).
