Skip to content

Commit 5ad69ed

Browse files
sarg3ntclaude
andcommitted
fix(#83): address Copilot review on #84
- Lint: golangci-lint's `unused` check runs without the `dev` build tag, so the shared constants in dev_bypass.go were flagged. Delete the shared file and inline the two constants into dev_bypass_on.go where they are actually referenced. - Doc accuracy (Copilot #1, #2): the `-tags dev` flag is set by the `dev:` target in gearbox/Makefile via `air --build.cmd "$(DEV_BUILD_CMD)"`, not by `.air.toml` (which is gitignored per-developer). Update the package doc and four function/header comments in dev_bypass_on.go, and the corresponding section in gearbox/docs/development.md, to reference the Makefile and drop the broken `../.air.toml` link. - Security (Copilot #3): EnsureDevUserExists previously returned early if a `dev` row already existed, leaving its password_hash untouched. A developer who manually set a password on that row could then form- login as a `dev` admin, contradicting the "form-login can never authenticate as this user" claim. Always rewrite password_hash to the caller-supplied dummyPasswordHash, plus reset status and must_change_password back to the safe defaults, on every call. Other fields (role, names) are still preserved across runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent d407887 commit 5ad69ed

4 files changed

Lines changed: 62 additions & 36 deletions

File tree

gearbox/docs/development.md

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -125,10 +125,13 @@ See [VS Code Integration](#vs-code-integration) below for debugger support.
125125

126126
## Dev-Only Loopback Auto-Login
127127

128-
`make dev` builds with `-tags dev` (see [.air.toml](../.air.toml)), which
129-
compiles in a localhost-only auto-login bypass. When all three of these
130-
conditions hold, the request is auto-authenticated as the seeded `dev`
131-
user — no login screen, no cookie management:
128+
`make dev` builds with `-tags dev` — the flag is set by the `dev:` target
129+
in [gearbox/Makefile](../Makefile), which overrides air's build command
130+
via `air --build.cmd "$(DEV_BUILD_CMD)"`. (`.air.toml` is per-developer
131+
and gitignored, so the build flag intentionally lives in the Makefile.)
132+
The tag compiles in a localhost-only auto-login bypass. When all three
133+
of these conditions hold, the request is auto-authenticated as the
134+
seeded `dev` user — no login screen, no cookie management:
132135

133136
1. The binary was built with `-tags dev`.
134137
2. The environment variable `GEARBOX_DEV_AUTO_LOGIN=1` is set.

gearbox/internal/framework/auth/dev_bypass.go

Lines changed: 0 additions & 17 deletions
This file was deleted.

gearbox/internal/framework/auth/dev_bypass_on.go

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
//go:build dev
22

33
// Dev-only loopback auto-login bypass. Compiled in only when the binary
4-
// is built with `-tags dev` (see gearbox/.air.toml and the production
5-
// build path which deliberately omits the tag). See issue #83.
4+
// is built with `-tags dev`. The tag is set by the `dev:` target in
5+
// gearbox/Makefile via `air --build.cmd "$(DEV_BUILD_CMD)"`; the
6+
// production build paths (`make build`, `make deploy-build`) deliberately
7+
// omit it, so this file and its symbols are not in release binaries at
8+
// all. See issue #83.
69

710
package auth
811

@@ -17,12 +20,26 @@ import (
1720
"github.com/sarg3nt/gearbox/internal/framework/models"
1821
)
1922

23+
const (
24+
// devBypassEnvVar gates whether the bypass is allowed to fire even
25+
// when the binary was built with `-tags dev`. Set to "1" to enable.
26+
devBypassEnvVar = "GEARBOX_DEV_AUTO_LOGIN"
27+
28+
// devBypassEmail is the email/username of the seeded dev account that
29+
// the bypass auto-authenticates as. The account must exist (and be
30+
// active) in the users table; the bypass never creates sessions or
31+
// auto-promotes a non-existent user.
32+
devBypassEmail = "dev"
33+
)
34+
2035
var devBypassBannerOnce sync.Once
2136

2237
// tryDevBypass returns the seeded `dev` user when ALL of these hold:
2338
//
24-
// 1. The binary was built with `-tags dev` (this file is compiled in;
25-
// the production sibling dev_bypass_off.go is not).
39+
// 1. The binary was built with `-tags dev` (gearbox/Makefile's `dev:`
40+
// target adds the tag via `air --build.cmd "$(DEV_BUILD_CMD)"`; this
41+
// file is compiled in. The production sibling dev_bypass_off.go is
42+
// compiled in for tag-less builds and provides a no-op stub.).
2643
// 2. GEARBOX_DEV_AUTO_LOGIN=1 is set in the process environment.
2744
// 3. r.RemoteAddr is a loopback address (127.0.0.0/8 or ::1).
2845
//
@@ -92,7 +109,9 @@ func SeedDevUserIfEnabled(db *database.DB, logger *slog.Logger) error {
92109
// Designed to be impossible to miss in logs so an operator never confuses
93110
// a dev binary for a production one.
94111
//
95-
// Production builds replace this with a no-op (dev_bypass_off.go).
112+
// Production builds replace this with a no-op (dev_bypass_off.go) — the
113+
// production build targets in gearbox/Makefile (`build`, `deploy-build`)
114+
// omit `-tags dev`, so this banner cannot fire on a release artifact.
96115
func LogDevBypassStartupBanner(logger *slog.Logger) {
97116
if os.Getenv(devBypassEnvVar) != "1" {
98117
logger.Info("dev auto-login: bypass compiled in (`-tags dev`) but disabled — set GEARBOX_DEV_AUTO_LOGIN=1 to enable")

gearbox/internal/framework/database/users_dev.go

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,24 +14,45 @@ import (
1414
"github.com/sarg3nt/gearbox/internal/framework/models"
1515
)
1616

17-
// EnsureDevUserExists creates the seeded `dev` user used by the dev
18-
// auto-login bypass if it doesn't already exist. The password hash is
19-
// expected to be the package-level dummyPasswordHash from auth, so the
20-
// form-login path can never authenticate as this user — only the
21-
// loopback bypass can.
17+
// EnsureDevUserExists ensures the seeded `dev` user exists for the dev
18+
// auto-login bypass AND that its password hash is the caller-supplied
19+
// `passwordHash` (the package-level dummyPasswordHash from auth). The
20+
// hash is rewritten unconditionally on every call so a row left over
21+
// from a prior manual setup — where a developer may have set a real
22+
// bcrypt-able password for testing — cannot be authenticated via the
23+
// form-login path. Only the loopback bypass can authenticate as `dev`.
2224
//
23-
// Returns (created, error) — `created` is true only on the first call
24-
// that actually inserted a row.
25+
// Returns (created, error) — `created` is true only on the call that
26+
// inserted the row; subsequent calls return false but still rewrite the
27+
// hash, status, and must_change_password fields back to the safe
28+
// defaults. Other fields (role, names) are not overwritten so a
29+
// developer can still adjust the dev user's display info without losing
30+
// it on next startup.
2531
func (d *DB) EnsureDevUserExists(email, passwordHash string) (bool, error) {
2632
d.mu.Lock()
2733
defer d.mu.Unlock()
2834

2935
var existingID string
3036
err := d.db.QueryRow(`SELECT id FROM users WHERE email = ? LIMIT 1`, email).Scan(&existingID)
31-
if err == nil {
37+
switch {
38+
case err == nil:
39+
// Row exists. Force the hash + status + must_change_password back to
40+
// the safe defaults so the form-login path remains unable to
41+
// authenticate as this user even if the row was tampered with.
42+
_, err := d.db.Exec(`
43+
UPDATE users
44+
SET password_hash = ?,
45+
status = ?,
46+
must_change_password = 0,
47+
updated_at = ?
48+
WHERE id = ?`,
49+
passwordHash, models.UserStatusActive, time.Now(), existingID,
50+
)
51+
if err != nil {
52+
return false, fmt.Errorf("failed to reset dev user safety fields: %w", err)
53+
}
3254
return false, nil
33-
}
34-
if err != sql.ErrNoRows {
55+
case err != sql.ErrNoRows:
3556
return false, err
3657
}
3758

0 commit comments

Comments
 (0)