feat: add auth frontend React app for Firefox Sync setup#191
feat: add auth frontend React app for Firefox Sync setup#191
Conversation
Implements the auth-frontend spec as a React + TypeScript SPA using Vite, Tailwind CSS v4, and shadcn/ui components. The app guides users through OIDC authentication (OAuth 2.0 + PKCE) and displays the Token Server URI needed to configure Firefox Sync in about:config. Also removes stale TODO section from lambda/README.md — all items listed there were already fully implemented. https://claude.ai/code/session_01H7N7JBgYGnkJjzj7mF88F7
|
Claude finished @layertwo's task in 1m 41s —— View job Code Review Complete ✓Review ChecklistCore Quality
Python & Development
Security
OWASP Top 10
Documentation
Security Findings SummaryMEDIUM Severity Issues1. Potential XSS via Error Message Display (ErrorPage.tsx:38-40)
<p className="text-sm text-muted-foreground whitespace-pre-line">
{details}
</p>LOW Severity Issues1. Sensitive Token Exposure in Console Logs (App.tsx:30)
console.error(`[ffsync] ${title}: ${message}`, details ?? "")2. Missing Content Security Policy (CSP)
3. Session Storage Data Not Encrypted
Code Quality ObservationsStrengths
Areas for Improvement1. Error Message Handling (MEDIUM Priority)
Fix suggestion: Add DOMPurify or ensure error messages are always developer-controlled strings. 2. README Documentation (LOW Priority)
3. Configuration Validation (LOW Priority) // Consider adding URL validation
try {
new URL(config.oidcProviderUrl);
new URL(config.tokenServerUrl);
} catch {
throw new Error(`Invalid URL format in configuration`);
}4. Missing Security Headers (LOW Priority)
Recommendations
Overall AssessmentThis is a well-implemented OAuth/OIDC frontend with proper PKCE support and good security practices. The code is clean, type-safe, and follows React best practices. The main concern is the potential XSS vector through error message rendering, which should be addressed before production deployment. The authentication flow is secure, properly validates state parameters to prevent CSRF, and follows OAuth 2.0 + PKCE best practices. The session management is appropriate for a temporary authentication flow. Recommendation: Approve after addressing the MEDIUM severity XSS issue. |
Coverage report
This PR does not seem to contain any modification to coverable code. |
|
Claude finished @layertwo's task in 1m 53s —— View job Code Review Complete ✓Review ChecklistCore Quality
Python & Development
Security
OWASP Top 10
Documentation
Security Issues SummaryMEDIUM SeverityPotential XSS via Unsanitized Error Display (ErrorPage.tsx:38-40)
<p className="text-sm text-muted-foreground whitespace-pre-line">
{details}
</p>
LOW Severity1. Console Logging of Sensitive Information (App.tsx:30)
2. Missing Content Security Policy
<meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; connect-src 'self' https://auth.example.com https://sync.example.com;">3. Unencrypted Session Storage
4. Generic README Documentation
Code Quality ObservationsStrengths
Security Strengths
Minor Observations
Overall AssessmentThis is a well-implemented OAuth/OIDC frontend with proper PKCE support and solid security practices. The code is clean, type-safe, and follows React best practices. The authentication flow correctly implements OAuth 2.0 + PKCE for public clients. Primary Concern: The MEDIUM severity XSS vulnerability in ErrorPage.tsx should be addressed before production deployment. While React escapes text content by default, the combination of displaying external error content without explicit sanitization creates unnecessary risk. Recommendation: Address the MEDIUM severity XSS issue, then approve for deployment. The LOW severity issues can be addressed in follow-up improvements. Additional Notes:
|
This reverts commit 5235824.
|
Claude finished @layertwo's task in 2m 25s —— View job Code Review Complete ✓Review ChecklistCore Quality
Python & Development
Security
OWASP Top 10
Documentation
Security Issues SummaryMEDIUM Severity1. Potential XSS via Unsanitized Error Display
LOW Severity1. Console Logging of Sensitive Information
2. Missing Content Security Policy
3. Generic README Documentation
4. Unencrypted Session Storage
Code Quality ObservationsStrengths
Security Strengths
Minor Observations1. URL Validation (
2. Error Message Construction (
3. Token Server Response Handling (
SummaryThis is a well-implemented OAuth/OIDC frontend with proper PKCE support and solid security practices. The code is clean, type-safe, and follows React best practices. The authentication flow correctly implements OAuth 2.0 + PKCE for public clients. Primary Concern: The error handling pattern, while protected by React's text escaping, creates unnecessary attack surface by displaying external error content. This should be addressed before production deployment. Status: The security issues identified in previous reviews remain unaddressed. The MEDIUM severity issue regarding error message handling should be resolved. Recommendation: Address the MEDIUM severity error handling issue, then approve for deployment. The LOW severity issues can be addressed in follow-up improvements. Note: This review covers the PR as of commit |
- Sanitize external error content (OAuth callback params, API error responses) by stripping HTML tags and truncating to prevent any potential XSS via error message display, even though React JSX auto-escapes text content - Gate verbose console.error logging behind import.meta.env.DEV so production builds only log error titles, not details - Add Content-Security-Policy meta tag restricting script-src to 'self', connect-src to 'self' and https:, and blocking object/ base/form-action — mitigates XSS and protects sessionStorage OAuth state as defense-in-depth https://claude.ai/code/session_01H7N7JBgYGnkJjzj7mF88F7
|
Claude finished @layertwo's task in 1m 44s —— View job Code Review Complete ✓Review ChecklistCore Quality
Python & Development
Security
OWASP Top 10
Documentation
Security Issues Addressed Since Previous ReviewsExcellent work! The following security issues from previous reviews have been resolved:
Remaining IssueLOW SeverityGeneric README Documentation
Code Quality ObservationsStrengths
Security Strengths
Minor Observations
Overall AssessmentAPPROVED ✓ This is a well-implemented OAuth/OIDC frontend with excellent security practices. The latest commit ( The authentication flow is secure with:
The remaining documentation issue is LOW priority and does not block deployment. Changes Summary:
Deployment Readiness: ✓ Ready for production deployment The only remaining item is improving the README documentation, which can be addressed in a follow-up PR without blocking this one. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| function sanitizeExternal(value: string, maxLength = 200): string { | ||
| return value.replace(/<[^>]*>/g, "").slice(0, maxLength) |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
In general, the right fix is to stop using a fragile “strip tags with one regex” approach, and instead either: (a) use a well-tested HTML sanitization library, or (b) simplify the sanitization to a safe transformation such as HTML-encoding or aggressive removal of tag delimiters (< and >), which cannot reintroduce tags on reprocessing. Because we must only touch this file and avoid adding nonstandard dependencies where possible, the most robust, low-impact fix here is to change sanitizeExternal to remove < and > characters themselves, instead of trying to parse tags. That avoids the multi-character sanitization pitfall and guarantees that no HTML elements, including <script, can be formed from the sanitized output.
Concretely, update sanitizeExternal in frontend/src/lib/oauth.ts so that it replaces all < and > characters (and optionally similar dangerous characters) with either nothing or safe escape sequences, and then truncates as before. This preserves existing behavior at the call sites (still returns a string, still enforces maxLength) while making the sanitization safe against the specific CodeQL concern. No other lines or imports need to change.
| @@ -35,7 +35,8 @@ | ||
| } | ||
|
|
||
| function sanitizeExternal(value: string, maxLength = 200): string { | ||
| return value.replace(/<[^>]*>/g, "").slice(0, maxLength) | ||
| // Remove angle brackets to prevent HTML or script injection, then truncate. | ||
| return value.replace(/[<>]/g, "").slice(0, maxLength) | ||
| } | ||
|
|
||
| export function validateCallback(params: URLSearchParams): string { |
| let detail = "" | ||
| try { | ||
| const body = await response.text() | ||
| detail = body.replace(/<[^>]*>/g, "").slice(0, 500) |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
In general, the best fix is to avoid ad‑hoc regex‑based HTML sanitization and instead use a well‑tested library designed for this purpose, or at minimum ensure that tag removal is robust against multi‑character partial matches by repeatedly applying the replacement until no tags remain. Since we must keep functionality (get up to 500 characters of tag‑stripped text) and not change imports beyond well‑known libraries, we can improve the existing regex‑based approach by looping until no more tags are removed. This directly addresses the “incomplete multi‑character sanitization” issue without changing how the result is used elsewhere.
Concretely, in frontend/src/lib/token-server.ts, lines around 35–37 should be updated so that after const body = await response.text(), we do not call body.replace(/<[^>]*>/g, "") just once. Instead, we should implement a small loop that repeatedly strips tags until subsequent iterations make no changes, then slice to 500 characters. For example, introduce a local sanitizedBody variable, run a do { ... } while(...) loop with the same regex, and then assign detail = sanitizedBody.slice(0, 500). This requires no new imports or global utilities and keeps existing behavior (plain text, max 500 chars) while ensuring that patterns like nested/overlapping tags cannot reappear after a single replacement.
| @@ -33,7 +33,13 @@ | ||
| let detail = "" | ||
| try { | ||
| const body = await response.text() | ||
| detail = body.replace(/<[^>]*>/g, "").slice(0, 500) | ||
| let sanitizedBody = body | ||
| let previousBody: string | ||
| do { | ||
| previousBody = sanitizedBody | ||
| sanitizedBody = sanitizedBody.replace(/<[^>]*>/g, "") | ||
| } while (sanitizedBody !== previousBody) | ||
| detail = sanitizedBody.slice(0, 500) | ||
| } catch { | ||
| detail = response.statusText | ||
| } |
2 participants
Implements the auth-frontend spec as a React + TypeScript SPA using
Vite, Tailwind CSS v4, and shadcn/ui components. The app guides users
through OIDC authentication (OAuth 2.0 + PKCE) and displays the Token
Server URI needed to configure Firefox Sync in about:config.
Also removes stale TODO section from lambda/README.md — all items
listed there were already fully implemented.
https://claude.ai/code/session_01H7N7JBgYGnkJjzj7mF88F7