Skip to content

Robust CBOR deserialization for IssuerSignedItem/List with deferred element identifier#531

Open
nodh wants to merge 10 commits intodevelopfrom
codex/add-tests-for-deserializing-elementvalue-error
Open

Robust CBOR deserialization for IssuerSignedItem/List with deferred element identifier#531
nodh wants to merge 10 commits intodevelopfrom
codex/add-tests-for-deserializing-elementvalue-error

Conversation

@nodh
Copy link
Member

@nodh nodh commented Mar 2, 2026

Mitigate errors like #311 by preventing deserialization of IssuerSignedItem without a namespace. Our code relies on deserializing IssuerSignedList instead, which has two advantages: Always has a namespace and uses OBOR generic CBOR parsing elementValue which should at least return the CBOR byte array as a fallback.

Started with the help of codex 5.3 high.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 nodh force-pushed the codex/add-tests-for-deserializing-elementvalue-error branch from c6fbb45 to aaad495 Compare March 2, 2026 18:47
@nodh nodh changed the base branch from main to develop March 2, 2026 19:47
@nodh nodh self-assigned this Mar 2, 2026
@nodh nodh requested a review from JesusMcCloud March 2, 2026 19:58
@nodh nodh added this to the 5.12.0 milestone Mar 2, 2026
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 nodh force-pushed the codex/add-tests-for-deserializing-elementvalue-error branch from b049fab to 7158112 Compare March 3, 2026 17:23
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 nodh force-pushed the codex/add-tests-for-deserializing-elementvalue-error branch from cdf31bb to e9fb71a Compare March 3, 2026 17:57
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

@nodh nodh marked this pull request as draft March 3, 2026 18:20
@nodh nodh marked this pull request as ready for review March 4, 2026 09:14
@nodh
Copy link
Member Author

nodh commented Mar 11, 2026

pinging @JesusMcCloud

Copy link
Collaborator

@JesusMcCloud JesusMcCloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.


private fun CborMap.toIssuerSignedItem(): IssuerSignedItem {
val digestId = coseCompliantSerializer.decodeFromByteArray(
Long.serializer(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long or ulong? important for KxS 1.10

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the CDDL it's uint, so major type 0. But there is no encodeUIntElement, so is it safe to use encodeLongElement or not?

@nodh nodh requested a review from JesusMcCloud March 13, 2026 10:28
@nodh nodh force-pushed the codex/add-tests-for-deserializing-elementvalue-error branch from d5fb5a0 to db9c163 Compare March 13, 2026 10:28
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

runCatching { return coseCompliantSerializer.decodeFromByteArray(InstantStringSerializer, bytes) }
runCatching { return coseCompliantSerializer.decodeFromByteArray(String.serializer(), bytes) }
runCatching { return coseCompliantSerializer.decodeFromByteArray(Long.serializer(), bytes) }
runCatching { return coseCompliantSerializer.decodeFromByteArray(Float.serializer(), bytes) }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Decode Double before Float in generic fallback

In decodeGenericElementValue, Float.serializer() is attempted before Double.serializer(), so unregistered 64-bit floating-point claims parsed via deserializeFromOborMap can be downcast to Float before Double is ever tried. This changes both runtime type and precision for valid double-valued elementValue fields in IssuerSignedList, which can break round-trips and numeric comparisons for consumers that expect full Double fidelity.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants