Skip to content

feat(oidc): support idp initialized auth #3678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fiftin wants to merge 1 commit into
base: develop
Choose a base branch
from
+51 −44
Open

feat(oidc): support idp initialized auth #3678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 46 additions & 43 deletions api/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,36 +673,53 @@

func oidcRedirect(w http.ResponseWriter, r *http.Request) {
pid := mux.Vars(r)["provider"]
oauthState, err := r.Cookie("oauthstate")
loginURL, _ := url.JoinPath(util.Config.WebHost, "auth/login")

if err != nil {
log.Error(err.Error())
provider, ok := util.Config.OidcProviders[pid]
if !ok {
log.Error(fmt.Errorf("no such provider: %s", pid))
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
return
}

s := r.FormValue("state")
b, err := base64.URLEncoding.DecodeString(s)

if err != nil {
log.Error(err.Error())
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
return
}
// IdP-initiated flow is detected by the absence of a state parameter.
// In SP-initiated flow, oidcLogin always sets state before redirecting to the IdP.
stateParam := r.FormValue("state")
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 {
Comment on lines +688 to +692
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Require request binding for IdP-initiated callbacks

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 👍 / 👎.

if !provider.AllowIdpInitiated {
log.Warn("IdP-initiated OIDC login attempted but not allowed for provider: " + pid)
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
return
}
} else {
oauthState, err := r.Cookie("oauthstate")
if err != nil {
log.Error(err.Error())
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
return
}

if stateData.Csrf != oauthState.Value {
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
return
b, err := base64.URLEncoding.DecodeString(stateParam)
if err != nil {
log.Error(err.Error())
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
return
}

if err = json.Unmarshal(b, &stateData); err != nil {
log.Error(err.Error())
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
return
}

if stateData.Csrf != oauthState.Value {
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
return
}
}

ctx := context.Background()
Expand All @@ -714,13 +731,6 @@
return
}

provider, ok := util.Config.OidcProviders[pid]
if !ok {
log.Error(fmt.Errorf("no such provider: %s", pid))
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
return
}

verifier := _oidc.Verifier(&oidc.Config{ClientID: oauth.ClientID})

code := r.URL.Query().Get("code")
Expand All @@ -734,12 +744,10 @@

var claims claimResult

// Extract the ID Token from OAuth2 token.
rawIDToken, ok := oauth2Token.Extra("id_token").(string)
rawIDToken, tokenOk := oauth2Token.Extra("id_token").(string)

if ok && rawIDToken != "" {
if tokenOk && rawIDToken != "" {
var idToken *oidc.IDToken
// Parse and verify ID Token payload.
idToken, err = verifier.Verify(ctx, rawIDToken)

if err == nil {
Expand Down Expand Up @@ -770,7 +778,7 @@
return
}

user, err := helpers.Store(r).GetUserByLoginOrEmail("", claims.email) // ignore username because it creates a lot of problems
user, err := helpers.Store(r).GetUserByLoginOrEmail("", claims.email)
if err != nil {
user = db.User{
Username: claims.username,
Expand All @@ -795,18 +803,13 @@

createSession(w, r, user, true)

config, ok := util.Config.OidcProviders[pid]
if !ok {
log.Error(fmt.Errorf("no such provider: %s", pid))
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
return
}

redirectPath := ""
if config.ReturnViaState {
redirectPath = stateData.Return
} else {
redirectPath = mux.Vars(r)["redirect_path"]
if !idpInitiated {
if provider.ReturnViaState {
redirectPath = stateData.Return
} else {
redirectPath = mux.Vars(r)["redirect_path"]
}

Check warning

Code scanning / CodeQL

Bad redirect check Medium

This is a check that
this value
Loading
, which flows into a
redirect
Loading
, has a leading slash, but not that it does not have '/' or '' in its second position.
This is a check that
this value
Loading
, which flows into a
redirect
Loading
, has a leading slash, but not that it does not have '/' or '' in its second position.

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:

  1. If redirectPath is empty, default to /.
  2. If redirectPath does not start with /, prepend /.
  3. 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 the oidcRedirect function:

    • 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 resetting redirectPath to /.

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.

Suggested changeset 1
api/login.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/api/login.go b/api/login.go
--- a/api/login.go
+++ b/api/login.go
@@ -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 {
EOF
@@ -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 {
Copilot is powered by AI and may make mistakes. Always verify output.
}

if !strings.HasPrefix(redirectPath, "/") {
Expand All @@ -821,7 +824,7 @@
}

if redirectURL == "" {
redirectURL = "/"

Check warning

Code scanning / CodeQL

Open URL redirect Medium

This path to an untrusted URL redirection depends on a
user-provided value
Loading
.
}

http.Redirect(w, r, redirectURL, http.StatusTemporaryRedirect)
Expand Down
6 changes: 5 additions & 1 deletion util/OdbcProvider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@ type OidcProvider struct {
EmailClaim string `json:"email_claim" default:"email"`
Order int `json:"order"`
// ReturnViaState when true, passes the return path via the OAuth state parameter instead of the redirect URL path. This is useful for OAuth providers that have strict redirect URL validation.
ReturnViaState bool `json:"return_via_state" default:"true"`
ReturnViaState bool `json:"return_via_state" default:"true"`
// AllowIdpInitiated when true, permits IdP-initiated OIDC login (e.g. clicking the app
// tile in Okta). This skips CSRF state validation since the flow does not originate from
// Semaphore. Only enable this for trusted identity providers.
AllowIdpInitiated bool `json:"allow_idp_initiated"`
}

type ClaimsProvider interface {
Expand Down
Loading