Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Add unsafe decoding? #62

Closed
bgins opened this issue Mar 30, 2022 · 10 comments
Closed

Add unsafe decoding? #62

bgins opened this issue Mar 30, 2022 · 10 comments
Labels
enhancement New feature or request

Comments

@bgins
Copy link
Contributor

bgins commented Mar 30, 2022

Summary

Problem

ts-ucan does not provide a raw decoding function.

Impact

Tools that wish to inspect a UCAN in its raw form are unable to do so without some level of parsing/validation.

Solution

Provide a raw decoding function.

Detail

Is your feature request related to a problem? Please describe.

Tools that wish to inspect a defective UCAN are unable to do so.

Describe the solution you'd like

The raw decoding function should:

  • Not validate UCANs
  • Should only parse at the top level to check for a header, payload, signature, and . separators
  • Be marked as explicitly unsafe

Naming and/or namespacing will be an important consideration for the last point. A few possibilities:

  • ucan.unsafeDecode
  • ucan.unutterableFoulDecode
  • Namespace unsafe with subtle, for example ucan.subtle.unsafeDecode
  • internal as a namespace where ucan.internal.decode + ucan.internal.validate => ucan.validate
  • parse instead of validate, because the that's decode + validate

{insert-bikeshedding-here}

Describe alternatives you've considered

Developers can write their own raw decoding function.

@bgins bgins added the enhancement New feature or request label Mar 30, 2022
@bgins
Copy link
Contributor Author

bgins commented Mar 30, 2022

In my opinion, we should stick with the name validate. Although parse more accurately describes what the validate function does, it will be less familiar to developers coming from other auth libraries.

@expede
Copy link
Member

expede commented Mar 30, 2022

Although parse more accurately describes what the validate function does

@bgins they're different! Parse is equivalent to decode + validate (but like... fancier). There should absolutely still be a validate function broken out.

@bgins
Copy link
Contributor Author

bgins commented Mar 31, 2022

@bgins they're different! Parse is equivalent to decode + validate (but like... fancier). There should absolutely still be a validate function broken out.

@expede Yep, I'm aware they are different. I didn't state that very well.

I think that most developers won't be familiar with the term parse in this context, and it could make the API less approachable. Another term we could consider is verify, which is what the jsonwebtoken library uses: https://github.com/auth0/node-jsonwebtoken#jwtverifytoken-secretorpublickey-options-callback

@matheus23
Copy link
Member

matheus23 commented Mar 31, 2022

What should the result type for decode be? Something like { header: Record<string, unknown>, payload: Record<string, unknown>, signature: string}?

Also big 👍 for adding stuff to the library, informed by usage sites. This makes sense.

@bgins
Copy link
Contributor Author

bgins commented Mar 31, 2022

What should the result type for decode be? Something like { header: Record<string, unknown>, payload: Record<string, unknown>, signature: string}?

What would you think about making this even less safe? 😆

Something like { header: unknown, payload: unknown, signature: unknown }. The use case I am thinking is a tool is attempts to decode a token, but the sections are out of order, for example signature.header.payload. The decode function would unsafely decode each section and let the tool decide what to do with it.

@bgins
Copy link
Contributor Author

bgins commented Apr 4, 2022

Something like { header: unknown, payload: unknown, signature: unknown }. The use case I am thinking is a tool is attempts to decode a token, but the sections are out of order, for example signature.header.payload. The decode function would unsafely decode each section and let the tool decide what to do with it.

I was thinking about this some more, and we should consider what tools might want to use the raw decode function. The use case I mention above is something that would be useful in ucan.xyz, but the safer type may be better for other tools and internally in ts-ucan. We could write a less safe version in ucan.xyz to handle the special cases.

@matheus23
Copy link
Member

The use case I mention above is something that would be useful in ucan.xyz, but the safer type may be better for other tools and internally in ts-ucan. We could write a less safe version in ucan.xyz to handle the special cases.

Just trying to understand: Would you say this unsafe decode function belongs into ucan.xyz and we should close this issue? Or is there something that you think belongs into ts-ucan?

@bgins
Copy link
Contributor Author

bgins commented Apr 5, 2022

Just trying to understand: Would you say this unsafe decode function belongs into ucan.xyz and we should close this issue? Or is there something that you think belongs into ts-ucan?

For a raw decoding function to be useful in ucan.xyz, it should only check that a token has three sections, the sections are seperated by ., and the first two sections can be decoded from base64url. If those are all true, it should return the sections as header, payload, and signature in order of appearance, with the first two sections decoded.

We do these steps in the parse function in ts-ucan, but we also JSON.parse the header and payload. Other apps that want a raw decoding function will probably want the additional check.

I would propose we:

  • Add a raw decoding function to ts-ucan with JSON parsing for the header and the payload, and the signature { header: Record<string, unknown>, payload: Record<string, unknown>, signature: string} suggested above
  • Implement a custom decoding function ucan.xyz for the special case, with the idea that it is more forgiving and willing to just show you what is there

Does that sound like a good way to approach this?

@matheus23
Copy link
Member

I would propose we:

  • Add a raw decoding function to ts-ucan with JSON parsing for the header and the payload, and the signature { header: Record<string, unknown>, payload: Record<string, unknown>, signature: string} suggested above
  • Implement a custom decoding function ucan.xyz for the special case, with the idea that it is more forgiving and willing to just show you what is there

Does that sound like a good way to approach this?

Hm. I think that means there's no one blocked by us putting a raw decoding function into ts-ucan. So I'd propose shelving this for now. Once we find that we have more "customer need" (for the lack of a better word) for this, we should look at it again 👍

@bgins
Copy link
Contributor Author

bgins commented Apr 6, 2022

Hm. I think that means there's no one blocked by us putting a raw decoding function into ts-ucan. So I'd propose shelving this for now. Once we find that we have more "customer need" (for the lack of a better word) for this, we should look at it again 👍

Agreed. Let's hold on this for now. 👌

One use case I can imagine for raw decoding is debugging while developing an app, but once we have raw decoding in ucan.xyz, that should fulfill that use case. Some developers may still want the raw decoding function, but we can hold off until someone requests it.

@walkah walkah changed the title Add unsafe decoding Add unsafe decoding? Apr 19, 2022
@ucan-wg ucan-wg locked and limited conversation to collaborators Apr 19, 2022
@walkah walkah converted this issue into discussion #76 Apr 19, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants