Repository navigation
Refuse a short 51Did payload of another version naming that version, and mark the raw flags byte deprecated - #115
Merged
Merged
Conversation
…rsion, is refused naming that version
…and mark the raw flags byte deprecated
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
unpackchecks the length against version 0's header before it reads the version (pipeline/fiftyone.pipeline.did/internal/payload.jslines 88 to 113 at 6d5d4fef), so a payload of 1 to 4 bytes whose flags byte names version 1, 2 or 3 getsPayloadTooShort, measured against a header that a later version need not have. A payload of normal length is refused naming its version, and tested (tests/fodId.test.jslines 611 to 632).FodIddeclares_flags: number(types/fodId.d.tsline 189), set atfodId.jsline 132 and line 480. The published fiftyone.pipeline.did 4.5.102 declares it too (itstypes/fodId.d.tsline 177), where the offsets and the Terms index are kept in modules that are not exported.What the specification says
From
did-specification/identifier-layout.mdlines 260 to 265 at 706de1f0:From
did-specification/package-surface.mdlines 129 to 134 at 706de1f0:The Rust package reads the version first and tests it (
fodid/src/fodid.rslines 460 to 477 at 113b963b).What changed
unpackreads the version from the flags byte before it checks the length, whenever the payload has a flags byte, and refuses a version it does not know withUnsupportedPayloadVersionnaming the version. A payload of version 0 that is too short is stillPayloadTooShort, and an empty payload still is too._flagsis marked deprecated infodId.jsand intypes/fodId.d.ts, pointing attype,usageandusageIsIndirect, which name its bits. It is not removed, because a patch release never removes a public name, and_flagsis in the published 4.5.102 types. A later release removes it.Tests
The new block "FodId short payloads of another version" in
pipeline/fiftyone.pipeline.did/tests/fodId.test.jschecks that payloads of 1, 2 and 4 bytes naming versions 1, 2 and 3 are refused bytryParsewithUnsupportedPayloadVersionand no value, and thatfromBase64throws with that status and a message naming the version. The existing test that payloads of 0 to 4 bytes of version 0 are refused as too short is unchanged and passes.Red, run 43614524319 on 92f5f2f, the test commit on
rc's code. On Ubuntu and on Windows the four new tests fail, each payload refused asPayloadTooShort, and the 864 other tests pass.Green, run 43614540094 on 06aadbd, this head, where every test passes on Ubuntu and on Windows.
The deprecation is a comment, so no test covers it.
Full build
Run 43615889364 on
tmp/pr-matrix-115, started bytools/pr-matrix.pywithout macOS. All 7Setting 'PullRequestSha'lines give this head, 06aadbd, and Configure printed "Left out 2 macOS entries". Each of the six Build and Test jobs, Ubuntu, Ubuntu ARM and Windows on Node.js 22 and 24, passed 1,497 unit tests with 2 skipped and 96 integration tests with 4 skipped.macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
What remains
_flagsis removed fromFodIdand its types in a release that may remove a public name, #128.