command: reject empty segments in Parse - #122
Open
Zahanturel wants to merge 1 commit into
Open
Conversation
Parse enforced the constraints stated in the spec's Segment Structure section (leading slash, no trailing slash, lowercase) but placed no constraint on the segments themselves, so "/crud//create" parsed successfully. rs-ucan rejects that input with a dedicated EmptySegment error, so the two implementations disagree on whether such a command is well formed. Add ErrEmptySegment and check for adjacent separators after the lowercase check, matching the order rs-ucan applies its checks in.
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.
command.Parseaccepts/crud//create. rs-ucan rejects it with a dedicatedEmptySegmenterror, so the two implementations currently disagree on whether that command is well formed.Parseis what runs on decode (token/delegation/delegation.go:252,token/invocation/invocation.go:290), so the disagreement is about which tokens each library accepts off the wire.Parseimplements the constraints the spec's Segment Structure section states (leading slash, no trailing slash, lowercase) and nothing beyond them, because that section says nothing about a segment being non-empty. ucan-wg/spec#196 proposes addingSegment MUST NOT be empty.to that paragraph, following from ucan-wg/delegation#26. That PR is not merged. rs-ucan made the corresponding change in ucan-wg/rs-ucan#178 while spec#196 was still open, which is the precedent this follows.The check uses
strings.Contains(s, separator+separator)rather than splitting into segments: at that point in the function the leading slash is mandatory and a trailing slash has already been rejected, so an empty segment can only appear as adjacent separators, and/needs no special case. It sits after the lowercase check so the error returned for a given input matches the one rs-ucan returns.//crud/create/crud//createErrEmptySegment/crud//ErrDisallowsTrailingSlash/CRUD//createErrRequiresLowercaseScope is
Parseonly;NewandJoinare untouched. Is their current contract intended?Joinshort-circuits on zero total length, soNew("")returns Top, andTestNew/TestJoinassert that normalizing behaviour, so I have left it alone rather than assume.This is a behaviour change:
Parsegets stricter, so a caller that currently accepts/crud//createwill start receiving an error. No fixture or test in the tree carries a command with an empty segment. The new case was confirmed to fail against unmodifiedParsebefore the check was added, in bothTestParseCommand/fails_whenandTestIsValidCommand/fails_when.go test ./... -tags jwx_es256kpasses.