diff --git a/CHANGELOG.md b/CHANGELOG.md index fc1c2e4a..6bc7139b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,9 @@ - Added `RealtimeConnectRequestEvent.MaxTimeout` field to specify the absolute max duration a realtime connection can remain open (default to 30mins). _This is in addition to the `IdeTimeout` of 5mins in order to prevent misuse and to allow the GC to run more regularly._ -- (@todo) Updated all `golang.org/x/` packages containing the [recent security fixes](https://groups.google.com/g/golang-announce/c/PdiGK3xulk4). +- Added extra checks for the connected user IP in the realtime APIs to prevent bruteforce guest subscription update attempts and to serve as an extra XSRF protection for the "all-in-one" OAuth2 realtime handler. + +- (@todo) Updated all `golang.org/x/` packages containing several [security fixes](https://groups.google.com/g/golang-announce/c/PdiGK3xulk4). ## v0.38.1 diff --git a/apis/realtime.go b/apis/realtime.go index c490c1c3..1cfe83d9 100644 --- a/apis/realtime.go +++ b/apis/realtime.go @@ -28,6 +28,9 @@ const clientsChunkSize = 150 // RealtimeClientAuthKey is the name of the realtime client store key that holds its auth state. const RealtimeClientAuthKey = "auth" +// RealtimeClientIPKey is the name of the realtime client store key that holds the IP of the connected client. +const RealtimeClientIPKey = "pbRealtimeClientIP" + // bindRealtimeApi registers the realtime api endpoints. func bindRealtimeApi(app core.App, rg *router.RouterGroup[*core.RequestEvent]) { sub := rg.Group("/realtime") @@ -63,9 +66,12 @@ func realtimeConnect(e *core.RequestEvent) error { connectEvent := new(core.RealtimeConnectRequestEvent) connectEvent.RequestEvent = e - connectEvent.Client = subscriptions.NewDefaultClient() connectEvent.IdleTimeout = 5 * time.Minute connectEvent.MaxTimeout = 30 * time.Minute + connectEvent.Client = subscriptions.NewDefaultClient() + + // could be used as an optional cross-reference check in other API endpoints + connectEvent.Client.Set(RealtimeClientIPKey, e.RealIP()) return e.App.OnRealtimeConnectRequest().Trigger(connectEvent, func(ce *core.RealtimeConnectRequestEvent) error { // register new subscription client @@ -74,7 +80,7 @@ func realtimeConnect(e *core.RequestEvent) error { e.App.SubscriptionsBroker().Unregister(ce.Client.Id()) }() - ce.App.Logger().Debug("Realtime connection established.", slog.String("clientId", ce.Client.Id())) + ce.App.Logger().Debug("Realtime connection established", slog.String("clientId", ce.Client.Id())) // signalize established connection (aka. fire "connect" message) connectMsgEvent := new(core.RealtimeMessageEvent) @@ -194,6 +200,21 @@ func realtimeSetSubscriptions(e *core.RequestEvent) error { return e.NotFoundError("Missing or invalid client id.", err) } + // for just in case to prevent someone changing a guest subscription + // + // note1: this is an extra precaution against clientId bruteforce attempts + // for installations allowing longer realtime connections duration + // + // note2: custom registered clients (aka. those without IP in the store) + // are excluded from the check for backward compatibility + clientIP, _ := client.Get(RealtimeClientIPKey).(string) + if clientIP != "" && clientIP != e.RealIP() { + return e.BadRequestError( + "Invalid realtime client.", + errors.New("the subscription request IP doesn't match with the realtime client IP"), + ) + } + // for now allow only guest->auth upgrades and any other auth change is forbidden clientAuth, _ := client.Get(RealtimeClientAuthKey).(*core.Record) if clientAuth != nil && !isSameAuth(clientAuth, e.Auth) { @@ -216,7 +237,7 @@ func realtimeSetSubscriptions(e *core.RequestEvent) error { e.Client.Subscribe(e.Subscriptions...) e.App.Logger().Debug( - "Realtime subscriptions updated.", + "Realtime subscriptions updated", slog.String("clientId", e.Client.Id()), slog.Any("subscriptions", e.Subscriptions), ) diff --git a/apis/realtime_test.go b/apis/realtime_test.go index f6e45431..6001ac85 100644 --- a/apis/realtime_test.go +++ b/apis/realtime_test.go @@ -26,6 +26,7 @@ func TestRealtimeConnect(t *testing.T) { Method: http.MethodGet, URL: "/api/realtime", Timeout: 100 * time.Millisecond, + Headers: map[string]string{"x-test-ip": "127.0.0.2"}, ExpectedStatus: 200, ExpectedContent: []string{ `id:`, @@ -37,6 +38,17 @@ func TestRealtimeConnect(t *testing.T) { "OnRealtimeConnectRequest": 1, "OnRealtimeMessageSend": 1, }, + BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) { + app.Settings().TrustedProxy.Headers = []string{"x-test-ip"} + + app.OnRealtimeConnectRequest().BindFunc(func(e *core.RealtimeConnectRequestEvent) error { + if ip, _ := e.Client.Get(apis.RealtimeClientIPKey).(string); ip != "127.0.0.2" { + t.Fatalf("Expected IP %q, got %q", "127.0.0.2", ip) + } + + return e.Next() + }) + }, AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { if len(app.SubscriptionsBroker().Clients()) != 0 { t.Errorf("Expected the subscribers to be removed after connection close, found %d", len(app.SubscriptionsBroker().Clients())) @@ -102,7 +114,8 @@ func TestRealtimeSubscribe(t *testing.T) { resetClient := func() { client.Unsubscribe() - client.Set(apis.RealtimeClientAuthKey, nil) + client.Unset(apis.RealtimeClientAuthKey) + client.Unset(apis.RealtimeClientIPKey) } validSubscriptionsLimit := make([]string, 1000) @@ -208,6 +221,26 @@ func TestRealtimeSubscribe(t *testing.T) { }, ExpectedEvents: map[string]int{"*": 0}, }, + { + Name: "existing client with different IP", + Method: http.MethodPost, + URL: "/api/realtime", + Body: strings.NewReader(`{"clientId":"` + client.Id() + `","subscriptions":["test"]}`), + Headers: map[string]string{"x-test-ip": "127.0.0.2"}, + ExpectedStatus: 400, + ExpectedContent: []string{`"data":{}`}, + ExpectedEvents: map[string]int{"*": 0}, + BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) { + app.Settings().TrustedProxy.Headers = []string{"x-test-ip"} + + client.Set(apis.RealtimeClientIPKey, "127.0.0.1") + + app.SubscriptionsBroker().Register(client) + }, + AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { + resetClient() + }, + }, { Name: "existing client with valid topic length", Method: http.MethodPost, diff --git a/apis/record_auth_with_oauth2_redirect.go b/apis/record_auth_with_oauth2_redirect.go index c2c73c7d..91f5d9d3 100644 --- a/apis/record_auth_with_oauth2_redirect.go +++ b/apis/record_auth_with_oauth2_redirect.go @@ -55,6 +55,16 @@ func oauth2SubscriptionRedirect(e *core.RequestEvent) error { } defer client.Unsubscribe(oauth2SubscriptionTopic) + // additional check to minimize the risk of XSRF attack vectors + // + // note: custom registered clients (aka. those without IP in the store) + // are excluded from the check for backward compatibility + clientIP, _ := client.Get(RealtimeClientIPKey).(string) + if clientIP != "" && clientIP != e.RealIP() { + e.App.Logger().Debug("The client IP that completed the authentication is different from the one that initialized the OAuth2 realtime connection") + return failureRedirect(e) + } + // temporary store the Apple user's name so that it can be later retrieved with the authWithOAuth2 call // (see https://github.com/pocketbase/pocketbase/issues/7090) if data.AppleUser != "" && data.Error == "" && data.Code != "" { diff --git a/apis/record_auth_with_oauth2_redirect_test.go b/apis/record_auth_with_oauth2_redirect_test.go index fbf7ac32..90632533 100644 --- a/apis/record_auth_with_oauth2_redirect_test.go +++ b/apis/record_auth_with_oauth2_redirect_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/pocketbase/pocketbase/apis" "github.com/pocketbase/pocketbase/core" "github.com/pocketbase/pocketbase/tests" "github.com/pocketbase/pocketbase/tools/subscriptions" @@ -16,9 +17,9 @@ import ( func TestRecordAuthWithOAuth2Redirect(t *testing.T) { t.Parallel() - clientStubs := make([]map[string]subscriptions.Client, 0, 10) + clientStubs := make([]map[string]subscriptions.Client, 0, 11) - for i := 0; i < 10; i++ { + for i := 0; i < 11; i++ { c1 := subscriptions.NewDefaultClient() c2 := subscriptions.NewDefaultClient() @@ -335,6 +336,28 @@ func TestRecordAuthWithOAuth2Redirect(t *testing.T) { } }, }, + { + Name: "client with different IP", + Method: http.MethodGet, + URL: "/api/oauth2-redirect?code=123&state=" + clientStubs[10]["c3"].Id(), + Headers: map[string]string{"x-test-ip": "127.0.0.2"}, + BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) { + app.Settings().TrustedProxy.Headers = []string{"x-test-ip"} + + clientStubs[10]["c3"].Set(apis.RealtimeClientIPKey, "127.0.0.1") + + beforeTestFunc(clientStubs[10], map[string][]string{ + "c3": {`"state":"` + clientStubs[10]["c3"].Id(), `"code":"123"`}, + })(t, app, e) + }, + ExpectedStatus: http.StatusTemporaryRedirect, + ExpectedEvents: map[string]int{"*": 0}, + AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { + app.Store().Get("cancelFunc").(context.CancelFunc)() + + checkFailureRedirect(t, app, res) + }, + }, } for _, scenario := range scenarios {