fix: return error for empty Redis URL list (#3101)

* fix: return error for empty Redis URL list

* add changelog entry

Signed-off-by: Jan Larwig <jan@larwig.com>

---------

Signed-off-by: Jan Larwig <jan@larwig.com>
Co-authored-by: Jan Larwig <jan@larwig.com>

CHANGELOG.md

@@ -11,6 +11,7 @@
 - [#3072](https://github.com/oauth2-proxy/oauth2-proxy/pull/3072) feat: support for multiple github orgs #3072 (@daniel-mersch)
 - [#3116](https://github.com/oauth2-proxy/oauth2-proxy/pull/3116) feat: bump to go1.24.5 and full dependency update (@wardviaene / @dolmen)
 - [#3097](https://github.com/oauth2-proxy/oauth2-proxy/pull/3097) chore(deps): update alpine base image to v3.22.0
+- [#3101](https://github.com/oauth2-proxy/oauth2-proxy/pull/3101) fix: return error for empty Redis URL list (@dgivens)
 
 # V7.9.0
 

pkg/sessions/redis/redis_store.go

@@ -218,6 +218,10 @@ func setupTLSConfig(opts options.RedisStoreOptions, opt *redis.Options) error {
 // parseRedisURLs parses a list of redis urls and returns a list
 // of addresses in the form of host:port and redis.Options that can be used to connect to Redis
 func parseRedisURLs(urls []string) ([]string, *redis.Options, error) {
+	if len(urls) == 0 {
+		return nil, nil, fmt.Errorf("unable to parse redis urls: no redis urls provided")
+	}
+
 	addrs := []string{}
 	var redisOptions *redis.Options
 	for _, u := range urls {

pkg/sessions/redis/redis_store_test.go

@@ -19,116 +19,41 @@ const (
 )
 
 var _ = Describe("Redis SessionStore Tests", func() {
-	// helper interface to allow us to close client connections
-	// All non-nil redis clients should implement this
-	type closer interface {
-		Close() error
-	}
-
-	var mr *miniredis.Miniredis
-	var ss sessionsapi.SessionStore
-
-	BeforeEach(func() {
-		var err error
-		mr, err = miniredis.Run()
-		Expect(err).ToNot(HaveOccurred())
-	})
-
-	AfterEach(func() {
-		mr.Close()
-	})
-
-	JustAfterEach(func() {
-		// Release any connections immediately after the test ends
-		if redisManager, ok := ss.(*persistence.Manager); ok {
-			if redisManager.Store.(*SessionStore).Client != nil {
-				Expect(redisManager.Store.(*SessionStore).Client.(closer).Close()).To(Succeed())
-			}
+	Describe("Redis SessionStore Creation", func() {
+		// helper interface to allow us to close client connections
+		// All non-nil redis clients should implement this
+		type closer interface {
+			Close() error
 		}
-	})
 
-	const redisProtocol = "redis://"
-	tests.RunSessionStoreTests(
-		func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
-			// Set the connection URL
-			opts.Type = options.RedisSessionStoreType
-			opts.Redis.ConnectionURL = redisProtocol + mr.Addr()
+		var mr *miniredis.Miniredis
+		var ss sessionsapi.SessionStore
 
-			// Capture the session store so that we can close the client
+		BeforeEach(func() {
 			var err error
-			ss, err = NewRedisSessionStore(opts, cookieOpts)
-			return ss, err
-		},
-		func(d time.Duration) error {
-			mr.FastForward(d)
-			return nil
-		},
-	)
-
-	Context("with sentinel", func() {
-		var ms *minisentinel.Sentinel
-
-		BeforeEach(func() {
-			ms = minisentinel.NewSentinel(mr)
-			Expect(ms.Start()).To(Succeed())
-		})
-
-		tests.RunSessionStoreTests(
-			func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
-				// Set the sentinel connection URL
-				sentinelAddr := redisProtocol + ms.Addr()
-				opts.Type = options.RedisSessionStoreType
-				opts.Redis.SentinelConnectionURLs = []string{sentinelAddr}
-				opts.Redis.UseSentinel = true
-				opts.Redis.SentinelMasterName = ms.MasterInfo().Name
-
-				// Capture the session store so that we can close the client
-				var err error
-				ss, err = NewRedisSessionStore(opts, cookieOpts)
-				return ss, err
-			},
-			func(d time.Duration) error {
-				mr.FastForward(d)
-				return nil
-			},
-		)
-	})
-
-	Context("with cluster", func() {
-		tests.RunSessionStoreTests(
-			func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
-				clusterAddr := redisProtocol + mr.Addr()
-				opts.Type = options.RedisSessionStoreType
-				opts.Redis.ClusterConnectionURLs = []string{clusterAddr}
-				opts.Redis.UseCluster = true
-
-				// Capture the session store so that we can close the client
-				var err error
-				ss, err = NewRedisSessionStore(opts, cookieOpts)
-				return ss, err
-			},
-			func(d time.Duration) error {
-				mr.FastForward(d)
-				return nil
-			},
-		)
-	})
-
-	Context("with a redis password", func() {
-		BeforeEach(func() {
-			mr.RequireAuth(redisPassword)
+			mr, err = miniredis.Run()
+			Expect(err).ToNot(HaveOccurred())
 		})
 
 		AfterEach(func() {
-			mr.RequireAuth("")
+			mr.Close()
 		})
 
+		JustAfterEach(func() {
+			// Release any connections immediately after the test ends
+			if redisManager, ok := ss.(*persistence.Manager); ok {
+				if redisManager.Store.(*SessionStore).Client != nil {
+					Expect(redisManager.Store.(*SessionStore).Client.(closer).Close()).To(Succeed())
+				}
+			}
+		})
+
+		const redisProtocol = "redis://"
 		tests.RunSessionStoreTests(
 			func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
 				// Set the connection URL
 				opts.Type = options.RedisSessionStoreType
 				opts.Redis.ConnectionURL = redisProtocol + mr.Addr()
-				opts.Redis.Password = redisPassword
 
 				// Capture the session store so that we can close the client
 				var err error
@@ -157,7 +82,6 @@ var _ = Describe("Redis SessionStore Tests", func() {
 					opts.Redis.SentinelConnectionURLs = []string{sentinelAddr}
 					opts.Redis.UseSentinel = true
 					opts.Redis.SentinelMasterName = ms.MasterInfo().Name
-					opts.Redis.Password = redisPassword
 
 					// Capture the session store so that we can close the client
 					var err error
@@ -178,6 +102,33 @@ var _ = Describe("Redis SessionStore Tests", func() {
 					opts.Type = options.RedisSessionStoreType
 					opts.Redis.ClusterConnectionURLs = []string{clusterAddr}
 					opts.Redis.UseCluster = true
+
+					// Capture the session store so that we can close the client
+					var err error
+					ss, err = NewRedisSessionStore(opts, cookieOpts)
+					return ss, err
+				},
+				func(d time.Duration) error {
+					mr.FastForward(d)
+					return nil
+				},
+			)
+		})
+
+		Context("with a redis password", func() {
+			BeforeEach(func() {
+				mr.RequireAuth(redisPassword)
+			})
+
+			AfterEach(func() {
+				mr.RequireAuth("")
+			})
+
+			tests.RunSessionStoreTests(
+				func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
+					// Set the connection URL
+					opts.Type = options.RedisSessionStoreType
+					opts.Redis.ConnectionURL = redisProtocol + mr.Addr()
 					opts.Redis.Password = redisPassword
 
 					// Capture the session store so that we can close the client
@@ -190,56 +141,134 @@ var _ = Describe("Redis SessionStore Tests", func() {
 					return nil
 				},
 			)
+
+			Context("with sentinel", func() {
+				var ms *minisentinel.Sentinel
+
+				BeforeEach(func() {
+					ms = minisentinel.NewSentinel(mr)
+					Expect(ms.Start()).To(Succeed())
+				})
+
+				tests.RunSessionStoreTests(
+					func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
+						// Set the sentinel connection URL
+						sentinelAddr := redisProtocol + ms.Addr()
+						opts.Type = options.RedisSessionStoreType
+						opts.Redis.SentinelConnectionURLs = []string{sentinelAddr}
+						opts.Redis.UseSentinel = true
+						opts.Redis.SentinelMasterName = ms.MasterInfo().Name
+						opts.Redis.Password = redisPassword
+
+						// Capture the session store so that we can close the client
+						var err error
+						ss, err = NewRedisSessionStore(opts, cookieOpts)
+						return ss, err
+					},
+					func(d time.Duration) error {
+						mr.FastForward(d)
+						return nil
+					},
+				)
+			})
+
+			Context("with cluster", func() {
+				tests.RunSessionStoreTests(
+					func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
+						clusterAddr := redisProtocol + mr.Addr()
+						opts.Type = options.RedisSessionStoreType
+						opts.Redis.ClusterConnectionURLs = []string{clusterAddr}
+						opts.Redis.UseCluster = true
+						opts.Redis.Password = redisPassword
+
+						// Capture the session store so that we can close the client
+						var err error
+						ss, err = NewRedisSessionStore(opts, cookieOpts)
+						return ss, err
+					},
+					func(d time.Duration) error {
+						mr.FastForward(d)
+						return nil
+					},
+				)
+			})
+		})
+
+		Context("with a redis username and password", func() {
+			BeforeEach(func() {
+				mr.RequireUserAuth(redisUsername, redisPassword)
+			})
+
+			AfterEach(func() {
+				mr.RequireUserAuth("", "")
+			})
+
+			tests.RunSessionStoreTests(
+				func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
+					// Set the connection URL
+					opts.Type = options.RedisSessionStoreType
+					opts.Redis.ConnectionURL = "redis://" + redisUsername + "@" + mr.Addr()
+					opts.Redis.Password = redisPassword
+
+					// Capture the session store so that we can close the client
+					var err error
+					ss, err = NewRedisSessionStore(opts, cookieOpts)
+					return ss, err
+				},
+				func(d time.Duration) error {
+					mr.FastForward(d)
+					return nil
+				},
+			)
+
+			Context("with cluster", func() {
+				tests.RunSessionStoreTests(
+					func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
+						clusterAddr := "redis://" + redisUsername + "@" + mr.Addr()
+						opts.Type = options.RedisSessionStoreType
+						opts.Redis.ClusterConnectionURLs = []string{clusterAddr}
+						opts.Redis.UseCluster = true
+						opts.Redis.Username = redisUsername
+						opts.Redis.Password = redisPassword
+
+						// Capture the session store so that we can close the client
+						var err error
+						ss, err = NewRedisSessionStore(opts, cookieOpts)
+						return ss, err
+					},
+					func(d time.Duration) error {
+						mr.FastForward(d)
+						return nil
+					},
+				)
+			})
 		})
 	})
 
-	Context("with a redis username and password", func() {
-		BeforeEach(func() {
-			mr.RequireUserAuth(redisUsername, redisPassword)
+	Describe("Redis URL Parsing", func() {
+		It("should parse valid redis URL", func() {
+			addrs, opts, err := parseRedisURLs([]string{"redis://localhost:6379"})
+			Expect(err).ToNot(HaveOccurred())
+			Expect(addrs).To(Equal([]string{"localhost:6379"}))
+			Expect(opts).ToNot(BeNil())
+			Expect(opts.Addr).To(Equal("localhost:6379"))
 		})
 
-		AfterEach(func() {
-			mr.RequireUserAuth("", "")
+		It("should return error for invalid redis URL", func() {
+			addrs, opts, err := parseRedisURLs([]string{"invalid://url"})
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("unable to parse redis url"))
+			Expect(err.Error()).To(Not(ContainSubstring("no redis urls provided")))
+			Expect(addrs).To(BeNil())
+			Expect(opts).To(BeNil())
 		})
 
-		tests.RunSessionStoreTests(
-			func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
-				// Set the connection URL
-				opts.Type = options.RedisSessionStoreType
-				opts.Redis.ConnectionURL = "redis://" + redisUsername + "@" + mr.Addr()
-				opts.Redis.Password = redisPassword
-
-				// Capture the session store so that we can close the client
-				var err error
-				ss, err = NewRedisSessionStore(opts, cookieOpts)
-				return ss, err
-			},
-			func(d time.Duration) error {
-				mr.FastForward(d)
-				return nil
-			},
-		)
-
-		Context("with cluster", func() {
-			tests.RunSessionStoreTests(
-				func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
-					clusterAddr := "redis://" + redisUsername + "@" + mr.Addr()
-					opts.Type = options.RedisSessionStoreType
-					opts.Redis.ClusterConnectionURLs = []string{clusterAddr}
-					opts.Redis.UseCluster = true
-					opts.Redis.Username = redisUsername
-					opts.Redis.Password = redisPassword
-
-					// Capture the session store so that we can close the client
-					var err error
-					ss, err = NewRedisSessionStore(opts, cookieOpts)
-					return ss, err
-				},
-				func(d time.Duration) error {
-					mr.FastForward(d)
-					return nil
-				},
-			)
+		It("should return error when no URLs provided", func() {
+			addrs, opts, err := parseRedisURLs([]string{})
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(Equal("unable to parse redis urls: no redis urls provided"))
+			Expect(addrs).To(BeNil())
+			Expect(opts).To(BeNil())
 		})
 	})
 })