`5.3.1` now treats malformed signatures as an error in `ParseUnverified`, which does not align with the documented behavior

[samfoxcode]

5.3.1 now treats malformed signatures as an error here in ParseUnverified which differs from the documented behavior of that method which before ignored signature parsing issues, matching the documented behavior of not validating the signature. This was pushed as a patch to 5.3.1 but feels more like a breaking change that should be published separately, as it changes client expectations for how signatures are treated.

[mfridman]

Let's continue the conversation here (cc @oxisto),

@samfoxcode is technically correct about the contract change, but I do wonder how practical it is. Like, the only case I can think of is feeding truly malformed strings into ParseUnverified and relying on it not to error on the signature part. That's kind of a niche edge case. The token is completely garbage .. not just unsigned or expired, but structurally corrupt.

But I've asserted that we take breaking changes extremely seriously, so let's think about it.

Afaics some options:

1. Revert the signature decoding in ParseUnverified, keep it true to its documented behavior (no signature validation of any kind). The base64 decode of the signature only happens in the verified Parse path
2. Update the documentation to say ParseUnverified now validates signature encoding (base64) but not cryptographic correctness. This keeps the current behavior but formally acknowledges the contract change, probably cut a v5.4.0
3. Add a lenient option, a ParserOption like WithLenientSignature() that skips base64 validation of the signature in ParseUnverified (I don't like this though).

Thoughts?

[samfoxcode]

@mfridman - some additional context is that the use case I'm looking at is we allow both standard and url base64 encoded signatures. But DecodeSegment only allows url encoding - so it could be that one other option or modification to the above options is to allow more types of base64 encodings to be valid.

[mfridman]

Ooof okay, I understand. I was only thinking about it from the perspective of the spec.

I guess that option would be adding WithStandardBase64Encoding() , when set, DecodeSegment falls back to standard base64 if URL-safe decoding fails. I think this would solve your problem?

[mfridman]

Looking at this more closely, we could add a ParserOption that lets callers provide their own segment decode function (and gets us out of bespoke options for every knob):

jwt.NewParser(jwt.WithDecodeSegment(func(seg string) ([]byte, error) {
	// try URL-safe first, fall back to standard base64
	b, err := base64.RawURLEncoding.DecodeString(seg)
	if err != nil {
		return base64.RawStdEncoding.DecodeString(seg)
	}
	return b, nil
}))

This keeps the library spec-conformant by default (base64url only) while giving fiolks dealing with non-conformant tokens full control over decoding. Works for both Parse and ParseUnverified.

Separately, we'd also move signature decoding out of ParseUnverified back into ParseWithClaims to restore the pre-5.3.1 behavior.

[mfridman]

I'll wait for @oxisto to chime in, but I suspect this would unblcok you. Took a quick stab over my lunch break.

#500

[mfridman]

Friendly ping @oxisto, any thoughts around this issue and how we want to approach this?

[oxisto]

Friendly ping @oxisto, any thoughts around this issue and how we want to approach this?

See my comment here. #503 (comment). The "new" implementation is not breaking any assumption about the documented feature in my opinion.

I am fine with the decode segment option. That might also actually be a very simple way to support third party base64 libraries - so we should also do that for encode. Maybe add the idx as an additional parameter to it.

I would keep ParseUnverified as it is now.