cubeops: propagate database errors from the settings and user getters

[dwin-gharibi]

Closes #1381.

Motivation

GetSystemSetting, GetSetting and GetUserPassword collapsed three different outcomes — row
missing, value empty, and any database error — into ("", nil). Because the scanned value is the
zero value whenever the query fails, val == "" was true for a real error too, so the error was
discarded.

The visible symptom is that a database outage is reported as 401 invalid credentials rather than a
500, which is the opposite of what AuthService.Login's own comment promises. It also made both
branches of if err != nil in Login unreachable dead code.

What this changes

CubeOps/internal/store/setting.go — all three getters now separate the two cases:

if err != nil {
    if errors.Is(err, sql.ErrNoRows) {
        return "", nil
    }
    return "", err
}
return val, nil

The resulting contract is: ("", nil) means absent; a non-nil error means the read failed. An
empty stored value is still reported as absent, which is what every caller already wanted.

CubeOps/internal/service/auth.go — Login and ChangePassword updated for that contract:

• Login returns 500 for a read failure and ErrInvalidCredentials when the user is absent
  (stored == "") or the password does not match. User enumeration is still not possible: absent
  and wrong-password are indistinguishable to the caller.
• ChangePassword likewise returns the wrapped error for a read failure and
  ErrInvalidOldPassword when the user is absent or the old password is wrong.

CubeOps/internal/service/auth_test.go — the fakeUserStore returned
("", errors.New("user not found")) for an unknown user, modelling not-found as an error, while
the real store returns ("", nil). That mismatch is why the tests passed against a contract
production never implemented. The fake now returns ("", nil); its assertion (unknown user →
ErrInvalidCredentials) is unchanged and still passes.

Callers that already discard the error (internal/service/openclaw.go:685-701,
internal/service/agenthub.go:457) are untouched — they still get "" and still ignore the error, so
their behaviour is unchanged.

bootstrapMasterKey (internal/store/db.go:83,93) and BootstrapJWTSecret (:139) already had
if err != nil guards that were previously dead; they now actually fire, which is the intended
fail-closed behaviour.

Testing

New: CubeOps/internal/service/auth_db_error_test.go

• TestLoginSurfacesInfrastructureError — a DB error is wrapped and returned, and is not
  ErrInvalidCredentials.
• TestLoginUnknownUserStillReportsInvalidCredentials — no enumeration regression.
• TestChangePasswordSurfacesInfrastructureError — same for the password-change path.
• TestChangePasswordUnknownUserReportsBadOldPassword — absent user still reports a bad old
  password.

$ cd CubeOps && go test ./...
ok  .../internal/auth        ok  .../internal/config     ok  .../internal/crypto
ok  .../internal/cubemaster  ok  .../internal/handler    ok  .../internal/httputil
ok  .../internal/logging     ok  .../internal/redact     ok  .../internal/server
ok  .../internal/service     ok  .../internal/store
12 packages, 0 failures

CI gates checked locally:

• gofmt -l ./internal ./cmd — clean (fmt-check).
• go build ./... — clean.
• go test ./... — 0 failures (unit-test-check → make cubeops-test).

Risk / rollout

This is a behaviour change on the login path and is the reason it is split out from #1: during a
database outage, /api/v1/auth/login now returns 500 instead of 401. Any monitoring or client
retry logic keyed on 401 for that case will see 500 instead — which is the correct signal, but
worth calling out.

The UserStore contract change is internal to CubeOps (internal/), so there is no external API
impact.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[cubesandboxbot]

This sql.ErrNoRows branch can't fire with the current GORM setup, so the "absent" case is actually detected by the fall-through below.

db.Raw(...).Scan(&val) goes through GORM's Scan (not *sql.Row), and GORM returns a nil error when the result set is empty — with the default &gorm.Config{} (no TranslateError) used in CubeDB/dao/driver/mysql/mysql.go:61, it never surfaces sql.ErrNoRows. That's why the codebase's other getters that genuinely need to detect a missing row use .Row().Scan(...) instead (refresh_token.go:33-35, agenthub.go:229-231). The old code's val == "" clause was the effective check, and here the err == nil path returning the zero value (return val, nil) is what keeps "absent → ("", nil)" working.

This is harmless (behavior is correct) and even a forward-compatible safety net if GORM ever changes, so no change is strictly required — but the PR description presents this branch as the mechanism that separates "absent" from "error", which isn't what happens today. A one-line comment noting the empty-value fall-through is the real path would prevent a future reader from "fixing" it. Same applies to the identical branches in GetSetting and GetUserPassword.

[cubesandboxbot]

This test is weaker than its Login sibling in two ways:

1. It never asserts the underlying error is preserved. TestLoginSurfacesInfrastructureError checks errors.Is(err, dbDown), but here only err != nil && !errors.Is(err, ErrInvalidOldPassword) is asserted. A regression to a different non-sentinel error that discards the cause (e.g. errors.New("read failed")) would still pass. Consider adding if !errors.Is(err, dbDown) { t.Fatalf(...) } for parity.

2. errors.New("context canceled") is a misleading stand-in for the "database unreachable" case — a context cancellation is a different failure mode and could mask a wrong reason. The Login test's "dial tcp ...: connection refused" is a better fit.

[cubesandboxbot]

