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

Validate pubkey, relay URL and secret of NWC URL #810

Merged
merged 5 commits into from
Feb 14, 2024
Merged

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Feb 12, 2024

Mhh, wasn't able to use Yup.transform so we can keep using validationSchema. I tried stuff along these lines:

export const nwcSchema = object({
  nwcUrl: string().required('required').trim().matches(/^nostr\+walletconnect:\/\//),
  relayUrl: string().required('relay url required').trim().wss('relay must use wss://'),
  walletPubkey: string().required('pubkey required').trim().matches(NOSTR_PUBKEY_HEX, 'pubkey must be 64 hex chars'),
  secret: string().required('secret required').trim().matches(/^[0-9a-z]{64}$/, 'secret must be 64 hex chars')
}).transform(url => ({ url, ...parseNwcUrl(url) }))

but in any case, it threw errors for fields that don't exist (relayUrl, walletPubkey, secret) which makes sense. So not sure if we can use transform to transform a string into an object but still pretend it's the same string we are validating?

2024-02-12.01-59-36.mp4

@ekzyis ekzyis added enhancement improvements to existing features and removed enhancement improvements to existing features labels Feb 12, 2024
@huumn
Copy link
Member

huumn commented Feb 12, 2024

I'd guess you'll want something like this but I haven't used their transform yet so not sure:

export const nwcSchema = object({
  nwc: string().required('required').transform(url => ({ url, ...parseNwcUrl(url) })).object({
      nwcUrl: string().required('required').trim().matches(/^nostr\+walletconnect:\/\//),
      relayUrl: string().required('relay url required').trim().wss('relay must use wss://'),
      walletPubkey: string().required('pubkey required').trim().matches(NOSTR_PUBKEY_HEX, 'pubkey must be 64 hex chars'),
      secret: string().required('secret required').trim().matches(/^[0-9a-z]{64}$/, 'secret must be 64 hex chars')
  })
})

@ekzyis ekzyis marked this pull request as draft February 13, 2024 15:41
@ekzyis ekzyis added the enhancement improvements to existing features label Feb 13, 2024
@ekzyis ekzyis closed this Feb 14, 2024
@ekzyis ekzyis deleted the webln-enforce-tls branch February 14, 2024 00:28
@ekzyis ekzyis restored the webln-enforce-tls branch February 14, 2024 02:07
@ekzyis ekzyis reopened this Feb 14, 2024
@ekzyis ekzyis force-pushed the webln-enforce-tls branch 2 times, most recently from a6c9eec to d813290 Compare February 14, 2024 20:35
@ekzyis
Copy link
Member Author

ekzyis commented Feb 14, 2024

Was able to use transform but still had to cast validation errors into the shape formik expects. So couldn't use Formik.validationSchema which works well with Yup and is what we use everywhere for validation.

However, even then, I still had problems with the order of validation (see jquense/yup#851), so I decided to not use transform at all and instead do this:

export const nwcSchema = object({
  nwcUrl: string()
    .required('required')
    .test(async (nwcUrl, context) => {
      // run validation in sequence to control order of errors
      // inspired by https://github.com/jquense/yup/issues/851#issuecomment-1049705180
      try {
        await string().required('required').validate(nwcUrl)
        await string().matches(/^nostr\+?walletconnect:\/\//, { message: 'must start with nostr+walletconnect://' }).validate(nwcUrl)
        let relayUrl, walletPubkey, secret
        try {
          ({ relayUrl, walletPubkey, secret } = parseNwcUrl(nwcUrl))
        } catch {
          // invalid URL error. handle as if pubkey validation failed to not confuse user.
          throw new Error('pubkey must be 64 hex chars')
        }
        await string().required('pubkey required').trim().matches(NOSTR_PUBKEY_HEX, 'pubkey must be 64 hex chars').validate(walletPubkey)
        await string().required('relay url required').trim().wss('relay must use wss://').validate(relayUrl)
        await string().required('secret required').trim().matches(/^[0-9a-fA-F]{64}$/, 'secret must be 64 hex chars').validate(secret)
      } catch (err) {
        return context.createError({ message: err.message })
      }
      return true
    })
})

I think this is also clearer now and we can stay consistent and also use validationSchema here.

@ekzyis ekzyis marked this pull request as ready for review February 14, 2024 20:48
@huumn huumn merged commit 1444ff4 into master Feb 14, 2024
1 check passed
@ekzyis ekzyis deleted the webln-enforce-tls branch February 14, 2024 21:09
@huumn
Copy link
Member

huumn commented Feb 18, 2024

Thread explaining why transforms don't work: How does yup's trim work?

Formik doesn't use the returned object from yup which contains the transforms. According to that thread, we'd just need to add this line to onSubmit:

onSubmit={(values) => {
  const castValues = mySchema.cast(values)
  ...
})

@ekzyis
Copy link
Member Author

ekzyis commented Feb 18, 2024

Thread explaining why transforms don't work: How does yup's trim work?

Formik doesn't use the returned object from yup which contains the transforms. According to that thread, we'd just need to add this line to onSubmit:

onSubmit={(values) => {
  const castValues = mySchema.cast(values)
  ...
})

But we would still need to "cast back" manually to properly show error messages for the input field named nwcUrl. This casting back to properly recognize errors was mostly the issue I was having iirc. And transforms only work one-way.

@huumn
Copy link
Member

huumn commented Feb 18, 2024

Ah I see. Still, might be useful for transforms like trim()

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

Successfully merging this pull request may close these issues.

2 participants