Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| redirectPath = stateData.Return | ||
| } else { | ||
| redirectPath = mux.Vars(r)["redirect_path"] | ||
| } |
Check warning
Code scanning / CodeQL
Bad redirect check Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general: When accepting a redirect path derived from untrusted input, ensure it is a safe, purely local path. At a minimum, require that it starts with a single / and that the second character is neither / nor \, to avoid scheme-relative (//...) and backslash-based (/\...) paths that browsers and URL parsers can misinterpret.
Best concrete fix here: Before using redirectPath in url.JoinPath, replace the current HasPrefix check with a stricter normalization/sanitization:
- If
redirectPathis empty, default to/. - If
redirectPathdoes not start with/, prepend/. - After that, if the second character exists and is
/or\, treat the whole thing as unsafe and fall back to/.
This preserves existing behavior for all normal, safe paths (like /dashboard or dashboard → /dashboard), while rejecting problematic ones like //evil and /\evil. The minimal code change is in api/login.go around lines 806–818, where redirectPath is post-processed. No new imports are needed; we can rely on len and direct byte indexing of the string.
Concretely:
- In
api/login.go, in theoidcRedirectfunction:-
Replace the existing block:
if !strings.HasPrefix(redirectPath, "/") { redirectPath = "/" + redirectPath }
-
With a new block that:
- Ensures leading
/. - Rejects paths whose second character is
/or\by resettingredirectPathto/.
- Ensures leading
-
This single change fixes both alert variants (for stateData.Return and mux.Vars(r)["redirect_path"]) because both flow into the same redirectPath variable and go through the same sanitization.
| @@ -812,9 +812,16 @@ | ||
| } | ||
| } | ||
|
|
||
| // Normalize and validate redirectPath to avoid open redirects. | ||
| if redirectPath == "" { | ||
| redirectPath = "/" | ||
| } | ||
| if !strings.HasPrefix(redirectPath, "/") { | ||
| redirectPath = "/" + redirectPath | ||
| } | ||
| if len(redirectPath) > 1 && (redirectPath[1] == '/' || redirectPath[1] == '\\') { | ||
| redirectPath = "/" | ||
| } | ||
|
|
||
| redirectURL, err := url.JoinPath(util.Config.WebHost, redirectPath) | ||
| if err != nil { |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a0a7545d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| idpInitiated := stateParam == "" | ||
|
|
||
| var stateData oAuthState | ||
| err = json.Unmarshal(b, &stateData) | ||
|
|
||
| if err != nil { | ||
| log.Error(err.Error()) | ||
| http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect) | ||
| return | ||
| } | ||
| if idpInitiated { |
There was a problem hiding this comment.
When allow_idp_initiated is enabled, oidcRedirect accepts any callback that omits state and skips all CSRF/session correlation, then immediately exchanges code and creates a session; this permits login CSRF/account confusion (an attacker can obtain an auth code for their own IdP account and send /redirect?code=... to a victim, who will be logged into the attacker's Semaphore account). This branch needs an additional binding signal (separate endpoint, signed IdP hint, or other anti-replay correlation) instead of treating every missing-state callback as trusted.
Useful? React with 👍 / 👎.
2 participants
No description provided.