-
Notifications
You must be signed in to change notification settings - Fork 50
Fix base64URL to base64 conversion character mapping #107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix base64URL to base64 conversion character mapping #107
Conversation
The conversion between base64URL and standard base64 had incorrect character mappings. Updated both directions: - base64URLToBase64: now correctly maps - → + and _ → / - base64ToBase64URL: now correctly maps + → - and / → _ Added comprehensive unit tests to verify the conversion logic including padding and character replacement.
|
This issue is already present in the JWT returned by the production IAP API, and with the current SDK version it results in app-store-server-library-swift/Sources/AppStoreServerLibrary/ChainVerifier.swift Lines 39 to 41 in a1013d5
|
| import XCTest | ||
| @testable import AppStoreServerLibrary | ||
|
|
||
| final class Base64URLToBase64Tests: XCTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not already tested by the ChainVerifier tests now that TestingUtility is corrected? Could we stick with those tests please, if so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TestingUtility method is only triggered when the Base64 generated from a JSON file contains one of -, +, /, or _. Right now the issue doesn’t occur simply by chance, and even if we prepare test data that hits this case, any future changes to the JSON could remove those characters from the Base64, making the test difficult to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public func testEscapedJWT() async {
/// {"czP": "ml>"}
/// base64: eyJjelAiOiAibWw+In0=
struct SignedString: DecodedSignedData, Codable, Hashable {
let czP: String
var signedDateOptional: Date? = nil
}
let chainVerifier = TestingUtility.getChainVerifier()
let result = await chainVerifier.verify(
signedData: "eyJhbGciOiJFUzI1NiJ9.eyJjelAiOiAibWw+In0=.stM6DaOF2ihXJHhU9vqcxxrtYctS7qc6IioF8co_00KiqvaLlSdpog11Yc8TbmSj099babIeSm_Ei53-ZioWFg",
type: SignedString.self,
onlineVerification: false,
environment: .localTesting
)
guard case .valid = result else {
XCTFail("Expected .valid, got \(result)")
return
}
}I tried a few approaches, and it seems we could at least prepare tests like the ones I’ve put together. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shimastripe Could we please remove this test now and break this out into a separate PR for separate consideration so we can go ahead and get this fix merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexanderjordanbaker Sure. 6c05478
alexanderjordanbaker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
WHAT
The conversion between base64URL and standard base64 had incorrect character mappings. Updated both directions:
-→+and_→/+→-and/→_Added comprehensive unit tests to verify the conversion logic including padding and character replacement.
ref