feat: update receiver regex validation #430

BlairCurrey · 2024-02-22T19:25:40Z

Changes proposed in this pull request

changes the receiver regex so that:

it now matches with http and any id (instead of just uuid)
it does not match with deprecated /connections/ path

Context

I noticed when using the client in the local environment that a receiver with an http url fails validation, which is necessary for local development. We are already using such a url, we're just not using the client for it (see the bruno/postman Create Quote request body). Widening that was the main goal and the other changes are more cleanup.

fixes #429

changeset-bot · 2024-02-22T19:25:44Z

🦋 Changeset detected

Latest commit: a4b56ec

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@interledger/open-payments	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2024-02-22T19:25:57Z

✅ Deploy Preview for openpayments-preview canceled.

Name	Link
🔨 Latest commit	`a4b56ec`
🔍 Latest deploy log	https://app.netlify.com/sites/openpayments-preview/deploys/65df77334778f90009bb56e2

BlairCurrey · 2024-02-22T19:27:00Z

⚠️ No Changeset found

Latest commit: e10f069

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets
Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Note: I purposefully did not add a changeset. This only changes the schema (and a test). Can just use the fetch-schemas script in rafiki to get these changes.

BlairCurrey · 2024-02-22T19:33:56Z

openapi/schemas.yaml

@@ -42,10 +42,11 @@ components:
      type: string
      description: The URL of the incoming payment or ILP STREAM connection that is being paid.
      format: uri
-      pattern: '^https://(.+)/(incoming-payments|connections)/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$'
+      pattern: '^(https|http)://(.+)/incoming-payments/(.+)$'


As for the id change:

figured it was correct to loosen it to any sort of id (not necessarily uuid) after discussing with Max. Thinking was that while we use uuid's in rafiki this is really a detail which might vary from implementation to implementation and it shouldn't (I think) matter what sort of id scheme the api implementation uses. More opinions welcome.

I changed it to match anything but perhaps chars/numbers only would be better? I just wasn't sure about forbidding the use of query variables and such. While we don't support anything like this I don't know that we should have an opinion against it: incoming-payments/123?something=1

I think matching anything is fine in this case

BlairCurrey · 2024-02-28T16:19:21Z

⚠️ No Changeset found

Latest commit: e10f069
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR

Note: I purposefully did not add a changeset. This only changes the schema (and a test). Can just use the fetch-schemas script in rafiki to get these changes.

Looks like I was wrong about this and do need to version bump open-payments because the response validator uses the schema from this repo. I noticed while using the client in Rafiki. I manually patched the regex in rafiki/openapi/schemas.yml and that fixed the request validation error but I got the same error during response validation. Manually patching the schema in node_modules fixed that.

@mkurapov do you happen to know off the top of your head why we pass in a spec but also refer to a different internal one like this? It just seems like we would either do one or the other.

mkurapov · 2024-02-28T16:36:11Z

@BlairCurrey re:

@mkurapov do you happen to know off the top of your head why we pass in a spec but also refer to a different internal one like this? It just seems like we would either do one or the other.

What do you mean by pass in a spec here?

openapi/schemas.yaml

mkurapov · 2024-02-28T16:25:21Z

packages/openapi/src/middleware.test.ts

+      test.each`
+        body                                                                                     | message                                                                              | description
+        ${{ receiver: 'ht999tp://something.com/incoming-payments' }}                             | ${'body.receiver must match pattern "^(https|http):..(.+).incoming-payments.(.+)$"'} | ${'invalid receiver, unknown protocol'}
+        ${{ receiver: 'http://something.com/incoming-payments' }}                                | ${'body.receiver must match pattern "^(https|http):..(.+).incoming-payments.(.+)$"'} | ${'invalid receiver, missing incoming payment id'}
+        ${{ receiver: 'http://something.com/connections/c3a0d182-b221-4612-a500-07ad106b5f5d' }} | ${'body.receiver must match pattern "^(https|http):..(.+).incoming-payments.(.+)$"'} | ${'invalid receiver, wrong path'}


maybe it would be good to have one happy path case

... In a separate test block, so that we can just hardcode the 'body.receiver must match pattern "^(https|http):..(.+).incoming-payments.(.+)$"' error message in the result without needing it in the test.each part

I'll make some happy-path tests in a new test group.

Im not necessarily against hardcoding the message here but just want to note that the test is for the quote resource in general and could be extended to validate different fields which would require parameterizing the message like this, like the existing tests do for /incoming-payments.

removed message param and added happy path cases

mkurapov · 2024-02-28T16:29:03Z

openapi/schemas.yaml

@@ -42,10 +42,11 @@ components:
      type: string
      description: The URL of the incoming payment or ILP STREAM connection that is being paid.
      format: uri
-      pattern: '^https://(.+)/(incoming-payments|connections)/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$'
+      pattern: '^(https|http)://(.+)/incoming-payments/(.+)$'


I think matching anything is fine in this case

BlairCurrey · 2024-02-28T17:11:21Z

@BlairCurrey re:

@mkurapov do you happen to know off the top of your head why we pass in a spec but also refer to a different internal one like this? It just seems like we would either do one or the other.

What do you mean by pass in a spec here?

What I meant was passing in a spec when creating the client in rafiki, but after a more careful reading I see that is not whats happening.

I guess what I was seeing was not a validation error from the client but from the backend route's validator middleware. Which makes sense. So I take back that concern - was just confusion on my end.

Co-authored-by: Max Kurapov <[email protected]>

mkurapov

LGTM but make sure to get someone else's eyes on this as well 👍

feat: update receiver regex validation

e10f069

fix: example url typo

b6e7dcd

BlairCurrey commented Feb 22, 2024

View reviewed changes

BlairCurrey requested review from mkurapov, sabineschaller, njlie and JoblersTune February 22, 2024 19:39

mkurapov reviewed Feb 28, 2024

View reviewed changes

BlairCurrey and others added 3 commits February 28, 2024 12:12

Update openapi/schemas.yaml

d881a37

Co-authored-by: Max Kurapov <[email protected]>

chore: add changeset

e48673c

test(openapi): add quote validation tests

78626fb

mkurapov approved these changes Feb 28, 2024

View reviewed changes

chore: generate new types

a4b56ec

njlie approved these changes Feb 29, 2024

View reviewed changes

BlairCurrey merged commit fe18cd0 into main Mar 1, 2024
11 checks passed

BlairCurrey deleted the bc/429/update-receiver-regex branch March 1, 2024 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update receiver regex validation #430

feat: update receiver regex validation #430

BlairCurrey commented Feb 22, 2024

changeset-bot bot commented Feb 22, 2024 •

edited

Loading

netlify bot commented Feb 22, 2024 •

edited

Loading

BlairCurrey commented Feb 22, 2024

⚠️ No Changeset found

BlairCurrey Feb 22, 2024 •

edited

Loading

mkurapov Feb 28, 2024

BlairCurrey commented Feb 28, 2024 •

edited

Loading

⚠️ No Changeset found

mkurapov commented Feb 28, 2024

mkurapov Feb 28, 2024

mkurapov Feb 28, 2024

BlairCurrey Feb 28, 2024

BlairCurrey Feb 28, 2024

mkurapov Feb 28, 2024

BlairCurrey commented Feb 28, 2024

mkurapov left a comment

feat: update receiver regex validation #430

feat: update receiver regex validation #430

Conversation

BlairCurrey commented Feb 22, 2024

Changes proposed in this pull request

Context

changeset-bot bot commented Feb 22, 2024 • edited Loading

🦋 Changeset detected

netlify bot commented Feb 22, 2024 • edited Loading

✅ Deploy Preview for openpayments-preview canceled.

BlairCurrey commented Feb 22, 2024

⚠️ No Changeset found

BlairCurrey Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

mkurapov Feb 28, 2024

Choose a reason for hiding this comment

BlairCurrey commented Feb 28, 2024 • edited Loading

⚠️ No Changeset found

mkurapov commented Feb 28, 2024

mkurapov Feb 28, 2024

Choose a reason for hiding this comment

mkurapov Feb 28, 2024

Choose a reason for hiding this comment

BlairCurrey Feb 28, 2024

Choose a reason for hiding this comment

BlairCurrey Feb 28, 2024

Choose a reason for hiding this comment

mkurapov Feb 28, 2024

Choose a reason for hiding this comment

BlairCurrey commented Feb 28, 2024

mkurapov left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Feb 22, 2024 •

edited

Loading

netlify bot commented Feb 22, 2024 •

edited

Loading

BlairCurrey Feb 22, 2024 •

edited

Loading

BlairCurrey commented Feb 28, 2024 •

edited

Loading