feat(i18n): migrate OIDC bind flow to ResponseErrorL (Part of #170)

[an9xyz]

Summary

Migrates the OIDC self-service bind flow (modules/oidc/api_bind.go) to the i18n error envelope and closes the raw-error leaks in the authorize protocol endpoint. Part of the i18n backend rollout (#170).

OIDC was outside the Phase 2 hotspot batch because it never used the c.ResponseError envelope — it emitted raw c.AbortWithStatusJSON(errMsg(...)) with a private {"msg": ...} shape and semantic HTTP status codes (400/401/409/410/422/429/503).

Closes #220.

Design decision — ResponseErrorLWithStatus (keeps real HTTP status)

The bind endpoints are a recent feature with no legacy clients depending on the fixed-400 compatibility behavior (D14). Pinning their 410/429/409/422/503 responses to 400 would be a regression (degraded semantics, broken status-based branching, extra churn at the Phase 4 switch).

This PR adds a second facade in pkg/httperr:

• ResponseErrorL (unchanged): wire status pinned to 400, real status in error.http_status (D14) — used by all legacy-bearing modules.
• ResponseErrorLWithStatus (new): wire status = the code's canonical HTTPStatus. Body envelope is byte-for-byte identical; only the transport status differs. The renderer already reads spec.TransportStatus, so there is zero renderer change.

This is the only consumer for now; it effectively brings the bind endpoints to their Phase 4 end state early. Flagged for maintainer review as it diverges from the current D14 fleet-wide consistency.

What changed

1. feat(i18n): ResponseErrorLWithStatus facade (+ shared respondL).
2. feat(i18n): 12 err.server.oidc.* codes + zh-CN translations (reuses shared rate.limited/internal/auth.required).
3. feat(i18n): migrate all 43 raw sites in api_bind.go via respondBindError.
4. fix(oidc): stop leaking err.Error() in authorize (5 sites → generic message + zap.Error).
5. test(i18n): source guard + i18n contract coverage.

Preserved contracts

• Anti-enumeration: ErrBindAuthRejected and the confirm-stage TOCTOU rejection all map to a single generic bind_invalid_credentials (401); the specific reason is logged via zap only. Test-locked.
• Audit + metrics: every writeAudit / bindResultFromErr side effect is kept; only the final response call changed.
• 5xx never leaks: 503/500 codes are Internal=true; clients identify the transient 503 via error.http_status.

Breaking-change assessment

HTTP status codes: zero drift. Per-status-bucket counts are identical before/after (400×12 / 401×3 / 409×6 / 410×5 / 422×1 / 429×4 / 500×5 / 503×1), and the existing 30 status-code assertions in api_bind_test.go all pass post-migration. Success responses (c.JSON(200)) are untouched. callback/logout are untouched.

Bind response body (incremental): {"msg": "..."} → {"msg": "<i18n>", "status": <real>, "error": {code, message, http_status, details}}. New status/error fields are additive (JSON-compatible); the msg value changes (short phrase → full localized sentence). Adds Content-Language + Vary headers (already in CORS ExposeHeaders).

⚠️ Front-end action item: the only possible breakage is a client that does an exact string match on the error msg to branch error handling. The correct approach is to read the HTTP status (unchanged) or error.code (new). Please confirm the bind wizard does not string-match msg.

Test plan

• go build ./..., go vet, gofmt (changed files clean)
• make i18n-extract-check (recall 100%), make i18n-lint
• go test ./modules/oidc -race (full suite, fresh test DB), pkg/httperr / pkg/errcode / pkg/i18n
• Front-end confirms bind wizard reads HTTP status / error.code (not msg string match)

[Jerry-Xin]

Scope check passed; this PR is relevant to octo-server and the OIDC/i18n backend rollout. I found no blocking correctness or security issues.

💬 Non-blocking

• 🔵 Suggestion — modules/oidc/api_i18n.go:26: codeSharedAuthRequired is initialized but unused. Removing it would avoid an unnecessary init-time dependency and keep this helper file tighter.
• 🟡 Warning — pkg/errcode/oidc.go:34-39 with pkg/i18n/renderer.go:68-71: ErrOIDCBindServiceUnavailable has a specific localized message and translation entries, but Internal: true means the renderer will always emit the shared internal-error message instead. That appears intentional for 5xx hygiene, but the code-specific translation is currently unreachable.

✅ Highlights

• pkg/httperr/respond.go:22-73 cleanly preserves the legacy ResponseErrorL behavior while adding a scoped semantic-status facade.
• modules/oidc/api_bind.go:61-617 consistently routes bind errors through respondBindError and preserves the existing status-code semantics.
• modules/oidc/api_bind_i18n_test.go:25-178 adds useful guards for raw response regressions, envelope shape, semantic status, anti-enumeration, and zh-CN localization.
• modules/oidc/api.go:317-368 closes the obvious err.Error() reflection/internal-error leaks in authorize.

Verification: go test ./pkg/httperr ./pkg/errcode ./pkg/i18n passed. go test ./modules/oidc -run 'TestOIDCBindNoLegacyResponseError|TestBindError_' passed. Full go test ./modules/oidc was blocked by local Redis/MySQL dependencies on 127.0.0.1:6379 and 127.0.0.1:3306, not by the changed tests.

[lml2468]

Summary

Clean, well-structured migration of the OIDC bind flow error responses from raw c.AbortWithStatusJSON(errMsg(...)) to the localized ResponseErrorLWithStatus envelope. No blocking issues.

What this PR does

1. pkg/httperr/respond.go — Adds ResponseErrorLWithStatus facade that keeps the code's canonical HTTP status on the wire (vs legacy fixed-400). Shared respondL implementation avoids duplication. Clean design.
2. pkg/errcode/oidc.go — 12 OIDC bind error codes. Anti-enumeration: single 401 bind_invalid_credentials for all auth failures. Internal=true only on 503 — correct.
3. modules/oidc/api_i18n.go — respondBindError + mustLookupSharedCode init-time resolution with panic guard. Correct: fail loudly at startup, not silently at request time.
4. modules/oidc/api_bind.go — ~30 call sites migrated. Every AbortWithStatusJSON replaced with respondBindError. respondBindError calls c.Abort() matching the prior AbortWithStatusJSON semantics — no control-flow regression.
5. modules/oidc/api.go — Bonus: closes err.Error() reflection leak in authorize endpoint. Generic messages + zap logging instead. Good security hygiene.
6. Tests — Comprehensive: code registration, Internal-flag invariant, anti-enumeration contract, source guard (no legacy patterns in api_bind.go), wire-status assertions, zh-CN translation check.
7. i18n — Both locale files (zh-CN TOML, en-US marker) updated. 12/12 codes covered.
8. Baseline — api_bind.go count correctly updated to 0 with EXEMPT annotation.

Findings

P2 (nit)

• modules/oidc/api_i18n.go:26 — codeSharedAuthRequired is declared but appears unused in this diff. If it's for a subsequent PR, a brief // used by api_bind_auth.go (next PR) comment would prevent dead-code confusion. If truly unused, consider removing.

Verdict

APPROVED. Architecture is clean, test coverage is thorough, anti-enumeration design is solid, no error-leak regressions.

[an9xyz]

Thanks for the reviews! Addressed both non-blocking findings in 6f7ac3e:

• codeSharedAuthRequired unused (@Jerry-Xin, @lml2468): removed. It was a leftover from an early plan to migrate the logout 401 — that endpoint stays a raw OAuth2/OIDC protocol response (not migrated to the envelope), so the shared code was dead. No follow-up PR depends on it.
• ErrOIDCBindServiceUnavailable unreachable translation (@Jerry-Xin): confirmed intentional (5xx hygiene — Internal=true so the renderer emits the shared internal copy). Added a NOTE on the code documenting that its specific message + zh-CN translation are unreachable on the wire by design; clients distinguish the transient 503 via error.http_status. Kept the text to satisfy Register and document intent rather than leaving a bare placeholder. (If we later decide a safe static 503 message should reach clients, that's a small SDK follow-up to allow non-Internal safe 5xx copy — out of scope here.)

[yujiawei]

Re-review of new commit 6f7ac3e (chore: address review nits) — APPROVED

verdict: approve
head-sha: 6f7ac3e
repo: Mininglamp-OSS/octo-server

This re-review covers only the delta since the prior approval at ced0869 (this commit is +8/-3 across 2 files):

• modules/oidc/api_i18n.go — drops the now-unused codeSharedAuthRequired var. Verified genuinely dead: zero references remain in modules/oidc; the shared code err.shared.auth.required stays registered in pkg/i18n/codes/shared.go and is still used by modules/user|message|group. The two retained vars (codeSharedRateLimited, codeSharedInternal) are both still referenced in api_bind.go.
• pkg/errcode/oidc.go — doc-only comment on ErrOIDCBindServiceUnavailable explaining the Internal=true 5xx-hygiene contract (specific message + zh-CN intentionally unreachable on the wire; clients distinguish the 503 via error.http_status). No behavior change; the code is still registered.

Local verification: go build + go vet clean on modules/oidc and pkg/errcode. Targeted tests pass: TestBindError_*, TestOIDCBindNoLegacyResponseError, TestAPI_BindCreate_NilBind_503, TestAPI_BindHandlers_NilBindReturnsServiceUnavailable, TestOIDCCodesRegistered/InternalFlag/InvalidCredentialsGeneric, and all of pkg/i18n/.... (The only failure is the pre-existing TestRedisBindStore_Behavior_Integration, which requires a live MySQL — unrelated to this change.) CI is green across Build/Lint/Vet/Test/i18n/CodeQL.

Anti-enumeration (generic 401) and the Internal-flag invariant from the prior review still hold. No P0/P1.

[yujiawei]

Code Review — PR #223 (octo-server)

Verdict: APPROVED — re-review of head 6f7ac3e. No blocking issues.

Migrates the OIDC self-service bind flow (modules/oidc/api_bind.go) to the i18n error envelope and closes the raw-error leaks in the authorize protocol endpoint.

What looks good

• ResponseErrorLWithStatus design is sound. The body envelope is byte-for-byte identical to ResponseErrorL; only the transport status differs (real HTTPStatus vs legacy compatibility 400). Shared respondL impl keeps the two facades in lockstep — no renderer change needed. Scoping it to the bind flow (a recent feature with no fixed-400 clients) is the right call, and the divergence from the D14 fleet behavior is clearly documented.
• Security is handled carefully. Anti-enumeration is enforced by collapsing ErrBindAuthRejected + the confirm-stage TOCTOU rejection into a single generic ErrOIDCBindInvalidCredentials (401), with the specific reason logged via zap only. oidc_test.go guards this with a message-leak assertion.
• 5xx hygiene. Internal=true is asserted to be present on (and only on) 5xx codes; the 503 message/translation are intentionally unreachable and documented as such. The authorize endpoint now logs internal errors and returns generic copy instead of echoing err.Error() — good defense-in-depth against reflecting client input.
• Test coverage is thorough. Source-guard test (TestOIDCBindNoLegacyResponseError) prevents regressions back to raw c.AbortWithStatusJSON, plus real-renderer i18n contract tests for status preservation (410), validation (400), anti-enumeration (401), and zh-CN translation. errcode registration + Internal-flag invariants are covered.
• i18n bookkeeping is complete. en-US and zh-CN entries added for all 12 codes; the lint baseline correctly drops api_bind.go to 0 and documents the api.go EXEMPT rationale.

Non-blocking notes

• api.go (authorize/callback/logout) intentionally stays on raw errMsg() as browser-facing protocol endpoints — consistent with the documented EXEMPT note and out of scope here.

CI is fully green (Build / Vet / Lint / Test / CodeQL / i18n checks all pass).

[Jerry-Xin]

✅ Re-Review: APPROVE

Delta since ced0869 → 6f7ac3e:

The new commits harden authorize() — all five err.Error() reflections are replaced with generic errMsg("internal error") + structured zap.Error(err) logging. A defense-in-depth comment on ValidateReturnTo explicitly calls out the echo-back risk.

Bind-flow migration (the bulk of the PR) is unchanged from the previous review.

No blocking issues. Previous non-blocking observations still apply:

• 🟡 codeSharedAuthRequired in api_i18n.go is declared but unused in this diff
• 🟡 ErrOIDCBindServiceUnavailable has Internal: true, making its localized message unreachable (acceptable for 5xx hygiene)

Clean delta, approve stands.