Non-Canonical varints in CIDs

[DavidBuchanan314]

The reference PDS implementation accepts records containing $link CIDs containing non-canonical varint encodings, whereas the spec requires them to be minimally encoded.

This looks like a bug/oversight in js-multiformats: https://github.com/multiformats/js-multiformats/blob/dcbd5d73d80da321c8ee64a648ded51199d731bb/src/vendor/varint.js#L44-L69 (side-note, this impl will also happily overflow JS's precision limits, resulting in loss of precision and/or NaNs)

On the other hand, it looks like the Relay implementation correctly(?) rejects them as invalid. This divergence means that commits to my repo no longer reach the AppView. Oops!

[DavidBuchanan314]

(after deleting the record containing the invalid CID, my repo is syncing again)

This record value should reproduce the issue:

{
  "$type": "app.bsky.feed.post",
  "createdAt": "2024-02-19T12:47:18.827Z",
  "text": "test",
  "langs": [
    "en"
  ],
  "secret": {
    "foo": {
      "$link": "bqgaabaeaacaiaaeaqaaa"
    }
  }
}

[DavidBuchanan314]

I think it's probably uncontroversial what to do about non-minimal varints, but what's perhaps less clear is whether software should even be trying to validate CIDs within records in the first place. Maybe the PDS's behavior here is overall more sensible.

The CID spec defines v0 and v1 formats, with v2 and v3 "reserved" but otherwise unspecified. You obviously can't validate a v2 or v3 CID today, so should they be rejected, at the risk of breaking compatibility with future software?

[DavidBuchanan314]

I think for my own software I'll try to pass CIDs through "untouched", even when they're definitely malformed, verifying specific formats only if and when it's necessary (e.g. when a CID is being used to resolve a reference to something).

[mary-ext]

Unless the PDS actually supports v2 and v3, which of course is currently unspecified, doesn't it make more sense to reject them?

[DavidBuchanan314]

imho the PDS should, in general, allow things unless it has a particular reason not to.

"is this CID a CIDv1 dag-cbor sha256?" is an easy thing to verify, but trying to validate CIDs in the general case seems fiddly, and I'd rather simply not do it.

[bnewbold]

This is a great little interoperability case study:

• what should the "blast radius" of invalid data of various types be? records, commit objects, MST notes, etc; at the relay, PDS, and downstream. we generally want broken records to only impact that record, not the whole repo; but broken commits and MST nodes impact the whole repo. we probably aren't consistent about this yet, isn't documented, and don't have test coverage.
• how to handle forward extensibility, like future CID formats. balancing simple broad interop today against evolution. we have leaned towards locking things down for now. the flexibility and extension points will make it clear how to extend the protocol, but that may be in explicit versioned iterations (eg, increment the repo version number beyond 3 to support higher CID types)
• when are implementations actually expected to check and enforce atproto-specific constraints? eg, a lot of IPLD packages will automatically parse CID Links; do all implementations actually need to additionally go out of their way to check the CID details, hash type, etc? what about things like CBOR and JSON key sort order, or CBOR integer encoding compactness? gets in to "SHOULD" / "MUST" territory. currently leaning that the highest leverage place to be strict is PDS implementation, because this catches good-actor issues (aka, mistakes and confusion) fast. having a gap between specs and reference implementation isn't great, but may persist a bit longer. hope to have a very detailed compliance test suite before too long.
• how to validate atproto data if the lexicon isn't present? eg, for CID Link fields, it is possible to parse CBOR/JSON, identify the type or $link format, and validate the CID syntax, even if the lexicon isn't know. but this doesn't work for string format=cid. how far should implementations go to validate records with unknown lexicons?

These are more questions than answers right now. Some related work-in-flight:

• atproto data model interop test files #2182
• Enable creation of unknown record types #2171
• Add data validation spec atproto-website#279