fetch icon indicating copy to clipboard operation
fetch copied to clipboard

Make get, decode, and split handle edge cases correctly

Open annevk opened this issue 1 year ago • 4 comments

Correct how it handles when a header is present but has no value (should be a list solely containing an empty string value) and when there is a trailing comma (should be a list ending with an empty string value).

Fixes #1768.


@domenic @MattMenke2 this was an oversight in #831 it appears. If you'd like to review in addition to @CarloCannas I'd appreciate it. Thanks!


Preview | Diff

annevk avatar Aug 19 '24 11:08 annevk

Hmm, what about an header with empty value?

If they are allowed (which seems the case to me, reading both RFC 9110 and header value definition) I think this algorithm should return a list of a single empty string, not an empty list.

What do you think?

Maybe something like this? (sorry for the bad formatting)

    Let input be the result of isomorphic decoding value.

    Let position be a position variable for input, initially pointing at the start of input.

    Let values be a list of strings, initially empty.

    Let temporaryValue be the empty string.

    While true:

        Append the result of collecting a sequence of code points that are not U+0022 (") or U+002C (,) from input, given position, to temporaryValue.

        The result might be the empty string.

        If position is not past the end of input and the code point at position within input is U+0022 ("), then:

                Append the result of collecting an HTTP quoted string from input, given position, to temporaryValue.

                If position is not past the end of input, then continue.

        Remove all HTTP tab or space from the start and end of temporaryValue.

        Append temporaryValue to values.

        If position is past the end of input

        	return values

        Set temporaryValue to the empty string.

        Assert: the code point at position within input is U+002C (,).

        Advance position by 1.

CarloCannas avatar Aug 19 '24 12:08 CarloCannas

Without reviewing the actual verbiage, I agree with CarloCannas that a terminal comma should result in adding an empty string.

Specs allow for:

Foo: Bar Foo:

to be replaced with:

Foo: Bar,

So the latter should presumably return two values when parsed.

MattMenke2 avatar Aug 19 '24 13:08 MattMenke2

@CarloCannas yeah, that seems reasonable and it looks like that should be compatible with existing callers. I guess with that in theory we could make the "parent" algorithm return the empty list instead of null when the header is missing, but null is clearer. I'll see about making that change.

annevk avatar Aug 19 '24 13:08 annevk

@CarloCannas any further thoughts on this?

I'll merge this Monday if there's no more feedback.

annevk avatar Aug 27 '24 14:08 annevk

Hi Anne, I reviewed the changes again, it looks fine to me. Go ahead and merge!

CarloCannas avatar Aug 29 '24 18:08 CarloCannas