Skip to content

Commit 72db8d3

Browse files
authored
Merge pull request semaphoreui#3494 from semaphoreui/oauth_return_in_state
feat: add return path to the state field
1 parent 528f997 commit 72db8d3

10 files changed

Lines changed: 31283 additions & 31144 deletions

File tree

api/login.go

Lines changed: 78 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -482,39 +482,74 @@ func getOidcProvider(id string, ctx context.Context, redirectPath string) (*oidc
482482
func oidcLogin(w http.ResponseWriter, r *http.Request) {
483483
pid := mux.Vars(r)["provider"]
484484
ctx := context.Background()
485+
loginURL, _ := url.JoinPath(util.Config.WebHost, "auth/login")
485486

487+
returnPath := ""
486488
redirectPath := ""
487489

488-
if r.URL.Query()["redirect"] != nil {
489-
// TODO: validate path
490-
redirectPath = r.URL.Query()["redirect"][0]
490+
config, ok := util.Config.OidcProviders[pid]
491+
if !ok {
492+
log.Error(fmt.Errorf("no such provider: %s", pid))
493+
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
494+
return
495+
}
496+
497+
returnValue := r.URL.Query().Get("return")
498+
if returnValue != "" {
499+
if config.ReturnViaState {
500+
returnPath = returnValue
501+
} else {
502+
redirectPath = returnValue
503+
}
491504
}
492505

493506
_, oauth, err := getOidcProvider(pid, ctx, redirectPath)
494507
if err != nil {
495508
log.Error(err.Error())
496-
loginURL, _ := url.JoinPath(util.Config.WebHost, "auth/login")
497509
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
498510
return
499511
}
500-
state := generateStateOauthCookie(w)
512+
state := generateStateOauthCookie(w, returnPath)
501513
u := oauth.AuthCodeURL(state)
502514
http.Redirect(w, r, u, http.StatusTemporaryRedirect)
503515
}
504516

505-
func generateStateOauthCookie(w http.ResponseWriter) string {
517+
type oAuthState struct {
518+
Csrf string `json:"csrf"`
519+
Return string `json:"return"`
520+
}
521+
522+
func generateStateOauthCookie(w http.ResponseWriter, returnPath string) string {
523+
506524
expiration := tz.Now().Add(365 * 24 * time.Hour)
507525

508526
b := make([]byte, 16)
509527
_, err := rand.Read(b)
510528
if err != nil {
511529
panic(err)
512530
}
513-
oauthState := base64.URLEncoding.EncodeToString(b)
514-
cookie := http.Cookie{Name: "oauthstate", Value: oauthState, Expires: expiration}
531+
532+
state := oAuthState{
533+
Csrf: base64.URLEncoding.EncodeToString(b),
534+
Return: returnPath,
535+
}
536+
537+
// Secure flag is not set to allow Semaphore to be used without HTTPS inside private networks
538+
cookie := http.Cookie{
539+
Name: "oauthstate",
540+
Value: state.Csrf,
541+
Expires: expiration,
542+
HttpOnly: true,
543+
SameSite: http.SameSiteLaxMode,
544+
}
515545
http.SetCookie(w, &cookie)
516546

517-
return oauthState
547+
stateBytes, err := json.Marshal(state)
548+
if err != nil {
549+
panic(err)
550+
}
551+
552+
return base64.URLEncoding.EncodeToString(stateBytes)
518553
}
519554

520555
type claimResult struct {
@@ -648,7 +683,25 @@ func oidcRedirect(w http.ResponseWriter, r *http.Request) {
648683
return
649684
}
650685

651-
if r.FormValue("state") != oauthState.Value {
686+
s := r.FormValue("state")
687+
b, err := base64.URLEncoding.DecodeString(s)
688+
689+
if err != nil {
690+
log.Error(err.Error())
691+
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
692+
return
693+
}
694+
695+
var stateData oAuthState
696+
err = json.Unmarshal(b, &stateData)
697+
698+
if err != nil {
699+
log.Error(err.Error())
700+
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
701+
return
702+
}
703+
704+
if stateData.Csrf != oauthState.Value {
652705
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
653706
return
654707
}
@@ -743,7 +796,19 @@ func oidcRedirect(w http.ResponseWriter, r *http.Request) {
743796

744797
createSession(w, r, user, true)
745798

746-
redirectPath := mux.Vars(r)["redirect_path"]
799+
config, ok := util.Config.OidcProviders[pid]
800+
if !ok {
801+
log.Error(fmt.Errorf("no such provider: %s", pid))
802+
http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
803+
return
804+
}
805+
806+
redirectPath := ""
807+
if config.ReturnViaState {
808+
redirectPath = stateData.Return
809+
} else {
810+
redirectPath = mux.Vars(r)["redirect_path"]
811+
}
747812

748813
redirectPath, err = url.JoinPath(util.Config.WebHost, redirectPath)
749814
if err != nil {
@@ -752,7 +817,8 @@ func oidcRedirect(w http.ResponseWriter, r *http.Request) {
752817
return
753818
}
754819

755-
if redirectPath == "" {
820+
// Only allow redirect paths starting with '/' but NOT with '//' or '/\'.
821+
if !(len(redirectPath) > 1 && redirectPath[0] == '/' && redirectPath[1] != '/' && redirectPath[1] != '\\') {
756822
redirectPath = "/"
757823
}
758824

api/login_test.go

Lines changed: 113 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,14 @@
11
package api
22

33
import (
4+
"encoding/base64"
5+
"encoding/json"
6+
"net/http"
7+
"net/http/httptest"
48
"testing"
9+
"time"
10+
11+
"github.com/stretchr/testify/assert"
512
)
613

714
func TestParseClaim(t *testing.T) {
@@ -13,13 +20,8 @@ func TestParseClaim(t *testing.T) {
1320

1421
res, ok := parseClaim("email | {{ .id }}@test.com", claims)
1522

16-
if !ok {
17-
t.Fail()
18-
}
19-
20-
if res != "1234567@test.com" {
21-
t.Fatalf("%s must be %d@test.com", res, claims["id"])
22-
}
23+
assert.True(t, ok, "parseClaim should succeed")
24+
assert.Equal(t, "1234567@test.com", res, "Result should be formatted correctly")
2325
}
2426

2527
func TestParseClaim2(t *testing.T) {
@@ -31,13 +33,8 @@ func TestParseClaim2(t *testing.T) {
3133

3234
res, ok := parseClaim("username", claims)
3335

34-
if !ok {
35-
t.Fail()
36-
}
37-
38-
if res != claims["username"] {
39-
t.Fail()
40-
}
36+
assert.True(t, ok, "parseClaim should succeed")
37+
assert.Equal(t, claims["username"], res, "Result should match username claim")
4138
}
4239

4340
func TestParseClaim3(t *testing.T) {
@@ -49,9 +46,7 @@ func TestParseClaim3(t *testing.T) {
4946

5047
_, ok := parseClaim("email", claims)
5148

52-
if ok {
53-
t.Fail()
54-
}
49+
assert.False(t, ok, "parseClaim should fail for empty email")
5550
}
5651

5752
func TestParseClaim4(t *testing.T) {
@@ -63,9 +58,7 @@ func TestParseClaim4(t *testing.T) {
6358

6459
_, ok := parseClaim("|", claims)
6560

66-
if ok {
67-
t.Fail()
68-
}
61+
assert.False(t, ok, "parseClaim should fail for invalid pattern")
6962
}
7063

7164
func TestParseClaim5(t *testing.T) {
@@ -79,7 +72,105 @@ func TestParseClaim5(t *testing.T) {
7972

8073
res, ok := parseClaim("{{ .id }}", claims)
8174

82-
if !ok || res != "123456757343" {
83-
t.Fatalf("Expected: %v, Got: %v", "123456757343", res)
75+
assert.True(t, ok, "parseClaim should succeed")
76+
assert.Equal(t, "123456757343", res, "Result should match formatted ID")
77+
}
78+
79+
func TestGenerateStateOauthCookie(t *testing.T) {
80+
w := httptest.NewRecorder()
81+
returnPath := "/dashboard"
82+
83+
stateStr := generateStateOauthCookie(w, returnPath)
84+
85+
// Test 1: Verify returned state is valid base64
86+
stateBytes, err := base64.URLEncoding.DecodeString(stateStr)
87+
assert.NoError(t, err, "Returned state should be valid base64")
88+
89+
// Test 2: Verify state contains valid JSON
90+
var state oAuthState
91+
err = json.Unmarshal(stateBytes, &state)
92+
assert.NoError(t, err, "State should contain valid JSON")
93+
94+
// Test 3: Verify return path is preserved
95+
assert.Equal(t, returnPath, state.Return, "Return path should be preserved")
96+
97+
// Test 4: Verify CSRF token is not empty
98+
assert.NotEmpty(t, state.Csrf, "CSRF token should not be empty")
99+
100+
// Test 5: Verify CSRF token is valid base64
101+
_, err = base64.URLEncoding.DecodeString(state.Csrf)
102+
assert.NoError(t, err, "CSRF token should be valid base64")
103+
104+
// Test 6: Verify cookie is set
105+
cookies := w.Result().Cookies()
106+
assert.NotEmpty(t, cookies, "At least one cookie should be set")
107+
108+
// Test 7: Verify cookie has correct name
109+
var oauthCookie *http.Cookie
110+
for _, cookie := range cookies {
111+
if cookie.Name == "oauthstate" {
112+
oauthCookie = cookie
113+
break
114+
}
115+
}
116+
assert.NotNil(t, oauthCookie, "Cookie 'oauthstate' should be set")
117+
118+
// Test 8: Verify cookie value matches CSRF token in state
119+
assert.Equal(t, state.Csrf, oauthCookie.Value, "Cookie value should match CSRF token")
120+
121+
// Test 9: Verify cookie has expiration set (should be ~365 days)
122+
assert.False(t, oauthCookie.Expires.IsZero(), "Cookie expiration should be set")
123+
124+
expectedExpiration := time.Now().Add(365 * 24 * time.Hour)
125+
timeDiff := oauthCookie.Expires.Sub(expectedExpiration)
126+
if timeDiff < 0 {
127+
timeDiff = -timeDiff
84128
}
129+
// Allow 5 seconds tolerance for test execution time
130+
assert.LessOrEqual(t, timeDiff, 5*time.Second, "Cookie expiration should be within 5 seconds of expected")
131+
}
132+
133+
func TestGenerateStateOauthCookieEmptyReturnPath(t *testing.T) {
134+
w := httptest.NewRecorder()
135+
returnPath := ""
136+
137+
stateStr := generateStateOauthCookie(w, returnPath)
138+
139+
// Decode and verify state
140+
stateBytes, err := base64.URLEncoding.DecodeString(stateStr)
141+
assert.NoError(t, err, "Returned state should be valid base64")
142+
143+
var state oAuthState
144+
err = json.Unmarshal(stateBytes, &state)
145+
assert.NoError(t, err, "State should contain valid JSON")
146+
147+
// Verify empty return path is preserved
148+
assert.Empty(t, state.Return, "Return path should be empty")
149+
}
150+
151+
func TestGenerateStateOauthCookieUniqueness(t *testing.T) {
152+
// Generate two states and verify they have different CSRF tokens
153+
w1 := httptest.NewRecorder()
154+
w2 := httptest.NewRecorder()
155+
156+
state1Str := generateStateOauthCookie(w1, "/path1")
157+
state2Str := generateStateOauthCookie(w2, "/path2")
158+
159+
// Decode states
160+
state1Bytes, err1 := base64.URLEncoding.DecodeString(state1Str)
161+
state2Bytes, err2 := base64.URLEncoding.DecodeString(state2Str)
162+
assert.NoError(t, err1, "First state should be valid base64")
163+
assert.NoError(t, err2, "Second state should be valid base64")
164+
165+
var state1, state2 oAuthState
166+
err1 = json.Unmarshal(state1Bytes, &state1)
167+
err2 = json.Unmarshal(state2Bytes, &state2)
168+
assert.NoError(t, err1, "First state should be valid JSON")
169+
assert.NoError(t, err2, "Second state should be valid JSON")
170+
171+
// Verify CSRF tokens are different
172+
assert.NotEqual(t, state1.Csrf, state2.Csrf, "Multiple calls should generate different CSRF tokens")
173+
174+
// Verify states are different
175+
assert.NotEqual(t, state1Str, state2Str, "Multiple calls should generate different state strings")
85176
}

pro/go.mod

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,7 @@ require (
3838
github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect
3939
github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3 // indirect
4040
github.com/sirupsen/logrus v1.9.3 // indirect
41-
github.com/skeema/knownhosts v1.3.1 // indirect
42-
github.com/xanzy/ssh-agent v0.3.3 // indirect
4341
golang.org/x/crypto v0.45.0 // indirect
44-
golang.org/x/exp v0.0.0-20250620022241-b7579e27df2b // indirect
45-
golang.org/x/net v0.47.0 // indirect
4642
golang.org/x/sys v0.38.0 // indirect
4743
gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect
4844
gopkg.in/warnings.v0 v0.1.2 // indirect

pro/go.sum

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -111,29 +111,12 @@ github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+
111111
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
112112
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
113113
github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
114-
github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U=
115-
github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U=
116-
github.com/xanzy/ssh-agent v0.3.3 h1:+/15pJfg/RsTxqYcX6fHqOXZwwMP+2VyYWJeWM2qQFM=
117-
github.com/xanzy/ssh-agent v0.3.3/go.mod h1:6dzNDKs0J9rVPHPhaGCukekBHKqfl+L3KghI1Bc68Uw=
118-
go.etcd.io/bbolt v1.4.1 h1:5mOV+HWjIPLEAlUGMsveaUvK2+byZMFOzojoi7bh7uI=
119-
go.etcd.io/bbolt v1.4.1/go.mod h1:c8zu2BnXWTu2XM4XcICtbGSl9cFwsXtcf9zLt2OncM8=
120-
golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4=
121-
golang.org/x/crypto v0.45.0 h1:jMBrvKuj23MTlT0bQEOBcAE0mjg8mK9RXFhRH6nyF3Q=
114+
github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA=
115+
github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
116+
golang.org/x/crypto v0.38.0 h1:jt+WWG8IZlBnVbomuhg2Mdq0+BBQaHbtqHEFEigjUV8=
117+
golang.org/x/crypto v0.38.0/go.mod h1:MvrbAqul58NNYPKnOra203SB9vpuZW0e+RRZV+Ggqjw=
118+
golang.org/x/crypto v0.44.0/go.mod h1:013i+Nw79BMiQiMsOPcVCB5ZIJbYkerPrGnOa00tvmc=
122119
golang.org/x/crypto v0.45.0/go.mod h1:XTGrrkGJve7CYK7J8PEww4aY7gM3qMCElcJQ8n8JdX4=
123-
golang.org/x/exp v0.0.0-20250620022241-b7579e27df2b h1:M2rDM6z3Fhozi9O7NWsxAkg/yqS/lQJ6PmkyIV3YP+o=
124-
golang.org/x/exp v0.0.0-20250620022241-b7579e27df2b/go.mod h1:3//PLf8L/X+8b4vuAfHzxeRUl04Adcb341+IGKfnqS8=
125-
golang.org/x/mod v0.27.0 h1:kb+q2PyFnEADO2IEF935ehFUXlWiNjJWtRNgBLSfbxQ=
126-
golang.org/x/mod v0.27.0/go.mod h1:rWI627Fq0DEoudcK+MBkNkCe0EetEaDSwJJkCcjpazc=
127-
golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
128-
golang.org/x/net v0.47.0 h1:Mx+4dIFzqraBXUugkia1OOvlD6LemFo1ALMHjrXDOhY=
129-
golang.org/x/net v0.47.0/go.mod h1:/jNxtkgq5yWUGYkaZGqo27cfGZ1c5Nen03aYrrKpVRU=
130-
golang.org/x/sync v0.16.0 h1:ycBJEhp9p4vXvUZNszeOq0kGTPghopOL8q0fq3vstxw=
131-
golang.org/x/sync v0.16.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA=
132-
golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
133-
golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
134-
golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
135-
golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
136-
golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
137120
golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
138121
golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
139122
golang.org/x/sys v0.38.0 h1:3yZWxaJjBmCWXqhN1qh02AkOnCQ1poK6oF+a7xWL6Gc=

util/OdbcProvider.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ type OidcProvider struct {
1616
NameClaim string `json:"name_claim" default:"preferred_username"`
1717
EmailClaim string `json:"email_claim" default:"email"`
1818
Order int `json:"order"`
19+
// 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.
20+
ReturnViaState bool `json:"return_via_state"`
1921
}
2022

2123
type ClaimsProvider interface {

0 commit comments

Comments
 (0)