From 92d6180cf8e3ed40665f9c1dccaea78cffc343f7 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 6 Jan 2025 15:02:20 +0100 Subject: [PATCH 1/2] fix: stricter JSON patch checking for PATCH identities --- identity/handler.go | 14 +++++++++++ identity/handler_test.go | 51 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/identity/handler.go b/identity/handler.go index 6d458636854d..665eecdffd61 100644 --- a/identity/handler.go +++ b/identity/handler.go @@ -12,6 +12,7 @@ import ( "time" "github.com/gofrs/uuid" + "github.com/tidwall/gjson" "github.com/ory/x/crdbx" "github.com/ory/x/pagination/keysetpagination" @@ -916,6 +917,19 @@ func (h *Handler) patch(w http.ResponseWriter, r *http.Request, ps httprouter.Pa patchedIdentity := WithAdminMetadataInJSON(*identity) + // We need to check for all sub-paths of /credentials, since jsonx.ApplyJSONPatch + // only checks full paths. + for _, path := range gjson.GetBytes(requestBody, "#.path").Array() { + if strings.HasPrefix(path.String(), "/credentials/") { + h.r.Writer().WriteError(w, r, errors.WithStack( + herodot. + ErrBadRequest. + WithReasonf("An error occured when applying the JSON patch"). + WithErrorf("patch includes denied sub-path of /credentials: %s", path.String()))) + return + } + } + if err := jsonx.ApplyJSONPatch(requestBody, &patchedIdentity, "/id", "/stateChangedAt", "/credentials"); err != nil { h.r.Writer().WriteError(w, r, errors.WithStack( herodot. diff --git a/identity/handler_test.go b/identity/handler_test.go index 66f4936961f3..4b1c8534ebd3 100644 --- a/identity/handler_test.go +++ b/identity/handler_test.go @@ -1203,6 +1203,57 @@ func TestHandler(t *testing.T) { } }) + t.Run("case=PATCH should fail if credential orgs are updated", func(t *testing.T) { + uuid := x.NewUUID().String() + email := uuid + "@ory.sh" + i := &identity.Identity{Traits: identity.Traits(`{"email":"` + email + `"}`)} + i.SetCredentials(identity.CredentialsTypeOIDC, identity.Credentials{ + Type: identity.CredentialsTypeOIDC, + Identifiers: []string{email}, + Config: sqlxx.JSONRawMessage(`{"providers": [{"provider": "some-provider"}]}`), + }) + require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i)) + + for name, ts := range map[string]*httptest.Server{"public": publicTS, "admin": adminTS} { + t.Run("endpoint="+name, func(t *testing.T) { + patch := []patch{ + {"op": "replace", "path": "/credentials/oidc/config/providers/0/organization", "value": "foo"}, + } + + res := send(t, ts, "PATCH", "/identities/"+i.ID.String(), http.StatusBadRequest, &patch) + + assert.EqualValues(t, "patch includes denied sub-path of /credentials: /credentials/oidc/config/providers/0/organization", res.Get("error.message").String(), "%s", res.Raw) + }) + } + }) + + t.Run("case=PATCH should fail to update credential password", func(t *testing.T) { + uuid := x.NewUUID().String() + email := uuid + "@ory.sh" + password := "ljanf123akf" + p, err := reg.Hasher(ctx).Generate(context.Background(), []byte(password)) + require.NoError(t, err) + i := &identity.Identity{Traits: identity.Traits(`{"email":"` + email + `"}`)} + i.SetCredentials(identity.CredentialsTypePassword, identity.Credentials{ + Type: identity.CredentialsTypePassword, + Identifiers: []string{email}, + Config: sqlxx.JSONRawMessage(`{"hashed_password":"` + string(p) + `"}`), + }) + require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i)) + + for name, ts := range map[string]*httptest.Server{"public": publicTS, "admin": adminTS} { + t.Run("endpoint="+name, func(t *testing.T) { + patch := []patch{ + {"op": "replace", "path": "/credentials/password/config/hashed_password", "value": "foo"}, + } + + res := send(t, ts, "PATCH", "/identities/"+i.ID.String(), http.StatusBadRequest, &patch) + + assert.EqualValues(t, "patch includes denied sub-path of /credentials: /credentials/password/config/hashed_password", res.Get("error.message").String(), "%s", res.Raw) + }) + } + }) + t.Run("case=PATCH should not invalidate credentials ory/cloud#148", func(t *testing.T) { // see https://github.com/ory/cloud/issues/148 From 4134830eba6c49cc6aa1b46d090f3b6b69d36f00 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 7 Jan 2025 11:58:09 +0100 Subject: [PATCH 2/2] code review --- go.mod | 2 +- go.sum | 2 ++ identity/handler.go | 16 +--------------- identity/handler_test.go | 4 ++-- 4 files changed, 6 insertions(+), 18 deletions(-) diff --git a/go.mod b/go.mod index deba6fbdf766..325a5d447573 100644 --- a/go.mod +++ b/go.mod @@ -76,7 +76,7 @@ require ( github.com/ory/jsonschema/v3 v3.0.8 github.com/ory/mail/v3 v3.0.0 github.com/ory/nosurf v1.2.7 - github.com/ory/x v0.0.675 + github.com/ory/x v0.0.689 github.com/peterhellberg/link v1.2.0 github.com/phayes/freeport v0.0.0-20220201140144-74d24b5ae9f5 github.com/pkg/errors v0.9.1 diff --git a/go.sum b/go.sum index 5f0133b6a64f..c6f36f82358f 100644 --- a/go.sum +++ b/go.sum @@ -642,6 +642,8 @@ github.com/ory/sessions v1.2.2-0.20220110165800-b09c17334dc2 h1:zm6sDvHy/U9XrGpi github.com/ory/sessions v1.2.2-0.20220110165800-b09c17334dc2/go.mod h1:dk2InVEVJ0sfLlnXv9EAgkf6ecYs/i80K/zI+bUmuGM= github.com/ory/x v0.0.675 h1:K6GpVo99BXBFv2UiwMjySNNNqCFKGswynrt7vWQJFU8= github.com/ory/x v0.0.675/go.mod h1:zJmnDtKje2FCP4EeFvRsKk94XXiqKCSGJMZcirAfhUs= +github.com/ory/x v0.0.689 h1:pMXmnw2aoHiq4jRX9xtGXqX+VU3USEwlUUbwNCxmiZQ= +github.com/ory/x v0.0.689/go.mod h1:UpPgjobuyIyHh1pG4LxqmfMpuNOnzf2BzwyouwBeCk4= github.com/pelletier/go-toml v1.9.5 h1:4yBQzkHv+7BHq2PQUZF3Mx0IYxG7LsP222s7Agd3ve8= github.com/pelletier/go-toml v1.9.5/go.mod h1:u1nR/EPcESfeI/szUZKdtJ0xRNbUoANCkoOuaOx1Y+c= github.com/pelletier/go-toml/v2 v2.2.2 h1:aYUidT7k73Pcl9nb2gScu7NSrKCSHIDE89b3+6Wq+LM= diff --git a/identity/handler.go b/identity/handler.go index 665eecdffd61..938344fb48fc 100644 --- a/identity/handler.go +++ b/identity/handler.go @@ -12,7 +12,6 @@ import ( "time" "github.com/gofrs/uuid" - "github.com/tidwall/gjson" "github.com/ory/x/crdbx" "github.com/ory/x/pagination/keysetpagination" @@ -917,20 +916,7 @@ func (h *Handler) patch(w http.ResponseWriter, r *http.Request, ps httprouter.Pa patchedIdentity := WithAdminMetadataInJSON(*identity) - // We need to check for all sub-paths of /credentials, since jsonx.ApplyJSONPatch - // only checks full paths. - for _, path := range gjson.GetBytes(requestBody, "#.path").Array() { - if strings.HasPrefix(path.String(), "/credentials/") { - h.r.Writer().WriteError(w, r, errors.WithStack( - herodot. - ErrBadRequest. - WithReasonf("An error occured when applying the JSON patch"). - WithErrorf("patch includes denied sub-path of /credentials: %s", path.String()))) - return - } - } - - if err := jsonx.ApplyJSONPatch(requestBody, &patchedIdentity, "/id", "/stateChangedAt", "/credentials"); err != nil { + if err := jsonx.ApplyJSONPatch(requestBody, &patchedIdentity, "/id", "/stateChangedAt", "/credentials", "/credentials/**"); err != nil { h.r.Writer().WriteError(w, r, errors.WithStack( herodot. ErrBadRequest. diff --git a/identity/handler_test.go b/identity/handler_test.go index 4b1c8534ebd3..2d002b9ee056 100644 --- a/identity/handler_test.go +++ b/identity/handler_test.go @@ -1222,7 +1222,7 @@ func TestHandler(t *testing.T) { res := send(t, ts, "PATCH", "/identities/"+i.ID.String(), http.StatusBadRequest, &patch) - assert.EqualValues(t, "patch includes denied sub-path of /credentials: /credentials/oidc/config/providers/0/organization", res.Get("error.message").String(), "%s", res.Raw) + assert.EqualValues(t, "patch includes denied path: /credentials/oidc/config/providers/0/organization", res.Get("error.message").String(), "%s", res.Raw) }) } }) @@ -1249,7 +1249,7 @@ func TestHandler(t *testing.T) { res := send(t, ts, "PATCH", "/identities/"+i.ID.String(), http.StatusBadRequest, &patch) - assert.EqualValues(t, "patch includes denied sub-path of /credentials: /credentials/password/config/hashed_password", res.Get("error.message").String(), "%s", res.Raw) + assert.EqualValues(t, "patch includes denied path: /credentials/password/config/hashed_password", res.Get("error.message").String(), "%s", res.Raw) }) } })