Spec should be very clear how to handle duplicate keys

[Gozala]

We end up having a lengthy discussion about duplicate key exploits in signed data https://github.com/ChainAgnostic/varsig/pull/6/files#r1038482364 where it become clear that current IPLD specification is not very clear about this, in fact case could be made that following section suggests such exploits are possible

ipld/specs/codecs/dag-json/spec.md

Lines 24 to 32 in 015ff80

	Codec implementations **MUST** do the following when encoding data in order to ensure hashes consistently match for the same block data.
	- Sort object keys by their (UTF-8) encoded representation, i.e. with byte comparisons
	- Strip whitespace
	This produces the most compact and consistent representation which will ensure that two codecs
	producing the same data end up with matching block hashes.
	Codec implementers should not enforce this strictness when decoding data in order to support historical data, and data produced by non-strict encoders. However, they may provide an opt-in for systems where round-trip determinism is a desireable feature and backward compatibility with old, non-strict data is unnecessary.

It was also unclear how individual implementations would behave in the face of blocks with duplicate keys, which again suggests that this subtle detail shouldn't be subtle, but rather should be well articulated and implementations should communicate how they'd behave in such scenario.

[rvagg]

I think we should change dag-json and dag-pb to have a **SHOULD** reject blocks with duplicate keys in maps. I know we're going to get some grief for potential slow-downs in decoding.

I believe Go should already be rejecting dupes: https://github.com/ipld/go-ipld-prime/blob/33475f04486e763616962f4dd2bda67b31758e27/node/basicnode/map.go#L249-L253

We probably ought to do an key in obj check and reject with the same thing in JS, so I'd consider this a bug: https://github.com/rvagg/cborg/blob/f05397b8df2d7a063f0d35cc756d74ce5cb2f4f1/lib/decode.js#L117

@Gozala would you care to draft some language for the specs to get this started?

[rvagg]

Another case for negative fixtures .. we have no way of asserting this kind of condition across our codecs.

FWIW dag-pb no longer allows this even though it's entirely valid PB and the old codecs used to allow it. As of the latest generation, Go and JS will reject blocks where there are duplicates of anything.

[BigLep]

2023-01-03 IPLD triage conversation:

1. We're hoping @Gozala can draft spec language per Spec should be very clear how to handle duplicate keys #259 (comment)
2. IPLD maintainers will do the JS fix (@rvagg is creating an issue now)
3. @rvagg will create a tracking issue for negative test fixtures

[rvagg]

Some issues as follow-up to get them on the board:

• First step in fixing JS: Option to bail on duplicate map keys rvagg/cborg#72
• Negative test fixtures: Add a mechanism for negative test fixtures codec-fixtures#80

This issue can serve as a placeholder for fixing the spec.

[DavidBuchanan314]

By the way, you can detect duplicate map keys during parsing for almost-free in most cases, by keeping track of how many keys you've added, and then checking if the size of the map matches what you expect once you reach the end of it - as opposed to doing a key in obj check (or similar) before each new addition.