Swift 6 #106
Swift 6 #106
Conversation
| case algES256 = -7 | ||
| /// AlgES384 ECDSA with SHA-384 | ||
| case algES384 = -35 | ||
| /// AlgES512 ECDSA with SHA-512 | ||
| case algES512 = -36 | ||
| case algES256 = -7 | ||
| /// AlgES384 ECDSA with SHA-384 | ||
| case algES384 = -35 | ||
| /// AlgES512 ECDSA with SHA-512 | ||
| case algES512 = -36 | ||
|
|
||
| // We don't support RSA yet | ||
| // We don't support RSA yet | ||
|
|
||
| // /// AlgRS1 RSASSA-PKCS1-v1_5 with SHA-1 | ||
| // case algRS1 = -65535 | ||
| // /// AlgRS256 RSASSA-PKCS1-v1_5 with SHA-256 | ||
| // case algRS256 = -257 | ||
| // /// AlgRS384 RSASSA-PKCS1-v1_5 with SHA-384 | ||
| // case algRS384 = -258 | ||
| // /// AlgRS512 RSASSA-PKCS1-v1_5 with SHA-512 | ||
| // case algRS512 = -259 | ||
| // /// AlgPS256 RSASSA-PSS with SHA-256 | ||
| // case algPS256 = -37 | ||
| // /// AlgPS384 RSASSA-PSS with SHA-384 | ||
| // case algPS384 = -38 | ||
| // /// AlgPS512 RSASSA-PSS with SHA-512 | ||
| // case algPS512 = -39 | ||
| // // AlgEdDSA EdDSA | ||
| // case algEdDSA = -8 | ||
| // /// AlgRS1 RSASSA-PKCS1-v1_5 with SHA-1 | ||
| // case algRS1 = -65535 | ||
| // /// AlgRS256 RSASSA-PKCS1-v1_5 with SHA-256 | ||
| // case algRS256 = -257 | ||
| // /// AlgRS384 RSASSA-PKCS1-v1_5 with SHA-384 | ||
| // case algRS384 = -258 | ||
| // /// AlgRS512 RSASSA-PKCS1-v1_5 with SHA-512 | ||
| // case algRS512 = -259 | ||
| // /// AlgPS256 RSASSA-PSS with SHA-256 | ||
| // case algPS256 = -37 | ||
| // /// AlgPS384 RSASSA-PSS with SHA-384 | ||
| // case algPS384 = -38 | ||
| // /// AlgPS512 RSASSA-PSS with SHA-512 | ||
| // case algPS512 = -39 | ||
| // // AlgEdDSA EdDSA | ||
| // case algEdDSA = -8 |
There was a problem hiding this comment.
These were all tabs 🤪
There was a problem hiding this comment.
Haha yeah this got picked up in #88
| /// Assuming valid CBOR, verify the public key's length by decoding the next CBOR item. | ||
| /// Assuming valid CBOR, verify the public key's length by decoding the next CBOR item, and checking how much data is left on the stream. | ||
| let inputStream = ByteInputStream(data[credentialIDEndIndex...]) | ||
| let decoder = CBORDecoder(stream: inputStream, options: CBOROptions(maximumDepth: 16)) | ||
| _ = try decoder.decodeItem() | ||
| do { | ||
| let decoder = CBORDecoder(stream: inputStream, options: CBOROptions(maximumDepth: 16)) | ||
| _ = try decoder.decodeItem() | ||
| } catch { throw .invalidPublicKeyLength } |
There was a problem hiding this comment.
Took a chance to define a new error case for this and clarify what is happening.
dimitribouniol
force-pushed
the
dimitri/swift-6
branch
from
31cf768 to
8cbe548
Compare
There was a problem hiding this comment.
Some comments
Overall, most of the changes here are stylistic rather then related to Swift 6 so I suggest we split out the Swift 6 changes into a separate PR and have this PR as a formatting PR, integrating swift-format at the same time to keep everything consistent going forward (with the relevant CI checks)
| throw WebAuthnError.authDataTooShort | ||
| } | ||
| guard bytes.count >= minAuthDataLength | ||
| else { throw .authDataTooShort } |
There was a problem hiding this comment.
Am not a fan of else being on a separate line 😆 we should get swift format integrated with an agreed style first
There was a problem hiding this comment.
And here I was being consistent with all my recent commits adhering to this style 🤣
Joking aside, I think this style is much more readable than the assembly of variants I replaced, as conditions can all be aligned, and the else clearly represents a one-line return or throw. When the else case has multiple statements, I agree that it looks better joined to the condition though. Either way, I've undone these fixes and will re-propose them in the context of a format PR 😄
| case algES256 = -7 | ||
| /// AlgES384 ECDSA with SHA-384 | ||
| case algES384 = -35 | ||
| /// AlgES512 ECDSA with SHA-512 | ||
| case algES512 = -36 | ||
| case algES256 = -7 | ||
| /// AlgES384 ECDSA with SHA-384 | ||
| case algES384 = -35 | ||
| /// AlgES512 ECDSA with SHA-512 | ||
| case algES512 = -36 | ||
|
|
||
| // We don't support RSA yet | ||
| // We don't support RSA yet | ||
|
|
||
| // /// AlgRS1 RSASSA-PKCS1-v1_5 with SHA-1 | ||
| // case algRS1 = -65535 | ||
| // /// AlgRS256 RSASSA-PKCS1-v1_5 with SHA-256 | ||
| // case algRS256 = -257 | ||
| // /// AlgRS384 RSASSA-PKCS1-v1_5 with SHA-384 | ||
| // case algRS384 = -258 | ||
| // /// AlgRS512 RSASSA-PKCS1-v1_5 with SHA-512 | ||
| // case algRS512 = -259 | ||
| // /// AlgPS256 RSASSA-PSS with SHA-256 | ||
| // case algPS256 = -37 | ||
| // /// AlgPS384 RSASSA-PSS with SHA-384 | ||
| // case algPS384 = -38 | ||
| // /// AlgPS512 RSASSA-PSS with SHA-512 | ||
| // case algPS512 = -39 | ||
| // // AlgEdDSA EdDSA | ||
| // case algEdDSA = -8 | ||
| // /// AlgRS1 RSASSA-PKCS1-v1_5 with SHA-1 | ||
| // case algRS1 = -65535 | ||
| // /// AlgRS256 RSASSA-PKCS1-v1_5 with SHA-256 | ||
| // case algRS256 = -257 | ||
| // /// AlgRS384 RSASSA-PKCS1-v1_5 with SHA-384 | ||
| // case algRS384 = -258 | ||
| // /// AlgRS512 RSASSA-PKCS1-v1_5 with SHA-512 | ||
| // case algRS512 = -259 | ||
| // /// AlgPS256 RSASSA-PSS with SHA-256 | ||
| // case algPS256 = -37 | ||
| // /// AlgPS384 RSASSA-PSS with SHA-384 | ||
| // case algPS384 = -38 | ||
| // /// AlgPS512 RSASSA-PSS with SHA-512 | ||
| // case algPS512 = -39 | ||
| // // AlgEdDSA EdDSA | ||
| // case algEdDSA = -8 |
There was a problem hiding this comment.
Haha yeah this got picked up in #88
dimitribouniol
force-pushed
the
dimitri/swift-6
branch
from
8cbe548 to
3df7bba
Compare
I was actually prepping a separate format PR, but was waiting for in progress PRs to finish so I don't cause merge conflicts for everyone haha. I've simplified this PR to only Swift 6 features, which includes the manifest, explicit any, and typed throws where it doesn't involve much lift. |
Made Swift 6 the default, and combed through the codebase to adopt the latest language features.
I would have liked to adopt more typed throws, but for now I limited it to non-public members. We can have a separate discussion as to whether it's useful to wrap the CBOR, decoding, and cert-related errors or not (ostensibly, none of them would be the user's fault, as they are receiving them from things like the browser, so I'd argue that in many cases we should wrap them? Either way, for another PR).