Merge pull request #2290 from bobcallaway/issue2289

correctly handle path escaping for connector IDs
This commit is contained in:
Maksim Nabokikh 2022-05-31 16:03:12 +04:00 committed by GitHub
commit b6cc099305
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 35 additions and 9 deletions

View file

@ -6,6 +6,7 @@ import (
"encoding/base64" "encoding/base64"
"encoding/json" "encoding/json"
"fmt" "fmt"
"html/template"
"net/http" "net/http"
"net/url" "net/url"
"path" "path"
@ -154,7 +155,7 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) {
if connectorID != "" { if connectorID != "" {
for _, c := range connectors { for _, c := range connectors {
if c.ID == connectorID { 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) http.Redirect(w, r, connURL.String(), http.StatusFound)
return return
} }
@ -164,18 +165,18 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) {
} }
if len(connectors) == 1 && !s.alwaysShowLogin { 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) http.Redirect(w, r, connURL.String(), http.StatusFound)
} }
connectorInfos := make([]connectorInfo, len(connectors)) connectorInfos := make([]connectorInfo, len(connectors))
for index, conn := range 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{ connectorInfos[index] = connectorInfo{
ID: conn.ID, ID: conn.ID,
Name: conn.Name, Name: conn.Name,
Type: conn.Type, Type: conn.Type,
URL: connURL.String(), URL: template.URL(connURL.String()),
} }
} }
@ -201,7 +202,13 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) {
return 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) conn, err := s.getConnector(connID)
if err != nil { if err != nil {
s.logger.Errorf("Failed to get connector: %v", err) s.logger.Errorf("Failed to get connector: %v", err)
@ -317,7 +324,12 @@ func (s *Server) handlePasswordLogin(w http.ResponseWriter, r *http.Request) {
return 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.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.") s.renderError(r, w, http.StatusInternalServerError, "Requested resource does not exist.")
return return
@ -402,7 +414,12 @@ func (s *Server) handleConnectorCallback(w http.ResponseWriter, r *http.Request)
return 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.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.") s.renderError(r, w, http.StatusInternalServerError, "Requested resource does not exist.")
return return

View file

@ -254,6 +254,15 @@ func mockConnectorDataTestStorage(t *testing.T, s storage.Storage) {
err = s.CreateConnector(c1) err = s.CreateConnector(c1)
require.NoError(t, err) 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) { func TestPasswordConnectorDataNotEmpty(t *testing.T) {

View file

@ -320,7 +320,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) { handle := func(p string, h http.Handler) {
r.Handle(path.Join(issuerURL.Path, p), instrumentHandlerCounter(p, h)) r.Handle(path.Join(issuerURL.Path, p), instrumentHandlerCounter(p, h))
} }

View file

@ -246,7 +246,7 @@ var scopeDescriptions = map[string]string{
type connectorInfo struct { type connectorInfo struct {
ID string ID string
Name string Name string
URL string URL template.URL
Type string Type string
} }