From 8e6955efd5563d5c0fe52e1e3e40c0953a55f9f8 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 17 Aug 2016 16:01:29 +0100 Subject: [PATCH 1/2] Base64 encode the service ID in the webhook path We don't put any restrictions on the format of the Service ID so we need to base64 it to prevent potentially losing data when it is inserted as a path parameter (e.g. due to weird escaping rules). Also, only remove webhooks if there was an old service to begin with. --- src/github.com/matrix-org/go-neb/api.go | 16 ++++++++++-- .../go-neb/services/github/github.go | 26 ++++++++++--------- .../matrix-org/go-neb/types/types.go | 5 +++- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/github.com/matrix-org/go-neb/api.go b/src/github.com/matrix-org/go-neb/api.go index 55a9599..c8356a0 100644 --- a/src/github.com/matrix-org/go-neb/api.go +++ b/src/github.com/matrix-org/go-neb/api.go @@ -2,6 +2,7 @@ package main import ( "database/sql" + "encoding/base64" "encoding/json" log "github.com/Sirupsen/logrus" "github.com/matrix-org/go-neb/clients" @@ -126,8 +127,19 @@ type webhookHandler struct { func (wh *webhookHandler) handle(w http.ResponseWriter, req *http.Request) { segments := strings.Split(req.URL.Path, "/") - // last path segment is the service ID which we will pass the incoming request to - srvID := segments[len(segments)-1] + // last path segment is the service ID which we will pass the incoming request to, + // but we've base64d it. + base64srvID := segments[len(segments)-1] + bytesSrvID, err := base64.StdEncoding.DecodeString(base64srvID) + srvID := string(bytesSrvID) + if err != nil { + log.WithError(err).WithField("base64_service_id", base64srvID).Print( + "Not a b64 encoded string", + ) + w.WriteHeader(400) + return + } + service, err := wh.db.LoadService(srvID) if err != nil { log.WithError(err).WithField("service_id", srvID).Print("Failed to load service") diff --git a/src/github.com/matrix-org/go-neb/services/github/github.go b/src/github.com/matrix-org/go-neb/services/github/github.go index 044aafa..893e869 100644 --- a/src/github.com/matrix-org/go-neb/services/github/github.go +++ b/src/github.com/matrix-org/go-neb/services/github/github.go @@ -221,20 +221,22 @@ func (s *githubService) PostRegister(oldService types.Service) { log.Errorf("PostRegister: %s does not have a github session", s.ClientUserID) return } - old, ok := oldService.(*githubService) - if !ok { - log.Error("PostRegister: Provided old service is not of type GithubService") - return - } + if oldService != nil { + old, ok := oldService.(*githubService) + if !ok { + log.Error("PostRegister: Provided old service is not of type GithubService") + return + } - // TODO: We should be adding webhooks in Register() then removing old hooks in PostRegister() - // - // By doing both operations in PostRegister(), if some of the requests fail we can end up in - // an inconsistent state. It is a lot simpler and easy to reason about this way though, so - // for now it will do. + // TODO: We should be adding webhooks in Register() then removing old hooks in PostRegister() + // + // By doing both operations in PostRegister(), if some of the requests fail we can end up in + // an inconsistent state. It is a lot simpler and easy to reason about this way though, so + // for now it will do. - // remove any existing webhooks this service created on the user's behalf - modifyWebhooks(old, cli, true) + // remove any existing webhooks this service created on the user's behalf + modifyWebhooks(old, cli, true) + } // make new webhooks according to service config modifyWebhooks(s, cli, false) diff --git a/src/github.com/matrix-org/go-neb/types/types.go b/src/github.com/matrix-org/go-neb/types/types.go index 9b7ef28..e8b642a 100644 --- a/src/github.com/matrix-org/go-neb/types/types.go +++ b/src/github.com/matrix-org/go-neb/types/types.go @@ -1,6 +1,7 @@ package types import ( + "encoding/base64" "encoding/json" "errors" "github.com/matrix-org/go-neb/matrix" @@ -72,7 +73,9 @@ func CreateService(serviceID, serviceType string, serviceJSON []byte) (Service, if f == nil { return nil, errors.New("Unknown service type: " + serviceType) } - webhookEndpointURL := baseURL + "services/hooks/" + serviceID + + base64ServiceID := base64.StdEncoding.EncodeToString([]byte(serviceID)) + webhookEndpointURL := baseURL + "services/hooks/" + base64ServiceID service := f(serviceID, webhookEndpointURL) if err := json.Unmarshal(serviceJSON, service); err != nil { return nil, err From 24a2ffc6bb804a6c556ba19da9661190c8abd465 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 17 Aug 2016 16:13:16 +0100 Subject: [PATCH 2/2] Also apply base64 encoding to realm redirect paths and use URL-safe base64 --- src/github.com/matrix-org/go-neb/api.go | 16 +++++++++++++--- src/github.com/matrix-org/go-neb/types/types.go | 5 +++-- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/github.com/matrix-org/go-neb/api.go b/src/github.com/matrix-org/go-neb/api.go index c8356a0..ab75ee3 100644 --- a/src/github.com/matrix-org/go-neb/api.go +++ b/src/github.com/matrix-org/go-neb/api.go @@ -63,8 +63,18 @@ type realmRedirectHandler struct { func (rh *realmRedirectHandler) handle(w http.ResponseWriter, req *http.Request) { segments := strings.Split(req.URL.Path, "/") - // last path segment is the realm ID which we will pass the incoming request to - realmID := segments[len(segments)-1] + // last path segment is the base64d realm ID which we will pass the incoming request to + base64realmID := segments[len(segments)-1] + bytesRealmID, err := base64.RawURLEncoding.DecodeString(base64realmID) + realmID := string(bytesRealmID) + if err != nil { + log.WithError(err).WithField("base64_realm_id", base64realmID).Print( + "Not a b64 encoded string", + ) + w.WriteHeader(400) + return + } + realm, err := rh.db.LoadAuthRealm(realmID) if err != nil { log.WithError(err).WithField("realm_id", realmID).Print("Failed to load realm") @@ -130,7 +140,7 @@ func (wh *webhookHandler) handle(w http.ResponseWriter, req *http.Request) { // last path segment is the service ID which we will pass the incoming request to, // but we've base64d it. base64srvID := segments[len(segments)-1] - bytesSrvID, err := base64.StdEncoding.DecodeString(base64srvID) + bytesSrvID, err := base64.RawURLEncoding.DecodeString(base64srvID) srvID := string(bytesSrvID) if err != nil { log.WithError(err).WithField("base64_service_id", base64srvID).Print( diff --git a/src/github.com/matrix-org/go-neb/types/types.go b/src/github.com/matrix-org/go-neb/types/types.go index e8b642a..d21db19 100644 --- a/src/github.com/matrix-org/go-neb/types/types.go +++ b/src/github.com/matrix-org/go-neb/types/types.go @@ -74,7 +74,7 @@ func CreateService(serviceID, serviceType string, serviceJSON []byte) (Service, return nil, errors.New("Unknown service type: " + serviceType) } - base64ServiceID := base64.StdEncoding.EncodeToString([]byte(serviceID)) + base64ServiceID := base64.RawURLEncoding.EncodeToString([]byte(serviceID)) webhookEndpointURL := baseURL + "services/hooks/" + base64ServiceID service := f(serviceID, webhookEndpointURL) if err := json.Unmarshal(serviceJSON, service); err != nil { @@ -109,7 +109,8 @@ func CreateAuthRealm(realmID, realmType string, realmJSON []byte) (AuthRealm, er if f == nil { return nil, errors.New("Unknown realm type: " + realmType) } - redirectURL := baseURL + "realms/redirects/" + realmID + base64RealmID := base64.RawURLEncoding.EncodeToString([]byte(realmID)) + redirectURL := baseURL + "realms/redirects/" + base64RealmID r := f(realmID, redirectURL) if err := json.Unmarshal(realmJSON, r); err != nil { return nil, err