Skip to content

guard client storage access Fixes [#331] #332

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.

Sign up for GitHub

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

Open
Th-Shivam wants to merge 1 commit into
base: main
Choose a base branch
from
Open

guard client storage access Fixes [#331] #332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions src/contexts/ConnectionProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
useState,
useCallback,
} from "react"
import { safeGetItem, safeSetItem } from "../utils/storage"

type ConnectionStatus = "connecting" | "connected" | "disconnected"

Expand Down Expand Up @@ -63,21 +64,12 @@ export function ConnectionProvider({
const urlParams = new URLSearchParams(window.location.search)
const urlToken = urlParams.get("token")

let storedToken: string | null = null
try {
storedToken = localStorage.getItem("rein_auth_token")
} catch (e) {
// Restricted context (e.g. private mode)
}
const storedToken = safeGetItem("rein_auth_token")

const token = urlToken || storedToken

if (urlToken && urlToken !== storedToken) {
try {
localStorage.setItem("rein_auth_token", urlToken)
} catch (e) {
// Failed to store
}
safeSetItem("rein_auth_token", urlToken)
Comment on lines +67 to +72
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid persisting auth tokens in localStorage-backed storage.

rein_auth_token remains script-readable at rest, which increases token theft risk in any XSS scenario (also written in src/routes/settings.tsx, Line 86). Prefer HttpOnly secure cookies (server-managed) or at least session-scoped memory strategy for browser clients.

As per coding guidelines, "No exposed API keys or sensitive data" and "Use expo-secure-store for sensitive storage".

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/contexts/ConnectionProvider.tsx` around lines 67 - 72, The code persists
the auth token under the key "rein_auth_token" using safeGetItem/safeSetItem in
ConnectionProvider (token/urlToken logic), which exposes it to script-accessible
storage; remove the localStorage-backed persistence and switch to a secure
storage strategy: stop calling safeSetItem("rein_auth_token", ...) and stop
reading from safeGetItem for auth tokens, instead keep the token in React
state/session memory or delegate storage to a server-set HttpOnly secure cookie
(preferred) or use a platform secure store (e.g., expo-secure-store) via
SecureStore.getItemAsync/ setItemAsync if client-side secure storage is
required; update any related usages (including the settings route) to read from
the new mechanism and ensure token propagation uses the ConnectionProvider state
or server cookie rather than localStorage.

}

let wsUrl = `${protocol}//${host}/ws`
Expand Down
4 changes: 2 additions & 2 deletions src/routes/__root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
useConnection,
} from "../contexts/ConnectionProvider"
import { useCaptureProvider } from "../hooks/useCaptureProvider"
import { safeGetItem } from "../utils/storage"

export const Route = createRootRoute({
component: AppWithConnection,
Expand Down Expand Up @@ -67,8 +68,7 @@ function DesktopCaptureProvider() {

function ThemeInit() {
useEffect(() => {
if (typeof localStorage === "undefined") return
const saved = localStorage.getItem(APP_CONFIG.THEME_STORAGE_KEY)
const saved = safeGetItem(APP_CONFIG.THEME_STORAGE_KEY)
const theme =
saved === THEMES.LIGHT || saved === THEMES.DARK ? saved : THEMES.DEFAULT
document.documentElement.setAttribute("data-theme", theme)
Expand Down
43 changes: 17 additions & 26 deletions src/routes/settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@ import QRCode from "qrcode"
import { useEffect, useState } from "react"
import { APP_CONFIG, THEMES } from "../config"
import serverConfig from "../server-config.json"
import {
getStoredBoolean,
getStoredNumber,
safeGetItem,
safeSetItem,
} from "../utils/storage"

export const Route = createFileRoute("/settings")({
component: SettingsPage,
Expand All @@ -19,40 +25,25 @@ function SettingsPage() {

// Client Side Settings (LocalStorage)
const [invertScroll, setInvertScroll] = useState(() => {
if (typeof window === "undefined") return false
try {
const saved = localStorage.getItem("rein_invert")
return saved === "true"
} catch {
return false
}
return getStoredBoolean("rein_invert", false)
})

const [sensitivity, setSensitivity] = useState(() => {
if (typeof window === "undefined") return 1.0
const saved = localStorage.getItem("rein_sensitivity")
const parsed = saved ? Number.parseFloat(saved) : Number.NaN
return Number.isFinite(parsed) ? parsed : 1.0
return getStoredNumber("rein_sensitivity", 1.0)
})

const [theme, setTheme] = useState(() => {
if (typeof window === "undefined") return THEMES.DEFAULT
try {
const saved = localStorage.getItem(APP_CONFIG.THEME_STORAGE_KEY)
return saved === THEMES.LIGHT || saved === THEMES.DARK
? saved
: THEMES.DEFAULT
} catch {
return THEMES.DEFAULT
}
const saved = safeGetItem(APP_CONFIG.THEME_STORAGE_KEY)
return saved === THEMES.LIGHT || saved === THEMES.DARK
? saved
: THEMES.DEFAULT
})

const [qrData, setQrData] = useState("")

// Load initial state (IP is not stored in localStorage; only sensitivity, invert, theme are client settings)
const [authToken, setAuthToken] = useState(() => {
if (typeof window === "undefined") return ""
return localStorage.getItem("rein_auth_token") || ""
return safeGetItem("rein_auth_token") || ""
})

// Derive URLs once at the top
Expand Down Expand Up @@ -92,7 +83,7 @@ function SettingsPage() {
if (data.type === "token-generated" && data.token) {
if (isMounted) {
setAuthToken(data.token)
localStorage.setItem("rein_auth_token", data.token)
safeSetItem("rein_auth_token", data.token)
}
socket.close()
}
Expand All @@ -114,17 +105,17 @@ function SettingsPage() {

// Effect: Update LocalStorage when settings change
useEffect(() => {
localStorage.setItem("rein_sensitivity", String(sensitivity))
safeSetItem("rein_sensitivity", String(sensitivity))
}, [sensitivity])

useEffect(() => {
localStorage.setItem("rein_invert", JSON.stringify(invertScroll))
safeSetItem("rein_invert", JSON.stringify(invertScroll))
}, [invertScroll])

// Effect: Theme
useEffect(() => {
if (typeof window === "undefined") return
localStorage.setItem(APP_CONFIG.THEME_STORAGE_KEY, theme)
safeSetItem(APP_CONFIG.THEME_STORAGE_KEY, theme)
document.documentElement.setAttribute("data-theme", theme)
}, [theme])

Expand Down
9 changes: 3 additions & 6 deletions src/routes/trackpad.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { TouchArea } from "../components/Trackpad/TouchArea"
import { useRemoteConnection } from "../hooks/useRemoteConnection"
import { useTrackpadGesture } from "../hooks/useTrackpadGesture"
import { ScreenMirror } from "../components/Trackpad/ScreenMirror"
import { getStoredBoolean, getStoredNumber } from "../utils/storage"

export const Route = createFileRoute("/trackpad")({
component: TrackpadPage,
Expand All @@ -25,15 +26,11 @@ function TrackpadPage() {

// Load Client Settings
const [sensitivity] = useState(() => {
if (typeof window === "undefined") return 1.0
const s = localStorage.getItem("rein_sensitivity")
return s ? Number.parseFloat(s) : 1.0
return getStoredNumber("rein_sensitivity", 1.0)
})

const [invertScroll] = useState(() => {
if (typeof window === "undefined") return false
const s = localStorage.getItem("rein_invert")
return s ? JSON.parse(s) : false
return getStoredBoolean("rein_invert", false)
})

const { send, sendCombo } = useRemoteConnection()
Expand Down
82 changes: 82 additions & 0 deletions src/utils/storage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import { afterEach, describe, expect, it, vi } from "vitest"
import {
getStoredBoolean,
getStoredNumber,
safeGetItem,
safeSetItem,
} from "./storage"

describe("storage utils", () => {
afterEach(() => {
vi.unstubAllGlobals()
})

it("returns null when localStorage access throws on read", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => {
throw new Error("blocked")
},
},
} as unknown as Window,
)

expect(safeGetItem("rein_auth_token")).toBeNull()
})

it("returns false when localStorage access throws on write", () => {
vi.stubGlobal(
"window",
{
localStorage: {
setItem: () => {
throw new Error("blocked")
},
},
} as unknown as Window,
)

expect(safeSetItem("rein_auth_token", "token")).toBe(false)
})

it("falls back when stored number is invalid", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => "not-a-number",
},
} as unknown as Window,
)

expect(getStoredNumber("rein_sensitivity", 1)).toBe(1)
})

it("falls back when stored boolean is malformed", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => "{oops",
},
} as unknown as Window,
)

expect(getStoredBoolean("rein_invert", false)).toBe(false)
})

it("supports JSON encoded booleans", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => "true",
},
} as unknown as Window,
)

expect(getStoredBoolean("rein_invert", false)).toBe(true)
})
Comment on lines +70 to +81
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test named "supports JSON encoded booleans" currently stubs getItem to return the exact string "true", which is handled by the early saved === "true" branch and never exercises the JSON.parse(saved) path. Either rename this test to reflect what it actually covers (boolean strings), or adjust/add a test case that forces the JSON.parse branch (e.g., leading/trailing whitespace) so that code path is covered.

Copilot uses AI. Check for mistakes.
Comment on lines +14 to +81
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Add tests for no-window branch and partially malformed numeric values.

Current coverage is good, but these two branches are still missing and are directly relevant to this PR’s resilience goal.

Suggested test additions 
+	it("returns null when window is unavailable", () => {
+		vi.stubGlobal("window", undefined as unknown as Window)
+		expect(safeGetItem("rein_auth_token")).toBeNull()
+	})
+
+	it("falls back when number has trailing invalid characters", () => {
+		vi.stubGlobal(
+			"window",
+			{
+				localStorage: {
+					getItem: () => "1.2abc",
+				},
+			} as unknown as Window,
+		)
+		expect(getStoredNumber("rein_sensitivity", 1)).toBe(1)
+	})

As per coding guidelines, "**/*.test.{ts,tsx,js,jsx}: Review test files for: Comprehensive coverage of component behavior".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("returns null when localStorage access throws on read", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => {
throw new Error("blocked")
},
},
} as unknown as Window,
)
expect(safeGetItem("rein_auth_token")).toBeNull()
})
it("returns false when localStorage access throws on write", () => {
vi.stubGlobal(
"window",
{
localStorage: {
setItem: () => {
throw new Error("blocked")
},
},
} as unknown as Window,
)
expect(safeSetItem("rein_auth_token", "token")).toBe(false)
})
it("falls back when stored number is invalid", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => "not-a-number",
},
} as unknown as Window,
)
expect(getStoredNumber("rein_sensitivity", 1)).toBe(1)
})
it("falls back when stored boolean is malformed", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => "{oops",
},
} as unknown as Window,
)
expect(getStoredBoolean("rein_invert", false)).toBe(false)
})
it("supports JSON encoded booleans", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => "true",
},
} as unknown as Window,
)
expect(getStoredBoolean("rein_invert", false)).toBe(true)
})
it("returns null when localStorage access throws on read", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => {
throw new Error("blocked")
},
},
} as unknown as Window,
)
expect(safeGetItem("rein_auth_token")).toBeNull()
})
it("returns false when localStorage access throws on write", () => {
vi.stubGlobal(
"window",
{
localStorage: {
setItem: () => {
throw new Error("blocked")
},
},
} as unknown as Window,
)
expect(safeSetItem("rein_auth_token", "token")).toBe(false)
})
it("falls back when stored number is invalid", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => "not-a-number",
},
} as unknown as Window,
)
expect(getStoredNumber("rein_sensitivity", 1)).toBe(1)
})
it("falls back when stored boolean is malformed", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => "{oops",
},
} as unknown as Window,
)
expect(getStoredBoolean("rein_invert", false)).toBe(false)
})
it("supports JSON encoded booleans", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => "true",
},
} as unknown as Window,
)
expect(getStoredBoolean("rein_invert", false)).toBe(true)
})
it("returns null when window is unavailable", () => {
vi.stubGlobal("window", undefined as unknown as Window)
expect(safeGetItem("rein_auth_token")).toBeNull()
})
it("falls back when number has trailing invalid characters", () => {
vi.stubGlobal(
"window",
{
localStorage: {
getItem: () => "1.2abc",
},
} as unknown as Window,
)
expect(getStoredNumber("rein_sensitivity", 1)).toBe(1)
})
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/utils/storage.test.ts` around lines 14 - 81, Add two tests: one that
stubs absence of window (vi.stubGlobal("window", undefined)) to exercise the
no-window branch for safeGetItem/safeSetItem/getStoredNumber/getStoredBoolean
and assert they return their safe defaults (null/false/default number/default
boolean); and one that stubs localStorage.getItem to return a partially
malformed numeric string (e.g., "42abc") and assert
getStoredNumber("rein_sensitivity", 1) falls back or coerces as intended (match
the implementation's expected behavior); reference safeGetItem, safeSetItem, and
getStoredNumber/getStoredBoolean when adding these test cases.

})
42 changes: 42 additions & 0 deletions src/utils/storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
export function safeGetItem(key: string): string | null {
if (typeof window === "undefined") return null

try {
return window.localStorage.getItem(key)
} catch {
return null
}
}

export function safeSetItem(key: string, value: string): boolean {
if (typeof window === "undefined") return false

try {
window.localStorage.setItem(key, value)
return true
} catch {
return false
}
}

export function getStoredNumber(key: string, fallback: number): number {
const saved = safeGetItem(key)
if (saved === null) return fallback

const parsed = Number.parseFloat(saved)
return Number.isFinite(parsed) ? parsed : fallback
Comment on lines +22 to +27
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In JavaScript, what do Number.parseFloat("1abc") and Number("1abc") return, and which approach rejects trailing non-numeric characters?

💡 Result:

Number.parseFloat("1abc") returns 1, while Number("1abc") returns NaN. The Number approach rejects trailing non-numeric characters, whereas Number.parseFloat parses the leading numeric portion and ignores the rest.

Citations:

Use strict number parsing to preserve malformed-value fallback.

Line 26 uses Number.parseFloat, which accepts partial values (e.g., "1abc" → 1). This conflicts with the malformed-value fallback requirement. Use Number() instead, which rejects strings with trailing non-numeric characters.

Proposed fix 
 export function getStoredNumber(key: string, fallback: number): number {
 	const saved = safeGetItem(key)
 	if (saved === null) return fallback

-	const parsed = Number.parseFloat(saved)
+	const normalized = saved.trim()
+	if (normalized === "") return fallback
+	const parsed = Number(normalized)
 	return Number.isFinite(parsed) ? parsed : fallback
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function getStoredNumber(key: string, fallback: number): number {
const saved = safeGetItem(key)
if (saved === null) return fallback
const parsed = Number.parseFloat(saved)
return Number.isFinite(parsed) ? parsed : fallback
export function getStoredNumber(key: string, fallback: number): number {
const saved = safeGetItem(key)
if (saved === null) return fallback
const normalized = saved.trim()
if (normalized === "") return fallback
const parsed = Number(normalized)
return Number.isFinite(parsed) ? parsed : fallback
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/utils/storage.ts` around lines 22 - 27, The getStoredNumber function
currently uses Number.parseFloat which accepts partial numeric strings (e.g.,
"1abc") and thus bypasses the malformed-value fallback; change the parsing to
use Number(saved) to force strict conversion, keep the existing
Number.isFinite(parsed) check to return the fallback on invalid values, and
update the return path in getStoredNumber accordingly so only fully numeric
strings are accepted.

}

export function getStoredBoolean(key: string, fallback: boolean): boolean {
const saved = safeGetItem(key)
if (saved === null) return fallback
if (saved === "true") return true
if (saved === "false") return false

try {
const parsed = JSON.parse(saved)
return typeof parsed === "boolean" ? parsed : fallback
} catch {
return fallback
}
}
Loading