From d814b6cdf0ffccc4fc9390d740b88ccd8308d533 Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Thu, 23 May 2024 21:01:18 +0530 Subject: [PATCH 1/9] Use standard URL parsing - WIP 1 --- web/apps/auth/src/services/code.ts | 65 +++++++++--------------------- 1 file changed, 20 insertions(+), 45 deletions(-) diff --git a/web/apps/auth/src/services/code.ts b/web/apps/auth/src/services/code.ts index 064cc7874..09e1ec567 100644 --- a/web/apps/auth/src/services/code.ts +++ b/web/apps/auth/src/services/code.ts @@ -1,5 +1,5 @@ +import { ensure } from "@/utils/ensure"; import { HOTP, TOTP } from "otpauth"; -import { URI } from "vscode-uri"; /** * A parsed representation of an *OTP code URI. @@ -32,7 +32,7 @@ export interface Code { /** The (HMAC) algorithm used by the OTP generator. */ algorithm: "sha1" | "sha256" | "sha512"; /** The original string from which this code was generated. */ - uriString?: string; + uriString: string; } /** @@ -47,50 +47,25 @@ export interface Code { * otpauth://totp/ACME:user@example.org?algorithm=SHA1&digits=6&issuer=acme&period=30&secret=ALPHANUM */ export const codeFromURIString = (id: string, uriString: string): Code => { - const santizedRawData = uriString - .replaceAll("+", "%2B") - .replaceAll(":", "%3A") - .replaceAll("\r", "") - // trim quotes - .replace(/^"|"$/g, ""); - - const uriParams = {}; - const searchParamsString = - decodeURIComponent(santizedRawData).split("?")[1]; - searchParamsString.split("&").forEach((pair) => { - const [key, value] = pair.split("="); - uriParams[key] = value; - }); - - const uri = URI.parse(santizedRawData); - let uriPath = decodeURIComponent(uri.path); - if (uriPath.startsWith("/otpauth://") || uriPath.startsWith("otpauth://")) { - uriPath = uriPath.split("otpauth://")[1]; - } else if (uriPath.startsWith("otpauth%3A//")) { - uriPath = uriPath.split("otpauth%3A//")[1]; - } + const url = new URL(uriString); return { id, - type: _getType(uriPath), + type: parseType(url), account: _getAccount(uriPath), issuer: _getIssuer(uriPath, uriParams), - digits: parseDigits(uriParams), - period: parsePeriod(uriParams), - secret: parseSecret(uriParams), - algorithm: parseAlgorithm(uriParams), + digits: parseDigits(url), + period: parsePeriod(url), + secret: parseSecret(url), + algorithm: parseAlgorithm(url), uriString, }; }; -const _getType = (uriPath: string): Code["type"] => { - const oauthType = uriPath.split("/")[0].substring(0); - if (oauthType.toLowerCase() === "totp") { - return "totp"; - } else if (oauthType.toLowerCase() === "hotp") { - return "hotp"; - } - throw new Error(`Unsupported format with host ${oauthType}`); +const parseType = (url: URL): Code["type"] => { + const t = url.host.toLowerCase(); + if (t == "totp" || t == "hotp") return t; + throw new Error(`Unsupported code with host ${t}`); }; const _getAccount = (uriPath: string): string => { @@ -131,14 +106,14 @@ const _getIssuer = (uriPath: string, uriParams: { get?: any }): string => { } }; -const parseDigits = (uriParams): number => - parseInt(uriParams["digits"] ?? "", 10) || 6; +const parseDigits = (url: URL): number => + parseInt(url.searchParams.get("digits") ?? "", 10) || 6; -const parsePeriod = (uriParams): number => - parseInt(uriParams["period"] ?? "", 10) || 30; +const parsePeriod = (url: URL): number => + parseInt(url.searchParams.get("period") ?? "", 10) || 30; -const parseAlgorithm = (uriParams): Code["algorithm"] => { - switch (uriParams["algorithm"]?.toLowerCase()) { +const parseAlgorithm = (url: URL): Code["algorithm"] => { + switch (url.searchParams.get("algorithm")?.toLowerCase()) { case "sha256": return "sha256"; case "sha512": @@ -148,8 +123,8 @@ const parseAlgorithm = (uriParams): Code["algorithm"] => { } }; -const parseSecret = (uriParams): string => - uriParams["secret"].replaceAll(" ", "").toUpperCase(); +const parseSecret = (url: URL): string => + ensure(url.searchParams.get("secret")).replaceAll(" ", "").toUpperCase(); /** * Generate a pair of OTPs (one time passwords) from the given {@link code}. From 0a01cac57baad9166bb21b30ef3e4e2fffb70553 Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Fri, 24 May 2024 09:27:28 +0530 Subject: [PATCH 2/9] Take 1 (incorrect) --- web/apps/auth/src/pages/auth.tsx | 15 ++++++--------- web/apps/auth/src/services/code.ts | 19 +++++-------------- web/apps/photos/package.json | 1 - web/yarn.lock | 5 ----- 4 files changed, 11 insertions(+), 29 deletions(-) diff --git a/web/apps/auth/src/pages/auth.tsx b/web/apps/auth/src/pages/auth.tsx index 4006cc9e1..526489779 100644 --- a/web/apps/auth/src/pages/auth.tsx +++ b/web/apps/auth/src/pages/auth.tsx @@ -46,14 +46,11 @@ const AuthenticatorCodesPage = () => { appContext.showNavBar(false); }, []); + const lcSearch = searchTerm.toLowerCase(); const filteredCodes = codes.filter( - (secret) => - (secret.issuer ?? "") - .toLowerCase() - .includes(searchTerm.toLowerCase()) || - (secret.account ?? "") - .toLowerCase() - .includes(searchTerm.toLowerCase()), + (code) => + code.issuer?.toLowerCase().includes(lcSearch) || + code.account?.toLowerCase().includes(lcSearch), ); if (!hasFetched) { @@ -270,7 +267,7 @@ const OTPDisplay: React.FC = ({ code, otp, nextOTP }) => { textAlign: "left", }} > - {code.issuer} + {code.issuer ?? ""}

= ({ code, otp, nextOTP }) => { color: "grey", }} > - {code.account} + {code.account ?? ""}

