Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement JSON value matching #500

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

piotr-roslaniec
Copy link
Contributor

@piotr-roslaniec piotr-roslaniec commented Feb 20, 2025

  • Fixes feat: Implement JSON value matching for Manifest validation #491
  • Introduces NotaryResponse and NotaryResponseBody to separate ManifestResponse and verification logic & parsing JSON
  • Manifest.body.json now describes JSON path with an optional value matching
    • For n elements n>0, initial elements are treated as object keys and array indices, and element n+1 treated as a literal value
    • This offers a middle ground between adding another workaround for value matching/DSL, while being extendable enough to be retrofitted into a DSL without breaking changes
    • Note: This is a breaking change

@piotr-roslaniec piotr-roslaniec force-pushed the feat/json-value-matching#491 branch 3 times, most recently from 4acf111 to 8231fa1 Compare February 21, 2025 16:05
@piotr-roslaniec piotr-roslaniec force-pushed the feat/json-value-matching#491 branch from 8231fa1 to be06abf Compare February 21, 2025 16:18
@devloper
Copy link
Contributor

devloper commented Feb 21, 2025

@drewjenkins This PR fixes binance and Spotify, but requires breaking changes to the attest-integrations.

@piotr-roslaniec , can you help Andrew update the attest-integrations (link) to reflect your changes?

Also, we will need to update the docs for this new manifest description, @0xFloyd can you work with Piotr to update the docs?

@drewjenkins
Copy link
Contributor

drewjenkins commented Feb 21, 2025

@piotr-roslaniec @devloper Does the rename from json->jsonPath have to happen for the fix? Asking because it won't only break attest-integrations, it will also break the SDK, and working version of the mobile app. It's fine if we want the change, just something to be aware of going forward that any breaking changes to the manifest could potentially break

  • The chrome extension
  • Docs/Manifest Builder
  • Various version of the deployed iOS App (not in this scenario since the deployed version doesn't use response)
  • Swift/Kotlin SDK
  • React Native SDK
  • Javascript/React SDK
  • iFrame/Android App once released

@piotr-roslaniec
Copy link
Contributor Author

piotr-roslaniec commented Feb 22, 2025

@drewjenkins The renaming of the field is optional, but how it's interpreted will change, breaking some stuff: Previously we didn't expect the last element in the json to be a concrete value (optionally), but now we do. That being said, I haven't seen a single manifest that would be affected yet (as they usually specify keys only)

I updated the issue description.

This is the only breaking change I foresee before we roll-out a DSL, which will break everything.

@devloper
Copy link
Contributor

devloper commented Feb 22, 2025

Thanks for writing up the great list Andrew! We should almost make this into a check list for releasing these breaking changes quickly.

imo these breaking changes are perfectly fine and we should not be "afraid" to make them as long as we have good semantic versioning we should have control over when the changes goes out to all our SDKs and users can choose when they upgrade. We may need to improve our release management processes however to minimize the headache for ourselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Implement JSON value matching for Manifest validation
3 participants