Skip to content

Conversation

betaphi
Copy link
Contributor

@betaphi betaphi commented Jul 2, 2025

Bug Description

When using the X5CVerifier to verify a Payload that conforms to ValidationTimePayload the signedDate of the ValidationTimePayload would be used to initialize a RFC5280Policy. Unfortunately the default @PolicyBuilder that is used in the respective verifyJWS function also contained a RFC5280Policy, which was always initialized to the current date.

As both policies need to be met in order for the verification to succeed, the ValidationTimePayload's signedDate was effectively ignored. Other negative side effects could be described but I'll keep it short here.

Proposed Solution

As @resultBuilders can not be optional in Swift, a way was needed to provide a default verification policy that would not affect the policy that would be created further down the code depending on the payload's conformance to ValidationTimePayload. I have therefore created a dummy EmptyPolicy, which is always met.
The solution does not have any negative side effects, as a RFC5280Policy is added further down the code in any case. Only this time, it is either initialized with the signedDate of a ValidationTimePayload or the current date.

@betaphi betaphi requested a review from ptoffy as a code owner July 2, 2025 15:39
@0xTim
Copy link
Member

0xTim commented Jul 2, 2025

Thanks! Can you add a test to ensure that the original bug doesn't get reintroduced?

@betaphi
Copy link
Contributor Author

betaphi commented Jul 2, 2025

We would need a correctly signed token with a signedDate that lies in the past. Also, one of the certificates in the chain should be expired by now. I have such tokens here for testing but they contain private data and therefore cannot be shared. At this point I have no capacity to create such a token.

@betaphi
Copy link
Contributor Author

betaphi commented Jul 25, 2025

May I please ask you again to accept the PR?
I know that not having a test ist non-optimal, but having non-functional software in a production environment is worse isn't it? 😉

Copy link
Member

@ptoffy ptoffy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I'm happy to accept it once the few nits are resolved. I doubt many people are using this anyway.

Comment on lines 1 to 7
//
// AlwaysMeetsPolicy.swift
// jwt-kit
//
// Created by Bastian Rössler on 02.07.25.
//

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this please?


@inlinable
public func chainMeetsPolicyRequirements(chain: UnverifiedCertificateChain) -> PolicyEvaluationResult {
return .meetsPolicy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return .meetsPolicy
.meetsPolicy

import SwiftASN1

/// This Policy acts as a placeholder. Its result is always positive.
@available(macOS 10.15, iOS 13, watchOS 6, tvOS 13, macCatalyst 13, visionOS 1.0, *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed as JWTKit already requires higher platform versions

@betaphi
Copy link
Contributor Author

betaphi commented Sep 15, 2025

@ptoffy I have just made the requested changes. Would be great if you could accept the PR now :-)

Copy link

codecov bot commented Sep 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.55%. Comparing base (84a328e) to head (2e09a95).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
+ Coverage   83.52%   83.55%   +0.03%     
==========================================
  Files          56       57       +1     
  Lines        1493     1496       +3     
==========================================
+ Hits         1247     1250       +3     
  Misses        246      246              
Files with missing lines Coverage Δ
Sources/JWTKit/X5C/EmptyPolicy.swift 100.00% <100.00%> (ø)
Sources/JWTKit/X5C/X5CVerifier.swift 85.88% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ptoffy ptoffy self-requested a review September 15, 2025 10:55
Copy link
Member

@ptoffy ptoffy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Unfortunately we're not quite there yet. Could you fix the compiler error please? Also, this needs to go through a swift format pass first (formatting file is here)

@betaphi
Copy link
Contributor Author

betaphi commented Sep 15, 2025

The compiler error was caused by an update to a dependency library. I just fixed it.

About the swift format thing: The whole repo does not comply to the current format. As a result, if I apply it, lots of files will be changed. Therefore I would highly suggest to do this in a separate PR.

@ptoffy ptoffy self-requested a review October 15, 2025 17:13
@ptoffy ptoffy merged commit ce1e5d0 into vapor:main Oct 15, 2025
15 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants