Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

Commit

Permalink
Validate callback and logout paths are unique (#33)
Browse files Browse the repository at this point in the history
  • Loading branch information
@sergicastro
sergicastro authored Feb 21, 2024
1 parent 3bef742 commit 89b761a
Show file tree
Hide file tree
Showing 13 changed files with 140 additions and 12 deletions.
32 changes: 32 additions & 0 deletions internal/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ var (
ErrInvalidURL = errors.New("invalid URL")
ErrRequiredURL = errors.New("required URL")
ErrHealthPortInUse = errors.New("health port is already in use by listen port")
ErrMustNotBeRootPath = errors.New("must not be root path")
ErrMustBeDifferentPath = errors.New("must be different path")
)

// LocalConfigFile is a run.Config that loads the configuration file.
Expand Down Expand Up @@ -152,6 +154,12 @@ func mergeAndValidateOIDCConfigs(cfg *configv1.Config) error {

// Set the defaults
applyOIDCDefaults(f.GetOidc())

// validate the callback and the logout path are different
callbackURI, _ := url.Parse(f.GetOidc().GetCallbackUri())
if f.GetOidc().GetLogout() != nil && callbackURI.Path == f.GetOidc().GetLogout().GetPath() {
errs = append(errs, fmt.Errorf("%w: callback and logout paths must be different in chain %q", ErrMustBeDifferentPath, fc.Name))
}
}
}
// Clear the default config as it has already been merged. This way there is only one
Expand Down Expand Up @@ -233,6 +241,15 @@ func validateOIDCConfigURLs(c *oidcv1.OIDCConfig) error {
return fmt.Errorf("%w: invalid Redis session store URL: %w", ErrInvalidURL, err)
}
}

if hasRootPath(c.GetCallbackUri()) {
return fmt.Errorf("%w: invalid callback URL", ErrMustNotBeRootPath)
}
if c.GetLogout() != nil {
if isRootPath(c.GetLogout().GetPath()) {
return fmt.Errorf("%w: invalid logout path", ErrMustNotBeRootPath)
}
}
return nil
}

Expand All @@ -243,3 +260,18 @@ func validateURL(u string) error {
_, err := url.Parse(u)
return err
}

// hasRootPath returns true if the path of the given URL is "/" or empty.
// prerequisite: u is a valid URL.
func hasRootPath(uri string) bool {
if uri == "" {
return false
}
parsed, _ := url.Parse(uri)
return isRootPath(parsed.Path)
}