Review: cubeops: propagate database errors from the settings and user getters (#1382)

Automated review — AI-generated, not a human approval.

Verdict

The change is sound and fixes a real bug. The three store getters now correctly separate "row absent" (("", nil)) from "read failed" (("", err)), and the auth service/handler layers were updated to match. The new tests are well-targeted, and fixing fakeUserStore to return ("", nil) for a missing user aligns the test double with what the real store always did. I found one incomplete-leak gap (the Refresh handler) and one comment nitpick in db.go; no correctness bugs in the core change.

What I verified

• Store contract (CubeOps/internal/store/setting.go). The old GORM .Scan(&val) left the scanned value at its zero value on any failure, so val == "" masked every real error (table-not-found, connection drop, …) as an absent row. Replacing it with Row().Scan(&sql.NullString) is correct: database/sql returns exactly sql.ErrNoRows for an empty result set and the driver error otherwise, and errors.Is separates the two. A stored NULL/empty value scans to String="" with no error, preserving the "empty value is reported as absent" contract every caller already relied on. Context propagation through GORM's WithContext → Row() works, so the canceled-context contract test will see the driver/context error rather than hang.
• Service (CubeOps/internal/service/auth.go). stored == "" || short-circuits the bcrypt check for absent users and maps them to ErrInvalidCredentials/ErrInvalidOldPassword; any non-nil error is wrapped and returned. The previously dead if stored == "" branch inside if err != nil is gone. Both fakes (service and handler packages) now model not-found as ("", nil), which is the contract production actually implements.
• Other callers are unaffected. openclaw.go:685-701 and agenthub.go:457 discard the error and already treated "" as "use default"; handler/agenthub.go:335 logs the (now-visible) read failure and falls back to defaults — strictly better visibility, same output. bootstrapMasterKey/BootstrapJWTSecret guards now genuinely fire, giving the intended fail-closed startup.
• Tests. The service- and handler-level tests are hermetic and always run; the store contract tests are Docker-gated like the existing store integration tests (skip locally without Docker, Fatal under CI). outageUserStore embeds the existing auth_test.fakeUserStore and reuses doRequest, so the new handler tests compile against the base tree.

Findings

1. Medium — Refresh still leaks DB error details to the caller (incomplete leak fix).
   The PR stops Login and ChangePassword from echoing the raw error and adds tests asserting no DB details reach the caller. But Refresh — also a public route (RegisterPublic) — still writes err.Error() on its 500 path (CubeOps/internal/auth/handler.go:140). svc.Refresh wraps errors from IsRefreshTokenRevoked / CreateRefreshToken, both of which hit the DB, so during an outage the response body exposes driver details (host:port) to any caller holding a valid refresh token. Not a regression (pre-existing), but it is the same leak class this PR is eliminating; the same generic message plus a matching test would close it out. (Also posted inline.)

2. Low — misleading comment on the bootstrapMasterKey fallback window (CubeOps/internal/store/db.go).
   The new comment says the fallback covers "the upgrade window where the migration has run and the key was copied to t_system_setting." If the key were already there, step 1 would have found it and the fallback would never be reached. The window that actually needs it is "the migration has run but the key has not yet been copied to t_system_setting" (plus the old-table/rollback case). Worth rewording for future maintainers. (Also posted inline.)

Minor notes

• Timing side-channel (pre-existing, unchanged). The PR's claim that "user enumeration is still not possible" holds for the response status (absent and wrong-password both → 401). A response-time difference between the two paths still exists (bcrypt vs. no bcrypt), but this predates the change and the new short-circuit does not meaningfully widen it.

Intentional behavior change (called out in the PR)

• /api/v1/auth/login now returns 500 with a generic body instead of 401 during a DB outage; /api/v1/auth/change-password likewise. Existing handler tests do not assert on 500 bodies, so no test breakage; any client/monitor keyed on 401 for the outage case will see 500, which is the correct signal.

[cubesandboxbot]

With the store now propagating DB errors, this branch is newly reachable — and internal/auth/handler.go:66-67 writes the wrapped error verbatim into a 500 body (httputil.WriteError(c, http.StatusInternalServerError, err.Error())). During a DB outage an unauthenticated caller now receives {"error":"failed to read user: dial tcp <db-host>:3306: connect: connection refused"}, exposing the internal DB endpoint and driver details. The detail is already logged server-side on the same handler line, so the 500 response should return a generic "internal server error" message instead of echoing err.Error(). The same applies to the ChangePassword 500 path (handler.go:119-123). Since this PR is what makes these paths live, it should close the leak rather than land it as-is.

[cubesandboxbot]

Same leak class exists in Refresh below (line 140): this public route (RegisterPublic) still writes err.Error() on its 500 path. svc.Refresh wraps errors from IsRefreshTokenRevoked / CreateRefreshToken, both of which hit the DB, so during an outage the response body exposes driver details (e.g. dial tcp 10.0.0.5:3306: connection refused) to any caller holding a valid refresh token. Not a regression (it predates this PR), but it's the exact leak these two changes eliminate — consider applying the same generic message and covering it with a test like TestLoginOutageDoesNotLeakDatabaseDetailsToTheCaller.

[cubesandboxbot]

Minor: this sentence is self-contradictory — if the key were already copied to t_system_setting, step 1 (GetSystemSetting) would have found it and this fallback would never be reached. The window this fallback actually covers is "the migration has run but the key has not yet been copied to t_system_setting" (plus the old-table/rollback case). Suggest rewording so future maintainers don't misread when this path fires.