fix(oauth): return stable error codes - #4825
Conversation
- Return i18n-key codes for OAuth business errors.\n- Stop matching localized Chinese text in the default OAuth callback.\n- Preserve existing OAuth login and binding behavior.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThis PR systematizes OAuth error handling across provider controllers by replacing hardcoded error messages with i18n-based, context-aware responses, adding local OAuth error writers in the orchestration layer, switching JSON decode/encode to ChangesOAuth Provider i18n Localization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
controller/linuxdo.go (2)
5-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace direct
encoding/jsondecoding withcommonJSON wrappers.This file still uses
json.NewDecoder(...).Decode(...)in controller business code, which violates the repo’s Go JSON rule.As per coding guidelines, "All JSON marshal/unmarshal operations MUST use wrapper functions from
common/json.go... Do NOT directly import or callencoding/jsonin business code."Also applies to: 118-120, 142-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/linuxdo.go` at line 5, This file imports and directly uses encoding/json via json.NewDecoder(...).Decode(...) (occurrences around lines referenced) which violates the rule; replace the import and all direct decode calls with the project JSON wrapper functions from common/json.go (e.g., replace json.NewDecoder(req.Body).Decode(&obj) with the common wrapper such as common.DecodeJSON(req.Body, &obj) or the specific wrapper function defined in common/json.go), remove the direct encoding/json import, and update all instances in controller/linuxdo.go (including the other occurrences you noted) to use the common wrapper functions so business code no longer calls encoding/json directly.
38-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOAuth helper errors are still emitted as plain errors, so stable
codecan be lost.
getLinuxdoUserInfoByCodereturns localizederrorstrings, and callers send them viacommon.ApiError(c, err). That path usually serializes message-only errors, which breaks the “stable code” contract for OAuth business failures.Please route these branches through the OAuth i18n error writer (or return a structured OAuth error carrying key/code) so responses consistently include
code.Also applies to: 76-79, 109-111, 122-124, 136-138, 146-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/linuxdo.go` around lines 38 - 41, The handlers currently pass localized plain errors from getLinuxdoUserInfoByCode (and similar helpers) into common.ApiError, which strips the stable OAuth error code; instead, when getLinuxdoUserInfoByCode (and the other call sites noted) returns an OAuth-related failure, route the response through the OAuth i18n error writer (or return a structured OAuth error that carries a stable Code/Key and message) so the HTTP response always includes the stable OAuth code; concretely, replace calls like common.ApiError(c, err) for OAuth failures with the OAuth i18n writer (e.g., oauth.I18nErrorWriter(c, err)) or wrap the error in a typed OAuthError { Code: "...", Message: err } before sending, and apply this change for all similar branches in linuxdoUser handling.controller/oidc.go (1)
4-4:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
commonJSON wrappers instead ofencoding/jsonin controller code.
json.NewDecoder(...).Decode(...)is still used in this Go business file and should be replaced with the repository’s JSON wrapper utilities.As per coding guidelines, "All JSON marshal/unmarshal operations MUST use wrapper functions from
common/json.go... Do NOT directly import or callencoding/jsonin business code."Also applies to: 67-69, 94-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/oidc.go` at line 4, Remove the direct import of "encoding/json" and replace all uses of json.NewDecoder(...).Decode(...) (and any json.NewEncoder(...).Encode(...)) in the oidc controller handlers with the repository's common JSON wrapper functions; e.g., change json.NewDecoder(r.Body).Decode(&obj) to the common JSON decode wrapper such as common.JSONDecode(r.Body, &obj) and change json.NewEncoder(w).Encode(resp) to common.JSONEncode(w, resp), and update imports to use the common package instead of "encoding/json" so all marshal/unmarshal calls use the common/json.go wrappers.controller/github.go (3)
36-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
common.Marshal()instead ofencoding/jsondirectly.Line 37 directly calls
json.Marshal(values), which violates the coding guidelines. All JSON marshal operations must use wrapper functions fromcommon/json.go.♻️ Proposed fix using common.Marshal
- jsonData, err := json.Marshal(values) + jsonData, err := common.Marshal(values)As per coding guidelines: "Use wrapper functions from
common/json.gofor all JSON marshal/unmarshal operations:common.Marshal(),common.Unmarshal(),common.UnmarshalJsonStr(),common.DecodeJson(),common.GetJsonType(). Do NOT directly import or callencoding/jsonin business code."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/github.go` around lines 36 - 40, Replace the direct call to json.Marshal in the GitHub token exchange code: instead of json.Marshal(values) assign jsonData, err = common.Marshal(values) and handle err as before; update the surrounding code that uses jsonData (variable jsonData and err) and remove any direct dependency on encoding/json in controller/github.go so all JSON marshalling uses the common.Marshal wrapper from common/json.go.
72-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
commonJSON wrapper instead ofencoding/jsondirectly.Line 73 directly calls
json.NewDecoder(res2.Body).Decode(...), which violates the coding guidelines. All JSON unmarshal operations must use wrapper functions fromcommon/json.go.♻️ Proposed fix using common.DecodeJson
- var githubUser GitHubUser - err = json.NewDecoder(res2.Body).Decode(&githubUser) - if err != nil { + var githubUser GitHubUser + err = common.DecodeJson(res2.Body, &githubUser) + if err != nil {As per coding guidelines: "Use wrapper functions from
common/json.gofor all JSON marshal/unmarshal operations:common.Marshal(),common.Unmarshal(),common.UnmarshalJsonStr(),common.DecodeJson(),common.GetJsonType(). Do NOT directly import or callencoding/jsonin business code."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/github.go` around lines 72 - 76, The code decodes JSON directly with json.NewDecoder(res2.Body).Decode(&githubUser) which violates the guideline; replace that call with the wrapper common.DecodeJson to decode res2.Body into the githubUser variable (e.g., call common.DecodeJson(res2.Body, &githubUser)), update any error handling to return the same error, and remove the direct dependency on encoding/json from imports so only the common JSON wrapper is used.
56-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
commonJSON wrapper instead ofencoding/jsondirectly.Line 57 directly calls
json.NewDecoder(res.Body).Decode(...), which violates the coding guidelines. All JSON unmarshal operations must use wrapper functions fromcommon/json.go.♻️ Proposed fix using common.DecodeJson
- var oAuthResponse GitHubOAuthResponse - err = json.NewDecoder(res.Body).Decode(&oAuthResponse) - if err != nil { + var oAuthResponse GitHubOAuthResponse + err = common.DecodeJson(res.Body, &oAuthResponse) + if err != nil {As per coding guidelines: "Use wrapper functions from
common/json.gofor all JSON marshal/unmarshal operations:common.Marshal(),common.Unmarshal(),common.UnmarshalJsonStr(),common.GetJsonType(). Do NOT directly import or callencoding/jsonin business code."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/github.go` around lines 56 - 60, Replace the direct use of encoding/json in the GitHub OAuth flow: instead of calling json.NewDecoder(res.Body).Decode(&oAuthResponse), read the response body into bytes (from res.Body) and call the common JSON wrapper (e.g. common.Unmarshal(bodyBytes, &oAuthResponse) or common.UnmarshalJsonStr(string(bodyBytes), &oAuthResponse)) so all JSON unmarshalling uses the common package; keep the same GitHubOAuthResponse target and return error on failure as before.controller/discord.go (2)
64-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
commonJSON wrapper instead ofencoding/jsondirectly.Line 65 directly calls
json.NewDecoder(res.Body).Decode(...), which violates the coding guidelines. All JSON unmarshal operations must use wrapper functions fromcommon/json.go.♻️ Proposed fix using common.DecodeJson
- var discordResponse DiscordResponse - err = json.NewDecoder(res.Body).Decode(&discordResponse) - if err != nil { + var discordResponse DiscordResponse + err = common.DecodeJson(res.Body, &discordResponse) + if err != nil {As per coding guidelines: "Use wrapper functions from
common/json.gofor all JSON marshal/unmarshal operations:common.Marshal(),common.Unmarshal(),common.UnmarshalJsonStr(),common.DecodeJson(),common.GetJsonType(). Do NOT directly import or callencoding/jsonin business code."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/discord.go` around lines 64 - 68, Replace the direct use of encoding/json decoder when decoding the HTTP response into DiscordResponse: instead of calling json.NewDecoder(res.Body).Decode(&discordResponse) (and importing encoding/json), call the wrapper common.DecodeJson to read and unmarshal the response body into the discordResponse variable; update the code that declares discordResponse (DiscordResponse) to pass its pointer into common.DecodeJson and handle/return any error from that call.
91-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
commonJSON wrapper instead ofencoding/jsondirectly.Line 92 directly calls
json.NewDecoder(res2.Body).Decode(...), which violates the coding guidelines. All JSON unmarshal operations must use wrapper functions fromcommon/json.go.♻️ Proposed fix using common.DecodeJson
- var discordUser DiscordUser - err = json.NewDecoder(res2.Body).Decode(&discordUser) - if err != nil { + var discordUser DiscordUser + err = common.DecodeJson(res2.Body, &discordUser) + if err != nil {As per coding guidelines: "Use wrapper functions from
common/json.gofor all JSON marshal/unmarshal operations:common.Marshal(),common.Unmarshal(),common.UnmarshalJsonStr(),common.DecodeJson(),common.GetJsonType(). Do NOT directly import or callencoding/jsonin business code."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/discord.go` around lines 91 - 95, The code decodes HTTP response JSON into DiscordUser using json.NewDecoder(res2.Body).Decode(&discordUser), which violates the guideline to use common JSON wrappers; replace this direct call with the wrapper common.DecodeJson on the response body to populate the variable discordUser (i.e., call common.DecodeJson(res2.Body, &discordUser) or the equivalent DecodeJson signature), and remove any direct use/import of encoding/json in the surrounding code or file.
🧹 Nitpick comments (1)
web/default/src/routes/oauth/$provider.tsx (1)
210-210: ⚡ Quick winAdd explicit type annotation for the
codevariable.The
codevariable lacks an explicit type annotation. Per TypeScript guidelines, prefer explicit types over implicit inference to avoidany.✨ Suggested type annotation
- const code = res?.data?.code + const code: string | undefined = res?.data?.codeAs per coding guidelines, avoid
anytype in TypeScript; prefer specific types orunknown; explicitly annotate parameter and return value types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/routes/oauth/`$provider.tsx at line 210, The local variable "code" (const code = res?.data?.code) in oauth/$provider.tsx should have an explicit TypeScript type to avoid implicit any; change its declaration to include an appropriate type (for example const code: string | undefined = res?.data?.code) and, if the value may be non-string, use a narrower type or use unknown then validate/cast before use (update any downstream uses in the same function to handle undefined/null accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/oidc.go`:
- Around line 39-42: Replace plain localized errors in getOidcUserInfoByCode
(and the other failing return sites you listed) with a structured OAuth error
that includes a stable machine-readable code plus the localized message; e.g.,
construct and return the project's OAuth error type (something like
common.OAuthError{Code:"invalid_oauth_code", Message:i18n.T(c,
i18n.MsgOAuthInvalidCode)}) or use the existing helper/constructor that builds
OAuth errors, and update each failing return (the empty-code check and the other
error paths at the referenced spots) to return that structured error so
common.ApiError(c, err) emits both the language-agnostic code and the localized
message.
In `@controller/telegram.go`:
- Around line 66-73: The FillUserByTelegramId error is being unconditionally
mapped to the OAuth-deleted business error; update the error handling in the
controller/telegram.go block that calls model.User.FillUserByTelegramId() so
that you distinguish "user not found" (map to i18n.MsgOAuthUserDeleted) from
transient/internal failures (log the actual err and return an appropriate
internal/server error response), e.g., by checking for the model-level sentinel
(like model.ErrUserNotFound) or sql.ErrNoRows and using ApiErrorI18n(c,
i18n.MsgOAuthUserDeleted) only for the not-found case, otherwise call the
internal error handler (and include err details in the log) for other errors.
---
Outside diff comments:
In `@controller/discord.go`:
- Around line 64-68: Replace the direct use of encoding/json decoder when
decoding the HTTP response into DiscordResponse: instead of calling
json.NewDecoder(res.Body).Decode(&discordResponse) (and importing
encoding/json), call the wrapper common.DecodeJson to read and unmarshal the
response body into the discordResponse variable; update the code that declares
discordResponse (DiscordResponse) to pass its pointer into common.DecodeJson and
handle/return any error from that call.
- Around line 91-95: The code decodes HTTP response JSON into DiscordUser using
json.NewDecoder(res2.Body).Decode(&discordUser), which violates the guideline to
use common JSON wrappers; replace this direct call with the wrapper
common.DecodeJson on the response body to populate the variable discordUser
(i.e., call common.DecodeJson(res2.Body, &discordUser) or the equivalent
DecodeJson signature), and remove any direct use/import of encoding/json in the
surrounding code or file.
In `@controller/github.go`:
- Around line 36-40: Replace the direct call to json.Marshal in the GitHub token
exchange code: instead of json.Marshal(values) assign jsonData, err =
common.Marshal(values) and handle err as before; update the surrounding code
that uses jsonData (variable jsonData and err) and remove any direct dependency
on encoding/json in controller/github.go so all JSON marshalling uses the
common.Marshal wrapper from common/json.go.
- Around line 72-76: The code decodes JSON directly with
json.NewDecoder(res2.Body).Decode(&githubUser) which violates the guideline;
replace that call with the wrapper common.DecodeJson to decode res2.Body into
the githubUser variable (e.g., call common.DecodeJson(res2.Body, &githubUser)),
update any error handling to return the same error, and remove the direct
dependency on encoding/json from imports so only the common JSON wrapper is
used.
- Around line 56-60: Replace the direct use of encoding/json in the GitHub OAuth
flow: instead of calling json.NewDecoder(res.Body).Decode(&oAuthResponse), read
the response body into bytes (from res.Body) and call the common JSON wrapper
(e.g. common.Unmarshal(bodyBytes, &oAuthResponse) or
common.UnmarshalJsonStr(string(bodyBytes), &oAuthResponse)) so all JSON
unmarshalling uses the common package; keep the same GitHubOAuthResponse target
and return error on failure as before.
In `@controller/linuxdo.go`:
- Line 5: This file imports and directly uses encoding/json via
json.NewDecoder(...).Decode(...) (occurrences around lines referenced) which
violates the rule; replace the import and all direct decode calls with the
project JSON wrapper functions from common/json.go (e.g., replace
json.NewDecoder(req.Body).Decode(&obj) with the common wrapper such as
common.DecodeJSON(req.Body, &obj) or the specific wrapper function defined in
common/json.go), remove the direct encoding/json import, and update all
instances in controller/linuxdo.go (including the other occurrences you noted)
to use the common wrapper functions so business code no longer calls
encoding/json directly.
- Around line 38-41: The handlers currently pass localized plain errors from
getLinuxdoUserInfoByCode (and similar helpers) into common.ApiError, which
strips the stable OAuth error code; instead, when getLinuxdoUserInfoByCode (and
the other call sites noted) returns an OAuth-related failure, route the response
through the OAuth i18n error writer (or return a structured OAuth error that
carries a stable Code/Key and message) so the HTTP response always includes the
stable OAuth code; concretely, replace calls like common.ApiError(c, err) for
OAuth failures with the OAuth i18n writer (e.g., oauth.I18nErrorWriter(c, err))
or wrap the error in a typed OAuthError { Code: "...", Message: err } before
sending, and apply this change for all similar branches in linuxdoUser handling.
In `@controller/oidc.go`:
- Line 4: Remove the direct import of "encoding/json" and replace all uses of
json.NewDecoder(...).Decode(...) (and any json.NewEncoder(...).Encode(...)) in
the oidc controller handlers with the repository's common JSON wrapper
functions; e.g., change json.NewDecoder(r.Body).Decode(&obj) to the common JSON
decode wrapper such as common.JSONDecode(r.Body, &obj) and change
json.NewEncoder(w).Encode(resp) to common.JSONEncode(w, resp), and update
imports to use the common package instead of "encoding/json" so all
marshal/unmarshal calls use the common/json.go wrappers.
---
Nitpick comments:
In `@web/default/src/routes/oauth/`$provider.tsx:
- Line 210: The local variable "code" (const code = res?.data?.code) in
oauth/$provider.tsx should have an explicit TypeScript type to avoid implicit
any; change its declaration to include an appropriate type (for example const
code: string | undefined = res?.data?.code) and, if the value may be non-string,
use a narrower type or use unknown then validate/cast before use (update any
downstream uses in the same function to handle undefined/null accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6153a012-4707-4756-b78f-d7c88448907a
📒 Files selected for processing (7)
controller/discord.gocontroller/github.gocontroller/linuxdo.gocontroller/oauth.gocontroller/oidc.gocontroller/telegram.goweb/default/src/routes/oauth/$provider.tsx
- Replace direct encoding/json calls with common JSON wrappers in OAuth controllers.\n- Return typed OAuth errors so machine-readable codes survive controller responses.\n- Distinguish Telegram not-found and internal lookup failures.
|
Addressed the current review feedback in
Validation rerun:
|
51fdfc5 to
2b6f1df
Compare
1 participant
变更描述 / Description
OAuth 登录和绑定流程里，部分业务错误之前只能从本地化 message 判断。这个改动让后端 OAuth 业务错误同时返回稳定的 i18n key
code，default 前端在 GitHub 已绑定账号的兼容登录分支中改为匹配code，不再依赖中文文案。变更不改变 OAuth 登录、绑定、注册、session 写入流程，只调整错误响应结构和前端判断条件。
变更类型 / Type of change
关联任务 / Related Issue
提交前检查项 / Checklist
运行证明 / Proof of Work
结果：
前端检查说明：
Summary by CodeRabbit
Improvements
Bug Fixes