ory Making PATCH operations more resilient: We’ve run into an issue with PATCH operations where when

metadata_public

does not exist and you wanted to add a

fieldA

to it, the whole operation would fail, instead of creating an object with the new

fieldA

. I’ve dug a bit into how the PATCH operations work and found the following dependencies: • API request calls

jsonx.ApplyJSONPatch

https://github.com/ory/kratos/blob/master/identity/handler.go#L779 • jsonx uses

json-patch

https://github.com/ory/x/blob/master/jsonx/patch.go#L50 One idea how to fix this would be to use

ApplyWithOptions

https://github.com/evanphx/json-patch/blob/master/v5/patch.go#L1058 with the option

EnsurePathExistsOnAdd

https://github.com/evanphx/json-patch/blob/master/v5/patch.go#L87 instead of just

patch.Apply()

// EnsurePathExistsOnAdd instructs json-patch to recursively create the missing parts of path on “add” operation.

// Default to false.

Would such that work for you, is there a (security) reason why that hasn’t been done before? For the use-case we ran into and this bug report mentions the current behavior encourages worse coding practices and may lead to bugs (more latency, two points of failure, prone to race conditions, etc.) — I wanted to ask here before to making the change to see if anything immediately speaks against the change in the

jsonx

dependency?

This sounds good to me. I don’t think there are security concerns here. This is mainly an oversight, IIRC. WDYT @steep-lamp-91158?

SGTM, I am just not sure whether we want to always do that or whether we want to forward the options

I went ahead and used the simplest approach for now. Meaning changing the behavior for all patch operations (instead of forwarding the options) https://github.com/ory/x/pull/692 @bland-eye-99092 the oversight might come from not having all features available, had to change the library/version/import 🤔 With the old json-patch package the

ApplyWithOptions

function was not available… I hope that’s fine 🥹

Ahh, I see. very nice! If the tests are green, this should be fine. Thanks for investigating! 🙂

Nice I’ve seen the PR was merged! What is the best way to bring this into Ory-network / Kratos? Should I open a PR to bump the x dependency?

yes exactly

I fixed on issue after dependency upgrades, however there are a couple of failures still that I couldn’t yet figure out the reason why they fail: https://github.com/ory/kratos/actions/runs/5193886885/jobs/9365002478 ✅ 1.) Failure to load certificate (Passed now)

=== RUN   TestServe
    e2e_server.go:98: Starting migrations...
Successfully applied SQL migrations!
    e2e_server.go:100: Migration done
    e2e_server.go:102: Starting server...
    e2e_server.go:170: {"status":"ok"}
    e2e_server.go:170: {"status":"ok"}
    e2e_server.go:170: {"status":"ok"}
    e2e_server.go:170: {"status":"ok"}
--- PASS: TestServe (2.42s)
=== RUN   TestServeTLSBase64
    e2e_server.go:217: wrote /tmp/TestServeTLSBase642370739528/001/test-1549471098-cert.pem
    e2e_server.go:235: wrote /tmp/TestServeTLSBase642370739528/001/test-445258613-key.pem
    e2e_server.go:98: Starting migrations...
Successfully applied SQL migrations!
    e2e_server.go:100: Migration done
    e2e_server.go:102: Starting server...
time=2023-06-06T22:26:00Z level=fatal msg=Unable to load HTTPS TLS Certificate audience=application error=map[message:unable to load X509 key pair: tls: failed to find any PEM data in key input] service_name=Ory Kratos service_version=master
FAIL	<http://github.com/ory/kratos/cmd/serve|github.com/ory/kratos/cmd/serve>	4.483s
FAIL
exit status 1make: *** [Makefile:86: test-coverage] Error 1

2.) Social Sign Up Successes (mysql)

for app express
      1) "before all" hook for "should be able to sign up with incomplete data and finally be signed in"


  8 passing (1m)
  1 failing

  1) Social Sign Up Successes
       for app express
         "before all" hook for "should be able to sign up with incomplete data and finally be signed in":
     Error: The following error originated from your application code, not from Cypress. It was caused by an unhandled promise rejection.

  > Request failed with status code 404

When Cypress detects uncaught errors originating from your application it will automatically fail the current test.

This behavior is configurable, and you can choose to turn this off by listening to the `uncaught:exception` event.

<https://on.cypress.io/uncaught-exception-from-application>

Because this error occurred during a `before all` hook we are skipping the remaining tests in the current suite: `for app express`

Although you have test retries enabled, we do not retry tests when `before all` or `after all` hooks fail
      at createError (webpack-internal:///./node_modules/@ory/client/node_modules/axios/lib/core/createError.js:16:15)
      at settle (webpack-internal:///./node_modules/@ory/client/node_modules/axios/lib/core/settle.js:17:12)
      at XMLHttpRequest.onloadend (webpack-internal:///./node_modules/@ory/client/node_modules/axios/lib/adapters/xhr.js:66:7)

For react it worked 🤔 Also for other backends postgres, sqlite it worked as well… 3.) vulnerable SSL version (see screenshot) — I’ve seen aeneas comment on my PR so maybe you are already working on fixing it 😊

The test failures may very well be flakes, especially the mysql test run looks fishy. I’ll re run 🙂

Will quickly resolve the conflicts in the train internet allows it 😄

We do that regularely. Going to take care of it now. Should be released to prod in the coming days.

Thanks for taking care of it 😍 Just tested it and it works like a charm 👌

Awesome to hear! Thanks for the great contribution.