-
Notifications
You must be signed in to change notification settings - Fork 15.1k
fix: safe env var parsing, error handling, localStorage validation, and hook fixes #29823
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
53fad91
3bed106
a19a7fc
fa25ece
39096ef
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,4 +1,5 @@ | ||
| export const contructEmailFromPhoneNumber = (phoneNumber: string) => { | ||
| if (!phoneNumber) return ""; | ||
| const cleanedPhoneNumber = phoneNumber.replace(/\+/g, ""); | ||
| return `${cleanedPhoneNumber}@sms.cal.com`; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import { downloadAsCsv, objectsToCsv, sanitizeValue } from "./csvUtils"; | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| describe("sanitizeValue", () => { | ||
| it("returns simple values unchanged", () => { | ||
| expect(sanitizeValue("hello")).toBe("hello"); | ||
| }); | ||
|
|
||
| it("wraps values with commas in quotes", () => { | ||
| expect(sanitizeValue("hello,world")).toBe('"hello,world"'); | ||
| }); | ||
|
|
||
| it("wraps values with newlines in quotes", () => { | ||
| expect(sanitizeValue("hello\nworld")).toBe('"hello\nworld"'); | ||
| }); | ||
|
|
||
| it("doubles quotes and wraps values containing double quotes", () => { | ||
| expect(sanitizeValue('he said "hello"')).toBe('"he said ""hello"""'); | ||
| }); | ||
|
|
||
| it("handles values with quotes and commas", () => { | ||
| expect(sanitizeValue('he said "hello", world')).toBe('"he said ""hello"", world"'); | ||
| }); | ||
|
|
||
| it("prefixes formula-trigger characters with single quote to prevent injection", () => { | ||
| expect(sanitizeValue("=HYPERLINK(http://evil.com)")).toBe("'=HYPERLINK(http://evil.com)"); | ||
| expect(sanitizeValue("+SUM(A1:A10)")).toBe("'+SUM(A1:A10)"); | ||
| expect(sanitizeValue("-SUM(A1:A10)")).toBe("'-SUM(A1:A10)"); | ||
| expect(sanitizeValue("@SUM(A1:A10)")).toBe("'@SUM(A1:A10)"); | ||
| }); | ||
|
|
||
| it("does not prefix non-formula values", () => { | ||
| expect(sanitizeValue("hello")).toBe("hello"); | ||
| expect(sanitizeValue("123")).toBe("123"); | ||
| expect(sanitizeValue("john@example.com")).toBe("john@example.com"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("objectsToCsv", () => { | ||
| it("handles values with newlines correctly", () => { | ||
| const data = [ | ||
| { name: "Alice", note: "hello\nworld" }, | ||
| { name: "Bob", note: "normal" }, | ||
| ]; | ||
| const csv = objectsToCsv(data); | ||
| expect(csv).toBe('name,note\nAlice,"hello\nworld"\nBob,normal'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| import { extractBaseEmail } from "./extract-base-email"; | ||
|
|
||
| describe("extractBaseEmail", () => { | ||
| it("should extract base email removing plus aliases", () => { | ||
| expect(extractBaseEmail("user+alias@example.com")).toBe("user@example.com"); | ||
| }); | ||
|
|
||
| it("should return email unchanged if no plus alias", () => { | ||
| expect(extractBaseEmail("user@example.com")).toBe("user@example.com"); | ||
| }); | ||
|
|
||
| it("should return input unchanged if no @ sign", () => { | ||
| expect(extractBaseEmail("notanemail")).toBe("notanemail"); | ||
| }); | ||
|
|
||
| it("should handle empty string", () => { | ||
| expect(extractBaseEmail("")).toBe(""); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| // Function to extract base email | ||
| export const extractBaseEmail = (email: string): string => { | ||
| const [localPart, domain] = email.split("@"); | ||
| const atIndex = email.indexOf("@"); | ||
| if (atIndex === -1) return email; | ||
| const localPart = email.substring(0, atIndex); | ||
| const domain = email.substring(atIndex + 1); | ||
| const baseLocalPart = localPart.split("+")[0]; | ||
| return `${baseLocalPart}@${domain}`; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,10 @@ | ||
| import { parsePhoneNumberWithError } from "libphonenumber-js/max"; | ||
|
|
||
| export const formatPhoneNumber = (phoneNumber: string) => { | ||
| const parsedPhoneNumber = parsePhoneNumberWithError(phoneNumber); | ||
| return parsedPhoneNumber?.isValid() ? parsedPhoneNumber.formatInternational() : phoneNumber; | ||
| try { | ||
| const parsedPhoneNumber = parsePhoneNumberWithError(phoneNumber); | ||
| return parsedPhoneNumber?.isValid() ? parsedPhoneNumber.formatInternational() : phoneNumber; | ||
| } catch { | ||
| return phoneNumber; | ||
| } | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,7 +37,12 @@ const banlistSchema = z.array(z.string()); | |
| export function isIpInBanlist(request: Request | NextApiRequest) { | ||
| const IP = getIP(request); | ||
| const rawBanListJson = process.env.IP_BANLIST || "[]"; | ||
| const banList = banlistSchema.parse(JSON.parse(rawBanListJson)); | ||
| let banList: string[] = []; | ||
| try { | ||
| banList = banlistSchema.parse(JSON.parse(rawBanListJson)); | ||
| } catch { | ||
| logger.error("Invalid IP_BANLIST JSON; treating as empty banlist"); | ||
| } | ||
|
Comment on lines
+40
to
+45
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. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift Do not fail open when Both catch blocks leave Also applies to: 55-60 🤖 Prompt for AI Agents |
||
| if (banList.includes(IP)) { | ||
| logger.warn(`Found banned IP: ${IP} in IP_BANLIST`); | ||
| return true; | ||
|
|
@@ -47,7 +52,12 @@ export function isIpInBanlist(request: Request | NextApiRequest) { | |
|
|
||
| export function isIpInBanListString(identifer: string) { | ||
| const rawBanListJson = process.env.IP_BANLIST || "[]"; | ||
| const banList = banlistSchema.parse(JSON.parse(rawBanListJson)); | ||
| let banList: string[] = []; | ||
| try { | ||
| banList = banlistSchema.parse(JSON.parse(rawBanListJson)); | ||
| } catch { | ||
| logger.error("Invalid IP_BANLIST JSON; treating as empty banlist"); | ||
| } | ||
| if (banList.includes(identifer)) { | ||
| logger.warn(`Found banned IP: ${identifer} in IP_BANLIST`); | ||
| return true; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,8 @@ | ||
| export const validJson = (jsonString: string) => { | ||
| try { | ||
| const o = JSON.parse(jsonString); | ||
| if (o && typeof o === "object") { | ||
| return o; | ||
| } | ||
| return JSON.parse(jsonString); | ||
| } catch (e) { | ||
| console.log("Invalid JSON:", e); | ||
| return false; | ||
| } | ||
|
Comment on lines
1
to
7
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. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Use an unambiguous parse-failure result. After accepting every JSON type, 🤖 Prompt for AI Agents |
||
| return false; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ export function getRecentImpersonations(): RecentImpersonation[] { | |
| export function addRecentImpersonation(usernameRaw: string): void { | ||
| try { | ||
| const recent = getRecentImpersonations(); | ||
| if (!Array.isArray(recent)) return; | ||
| const username = usernameRaw.trim().toLowerCase(); | ||
| if (!username) return; | ||
| const filtered = recent.filter((item) => item.username !== username); | ||
|
Comment on lines
23
to
27
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Validate array entries before filtering.
🤖 Prompt for AI Agents |
||
|
|
||
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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse the environment value itself and validate the result.
Wrapping the value in brackets makes valid JSON arrays become nested arrays (
["a"]→[["a"]]), whileJSON.parsecan also return non-string values despite thestring[]annotation. ParseenvVardirectly, accept only arrays of strings, then use the CSV fallback.Proposed fix
const safeParseCsvEnvVar = (envVar: string | undefined): string[] => { if (!envVar) return []; try { - return JSON.parse(`[${envVar}]`); + const parsed: unknown = JSON.parse(envVar); + if (Array.isArray(parsed) && parsed.every((value) => typeof value === "string")) { + return parsed; + } } catch { - return envVar.split(",").map((s) => s.trim().replace(/^"|"$/g, "")); } + return envVar.split(",").map((s) => s.trim().replace(/^"|"$/g, "")); };📝 Committable suggestion
🤖 Prompt for AI Agents