-
Notifications
You must be signed in to change notification settings - Fork 1
Add test apps for react router #18
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
base: main
Are you sure you want to change the base?
Conversation
| className="flex items-center gap-2 px-4 py-2 bg-indigo-50 rounded-lg hover:bg-indigo-100 transition" | ||
| > | ||
| <img | ||
| src={user.avatar} |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
DOM text
DOM text
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 33 minutes ago
General approach: Ensure any untrusted text taken from the DOM and later used in a context that could be interpreted as HTML or part of a URL is properly encoded/escaped. In this case, we should not interpolate username verbatim into the avatar URL; instead, we should URL-encode it so meta-characters cannot break out of the intended seed parameter value.
Best concrete fix: In apps/react-router/react-router-v7-project/app/lib/utils/auth.ts, update the fakeSignup function so that the avatar URL uses encodeURIComponent(username) when building the Dicebear API URL. This keeps the existing functionality (avatar still deterministically derived from username) while ensuring any special characters in username are safely encoded and cannot affect the structure of the URL or any later HTML interpretation.
Specific changes:
- File:
apps/react-router/react-router-v7-project/app/lib/utils/auth.ts- In
fakeSignup, change:const avatar = \https://api.dicebear.com/7.x/avataaars/svg?seed=${username}\``
- To:
- In
- No additional imports are needed because
encodeURIComponentis a standard global function. - No changes are necessary in
NavbarorSignup; they will continue to work as before, now with a safely encoded avatar URL.
This single change ensures the tainted username is encoded before it flows into user.avatar and ultimately into the <img src> attribute, addressing all CodeQL variants related to this flow.
-
Copy modified line R68
| @@ -65,7 +65,7 @@ | ||
|
|
||
| export function fakeSignup(username: string, email: string, password: string): FakeUser { | ||
| // Fake signup - creates a new user | ||
| const avatar = `https://api.dicebear.com/7.x/avataaars/svg?seed=${username}` | ||
| const avatar = `https://api.dicebear.com/7.x/avataaars/svg?seed=${encodeURIComponent(username)}` | ||
| const newUser: FakeUser = { | ||
| id: `user_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, | ||
| username, |
| if (loggedInUser) { | ||
| // Identify the user in PostHog | ||
| posthog?.identify(loggedInUser.id, { | ||
| username: loggedInUser.username, |
Check failure
Code scanning / CodeQL
Insecure randomness High
Math.random()
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 31 minutes ago
To fix the problem, we should stop using Math.random() to generate the user ID in fakeSignup and instead create the ID using a cryptographically secure random generator. In a browser environment, we can rely on crypto.getRandomValues; in Node we could use crypto.randomBytes. Since this code is in a Remix/React app and appears to run in the browser (using localStorage), window.crypto.getRandomValues is appropriate.
Concretely, in apps/react-router/react-router-v7-project/app/lib/utils/auth.ts, we will replace the id field construction in fakeSignup:
id: `user_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`,with a helper call that uses crypto.getRandomValues to generate a URL‑safe random string of comparable length. We’ll define a small helper function generateSecureId(prefix: string, length: number): string near the top of the file, which creates a random byte array, encodes it in base64, and makes it URL‑safe, then truncates to the desired length. No external libraries are needed; we use the built‑in Web Crypto API. This preserves the overall shape of the IDs (user_<timestamp>_<randomPart>), so existing users already stored in localStorage remain valid, while new users get cryptographically strong IDs.
No changes are needed in app/routes/login.tsx; the only insecure randomness is in auth.ts.
-
Copy modified lines R20-R43 -
Copy modified line R94
| @@ -17,6 +17,30 @@ | ||
| const STORAGE_KEY = 'fake_country_explorer_user' | ||
| const USERS_KEY = 'fake_country_explorer_users' | ||
|
|
||
| function generateSecureId(prefix: string, randomLength: number): string { | ||
| if (typeof window === 'undefined' || !window.crypto || !window.crypto.getRandomValues) { | ||
| // Fallback for non-browser environments; still avoids Math.random() in main path | ||
| return `${prefix}_${Date.now()}` | ||
| } | ||
|
|
||
| const byteLength = Math.ceil((randomLength * 3) / 4) | ||
| const bytes = new Uint8Array(byteLength) | ||
| window.crypto.getRandomValues(bytes) | ||
|
|
||
| // Base64-encode the bytes | ||
| let binary = '' | ||
| for (let i = 0; i < bytes.length; i++) { | ||
| binary += String.fromCharCode(bytes[i]) | ||
| } | ||
| const base64 = btoa(binary) | ||
|
|
||
| // Make it URL-safe and trim to requested length | ||
| const safe = base64.replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/g, '') | ||
| const randomPart = safe.slice(0, randomLength) | ||
|
|
||
| return `${prefix}_${Date.now()}_${randomPart}` | ||
| } | ||
|
|
||
| export function getCurrentUser(): FakeUser | null { | ||
| if (typeof window === 'undefined') return null | ||
| const stored = localStorage.getItem(STORAGE_KEY) | ||
| @@ -67,7 +91,7 @@ | ||
| // Fake signup - creates a new user | ||
| const avatar = `https://api.dicebear.com/7.x/avataaars/svg?seed=${encodeURIComponent(username)}` | ||
| const newUser: FakeUser = { | ||
| id: `user_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, | ||
| id: generateSecureId('user', 9), | ||
| username, | ||
| email, | ||
| avatar, |
| <div className="bg-white rounded-2xl shadow-xl p-8 mb-6"> | ||
| <div className="flex flex-col md:flex-row items-center md:items-start gap-6"> | ||
| <img | ||
| src={currentUser.avatar} |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
DOM text
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 31 minutes ago
General approach: Ensure that untrusted input flowing into the avatar field cannot produce an unsafe or unexpected URL when used in the <img src>. The simplest way, without changing user-visible behavior, is to sanitize or validate the avatar value before using it in JSX, and fall back to a known-safe default if it is malformed or uses an unexpected scheme or host.
Best concrete fix here: Introduce a small helper in profile.tsx that takes currentUser.avatar, validates it as a URL and ensures it uses https: and the expected DiceBear host, and otherwise falls back to a safe default avatar URL. Then render safeAvatarUrl as the src instead of the raw currentUser.avatar. This keeps existing functionality (DiceBear avatars based on username) while preventing arbitrary schemes/hosts from being rendered if avatar ever becomes user-controlled more broadly, and it resolves the taint warning at the sink.
Specific changes:
- File:
apps/react-router/react-router-v7-project/app/routes/profile.tsx- Above the
Profilecomponent, add a smallgetSafeAvatarUrlfunction that:- Accepts a
string | undefined | null. - Tries to construct a
URLfrom it. - Checks that
url.protocol === 'https:'andurl.hostname === 'api.dicebear.com'. - Returns the string if valid; otherwise returns a hard-coded safe DiceBear URL (or similar default).
- Accepts a
- Inside
Profile, after computingcurrentUser, computeconst avatarUrl = getSafeAvatarUrl(currentUser.avatar). - Change
<img src={currentUser.avatar}to<img src={avatarUrl}.
- Above the
- No new imports are needed;
URLis a standard global.
This fix addresses all alert variants because they all share the same sink (currentUser.avatar used in <img src>).
-
Copy modified lines R6-R25 -
Copy modified line R34 -
Copy modified line R43
| @@ -3,6 +3,26 @@ | ||
| import { getCurrentUser } from '~/lib/utils/auth' | ||
| import type { Route } from './+types/profile' | ||
|
|
||
| function getSafeAvatarUrl(avatar: string | null | undefined): string { | ||
| const defaultAvatar = | ||
| 'https://api.dicebear.com/7.x/avataaars/svg?seed=default-avatar' | ||
|
|
||
| if (!avatar) { | ||
| return defaultAvatar | ||
| } | ||
|
|
||
| try { | ||
| const url = new URL(avatar) | ||
| if (url.protocol === 'https:' && url.hostname === 'api.dicebear.com') { | ||
| return avatar | ||
| } | ||
| } catch { | ||
| // ignore parse errors and fall back to default | ||
| } | ||
|
|
||
| return defaultAvatar | ||
| } | ||
|
|
||
| export default function Profile() { | ||
| const { user, logout } = useAuth() | ||
|
|
||
| @@ -11,6 +31,7 @@ | ||
| } | ||
|
|
||
| const currentUser = getCurrentUser() || user | ||
| const avatarUrl = getSafeAvatarUrl(currentUser.avatar) | ||
|
|
||
| return ( | ||
| <div className="min-h-screen bg-gradient-to-br from-indigo-50 to-purple-50 p-6"> | ||
| @@ -19,7 +40,7 @@ | ||
| <div className="bg-white rounded-2xl shadow-xl p-8 mb-6"> | ||
| <div className="flex flex-col md:flex-row items-center md:items-start gap-6"> | ||
| <img | ||
| src={currentUser.avatar} | ||
| src={avatarUrl} | ||
| alt={currentUser.username} | ||
| className="w-32 h-32 rounded-full border-4 border-indigo-500" | ||
| /> |
| {index === 0 ? '🥇' : index === 1 ? '🥈' : index === 2 ? '🥉' : `#${index + 1}`} | ||
| </div> | ||
| <img | ||
| src={u.avatar} |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
DOM text
DOM text
Copilot Autofix
AI 32 minutes ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
edwinyjlim
left a comment
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.
let's go
do you think we should delete the .cursor and .vscode directories? I feel like they might cause some unexpected behavior if the Wizard reads them
| EXAMPLES_PATH=~/development/examples | ||
| MCP_PATH=~/development/posthog/products/mcp | ||
| WIZARD_PATH=~/development/wizard | ||
| WIZARD_BIN=~/development/wizard/dist/bin.js |
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.
i have an update in the upcoming CI PR that'll fix this
| WIZARD_BIN=~/development/wizard/dist/bin.js |
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.
you can remove these changes
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.
These rules are added by the Wizard, no? We should remove
I'll batch ignore them |
…as HTML Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
test apps for PostHog/wizard#215