// isRootPath returns true if the path is "/" or empty.
func isRootPath(path string) bool {
return path == "/" || path == ""
}
8 changes: 6 additions & 2 deletions internal/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ func TestValidateConfig(t *testing.T) {
{"invalid-redis", "testdata/invalid-redis.json", errCheck{is: ErrInvalidURL}},
{"invalid-oidc-uris", "testdata/invalid-oidc-uris.json", errCheck{is: ErrRequiredURL}},
{"invalid-health-port", "testdata/invalid-health-port.json", errCheck{is: ErrHealthPortInUse}},
{"invalid-callback-uri", "testdata/invalid-callback.json", errCheck{is: ErrMustNotBeRootPath}},
{"invalid-logout-path", "testdata/invalid-logout.json", errCheck{is: ErrMustNotBeRootPath}},
{"invalid-callback-and-logout-path", "testdata/invalid-callback-logout.json", errCheck{is: ErrMustBeDifferentPath}},
{"oidc-dynamic", "testdata/oidc-dynamic.json", errCheck{is: nil}},
{"valid", "testdata/mock.json", errCheck{is: nil}},
}
Expand All @@ -76,7 +79,7 @@ func TestValidateConfig(t *testing.T) {

func TestValidateURLs(t *testing.T) {
const (
validURL = "http://fake"
validURL = "http://fake/path"
invalidURL = "ht tp://invalid"
validRedisURL = "redis://localhost:6379/0"
)
Expand Down Expand Up @@ -202,7 +205,7 @@ func TestLoadOIDC(t *testing.T) {
Oidc: &oidcv1.OIDCConfig{
AuthorizationUri: "http://fake",
TokenUri: "http://fake",
CallbackUri: "http://fake",
CallbackUri: "http://fake/callback",
JwksConfig: &oidcv1.OIDCConfig_Jwks{Jwks: "fake-jwks"},
ClientId: "fake-client-id",
ClientSecret: "fake-client-secret",
Expand All @@ -211,6 +214,7 @@ func TestLoadOIDC(t *testing.T) {
ProxyUri: "http://fake",
RedisSessionStoreConfig: &oidcv1.RedisConfig{ServerUri: "redis://localhost:6379/0"},
Scopes: []string{scopeOIDC},
Logout: &oidcv1.LogoutConfig{Path: "/logout", RedirectUri: "http://fake"},
},
},
},
Expand Down
4 changes: 2 additions & 2 deletions internal/testdata/duplicate-oidc.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"default_oidc_config": {
"authorization_uri": "http://fake",
"token_uri": "http://fake",
"callback_uri": "http://fake",
"callback_uri": "http://fake/callback",
"proxy_uri": "http://fake",
"jwks": "fake-jwks",
"client_id": "fake-client-id",
Expand All @@ -23,7 +23,7 @@
"oidc": {
"authorization_uri": "http://fake",
"token_uri": "http://fake",
"callback_uri": "http://fake",
"callback_uri": "http://fake/callback",
"proxy_uri": "http://fake",
"jwks": "fake-jwks",
"client_id": "fake-client-id",
Expand Down
29 changes: 29 additions & 0 deletions internal/testdata/invalid-callback-logout.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"listen_address": "0.0.0.0",
"listen_port": 8080,
"log_level": "debug",
"chains": [
{
"name": "mock",
"filters": [
{
"oidc": {
"callback_uri": "http://fake/same",
"authorization_uri": "http://fake",
"token_uri": "http://fake",
"client_id": "fake",
"client_secret": "fake",
"jwks": "fake",
"id_token": {
"header": "authorization",
"preamble": "Bearer"
},
"logout": {
"path": "/same"
}
}
}
]
}
]
}
26 changes: 26 additions & 0 deletions internal/testdata/invalid-callback.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"listen_address": "0.0.0.0",
"listen_port": 8080,
"log_level": "debug",
"chains": [
{
"name": "mock",
"filters": [
{
"oidc": {
"callback_uri": "http://fake",
"authorization_uri": "http://fake",
"token_uri": "http://fake",
"client_id": "fake",
"client_secret": "fake",
"jwks": "fake",
"id_token": {
"header": "authorization",
"preamble": "Bearer"
}
}
}
]
}
]
}
29 changes: 29 additions & 0 deletions internal/testdata/invalid-logout.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"listen_address": "0.0.0.0",
"listen_port": 8080,
"log_level": "debug",
"chains": [
{
"name": "mock",
"filters": [
{
"oidc": {
"callback_uri": "http://fake/callback",
"authorization_uri": "http://fake",
"token_uri": "http://fake",
"client_id": "fake",
"client_secret": "fake",
"jwks": "fake",
"id_token": {
"header": "authorization",
"preamble": "Bearer"
},
"logout": {
"path": "/"
}
}
}
]
}
]
}
2 changes: 1 addition & 1 deletion internal/testdata/invalid-oidc-override.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"oidc_override": {
"authorization_uri": "http://fake",
"token_uri": "http://fake",
"callback_uri": "http://fake",
"callback_uri": "http://fake/callback",
"proxy_uri": "http://fake",
"jwks": "fake-jwks",
"client_id": "fake-client-id",
Expand Down
2 changes: 1 addition & 1 deletion internal/testdata/invalid-oidc-uris.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"filters": [
{
"oidc": {
"callback_uri": "http://fake",
"callback_uri": "http://fake/callback",
"proxy_uri": "http://fake",
"client_id": "fake-client-id",
"client_secret": "fake-client-secret",
Expand Down
2 changes: 1 addition & 1 deletion internal/testdata/invalid-redis.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"oidc": {
"authorization_uri": "http://fake",
"token_uri": "http://fake",
"callback_uri": "http://fake",
"callback_uri": "http://fake/callback",
"proxy_uri": "http://fake",
"jwks": "fake-jwks",
"client_id": "fake-client-id",
Expand Down
4 changes: 2 additions & 2 deletions internal/testdata/multiple-oidc.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"oidc": {
"authorization_uri": "http://fake",
"token_uri": "http://fake",
"callback_uri": "http://fake",
"callback_uri": "http://fake/callback",
"proxy_uri": "http://fake",
"jwks": "fake-jwks",
"client_id": "fake-client-id",
Expand All @@ -25,7 +25,7 @@
"oidc": {
"authorization_uri": "http://fake",
"token_uri": "http://fake",
"callback_uri": "http://fake",
"callback_uri": "http://fake/callback",
"proxy_uri": "http://fake",
"jwks": "fake-jwks",
"client_id": "fake-client-id",
Expand Down
2 changes: 1 addition & 1 deletion internal/testdata/oidc-dynamic.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
{
"oidc": {
"configuration_uri": "http://fake",
"callback_uri": "http://fake",
"callback_uri": "http://fake/callback",
"proxy_uri": "http://fake",
"client_id": "fake-client-id",
"client_secret": "fake-client-secret",
Expand Down
6 changes: 5 additions & 1 deletion internal/testdata/oidc-override.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"default_oidc_config": {
"authorization_uri": "http://default",
"token_uri": "http://default",
"callback_uri": "http://fake",
"callback_uri": "http://fake/callback",
"proxy_uri": "http://fake"
},
"chains": [
Expand All @@ -25,6 +25,10 @@
},
"redis_session_store_config": {
"server_uri": "tcp://localhost:6379/0"
},
"logout": {
"path": "/logout",
"redirect_uri": "http://fake"
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion internal/testdata/oidc.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"oidc": {
"authorization_uri": "http://fake",
"token_uri": "http://fake",
"callback_uri": "http://fake",
"callback_uri": "http://fake/callback",
"proxy_uri": "http://fake",
"jwks": "fake-jwks",
"client_id": "fake-client-id",
Expand All @@ -21,6 +21,10 @@
},
"redis_session_store_config": {
"server_uri": "redis://localhost:6379/0"
},
"logout": {
"path": "/logout",
"redirect_uri": "http://fake"
}
}
}
Expand Down

0 comments on commit 89b761a

Please sign in to comment.