-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Require matching email for iOS pairing #6028
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
Changes from all commits
ddaaa86
0050317
57b8589
b5b1fa3
de56d8e
0baf8a7
64e861e
2339bd2
a42dc7d
5cb9b73
5995237
bbacd8c
bb13811
b363f86
74632bb
c313540
becf69f
b3e3019
26b87b5
4be32eb
0452e49
d759c83
f7043f1
38f745e
e5beff0
7518ab7
6cb1647
a74a9fc
4a5dae3
af3c18b
7142689
3e506ec
173742f
005cedc
454c29f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,13 +1,14 @@ | ||||||||||||||||||||||
| import Foundation | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// The minimal pairing-QR grammar: plain `host:port` routes in the URL query, | ||||||||||||||||||||||
| /// nothing else. | ||||||||||||||||||||||
| /// The minimal pairing-QR grammar: expected Mac account/build metadata plus | ||||||||||||||||||||||
| /// plain `host:port` routes in the URL query. | ||||||||||||||||||||||
| /// | ||||||||||||||||||||||
| /// `cmux-ios://attach?v=2&r=<host>:<port>[&r=<host>:<port>...]` | ||||||||||||||||||||||
| /// `cmux-ios://attach?v=2&ub=<stack-user-id>&pc=<compat>&av=<version>&ab=<build>&r=<host>:<port>[&r=<host>:<port>...]` | ||||||||||||||||||||||
| /// | ||||||||||||||||||||||
| /// A pairing QR needs to tell the phone exactly one thing: where to dial. | ||||||||||||||||||||||
| /// Everything else the earlier grammars carried has a better channel or no | ||||||||||||||||||||||
| /// reason to exist: | ||||||||||||||||||||||
| /// A pairing QR needs to tell the phone where to dial and which non-secret | ||||||||||||||||||||||
| /// account/build context to check before dialing. The account value is the | ||||||||||||||||||||||
| /// opaque Stack user id, never the email itself. Everything else the earlier | ||||||||||||||||||||||
| /// grammars carried has a better channel or no reason to exist: | ||||||||||||||||||||||
| /// - **No auth token.** The owner's Stack access token is the host's sole | ||||||||||||||||||||||
| /// authorization gate; a token in the QR authorized nothing and made the | ||||||||||||||||||||||
| /// code look like a leaked credential. | ||||||||||||||||||||||
|
|
@@ -62,14 +63,28 @@ public struct CmxPairingQRCode: Sendable { | |||||||||||||||||||||
| guard let routes = encodableRoutes(of: ticket) else { | ||||||||||||||||||||||
| return nil | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| var items: [String] = ["v=\(Self.version)"] | ||||||||||||||||||||||
| if let userID = normalizedNonEmpty(ticket.macUserID) { | ||||||||||||||||||||||
| items.append("ub=\(percentEncodeQueryValue(userID))") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if let compatibilityVersion = ticket.macPairingCompatibilityVersion { | ||||||||||||||||||||||
| items.append("pc=\(compatibilityVersion)") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if let version = normalizedNonEmpty(ticket.macAppVersion) { | ||||||||||||||||||||||
| items.append("av=\(percentEncodeQueryValue(version))") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if let build = normalizedNonEmpty(ticket.macAppBuild) { | ||||||||||||||||||||||
| items.append("ab=\(percentEncodeQueryValue(build))") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| let routeItems = routes.map { route -> String in | ||||||||||||||||||||||
| guard case let .hostPort(host, port) = route.endpoint else { | ||||||||||||||||||||||
| // Unreachable: `encodableRoutes` admits host/port endpoints only. | ||||||||||||||||||||||
| return "" | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return "r=\(hostPortString(host: host, port: port))" | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return "cmux-ios://attach?v=\(Self.version)&" + routeItems.joined(separator: "&") | ||||||||||||||||||||||
| items.append(contentsOf: routeItems) | ||||||||||||||||||||||
| return "cmux-ios://attach?" + items.joined(separator: "&") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// Whether `ticket` is expressible in the minimal grammar; see | ||||||||||||||||||||||
|
|
@@ -171,6 +186,11 @@ public struct CmxPairingQRCode: Sendable { | |||||||||||||||||||||
| terminalID: nil, | ||||||||||||||||||||||
| macDeviceID: "", | ||||||||||||||||||||||
| macDisplayName: nil, | ||||||||||||||||||||||
| macUserEmail: queryValue(named: "e", in: components), | ||||||||||||||||||||||
| macUserID: queryValue(named: "ub", in: components), | ||||||||||||||||||||||
| macPairingCompatibilityVersion: queryInt(named: "pc", in: components) ?? 0, | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reject malformed On Line 191, Suggested fix- macPairingCompatibilityVersion: queryInt(named: "pc", in: components) ?? 0,
+ macPairingCompatibilityVersion: try parseCompatibilityVersion(in: components), private extension CmxPairingQRCode {
+ func parseCompatibilityVersion(in components: URLComponents) throws -> Int {
+ guard let raw = components.queryItems?.first(where: { $0.name == "pc" })?.value else {
+ return 0
+ }
+ guard let parsed = Int(raw.trimmingCharacters(in: .whitespacesAndNewlines)) else {
+ throw MobileSyncPairingPayloadError.invalidURL
+ }
+ return parsed
+ }
+
func queryInt(named name: String, in components: URLComponents) -> Int? {
guard let value = queryValue(named: name, in: components) else { return nil }
return Int(value)
}🤖 Prompt for AI Agents |
||||||||||||||||||||||
| macAppVersion: queryValue(named: "av", in: components), | ||||||||||||||||||||||
| macAppBuild: queryValue(named: "ab", in: components), | ||||||||||||||||||||||
| routes: routes, | ||||||||||||||||||||||
| expiresAt: nil, | ||||||||||||||||||||||
| authToken: nil | ||||||||||||||||||||||
|
|
@@ -243,4 +263,24 @@ private extension CmxPairingQRCode { | |||||||||||||||||||||
| || byte == UInt8(ascii: ":") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| func queryValue(named name: String, in components: URLComponents) -> String? { | ||||||||||||||||||||||
| normalizedNonEmpty(components.queryItems?.first(where: { $0.name == name })?.value) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| func queryInt(named name: String, in components: URLComponents) -> Int? { | ||||||||||||||||||||||
| guard let value = queryValue(named: name, in: components) else { return nil } | ||||||||||||||||||||||
| return Int(value) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| func normalizedNonEmpty(_ value: String?) -> String? { | ||||||||||||||||||||||
| let trimmed = value?.trimmingCharacters(in: .whitespacesAndNewlines) | ||||||||||||||||||||||
| return trimmed?.isEmpty == false ? trimmed : nil | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| func percentEncodeQueryValue(_ value: String) -> String { | ||||||||||||||||||||||
| var allowed = CharacterSet.urlQueryAllowed | ||||||||||||||||||||||
| allowed.remove(charactersIn: "&=+") | ||||||||||||||||||||||
| return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? value | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+281
to
+285
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | 💤 Low value Consider explicit handling when percent-encoding fails. The fallback While encoding failure is rare for typical email/version strings, explicit handling would be more defensive. 🛡️ Safer fallback optionsOption 1: Return empty string and skip the parameter: func percentEncodeQueryValue(_ value: String) -> String {
var allowed = CharacterSet.urlQueryAllowed
allowed.remove(charactersIn: "&=+")
- return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? value
+ return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? ""
}Then filter empty results: if let email = normalizedNonEmpty(ticket.macUserEmail) {
- items.append("e=\(percentEncodeQueryValue(email))")
+ let encoded = percentEncodeQueryValue(email)
+ if !encoded.isEmpty {
+ items.append("e=\(encoded)")
+ }
}Option 2: Use a safe placeholder: - return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? value
+ return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? "ENCODING_FAILED"📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| } | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,13 +12,21 @@ struct CompactAttachTicket: Codable { | |
| let w: String? | ||
| let t: String? | ||
| let d: String | ||
| let u: String? | ||
| let pc: Int? | ||
| let av: String? | ||
| let ab: String? | ||
| let r: [CompactAttachRoute] | ||
|
|
||
| init(_ ticket: CmxAttachTicket) { | ||
| v = ticket.version | ||
| w = Self.normalizedNonEmpty(ticket.workspaceID) | ||
| t = Self.normalizedNonEmpty(ticket.terminalID) | ||
| d = ticket.macDeviceID | ||
| u = Self.normalizedNonEmpty(ticket.macUserID) | ||
| pc = ticket.macPairingCompatibilityVersion | ||
| av = Self.normalizedNonEmpty(ticket.macAppVersion) | ||
| ab = Self.normalizedNonEmpty(ticket.macAppBuild) | ||
| r = Self.compactedRoutes(ticket.routes) | ||
| } | ||
|
Comment on lines
21
to
31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normalization inconsistency across metadata fields.
Since these values come from controlled sources (authenticated email, Bundle info), the practical risk is low, but consistency would prevent subtle edge-case bugs. 🔧 Proposed fix to align normalizationUpdate private static func normalizedNonEmpty(_ value: String?) -> String? {
- guard let value, !value.isEmpty else {
- return nil
- }
- return value
+ let trimmed = value?.trimmingCharacters(in: .whitespacesAndNewlines)
+ return trimmed?.isEmpty == false ? trimmed : nil
}Note: This also affects 🤖 Prompt for AI Agents |
||
|
|
||
|
|
@@ -29,6 +37,11 @@ struct CompactAttachTicket: Codable { | |
| terminalID: t, | ||
| macDeviceID: d, | ||
| macDisplayName: nil, | ||
| macUserEmail: u?.contains("@") == true ? u : nil, | ||
| macUserID: u?.contains("@") == false ? u : nil, | ||
| macPairingCompatibilityVersion: pc ?? 0, | ||
| macAppVersion: av, | ||
| macAppBuild: ab, | ||
| routes: Self.expandedRoutes(r), | ||
| expiresAt: nil | ||
| ) | ||
|
|
@@ -40,7 +53,9 @@ struct CompactAttachTicket: Codable { | |
| } | ||
| return value | ||
| } | ||
|
|
||
| } | ||
|
|
||
| private extension CompactAttachTicket { | ||
| /// Encode routes, omitting each route id the decoder can resynthesize | ||
| /// (`kind` for the first route of a kind, `kind_N` for the Nth; exactly | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.