{ return { id, type: parseType(url), - account: _getAccount(uriPath), + account: parseAccount(url), issuer: _getIssuer(uriPath, uriParams), digits: parseDigits(url), period: parsePeriod(url), @@ -68,18 +68,9 @@ const parseType = (url: URL): Code["type"] => { throw new Error(`Unsupported code with host ${t}`); }; -const _getAccount = (uriPath: string): string => { - try { - const path = decodeURIComponent(uriPath); - if (path.includes(":")) { - return path.split(":")[1]; - } else if (path.includes("/")) { - return path.split("/")[1]; - } - } catch (e) { - return ""; - } -}; +/** Convert the pathname from "/ACME:user@example.org" => "user@example.org" */ +const parseAccount = (url: URL): string => + url.pathname.split(":").at(-1).split("/").at(-1); const _getIssuer = (uriPath: string, uriParams: { get?: any }): string => { try { diff --git a/web/apps/photos/package.json b/web/apps/photos/package.json index 1541878c5..0ec924b29 100644 --- a/web/apps/photos/package.json +++ b/web/apps/photos/package.json @@ -43,7 +43,6 @@ "similarity-transformation": "^0.0.1", "transformation-matrix": "^2.16", "uuid": "^9.0.1", - "vscode-uri": "^3.0.7", "xml-js": "^1.6.11", "zxcvbn": "^4.4.2" }, diff --git a/web/yarn.lock b/web/yarn.lock index 894a44dd0..aaa0d517a 100644 --- a/web/yarn.lock +++ b/web/yarn.lock @@ -4804,11 +4804,6 @@ void-elements@3.1.0: resolved "https://registry.yarnpkg.com/void-elements/-/void-elements-3.1.0.tgz#614f7fbf8d801f0bb5f0661f5b2f5785750e4f09" integrity sha512-Dhxzh5HZuiHQhbvTW9AMetFfBHDMYpo23Uo9btPXgdYP+3T5S+p+jgNy7spra+veYhBP2dCSgxR/i2Y02h5/6w== -vscode-uri@^3.0.7: - version "3.0.8" - resolved "https://registry.yarnpkg.com/vscode-uri/-/vscode-uri-3.0.8.tgz#1770938d3e72588659a172d0fd4642780083ff9f" - integrity sha512-AyFQ0EVmsOZOlAnxoFOGOq1SQDWAB7C6aqMGS23svWAllfOaxbuFvcT8D1i8z3Gyn8fraVeZNNmN6e9bxxXkKw== - webidl-conversions@^3.0.0: version "3.0.1" resolved "https://registry.yarnpkg.com/webidl-conversions/-/webidl-conversions-3.0.1.tgz#24534275e2a7bc6be7bc86611cc16ae0a5654871" From bfe8fd83acfb7974cf39f827afd2ebee8d9eb255 Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Fri, 24 May 2024 09:29:54 +0530 Subject: [PATCH 3/9] Take 2 --- web/apps/auth/src/services/code.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/web/apps/auth/src/services/code.ts b/web/apps/auth/src/services/code.ts index ff29c58fd..0acc537f0 100644 --- a/web/apps/auth/src/services/code.ts +++ b/web/apps/auth/src/services/code.ts @@ -69,8 +69,12 @@ const parseType = (url: URL): Code["type"] => { }; /** Convert the pathname from "/ACME:user@example.org" => "user@example.org" */ -const parseAccount = (url: URL): string => - url.pathname.split(":").at(-1).split("/").at(-1); +const parseAccount = (url: URL): string => { + let p = url.pathname; + if (p.startsWith("/")) p = p.slice(1); + if (p.includes(":")) p = p.split(":").slice(1).join(":"); + return p; +}; const _getIssuer = (uriPath: string, uriParams: { get?: any }): string => { try { From 623b71715d08e8428a19edec97b1304981712e4e Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Fri, 24 May 2024 09:42:23 +0530 Subject: [PATCH 4/9] Wrap --- web/apps/auth/src/services/code.ts | 45 ++++++++++++++---------------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/web/apps/auth/src/services/code.ts b/web/apps/auth/src/services/code.ts index 0acc537f0..4eb964d31 100644 --- a/web/apps/auth/src/services/code.ts +++ b/web/apps/auth/src/services/code.ts @@ -53,7 +53,7 @@ export const codeFromURIString = (id: string, uriString: string): Code => { id, type: parseType(url), account: parseAccount(url), - issuer: _getIssuer(uriPath, uriParams), + issuer: parseIssuer(url), digits: parseDigits(url), period: parsePeriod(url), secret: parseSecret(url), @@ -68,37 +68,34 @@ const parseType = (url: URL): Code["type"] => { throw new Error(`Unsupported code with host ${t}`); }; -/** Convert the pathname from "/ACME:user@example.org" => "user@example.org" */ -const parseAccount = (url: URL): string => { +const parseAccount = (url: URL): string | undefined => { + // "/ACME:user@example.org" => "user@example.org" let p = url.pathname; if (p.startsWith("/")) p = p.slice(1); if (p.includes(":")) p = p.split(":").slice(1).join(":"); return p; }; -const _getIssuer = (uriPath: string, uriParams: { get?: any }): string => { - try { - if (uriParams["issuer"] !== undefined) { - let issuer = uriParams["issuer"]; - // This is to handle bug in the ente auth app - if (issuer.endsWith("period")) { - issuer = issuer.substring(0, issuer.length - 6); - } - return issuer; +const parseIssuer = (url: URL): string => { + // If there is a "issuer" search param, use that. + let issuer = url.searchParams.get("issuer"); + if (issuer !== undefined) { + // This is to handle bug in old versions of Ente Auth app. + if (issuer.endsWith("period")) { + issuer = issuer.substring(0, issuer.length - 6); } - let path = decodeURIComponent(uriPath); - if (path.startsWith("totp/") || path.startsWith("hotp/")) { - path = path.substring(5); - } - if (path.includes(":")) { - return path.split(":")[0]; - } else if (path.includes("-")) { - return path.split("-")[0]; - } - return path; - } catch (e) { - return ""; + return issuer; } + + // Otherwise use the `prefix:` from the account as the issuer. + // "/ACME:user@example.org" => "ACME" + let p = url.pathname; + if (p.startsWith("/")) p = p.slice(1); + + if (p.includes(":")) p = p.split(":")[0]; + else if (p.includes("-")) p = p.split("-")[0]; + + return p; }; const parseDigits = (url: URL): number => From 59ed89cba172fdb668502d4d79345710e1e67225 Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Fri, 24 May 2024 09:49:20 +0530 Subject: [PATCH 5/9] .get returns null when the property is not present --- web/apps/auth/src/services/code.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/apps/auth/src/services/code.ts b/web/apps/auth/src/services/code.ts index 4eb964d31..835bb689d 100644 --- a/web/apps/auth/src/services/code.ts +++ b/web/apps/auth/src/services/code.ts @@ -79,7 +79,7 @@ const parseAccount = (url: URL): string | undefined => { const parseIssuer = (url: URL): string => { // If there is a "issuer" search param, use that. let issuer = url.searchParams.get("issuer"); - if (issuer !== undefined) { + if (issuer) { // This is to handle bug in old versions of Ente Auth app. if (issuer.endsWith("period")) { issuer = issuer.substring(0, issuer.length - 6); From 2ce921245778a031d8a817193d6b20d32474c67c Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Fri, 24 May 2024 09:57:16 +0530 Subject: [PATCH 6/9] We encodeURIComponent the pathname --- web/apps/auth/src/services/code.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/web/apps/auth/src/services/code.ts b/web/apps/auth/src/services/code.ts index 835bb689d..a43a3d473 100644 --- a/web/apps/auth/src/services/code.ts +++ b/web/apps/auth/src/services/code.ts @@ -45,6 +45,8 @@ export interface Code { * * - (TOTP) * otpauth://totp/ACME:user@example.org?algorithm=SHA1&digits=6&issuer=acme&period=30&secret=ALPHANUM + * + * See also `auth/test/models/code_test.dart`. */ export const codeFromURIString = (id: string, uriString: string): Code => { const url = new URL(uriString); @@ -70,7 +72,7 @@ const parseType = (url: URL): Code["type"] => { const parseAccount = (url: URL): string | undefined => { // "/ACME:user@example.org" => "user@example.org" - let p = url.pathname; + let p = decodeURIComponent(url.pathname); if (p.startsWith("/")) p = p.slice(1); if (p.includes(":")) p = p.split(":").slice(1).join(":"); return p; @@ -89,7 +91,7 @@ const parseIssuer = (url: URL): string => { // Otherwise use the `prefix:` from the account as the issuer. // "/ACME:user@example.org" => "ACME" - let p = url.pathname; + let p = decodeURIComponent(url.pathname); if (p.startsWith("/")) p = p.slice(1); if (p.includes(":")) p = p.split(":")[0]; From eaf8b9cebc770eee44d418cb7aa7a61124ae82af Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Fri, 24 May 2024 10:10:59 +0530 Subject: [PATCH 7/9] Also include same workaround as mobile app --- web/apps/auth/src/services/code.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/web/apps/auth/src/services/code.ts b/web/apps/auth/src/services/code.ts index a43a3d473..a04409cee 100644 --- a/web/apps/auth/src/services/code.ts +++ b/web/apps/auth/src/services/code.ts @@ -49,6 +49,19 @@ export interface Code { * See also `auth/test/models/code_test.dart`. */ export const codeFromURIString = (id: string, uriString: string): Code => { + try { + return _codeFromURIString(id, uriString); + } catch (e) { + // We might have legacy encodings of account names that contain a "#", + // which causes the rest of the URL to be treated as a fragment, and + // ignored. See if this was potentially such a case, otherwise rethrow. + if (uriString.includes("#")) + return _codeFromURIString(id, uriString.replaceAll("#", "%23")); + throw e; + } +}; + +const _codeFromURIString = (id: string, uriString: string): Code => { const url = new URL(uriString); return { From fec040e5285b843cad9a28bcf8ff1ec87e37dfe9 Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Fri, 24 May 2024 10:19:53 +0530 Subject: [PATCH 8/9] Tweak error report --- web/apps/auth/src/services/code.ts | 2 +- web/apps/auth/src/services/remote.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/web/apps/auth/src/services/code.ts b/web/apps/auth/src/services/code.ts index a04409cee..901411dd3 100644 --- a/web/apps/auth/src/services/code.ts +++ b/web/apps/auth/src/services/code.ts @@ -80,7 +80,7 @@ const _codeFromURIString = (id: string, uriString: string): Code => { const parseType = (url: URL): Code["type"] => { const t = url.host.toLowerCase(); if (t == "totp" || t == "hotp") return t; - throw new Error(`Unsupported code with host ${t}`); + throw new Error(`Unsupported code with host "${t}"`); }; const parseAccount = (url: URL): string | undefined => { diff --git a/web/apps/auth/src/services/remote.ts b/web/apps/auth/src/services/remote.ts index 07b15d7d7..11d57aa23 100644 --- a/web/apps/auth/src/services/remote.ts +++ b/web/apps/auth/src/services/remote.ts @@ -35,7 +35,7 @@ export const getAuthCodes = async (): Promise => { ); return codeFromURIString(entity.id, decryptedCode); } catch (e) { - log.error(`failed to parse codeId = ${entity.id}`); + log.error(`Failed to parse codeID ${entity.id}`, e); return null; } }), From dc38a8bc9f851d6c7a9db3a14d36ccc7b93d211c Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Fri, 24 May 2024 10:51:19 +0530 Subject: [PATCH 9/9] Account for node/browser discrepancy --- web/apps/auth/src/services/code.ts | 39 +++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/web/apps/auth/src/services/code.ts b/web/apps/auth/src/services/code.ts index 901411dd3..c0be011ea 100644 --- a/web/apps/auth/src/services/code.ts +++ b/web/apps/auth/src/services/code.ts @@ -64,11 +64,27 @@ export const codeFromURIString = (id: string, uriString: string): Code => { const _codeFromURIString = (id: string, uriString: string): Code => { const url = new URL(uriString); + // A URL like + // + // new URL("otpauth://hotp/Test?secret=AAABBBCCCDDDEEEFFF&issuer=Test&counter=0") + // + // is parsed differently by the browser and Node depending on the scheme. + // When the scheme is http(s), then both of them consider "hotp" as the + // `host`. However, when the scheme is "otpauth", as is our case, the + // browser considers the entire thing as part of the pathname. so we get. + // + // host: "" + // pathname: "//hotp/Test" + // + // Since this code run on browsers only, we parse as per that behaviour. + + const [type, path] = parsePathname(url); + return { id, - type: parseType(url), - account: parseAccount(url), - issuer: parseIssuer(url), + type, + account: parseAccount(path), + issuer: parseIssuer(url, path), digits: parseDigits(url), period: parsePeriod(url), secret: parseSecret(url), @@ -77,21 +93,22 @@ const _codeFromURIString = (id: string, uriString: string): Code => { }; }; -const parseType = (url: URL): Code["type"] => { - const t = url.host.toLowerCase(); - if (t == "totp" || t == "hotp") return t; - throw new Error(`Unsupported code with host "${t}"`); +const parsePathname = (url: URL): [type: Code["type"], path: string] => { + const p = url.pathname.toLowerCase(); + if (p.startsWith("//totp")) return ["totp", url.pathname.slice(6)]; + if (p.startsWith("//hotp")) return ["hotp", url.pathname.slice(6)]; + throw new Error(`Unsupported code or unparseable path "${url.pathname}"`); }; -const parseAccount = (url: URL): string | undefined => { +const parseAccount = (path: string): string | undefined => { // "/ACME:user@example.org" => "user@example.org" - let p = decodeURIComponent(url.pathname); + let p = decodeURIComponent(path); if (p.startsWith("/")) p = p.slice(1); if (p.includes(":")) p = p.split(":").slice(1).join(":"); return p; }; -const parseIssuer = (url: URL): string => { +const parseIssuer = (url: URL, path: string): string => { // If there is a "issuer" search param, use that. let issuer = url.searchParams.get("issuer"); if (issuer) { @@ -104,7 +121,7 @@ const parseIssuer = (url: URL): string => { // Otherwise use the `prefix:` from the account as the issuer. // "/ACME:user@example.org" => "ACME" - let p = decodeURIComponent(url.pathname); + let p = decodeURIComponent(path); if (p.startsWith("/")) p = p.slice(1); if (p.includes(":")) p = p.split(":")[0];