From 8fd69c16f5cb80f58e508bd788e55dc22db07f06 Mon Sep 17 00:00:00 2001 From: Bob Callaway Date: Fri, 1 Oct 2021 16:02:59 -0400 Subject: [PATCH 1/2] correctly handle path escaping for connector IDs Signed-off-by: Bob Callaway --- server/handlers.go | 28 ++++++++++++++++++++++------ server/handlers_test.go | 9 +++++++++ server/server.go | 2 +- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/server/handlers.go b/server/handlers.go index 2a4f8c71..bf7f8e07 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -153,7 +153,7 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) { if connectorID != "" { for _, c := range connectors { if c.ID == connectorID { - connURL.Path = s.absPath("/auth", c.ID) + connURL.Path = s.absPath("/auth", url.PathEscape(c.ID)) http.Redirect(w, r, connURL.String(), http.StatusFound) return } @@ -163,13 +163,13 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) { } if len(connectors) == 1 && !s.alwaysShowLogin { - connURL.Path = s.absPath("/auth", connectors[0].ID) + connURL.Path = s.absPath("/auth", url.PathEscape(connectors[0].ID)) http.Redirect(w, r, connURL.String(), http.StatusFound) } connectorInfos := make([]connectorInfo, len(connectors)) for index, conn := range connectors { - connURL.Path = s.absPath("/auth", conn.ID) + connURL.Path = s.absPath("/auth", url.PathEscape(conn.ID)) connectorInfos[index] = connectorInfo{ ID: conn.ID, Name: conn.Name, @@ -200,7 +200,13 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) { return } - connID := mux.Vars(r)["connector"] + connID, err := url.PathUnescape(mux.Vars(r)["connector"]) + if err != nil { + s.logger.Errorf("Failed to parse connector: %v", err) + s.renderError(r, w, http.StatusBadRequest, "Requested resource does not exist") + return + } + conn, err := s.getConnector(connID) if err != nil { s.logger.Errorf("Failed to get connector: %v", err) @@ -316,7 +322,12 @@ func (s *Server) handlePasswordLogin(w http.ResponseWriter, r *http.Request) { return } - if connID := mux.Vars(r)["connector"]; connID != "" && connID != authReq.ConnectorID { + connID, err := url.PathUnescape(mux.Vars(r)["connector"]) + if err != nil { + s.logger.Errorf("Failed to parse connector: %v", err) + s.renderError(r, w, http.StatusBadRequest, "Requested resource does not exist") + return + } else if connID != "" && connID != authReq.ConnectorID { s.logger.Errorf("Connector mismatch: authentication started with id %q, but password login for id %q was triggered", authReq.ConnectorID, connID) s.renderError(r, w, http.StatusInternalServerError, "Requested resource does not exist.") return @@ -401,7 +412,12 @@ func (s *Server) handleConnectorCallback(w http.ResponseWriter, r *http.Request) return } - if connID := mux.Vars(r)["connector"]; connID != "" && connID != authReq.ConnectorID { + connID, err := url.PathUnescape(mux.Vars(r)["connector"]) + if err != nil { + s.logger.Errorf("Failed to get connector with id %q : %v", authReq.ConnectorID, err) + s.renderError(r, w, http.StatusInternalServerError, "Requested resource does not exist.") + return + } else if connID != "" && connID != authReq.ConnectorID { s.logger.Errorf("Connector mismatch: authentication started with id %q, but callback for id %q was triggered", authReq.ConnectorID, connID) s.renderError(r, w, http.StatusInternalServerError, "Requested resource does not exist.") return diff --git a/server/handlers_test.go b/server/handlers_test.go index 61b3ece6..fb1a0506 100644 --- a/server/handlers_test.go +++ b/server/handlers_test.go @@ -254,6 +254,15 @@ func mockConnectorDataTestStorage(t *testing.T, s storage.Storage) { err = s.CreateConnector(c1) require.NoError(t, err) + + c2 := storage.Connector{ + ID: "http://any.valid.url/", + Type: "mock", + Name: "mockURLID", + } + + err = s.CreateConnector(c2) + require.NoError(t, err) } func TestPasswordConnectorDataNotEmpty(t *testing.T) { diff --git a/server/server.go b/server/server.go index 957b62dc..c9e3c9ee 100644 --- a/server/server.go +++ b/server/server.go @@ -302,7 +302,7 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy) } } - r := mux.NewRouter() + r := mux.NewRouter().SkipClean(true).UseEncodedPath() handle := func(p string, h http.Handler) { r.Handle(path.Join(issuerURL.Path, p), instrumentHandlerCounter(p, h)) } From 2e0041f95fc774f8129ebe008aacd83fc46b2dd9 Mon Sep 17 00:00:00 2001 From: Bob Callaway Date: Wed, 6 Oct 2021 10:16:55 -0400 Subject: [PATCH 2/2] ensure template does not double-escape URL Signed-off-by: Bob Callaway --- server/handlers.go | 3 ++- server/templates.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/server/handlers.go b/server/handlers.go index bf7f8e07..bb15d692 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "encoding/json" "fmt" + "html/template" "net/http" "net/url" "path" @@ -174,7 +175,7 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) { ID: conn.ID, Name: conn.Name, Type: conn.Type, - URL: connURL.String(), + URL: template.URL(connURL.String()), } } diff --git a/server/templates.go b/server/templates.go index e46855b1..0307437c 100644 --- a/server/templates.go +++ b/server/templates.go @@ -244,7 +244,7 @@ var scopeDescriptions = map[string]string{ type connectorInfo struct { ID string Name string - URL string + URL template.URL Type string }