Robust CBOR deserialization for IssuerSignedItem/List with deferred element identifier#531
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 572bebaae0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nodh
force-pushed
the
codex/add-tests-for-deserializing-elementvalue-error
branch
from
c6fbb45 to
aaad495
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63676a3cca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c55911e40a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nodh
force-pushed
the
codex/add-tests-for-deserializing-elementvalue-error
branch
from
b049fab to
7158112
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 715811254c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nodh
force-pushed
the
codex/add-tests-for-deserializing-elementvalue-error
branch
from
cdf31bb to
e9fb71a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f70bc527bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
pinging @JesusMcCloud |
|
|
||
| private fun CborMap.toIssuerSignedItem(): IssuerSignedItem { | ||
| val digestId = coseCompliantSerializer.decodeFromByteArray( | ||
| Long.serializer(), |
There was a problem hiding this comment.
long or ulong? important for KxS 1.10
There was a problem hiding this comment.
In the CDDL it's uint, so major type 0. But there is no encodeUIntElement, so is it safe to use encodeLongElement or not?
There was a problem hiding this comment.
Isn't there an ULong.serializer()?
There was a problem hiding this comment.
But digestId is an UInt, is it really safe to deserialize it as ULong?
There was a problem hiding this comment.
we need to wait for the next KxS version anyhow to have number parsing fixed. I'll then look into this version of the CBOR decoder in KxS. but it should probably be ULong.serializer()
There was a problem hiding this comment.
There will be major upcoming changes in the CBOR codec upstream, so my take: deserialize as ULong, then do a range-check and manually throw. Valid CBOR can still overflow Kotlin's ULong, but this is something that cannot be remedied until the next major version of the CBOR codec.
There was a problem hiding this comment.
NVM. Upstream has the range checks in place.
nodh
force-pushed
the
codex/add-tests-for-deserializing-elementvalue-error
branch
from
d5fb5a0 to
db9c163
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db9c163880
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nodh
force-pushed
the
codex/add-tests-for-deserializing-elementvalue-error
branch
from
db9c163 to
755876c
Compare
JesusMcCloud
left a comment
JesusMcCloud
left a comment
There was a problem hiding this comment.
Just skim over it whether signed/unsigned numbers are directly (de)serialized apprpriately
2 participants
Mitigate errors like #311 by preventing deserialization of
IssuerSignedItemwithout a namespace. Our code relies on deserializingIssuerSignedListinstead, which has two advantages: Always has anamespaceand uses OBOR generic CBOR parsingelementValuewhich should at least return the CBOR byte array as a fallback.Started with the help of codex 5.3 high.