Revamp error hierarchy by anakinj · Pull Request #722 · jwt/ruby-jwt

anakinj · 2026-03-27T19:26:10Z

Description

Tries to address #606

Checklist

Before the PR can be merged be sure the following are checked:

There are tests for the fix or feature added/changed
A description of the changes and a reference to the PR has been added to CHANGELOG.md. More details in the CONTRIBUTING.md

Copilot

Pull request overview

This PR restructures the JWT error class hierarchy to better distinguish between malformed tokens, signature/algorithm failures, and claim validation failures (notably separating expiration/claim validation from structural decode failures), in response to #606.

Changes:

Introduces a new base JWT::Error and adds grouped subclasses (TokenError, MalformedTokenError, SignatureError, ClaimValidationError) with updated inheritance for existing error types.
Updates various code paths to raise more specific error subclasses (e.g., malformed vs signature vs verification vs claim validation).
Adds an RSpec spec validating the intended error hierarchy and non-overlap between the new groups.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
spec/jwt/error_spec.rb	Adds tests asserting the new error inheritance structure and group separation.
lib/jwt/error.rb	Defines the revamped error hierarchy and a backwards-compatibility `DecodeError` mapping.
lib/jwt/decode.rb	Updates raised error classes for malformed tokens vs missing verification keys.
lib/jwt/encoded_token.rb	Updates raised error classes for malformed payload/segments and adjusts YARD docs.
lib/jwt/jwk/key_finder.rb	Changes errors raised during JWK key resolution to signature-related errors.
lib/jwt/jwa/signing_algorithm.rb	Raises `VerificationError` (instead of the prior decode error) for verify failures.
lib/jwt/token.rb	Updates YARD doc for claim verification to reference claim validation errors.
lib/jwt/claims/verifier.rb	Narrows rescued errors to `ClaimValidationError` when collecting claim errors.
lib/jwt/claims.rb	Updates YARD doc to reference claim validation errors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (2)

spec/jwt/jwa/ecdsa_spec.rb:98

The example text says it raises JWT::VerificationError, but the spec expects JWT::EncodeError. This mismatch is confusing and makes it unclear which error class is intended for invalid signing keys.

    context 'when the signing key is not an OpenSSL::PKey::EC instance' do
      it 'raises a JWT::VerificationError' do
        expect do
          instance.sign(data: data, signing_key: 'not_a_key')
        end.to raise_error(JWT::EncodeError, 'The given key is a String. It has to be an OpenSSL::PKey::EC instance')
      end

spec/jwt/jwa/ecdsa_spec.rb:90

The example text says it raises JWT::VerificationError, but the expectation is for JWT::EncodeError. Either update the description to match the actual error class, or (if the intent is to change behavior) adjust the implementation/spec to raise VerificationError consistently.

    context 'when the signing key is a public key' do
      it 'raises a JWT::VerificationError' do
        public_key = test_pkey('ec256-public.pem')
        expect do
          instance.sign(data: data, signing_key: public_key)
        end.to raise_error(JWT::EncodeError, 'The given key is not a private key')
      end

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

spec/jwt/jwa/ecdsa_spec.rb:90

The example description says this case raises JWT::VerificationError, but the expectation (and implementation in JWT::JWA::Ecdsa#sign) raises JWT::EncodeError when a non-private key is used for signing. Please update the it description to match the actual error class so the spec remains clear.

    context 'when the signing key is a public key' do
      it 'raises a JWT::VerificationError' do
        public_key = test_pkey('ec256-public.pem')
        expect do
          instance.sign(data: data, signing_key: public_key)
        end.to raise_error(JWT::EncodeError, 'The given key is not a private key')
      end

spec/jwt/jwa/ecdsa_spec.rb:98

This it description says JWT::VerificationError, but the assertion expects JWT::EncodeError (and #sign uses raise_sign_error!, which raises EncodeError for invalid signing keys). Update the spec description to avoid confusion about which error is raised during signing.

    context 'when the signing key is not an OpenSSL::PKey::EC instance' do
      it 'raises a JWT::VerificationError' do
        expect do
          instance.sign(data: data, signing_key: 'not_a_key')
        end.to raise_error(JWT::EncodeError, 'The given key is a String. It has to be an OpenSSL::PKey::EC instance')
      end

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

lib/jwt/jwa/signing_algorithm.rb:43

raise_verify_error! now raises JWT::VerificationError. Because this helper is used from some #sign implementations (e.g., JWT::JWA::Hmac#sign), JWT.encode can now raise a verification-focused error for signing/encoding failures (invalid signing key, empty HMAC secret on OpenSSL 3, enforced key length, etc.). Consider ensuring encoding-time failures raise JWT::EncodeError (e.g., update algorithm #sign implementations to use raise_sign_error! for signing key validation, or introduce a dedicated helper for signing-key errors) so callers can reliably distinguish encode vs verify failures.

      def raise_verify_error!(message)
        raise(VerificationError.new(message).tap { |e| e.set_backtrace(caller(1)) })
      end

      def raise_sign_error!(message)
        raise(EncodeError.new(message).tap { |e| e.set_backtrace(caller(1)) })
      end

spec/jwt/jwa/ecdsa_spec.rb:89

The example description says it “raises a JWT::VerificationError”, but the expectation is raise_error(JWT::EncodeError, ...). Please align the spec description with the actual error being asserted (likely JWT::EncodeError for signing failures).

      it 'raises a JWT::VerificationError' do
        public_key = test_pkey('ec256-public.pem')
        expect do
          instance.sign(data: data, signing_key: public_key)
        end.to raise_error(JWT::EncodeError, 'The given key is not a private key')

spec/jwt/jwa/ecdsa_spec.rb:98

This example description says it “raises a JWT::VerificationError”, but the assertion expects JWT::EncodeError. Update the spec wording so it matches the behavior under test (encode/sign errors vs verification errors).

      it 'raises a JWT::VerificationError' do
        expect do
          instance.sign(data: data, signing_key: 'not_a_key')
        end.to raise_error(JWT::EncodeError, 'The given key is a String. It has to be an OpenSSL::PKey::EC instance')
      end

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

🔵 Needs a closer look

A moderate signing-path hierarchy issue remains unresolved.

Review details

Suppressed comments (1)

spec/jwt/jwa/ecdsa_spec.rb:94

This signing test also says VerificationError while asserting EncodeError. Rename it to match the expected exception.

      it 'raises a JWT::VerificationError' do

Files reviewed: 22/22 changed files
Comments generated: 2
Review effort level: Balanced

Signing with an ECDSA key on a mismatched or unsupported curve raised IncorrectAlgorithm or UnsupportedEcdsaCurve, and an invalid HMAC key raised through the verify helper. Under the new hierarchy those are SignatureError < TokenError, so `JWT.encode` could fail with an error that is not an EncodeError, unlike its other signing failures and the Token#sign! contract. The ECDSA sign path now resolves the curve through a signing-side helper that raises EncodeError, and the HMAC key validation yields its message so sign and verify each raise their own error class. Verify paths are unchanged. Specs: rename the ECDSA sign examples that claimed VerificationError while asserting EncodeError, cover the curve mismatch on sign, and split the curve_name spec into an encode and a decode case. The JWK "ES384 key pointed to as ES512 key" spec was loading the P-384 fixture for both keys and only passed because encoding failed first; it now uses the P-521 fixture and exercises verification.

Claims::Verifier.errors used to rescue the catch-all DecodeError, so a payload segment that could not be decoded showed up as a claim error and EncodedToken#valid_claims?, #claim_errors and #valid? returned false or a list. Narrowing the rescue to ClaimValidationError let the MalformedTokenError raised while decoding the payload escape, turning those predicates into raisers. Rescue MalformedTokenError alongside ClaimValidationError so the predicate API keeps its contract, and cover the detached-and-missing payload and non-JSON payload cases, including #valid? with a signature that verifies.

Making DecodeError an alias for Error changed JWT::DecodeError.name to "JWT::Error", which silently breaks error-tracking groupings and log filters keyed on the class name, and widened rescue JWT::DecodeError to also catch JWT::EncodeError. Keeping it as a deprecated class between Error and TokenError preserves both, and leaves the only intended behaviour change: signing failures raise JWT::EncodeError. raise_verify_error! is only ever used for a key or algorithm that cannot be used, never for a signature that does not match, so it now raises a separate JWT::VerificationKeyError. On main those cases were plain DecodeError, so rescue JWT::VerificationError keeps meaning exactly what it means today rather than widening to cover key problems. The backwards compatibility spec now discovers the error classes instead of listing them, so a class added later is covered without touching it.

Layout/MultilineMethodCallIndentation disagreed about which call in the chain to align with, so drop the chain. Layout/LineLength is disabled in this project.

Copilot

🟡 Changes recommended

Verification-key failures remain inconsistently classified, and RSA/PSS can still leak NoMethodError.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 24/24 changed files
Comments generated: 3
Review effort level: Balanced

VerificationError was never limited to signature mismatches: JWA.create_verifiers raises it for a JWK that does not support the requested algorithm, and JWA::Unsupported#verify for an algorithm we cannot verify with. A sibling VerificationKeyError therefore left two paths contradicting its own contract, and moving them onto a sibling would have stopped 'rescue JWT::VerificationError' catching them. Making VerificationKeyError a subclass of VerificationError instead lets both move without narrowing anything: every rescue that worked before still works, and callers that want to tell an unusable key from a signature that does not match now can. While here, give RS* and PS* verification the key type guard their signing counterparts already have. Pointing an HMAC secret at an RSA algorithm used to raise a bare NoMethodError out of JWT.decode, which no JWT rescue caught, and the EC equivalent right next to it in the same spec file already raised a JWT error.

Copilot

🔵 Needs a closer look

The broad public error-contract change has conflicting readiness assessments and two unresolved documentation/test follow-ups.

Review details

Suppressed comments (2)

lib/jwt/encoded_token.rb:155

This public method can also raise JWT::MalformedTokenError when the encoded payload is empty or invalid JSON (as the updated specs exercise). Document that separately so callers using the new hierarchy know that claim verification can fail before claim validation begins.

    # @raise [JWT::ClaimValidationError] if the claims are invalid.

lib/jwt/jwa/ps.rb:22

This new PS verification branch is not covered by the existing PS specs: they exercise valid RSA keys, invalid signatures, and PKeyError, but never pass a non-RSA key. Add an example asserting that a String or EC verification key raises JWT::VerificationKeyError; otherwise the advertised replacement of the escaping NoMethodError can regress unnoticed.

        raise_verify_error!("The given key is a #{verification_key.class}. It has to be an OpenSSL::PKey::RSA instance") unless verification_key.is_a?(::OpenSSL::PKey::RSA)

Files reviewed: 28/28 changed files
Comments generated: 0 new
Review effort level: Balanced

The RS*/PS* key type guards only had integration coverage through decode_with_jwk_spec, and PS none at all, so a regression would have restored the escaping NoMethodError unnoticed. Both now have a unit example alongside their signing counterparts. EncodedToken#verify_claims! decodes the payload before validating anything, so it can raise JWT::MalformedTokenError as well as the documented JWT::ClaimValidationError.

Two narrow rescues stop working: rescue JWT::IncorrectAlgorithm and rescue JWT::UnsupportedEcdsaCurve around JWT.encode now see JWT::EncodeError instead. That is more than a patch release. Also qualify JWT::Claims::Error at its only use. It resolves through lexical scope today, but JWT::Error now exists one level up, so a bare Error under JWT::Claims is a trap for whoever writes the next file there.

Copilot

🔵 Needs a closer look

The changelog’s signing-error guarantee is inaccurate and must be narrowed or implemented and tested.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

CHANGELOG.md:10

The statement that signing failures now consistently raise JWT::EncodeError is broader than the implementation. JWA::Rsa#sign and JWA::Ps#sign still pass a public RSA key to OpenSSL, which raises ArgumentError (private key is needed) rather than a JWT error. Either translate and test those failures or narrow this release note to the ECDSA/HMAC cases actually covered, so consumers are not advised to rescue a class that will not catch every signing failure.

lib/jwt/jwa/ps.rb:22

The new user-facing message has the wrong article: “a OpenSSL…” should be “an OpenSSL…”. Please update this message and its matching spec expectation.

        raise_verify_error!("The given key is a #{verification_key.class}. It has to be an OpenSSL::PKey::RSA instance") unless verification_key.is_a?(::OpenSSL::PKey::RSA)

lib/jwt/jwa/rsa.rb:22

The new user-facing message has the wrong article: “a OpenSSL…” should be “an OpenSSL…”. Please update this message and its matching spec expectations.

        raise_verify_error!("The given key is a #{verification_key.class}. It has to be an OpenSSL::PKey::RSA instance") unless verification_key.is_a?(::OpenSSL::PKey::RSA)

Files reviewed: 31/31 changed files
Comments generated: 0 new
Review effort level: Balanced

The changelog promises that every signing failure raises JWT::EncodeError, but JWA::Rsa#sign and JWA::Ps#sign passed a public key straight to OpenSSL, which raised ArgumentError('private key is needed') out of JWT.encode. JWA::Ecdsa#sign already guards this; both now do the same.

public_to_pem needs a newer openssl gem than the Ruby 2.5 through 2.7 builds ship, and this gem supports Ruby >= 2.5. Read the public key from the fixture instead, matching how the ECDSA spec covers the same case.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The changelog entry had grown to five sentences and roughly 980 characters, against a 226 character longest entry anywhere else in the file. The claim verification revamp set the precedent: a one line changelog entry, with the migration detail in UPGRADING.md. The new section states the compatibility position plainly. Decoding is unaffected, so the common case needs no work. Signing failures moving to JWT::EncodeError is the part that breaks, and it gets a table of every affected case.

anakinj marked this pull request as ready for review March 27, 2026 19:28

anakinj mentioned this pull request Mar 27, 2026

ExpiredSignature should perhaps not be a subclass of DecodeError #606

Closed

anakinj requested a review from Copilot March 27, 2026 19:38

Copilot started reviewing on behalf of anakinj March 27, 2026 19:38 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Comment thread lib/jwt/error.rb Outdated

Comment thread spec/jwt/error_spec.rb Outdated

Comment thread lib/jwt/jwk/key_finder.rb Outdated

Comment thread lib/jwt/jwk/key_finder.rb

Comment thread lib/jwt/encoded_token.rb Outdated

anakinj requested a review from Copilot March 28, 2026 08:12

Copilot started reviewing on behalf of anakinj March 28, 2026 08:13 View session

Copilot AI reviewed Mar 28, 2026

View reviewed changes

Comment thread spec/jwt/jwk/decode_with_jwk_spec.rb Outdated

Comment thread spec/jwt/jwk/decode_with_jwk_spec.rb Outdated

Comment thread CHANGELOG.md Outdated

Comment thread spec/integration/readme_examples_spec.rb

Comment thread spec/jwt/token_spec.rb Outdated

Comment thread lib/jwt/decode.rb

anakinj requested a review from Copilot March 28, 2026 08:27

Copilot started reviewing on behalf of anakinj March 28, 2026 08:27 View session

Copilot AI reviewed Mar 28, 2026

View reviewed changes

Comment thread spec/jwt/jwt_spec.rb Outdated

Comment thread spec/jwt/jwt_spec.rb Outdated

anakinj requested a review from Copilot March 28, 2026 08:37

Copilot started reviewing on behalf of anakinj March 28, 2026 08:37 View session

Copilot AI reviewed Mar 28, 2026

View reviewed changes

anakinj force-pushed the error-hierarchy-revamp branch from 2f4926c to bec9291 Compare May 30, 2026 06:33

anakinj added 10 commits September 2, 2026 17:28

Revamp error hierarchy

deb6d43

More specific exception assertions and specific error types

de6d3fd

Deprecation comment

e2dac62

Changelog entry

4a0c991

React on copilot comments

5743c0e

Make specs test what they claim they are testing

07491ef

Sharpen examples and changelog

0f9aee7

Assert the specific error class in HMAC specs

be10522

Remove EC fixture no longer used by any spec

7a8a867

Correct changelog note on what rescue JWT::DecodeError now catches

a694655

anakinj force-pushed the error-hierarchy-revamp branch from 474e585 to a694655 Compare September 2, 2026 14:42

anakinj requested a balanced review from Copilot September 3, 2026 12:09

Copilot started reviewing on behalf of anakinj September 3, 2026 12:10 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Comment thread lib/jwt/error.rb

Comment thread spec/jwt/jwa/ecdsa_spec.rb Outdated

anakinj force-pushed the error-hierarchy-revamp branch from 80a69bb to 50bb298 Compare September 3, 2026 13:07

anakinj added 4 commits September 3, 2026 20:40

Merge branch 'main' into error-hierarchy-revamp

ce59589

Keep the error class lookup on one line

1959028

Layout/MultilineMethodCallIndentation disagreed about which call in the chain to align with, so drop the chain. Layout/LineLength is disabled in this project.

anakinj requested a balanced review from Copilot September 5, 2026 17:07

Copilot started reviewing on behalf of anakinj September 5, 2026 17:08 View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

Comment thread lib/jwt/error.rb

Comment thread lib/jwt/jwa.rb

Comment thread lib/jwt/jwa/signing_algorithm.rb

anakinj requested a balanced review from Copilot September 5, 2026 17:18

Copilot started reviewing on behalf of anakinj September 5, 2026 17:19 View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

anakinj added 2 commits September 5, 2026 20:25

anakinj requested a balanced review from Copilot September 5, 2026 17:33

Copilot started reviewing on behalf of anakinj September 5, 2026 17:33 View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

anakinj added 2 commits September 5, 2026 20:41

Use the RSA public key fixture in the new sign specs

957dc73

public_to_pem needs a newer openssl gem than the Ruby 2.5 through 2.7 builds ship, and this gem supports Ruby >= 2.5. Read the public key from the fixture instead, matching how the ECDSA spec covers the same case.

anakinj requested a balanced review from Copilot September 5, 2026 17:50

Copilot AI reviewed Sep 5, 2026

anakinj merged commit 6ab1be7 into jwt:main Sep 5, 2026
15 checks passed

Uh oh!

Conversation

anakinj commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (3)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anakinj commented Mar 27, 2026 •

edited

Loading