Monorepo test naren#44
Conversation
- Migrated from Flask backend to Netlify Functions serverless architecture - Restructured project as monorepo with apps/web/ and netlify/functions/ - Added 7 serverless functions for auth, game state, and achievements - Fixed TypeScript compilation errors and update dependencies - Created professional documentation suite (README, full docs, installation guide) - Added version constraints and installation requirements - Cleaned up duplicate files and organize project structure - Configured for deployment on Netlify with environment setup
…in local sys -- always read readme whenever stuck and for easy setup
- Fix build command from 'npm run build' to 'cd apps/web && npm run build' - Add build script to root package.json for consistency - Resolves Netlify deployment error: Missing script: 'build'
- Update build command to install dependencies before building - Ensures Netlify has all required packages (vite, react, three.js) - Resolves 'vite: not found' deployment error
Added list of game from [QGames document](https://docs.google.com/document/d/1LaDsZcypeMMwwvEyprmxOzr9r3jNk4kyQ3qIGtjjoAc/edit?tab=t.0) Work in progress - needs brief overview (summary of game + type of game), quantum concepts covered, potential mechanics to implement, effective learning benefits, areas of improvement
Create QGames_List.md
Reviewer's GuideMigrate QPlay-Core from a Flask-based single repo to a Netlify Functions–backed monorepo: introduce a dedicated React web app under apps/web, replace Flask APIs with Netlify serverless functions using Supabase, tighten validation/CORS/error handling, and add CI + docs to support the new workflow and deployment model. Sequence diagram for email login via Netlify FunctionsequenceDiagram
actor User
participant WebApp as Web_App_UI
participant Api as ApiService
participant Fn as auth_login_function
participant SB as Supabase_REST
User->>WebApp: Submit_email_for_login
WebApp->>Api: signIn(email, password)
Api->>Fn: POST /auth-login {email}
Fn->>Fn: validateSupabaseConfig()
Fn->>Fn: isValidEmail(email)
Fn->>SB: GET /rest/v1/users?email=eq.email
SB-->>Fn: user_row_or_empty
alt user_found
Fn->>SB: PATCH /rest/v1/users?id=eq.user_id {last_login}
Fn-->>Api: {success:true, user}
Api->>Api: setAuthToken(user.id)
Api-->>WebApp: SignInResponse{user,success}
WebApp-->>User: Login_success
else user_not_found
Fn-->>Api: error USER_NOT_FOUND
Api-->>WebApp: throw_error
WebApp-->>User: Show_login_error
end
Class diagram for ApiService and Netlify utilitiesclassDiagram
class ApiService {
-baseUrl : string
-authToken : string
+constructor(baseUrl : string)
+setAuthToken(token : string) void
+clearAuthToken() void
+request(endpoint : string, options : RequestInit) Promise~any~
+signIn(email : string, password : string) Promise~SignInResponse~
+signUp(data : SignUpData) Promise~SignInResponse~
+getCurrentUser() Promise~any~
+getSpeedLeaderboard(limit : number, difficulty : string) Promise~any~
+getScoreLeaderboard(limit : number, difficulty : string) Promise~any~
+getMasteryLeaderboard(limit : number) Promise~any~
+startGameSession(userId : string, difficulty : string) Promise~any~
+saveGameProgress(sessionId : string, currentRoom : string, roomTimes : any, roomAttempts : any, roomScores : any) Promise~any~
+completeGameSession(sessionId : string, userId : string, completionTime : number, totalScore : number, roomsCompleted : number, hintsUsed : number, difficulty : string) Promise~any~
+logQuantumMeasurement(sessionId : string, roomId : string, measurementType : string, measurementData : any) Promise~any~
+unlockAchievement(achievementId : string, sessionId : string, userId : string) Promise~any~
}
class SignInResponse {
+access_token : string
+token_type : string
+expires_in : number
+user : any
+success : boolean
}
class SignUpData {
+email : string
+password : string
+username : string
+full_name : string
}
class AuthResponse {
+success : boolean
+message : string
+user : any
+error : string
}
class CorsUtils {
+getAllowedOrigins() string[]
+getCorsHeaders(requestOrigin : string, methods : string[]) Object
+handleCorsPreflightRequest(event : Object, methods : string[]) Object
}
class SupabaseUtils {
+validateSupabaseConfig() Object
+validateServiceKeyConfig() Object
+hasServiceKey() boolean
+getSupabaseHeaders(useServiceKey : boolean) Object
+getSupabaseUrl() string
}
class ValidationUtils {
+isValidEmail(email : string) boolean
+isValidUserId(userId : string) boolean
+isValidCredential(credential : string) boolean
+isValidSessionId(sessionId : string) boolean
+isValidUsername(username : string) boolean
}
class ErrorUtils {
+createErrorResponse(statusCode : number, userMessage : string, headers : Object, internalError : Error) Object
+ErrorMessages : Object
}
class AuthLoginFunction {
+handler(event : Object, context : Object) Promise~Object~
}
class AuthSignupFunction {
+handler(event : Object, context : Object) Promise~Object~
}
class AuthGoogleFunction {
+handler(event : Object, context : Object) Promise~Object~
-generateUsername(email : string) string
}
class GameSessionFunction {
+handler(event : Object, context : Object) Promise~Object~
}
class LeaderboardFunction {
+handler(event : Object, context : Object) Promise~Object~
-enrichLeaderboardWithUserData(leaderboardData : any[], supabaseUrl : string) Promise~any[]~
}
class AchievementsFunction {
+handler(event : Object, context : Object) Promise~Object~
}
class QuantumMeasurementsFunction {
+handler(event : Object, context : Object) Promise~Object~
}
ApiService --> SignInResponse
ApiService --> SignUpData
ApiService --> AuthResponse
AuthLoginFunction ..> SupabaseUtils
AuthLoginFunction ..> ValidationUtils
AuthLoginFunction ..> ErrorUtils
AuthLoginFunction ..> CorsUtils
AuthSignupFunction ..> SupabaseUtils
AuthSignupFunction ..> ValidationUtils
AuthSignupFunction ..> ErrorUtils
AuthSignupFunction ..> CorsUtils
AuthGoogleFunction ..> SupabaseUtils
AuthGoogleFunction ..> ValidationUtils
AuthGoogleFunction ..> ErrorUtils
AuthGoogleFunction ..> CorsUtils
GameSessionFunction ..> SupabaseUtils
GameSessionFunction ..> ValidationUtils
GameSessionFunction ..> ErrorUtils
GameSessionFunction ..> CorsUtils
LeaderboardFunction ..> SupabaseUtils
LeaderboardFunction ..> ErrorUtils
LeaderboardFunction ..> CorsUtils
AchievementsFunction ..> SupabaseUtils
AchievementsFunction ..> ValidationUtils
AchievementsFunction ..> ErrorUtils
AchievementsFunction ..> CorsUtils
QuantumMeasurementsFunction ..> SupabaseUtils
QuantumMeasurementsFunction ..> ValidationUtils
QuantumMeasurementsFunction ..> ErrorUtils
QuantumMeasurementsFunction ..> CorsUtils
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Deploy Preview for quantumescape ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for quantum-escape ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries. (link)
- Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries. (link)
- Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries. (link)
- Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries. (link)
- Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries. (link)
General comments:
- The new
game-sessionNetlify function routes onevent.pathcontaining/game/start,/game/complete, and/game/save-progress, but the frontend now always calls/.netlify/functions/game-sessionwithout those path fragments, so none of these branches will match—consider switching the routing to use a body field or query parameter (e.g.action: 'start' | 'save-progress' | 'complete') or split these into separate functions. Project_documentation.mdcontains large duplicated sections (e.g., the Technical Details, Troubleshooting, and Deployment sections appear twice), which will make it harder to maintain—consider deduplicating this file so each section appears only once.- In several Netlify functions you fall back to wildcard CORS (
Access-Control-Allow-Origin: '*') and proceed even when Supabase calls fail (logging warnings only); if this is not intentional, you may want to tighten CORS and surface backend failures to the client so issues are visible rather than silently ignored.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `game-session` Netlify function routes on `event.path` containing `/game/start`, `/game/complete`, and `/game/save-progress`, but the frontend now always calls `/.netlify/functions/game-session` without those path fragments, so none of these branches will match—consider switching the routing to use a body field or query parameter (e.g. `action: 'start' | 'save-progress' | 'complete'`) or split these into separate functions.
- `Project_documentation.md` contains large duplicated sections (e.g., the Technical Details, Troubleshooting, and Deployment sections appear twice), which will make it harder to maintain—consider deduplicating this file so each section appears only once.
- In several Netlify functions you fall back to wildcard CORS (`Access-Control-Allow-Origin: '*'`) and proceed even when Supabase calls fail (logging warnings only); if this is not intentional, you may want to tighten CORS and surface backend failures to the client so issues are visible rather than silently ignored.
## Individual Comments
### Comment 1
<location> `apps/web/src/services/api.ts:160-162` </location>
<code_context>
}
- // Game Session API calls - Updated to match backend endpoints
+ // Game Session API calls - Updated to match Netlify Functions
async startGameSession(userId: string, difficulty: string = 'easy') {
- return this.request('/api/game/start', {
+ return this.request('/game-session', {
method: 'POST',
body: JSON.stringify({
</code_context>
<issue_to_address>
**issue (bug_risk):** Game session API now always posts to `/game-session` but the Netlify function routes on `/game/start`, `/game/complete`, `/game/save-progress` paths, so requests won’t hit the intended branches.
These methods all POST to `/game-session`, but `netlify/functions/game-session.js` routes based on `event.path` containing `/game/start`, `/game/complete`, or `/game/save-progress`. When called as `/.netlify/functions/game-session`, none of those conditions match, so the function will return a 404.
To resolve this, either:
- Expose separate function endpoints (`/game/start`, `/game/complete`, `/game/save-progress`) and call those from the frontend, or
- Keep a single `game-session` function and route based on a request field (e.g. `action: 'start' | 'complete' | 'save-progress'`) or a query parameter instead of `path.includes(...)`.
</issue_to_address>
### Comment 2
<location> `netlify/functions/game-session.js:34-43` </location>
<code_context>
+ const path = event.path;
</code_context>
<issue_to_address>
**issue (bug_risk):** Routing inside `game-session` relies on URL substrings that will not appear when called as a Netlify function, causing all valid POSTs to fall through to the 404 case.
Because the function is deployed at `/.netlify/functions/game-session`, `event.path` will be that exact string. The current `includes('/game/start' | '/game/complete' | '/game/save-progress')` checks will therefore never match, and every POST will fall through to the 404 handler.
Please adjust the routing so it keys off something that is actually available at this endpoint (e.g. a field in the request body, a query parameter, or by splitting into separate functions), and keep it consistent with the URLs/contract defined in `ApiService`.
</issue_to_address>
### Comment 3
<location> `netlify/functions/auth-login.js:43-52` </location>
<code_context>
+ }
+
+ try {
+ const { email } = JSON.parse(event.body);
+
+ if (!email) {
+ return {
+ statusCode: 400,
+ headers: corsHeaders,
+ body: JSON.stringify({
+ success: false,
+ error: 'Email is required'
+ })
+ };
+ }
+
+ // Find user by email
+ const response = await fetch(`${SUPABASE_URL}/rest/v1/users?email=eq.${email}`, {
+ headers: getSupabaseHeaders()
+ });
+
+ if (!response.ok) {
+ return {
+ statusCode: 500,
+ headers: corsHeaders,
+ body: JSON.stringify({
+ success: false,
+ error: 'Database connection failed'
+ })
+ };
+ }
+
+ const users = await response.json();
+
+ if (users.length === 0) {
</code_context>
<issue_to_address>
**🚨 issue (security):** The login function treats knowledge of an email address as sufficient for authentication, which is a significant security weakness.
The `/auth-login` endpoint reads only `email` from the request body and returns the matching user; the frontend still sends `email` and `password`, but the password is ignored. As a result, anyone who knows a user’s email can authenticate as them.
If this is a temporary stub, clearly mark it as such and ensure it cannot be enabled in production. Otherwise, either implement proper authentication (e.g., Supabase Auth, magic links, or password verification) or restrict this endpoint to non-production and fail in production until real auth is in place.
</issue_to_address>
### Comment 4
<location> `netlify/functions/leaderboard.js:24-33` </location>
<code_context>
+});
+
+// Enrich leaderboard with user data
+const enrichLeaderboardWithUserData = async (leaderboardData) => {
+ const enriched = [];
+
+ for (let i = 0; i < leaderboardData.length; i++) {
+ const entry = { ...leaderboardData[i], rank: i + 1 };
+
+ try {
+ const userResponse = await fetch(
+ `${SUPABASE_URL}/rest/v1/users?id=eq.${entry.user_id}&select=username,full_name`,
+ { headers: getSupabaseHeaders() }
+ );
+
+ if (userResponse.ok) {
+ const userData = await userResponse.json();
+ if (userData.length > 0) {
+ entry.username = userData[0].username;
+ entry.full_name = userData[0].full_name;
+ }
+ }
+ } catch (error) {
+ console.warn('Failed to fetch user data for entry:', entry.user_id);
+ }
+
+ enriched.push(entry);
+ }
+
+ return enriched;
+};
+
</code_context>
<issue_to_address>
**suggestion (performance):** User enrichment for leaderboard entries performs sequential requests, which will be slow with many entries.
`enrichLeaderboardWithUserData` awaits each `/rest/v1/users` fetch inside the loop, so N entries cause N sequential HTTP calls. Consider either batching with `Promise.all` or doing a single Supabase query using an `in` filter on `user_id` and then joining the results in memory to cut round-trips and latency.
Suggested implementation:
```javascript
const SUPABASE_URL = process.env.SUPABASE_URL;
const SUPABASE_ANON_KEY = process.env.SUPABASE_ANON_KEY;
// Enrich leaderboard with user data
const enrichLeaderboardWithUserData = async (leaderboardData) => {
// Preserve existing behavior for empty input
if (!leaderboardData || leaderboardData.length === 0) {
return [];
}
// Collect unique user IDs from leaderboard entries
const userIds = [
...new Set(
leaderboardData
.map((entry) => entry.user_id)
.filter((id) => id !== null && id !== undefined)
)
];
// If no user IDs, just return leaderboard with rank added
if (userIds.length === 0) {
return leaderboardData.map((entry, index) => ({
...entry,
rank: index + 1
}));
}
// Build a single Supabase query using `in` filter
// Example: id=in.(id1,id2,...) with proper URL encoding
const inFilter = userIds.map((id) => encodeURIComponent(id)).join(',');
const usersUrl = `${SUPABASE_URL}/rest/v1/users?id=in.(${inFilter})&select=id,username,full_name`;
let usersById = {};
try {
const userResponse = await fetch(usersUrl, {
headers: getSupabaseHeaders()
});
if (userResponse.ok) {
const users = await userResponse.json();
usersById = users.reduce((acc, user) => {
acc[user.id] = {
username: user.username,
full_name: user.full_name
};
return acc;
}, {});
} else {
console.warn('Failed to fetch users for leaderboard enrichment:', userResponse.statusText);
}
} catch (error) {
console.warn('Error fetching users for leaderboard enrichment:', error);
}
// Enrich leaderboard entries with user data and rank
return leaderboardData.map((entry, index) => {
const user = usersById[entry.user_id];
return {
...entry,
rank: index + 1,
...(user
? {
username: user.username,
full_name: user.full_name
}
: {})
};
});
};
```
```javascript
```
1. Ensure that `enrichLeaderboardWithUserData` is still being called where the leaderboard is generated, but no call-site changes are required because the function signature remains the same.
2. If the `users` table uses a different primary key than `id` (for example `user_id`), adjust the `id` field in the `select` and `id=in.(...)` filter, as well as the key used in `usersById` to match the actual column name.
</issue_to_address>
### Comment 5
<location> `Project_documentation.md:164` </location>
<code_context>
+│ ├── EntanglementBridge.tsx # Quantum entanglement concepts
+│ ├── ProbabilityBay.tsx # Probability distributions
+│ ├── QuantumArchive.tsx # Historical quantum experiments
+│ ├── StateChambrer.tsx # Quantum state manipulation
+│ ├── SuperpositionTower.tsx # Superposition principles
+│ └── TunnelingVault.tsx # Quantum tunneling effects
</code_context>
<issue_to_address>
**issue (typo):** Typo in component name: likely should be `StateChamber.tsx`.
`StateChambrer.tsx` looks misspelled. Consider renaming to `StateChamber.tsx` (or similar) to better match the description and improve discoverability.
```suggestion
│ ├── StateChamber.tsx # Quantum state manipulation
```
</issue_to_address>
### Comment 6
<location> `Project_documentation.md:298-307` </location>
<code_context>
+### **Port Architecture**
</code_context>
<issue_to_address>
**suggestion:** Large sections of the documentation appear duplicated in this file.
From this `### **Port Architecture**` heading onward, the document repeats earlier sections (Technical Details, Troubleshooting, Deployment, metrics, roadmap, contributing). Please remove or consolidate this duplicated block to keep the documentation concise and maintainable.
Suggested implementation:
```
└─────────────────────────────────────────────────┘
◈ Loaded function auth-google
◈ Loaded function auth-login
◈ Loaded function achievements
◈ Loaded function game-session
◈ Loaded function leaderboard
◈ Loaded function quantum-measurements
```
```
If there are other repeated sections after this snippet (e.g., Technical Details, Troubleshooting, Deployment, metrics, roadmap, contributing) that are not visible here, apply the same strategy: remove the duplicated later copy and keep only the earlier, canonical sections. Ensure the document still ends cleanly (no stray backticks or separators).
</issue_to_address>
### Comment 7
<location> `netlify/functions/achievements.js:51` </location>
<code_context>
`${SUPABASE_URL}/rest/v1/game_sessions?id=eq.${session_id}&select=user_id`,
</code_context>
<issue_to_address>
**security (javascript.aws-lambda.security.tainted-sql-string):** Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries.
*Source: opengrep*
</issue_to_address>
### Comment 8
<location> `netlify/functions/auth-google.js:148` </location>
<code_context>
error: `Failed to create user. Status: ${createUserResponse.status}`
</code_context>
<issue_to_address>
**security (javascript.aws-lambda.security.tainted-sql-string):** Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries.
*Source: opengrep*
</issue_to_address>
### Comment 9
<location> `netlify/functions/auth-google.js:148` </location>
<code_context>
error: `Failed to create user. Status: ${createUserResponse.status}`
</code_context>
<issue_to_address>
**security (javascript.express.security.injection.tainted-sql-string):** Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries.
*Source: opengrep*
</issue_to_address>
### Comment 10
<location> `netlify/functions/auth-signup.js:103` </location>
<code_context>
error: `Failed to create user. Status: ${response.status}`
</code_context>
<issue_to_address>
**security (javascript.aws-lambda.security.tainted-sql-string):** Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries.
*Source: opengrep*
</issue_to_address>
### Comment 11
<location> `netlify/functions/auth-signup.js:103` </location>
<code_context>
error: `Failed to create user. Status: ${response.status}`
</code_context>
<issue_to_address>
**security (javascript.express.security.injection.tainted-sql-string):** Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries.
*Source: opengrep*
</issue_to_address>
### Comment 12
<location> `netlify/functions/achievements.js:6` </location>
<code_context>
const SUPABASE_URL = process.env.SUPABASE_URL;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_URL} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 13
<location> `netlify/functions/achievements.js:7` </location>
<code_context>
const SUPABASE_ANON_KEY = process.env.SUPABASE_ANON_KEY;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_ANON_KEY} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 14
<location> `netlify/functions/auth-google.js:8` </location>
<code_context>
const GOOGLE_CLIENT_ID = process.env.GOOGLE_CLIENT_ID;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {GOOGLE_CLIENT_ID} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 15
<location> `netlify/functions/auth-google.js:9` </location>
<code_context>
const SUPABASE_URL = process.env.SUPABASE_URL;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_URL} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 16
<location> `netlify/functions/auth-google.js:10` </location>
<code_context>
const SUPABASE_ANON_KEY = process.env.SUPABASE_ANON_KEY;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_ANON_KEY} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 17
<location> `netlify/functions/auth-google.js:11` </location>
<code_context>
const SUPABASE_SERVICE_KEY = process.env.SUPABASE_SERVICE_KEY;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_SERVICE_KEY} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 18
<location> `netlify/functions/auth-google.js:76` </location>
<code_context>
const email = payload.email;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {email} = payload;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 19
<location> `netlify/functions/auth-login.js:6` </location>
<code_context>
const SUPABASE_URL = process.env.SUPABASE_URL;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_URL} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 20
<location> `netlify/functions/auth-login.js:7` </location>
<code_context>
const SUPABASE_ANON_KEY = process.env.SUPABASE_ANON_KEY;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_ANON_KEY} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 21
<location> `netlify/functions/auth-signup.js:6` </location>
<code_context>
const SUPABASE_URL = process.env.SUPABASE_URL;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_URL} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 22
<location> `netlify/functions/auth-signup.js:7` </location>
<code_context>
const SUPABASE_ANON_KEY = process.env.SUPABASE_ANON_KEY;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_ANON_KEY} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 23
<location> `netlify/functions/game-session.js:6` </location>
<code_context>
const SUPABASE_URL = process.env.SUPABASE_URL;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_URL} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 24
<location> `netlify/functions/game-session.js:7` </location>
<code_context>
const SUPABASE_ANON_KEY = process.env.SUPABASE_ANON_KEY;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_ANON_KEY} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 25
<location> `netlify/functions/game-session.js:34` </location>
<code_context>
const path = event.path;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {path} = event;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 26
<location> `netlify/functions/leaderboard.js:6` </location>
<code_context>
const SUPABASE_URL = process.env.SUPABASE_URL;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_URL} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 27
<location> `netlify/functions/leaderboard.js:7` </location>
<code_context>
const SUPABASE_ANON_KEY = process.env.SUPABASE_ANON_KEY;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_ANON_KEY} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 28
<location> `netlify/functions/leaderboard.js:84` </location>
<code_context>
const path = event.path;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {path} = event;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 29
<location> `netlify/functions/quantum-measurements.js:6` </location>
<code_context>
const SUPABASE_URL = process.env.SUPABASE_URL;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_URL} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 30
<location> `netlify/functions/quantum-measurements.js:7` </location>
<code_context>
const SUPABASE_ANON_KEY = process.env.SUPABASE_ANON_KEY;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {SUPABASE_ANON_KEY} = process.env;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@copilot review |
|
@ArnavBallinCode I've opened a new pull request, #45, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
@copilot review |
|
@ArnavBallinCode I've opened a new pull request, #46, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot implement above suggestions of sourcery and code review |
|
@ArnavBallinCode I've opened a new pull request, #50, to work on those changes. Once the pull request is ready, I'll request review from you. |
Security hardening for Netlify serverless functions
…time [WIP] Migrate project to Netlify serverless monorepo
Code review for monorepo migration from Flask to Netlify serverless
- Fix game-session.js: Changed from path-based to mode-based routing (frontend sends mode: 'start'|'complete'|'save-progress' in body) - Fix CORS: Added quantum-escape.netlify.app to allowed origins - Fix api.ts: Added mode parameter to game session API calls - Fix netlify.toml: Added CORS headers and esbuild bundler config - Add netlify-deploy.yml: New GitHub Actions workflow for deployment - Remove deploy-railway.yml: Empty/unused workflow file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 111 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- netlify/functions/package-lock.json: Language not supported
Comments suppressed due to low confidence (4)
netlify/functions/auth-google.js:1
- The username generation combines timestamp and random values, but there's a potential race condition if two users sign up at the exact same millisecond. While the random component reduces this risk, consider using a cryptographically secure random generator (crypto.randomBytes) or UUID for better uniqueness guarantees, especially since usernames are checked in a while loop that could theoretically run indefinitely.
netlify/functions/leaderboard.js:1 - While user IDs are being encoded individually, the Supabase query uses
in.()syntax with comma-separated values. Ensure that Supabase properly handles these encoded values in theinoperator to prevent potential injection issues. Consider using Supabase's native array parameter binding if available instead of string concatenation.
netlify/functions/utils/cors.js:1 - When the origin is not in the allowed list, the function returns CORS headers without 'Access-Control-Allow-Origin', which will cause browsers to block the request. However, this silently fails without logging or explicit error response. Consider logging unauthorized origin attempts for security monitoring and debugging purposes.
netlify/functions/game-session.js:1 - The user_id is validated with isValidUserId before this point, but the validation allows alphanumeric, hyphens, and underscores up to 128 characters. Ensure that encodeURIComponent provides sufficient protection for all valid user_id formats in the Supabase query context. Consider using parameterized queries if Supabase client library supports them instead of URL string interpolation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Security Fixes: - Add validateServiceKeyConfig() to validate SERVICE_KEY before usage - Update auth-google.js to use validateServiceKeyConfig (fixes bug_risk issue) - Add hasServiceKey() helper for optional service key checks Documentation: - Update .env.example with placeholder values (remove real credentials) - Add helpful comments explaining where to get each env variable - Update README with Netlify Functions API reference table - Add detailed project structure showing all 7 serverless functions - Update live demo link to quantum-escape.netlify.app
- Add explicit security comments explaining validation - Extract validated session_id to separate variable - Clarify that isValidSessionId only allows [a-zA-Z0-9_-] - Note: This is a REST API URL param, not SQL injection risk
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries. (link)
General comments:
- The root
install:allscript usescd apps/web && npm install && cd ../../netlify/functions && npm install, which will cd out of the repo; fromapps/webit should be something likecd ../netlify/functionsinstead to correctly reach the functions folder. - The frontend
ApiService.signInstill sends{ email, password }to/auth-login, but theauth-loginNetlify function only expects/usesemail, so you may want to either remove the password path on the client or update the function to enforce the same credential model to avoid confusing or insecure behavior. - Similarly, the signup flow on the frontend appears to use a richer
SignUpDatashape (likely including password), whileauth-signuponly consumesemail,username, andfull_name; aligning the request/response contract between client and function will make the API boundary clearer and prevent unused fields from being sent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The root `install:all` script uses `cd apps/web && npm install && cd ../../netlify/functions && npm install`, which will cd out of the repo; from `apps/web` it should be something like `cd ../netlify/functions` instead to correctly reach the functions folder.
- The frontend `ApiService.signIn` still sends `{ email, password }` to `/auth-login`, but the `auth-login` Netlify function only expects/uses `email`, so you may want to either remove the password path on the client or update the function to enforce the same credential model to avoid confusing or insecure behavior.
- Similarly, the signup flow on the frontend appears to use a richer `SignUpData` shape (likely including password), while `auth-signup` only consumes `email`, `username`, and `full_name`; aligning the request/response contract between client and function will make the API boundary clearer and prevent unused fields from being sent.
## Individual Comments
### Comment 1
<location> `apps/web/src/services/api.ts:74-81` </location>
<code_context>
- // Authentication API calls (Supabase-powered)
+ // Authentication API calls (Netlify Functions)
async signIn(email: string, password: string): Promise<SignInResponse> {
- const response = await this.request('/api/auth/signin', {
+ const response = await this.request('/auth-login', {
method: 'POST',
body: JSON.stringify({ email, password })
});
-
+
// Store token automatically
if (response.access_token) {
this.setAuthToken(response.access_token);
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Login API now calls /auth-login, which doesn’t appear to return an access_token, so the client’s token handling and SignInResponse typing are likely inconsistent.
`signIn` still assumes a `SignInResponse` with `access_token` and calls `setAuthToken`, but `/auth-login` only returns `{ success, user }`. This makes `response.access_token` undefined and will break any flows that depend on a stored token.
Please either update `/auth-login` to return a token, or adjust `SignInResponse`, `signIn`/`setAuthToken`, and downstream consumers to use the new session/user-based model without tokens.
</issue_to_address>
### Comment 2
<location> `apps/web/src/services/api.ts:88-81` </location>
<code_context>
return response;
}
async signUp(data: SignUpData): Promise<SignInResponse> {
- const response = await this.request('/api/auth/signup', {
+ const response = await this.request('/auth-signup', {
method: 'POST',
body: JSON.stringify(data)
});
-
+
// Store token automatically
if (response.access_token) {
this.setAuthToken(response.access_token);
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Signup flow expects a token-bearing SignInResponse, but the auth-signup Netlify function returns a user object without access_token.
`signUp` still uses `SignInResponse` and token handling, but `/auth-signup` returns `{ success, message, user }` with no `access_token`, so the typing is wrong and `setAuthToken` never runs.
Please either:
- Update `/auth-signup` to return an `access_token` so it matches `SignInResponse`, or
- Update `SignInResponse`/`signUp` to the new shape and remove token-specific logic if signup is now tokenless.
</issue_to_address>
### Comment 3
<location> `netlify/functions/leaderboard.js:20-27` </location>
<code_context>
+
+ try {
+ // Batch fetch all user data in a single query
+ const userIds = leaderboardData.map(entry => entry.user_id).filter(Boolean);
+ if (userIds.length === 0) {
+ return leaderboardData.map((entry, index) => ({ ...entry, rank: index + 1 }));
+ }
+
+ // Encode each user ID individually before joining
+ const userIdsParam = userIds.map(id => encodeURIComponent(id)).join(',');
+ const userResponse = await fetch(
+ `${SUPABASE_URL}/rest/v1/users?id=in.(${userIdsParam})&select=id,username,full_name`,
</code_context>
<issue_to_address>
**suggestion (performance):** Consider de-duplicating user IDs before building the Supabase in() filter to avoid unnecessarily large URLs and redundant work.
Because `userIdsParam` includes duplicates from `leaderboardData`, large leaderboards can generate bloated query strings and repeated lookups. Consider de-duplicating first, e.g. `const ids = [...new Set(userIds)]`, and using `ids` when building the `id=in.(...)` filter to reduce URL size and DB/network overhead.
```suggestion
// Batch fetch all user data in a single query
const userIds = leaderboardData.map(entry => entry.user_id).filter(Boolean);
// De-duplicate user IDs to avoid bloated query strings and redundant lookups
const uniqueUserIds = [...new Set(userIds)];
if (uniqueUserIds.length === 0) {
return leaderboardData.map((entry, index) => ({ ...entry, rank: index + 1 }));
}
// Encode each unique user ID individually before joining
const userIdsParam = uniqueUserIds.map(id => encodeURIComponent(id)).join(',');
```
</issue_to_address>
### Comment 4
<location> `package.json:13` </location>
<code_context>
- "three": "0.151.3"
+ "scripts": {
+ "dev": "netlify dev",
+ "build": "npm run install:all && cd apps/web && npm run build",
+ "preview": "cd apps/web && npm run preview",
+ "install:all": "cd apps/web && npm install && cd ../../netlify/functions && npm install"
</code_context>
<issue_to_address>
**suggestion (performance):** Running npm install from the build script will slow builds and can conflict with CI pipelines that already install dependencies.
Here `build` runs `install:all`, which calls `npm install` in both `apps/web` and `netlify/functions`. In most CI setups, dependencies are already installed once (e.g., via `npm ci`), so this duplicates work and may mutate `package-lock.json` during the build. Prefer keeping installs in CI/setup scripts and having `build` only run the actual build (e.g., `cd apps/web && npm run build`).
</issue_to_address>
### Comment 5
<location> `Project_documentation.md:298` </location>
<code_context>
+◈ Loaded function quantum-measurements
+```
+
+### **Port Architecture**
+- **Port 8888**: Main development server (your access point)
+- **Port 3999**: Internal static file server (background)
</code_context>
<issue_to_address>
**issue:** Large sections from "Port Architecture" onward appear duplicated at the end of the file.
From this second "### **Port Architecture**" onward, the content (Port Architecture, Technical Details, Troubleshooting, Deployment, Project Metrics, Next Steps, Additional Resources, Contributing, and the closing section) duplicates material earlier in the document. Please remove the duplicate block or consolidate so each section appears only once.
</issue_to_address>
### Comment 6
<location> `netlify/functions/achievements.js:52` </location>
<code_context>
`${SUPABASE_URL}/rest/v1/game_sessions?id=eq.${validatedSessionId}&select=user_id`,
</code_context>
<issue_to_address>
**security (javascript.aws-lambda.security.tainted-sql-string):** Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
- Add AuthResponse interface matching backend response
- Make SignInResponse.access_token optional
- Update signIn/signUp to use user.id as session identifier
- Backend returns { success, user } without JWT tokens
Addresses Sourcery AI bug_risk issue about undefined access_token
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries. (link)
General comments:
- The new auth-login/auth-signup flow only uses email (ignoring the password the frontend still sends) and never provisions real Supabase auth credentials, which effectively turns login into a passwordless “find user by email” lookup; consider either enforcing proper password-based auth or explicitly implementing a passwordless flow and aligning the frontend UX and payloads accordingly.
- The base URL resolution logic for Netlify Functions is duplicated between the API service (apps/web/src/services/api.ts) and the Google auth flow in AuthContext; consider centralizing this into a single helper so changes to dev/prod endpoints only need to be made in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new auth-login/auth-signup flow only uses email (ignoring the password the frontend still sends) and never provisions real Supabase auth credentials, which effectively turns login into a passwordless “find user by email” lookup; consider either enforcing proper password-based auth or explicitly implementing a passwordless flow and aligning the frontend UX and payloads accordingly.
- The base URL resolution logic for Netlify Functions is duplicated between the API service (apps/web/src/services/api.ts) and the Google auth flow in AuthContext; consider centralizing this into a single helper so changes to dev/prod endpoints only need to be made in one place.
## Individual Comments
### Comment 1
<location> `netlify/functions/auth-login.js:11-20` </location>
<code_context>
+1. **Created Netlify Functions**
+ ```javascript
+ // Example: auth-login.js
+ exports.handler = async (event, context) => {
+ // Serverless authentication logic
+ // Replaced Flask route handling
</code_context>
<issue_to_address>
**🚨 issue (security):** Login currently authenticates users by email only without any password or token verification.
This endpoint effectively grants access to anyone who knows a valid email, since it only checks format and returns the first matching user with no credential verification. If it’s meant to be called only after a separate, trusted auth step (e.g., magic link), it should be clearly named, documented, and access‑restricted to that context. Otherwise, it must require and validate a credential (password, signed token, etc.) before treating the request as a successful login.
</issue_to_address>
### Comment 2
<location> `netlify/functions/auth-signup.js:69-71` </location>
<code_context>
+ };
+
+ // Create user
+ const response = await fetch(`${SUPABASE_URL}/rest/v1/users`, {
+ method: 'POST',
+ headers: getSupabaseHeaders(),
+ body: JSON.stringify(userData)
+ });
</code_context>
<issue_to_address>
**🚨 issue (security):** User creation uses the anon key instead of the service key, which is risky with typical Supabase RLS settings.
This backend-only call uses `getSupabaseHeaders()` (anon key) to write to `users`. In most Supabase setups, `users` writes should require the service key to avoid clients forging arbitrary rows. Please switch to `getSupabaseHeaders(true)` and ensure `validateServiceKeyConfig()` (as in `auth-google`) is used, or otherwise guarantee via RLS that only trusted, system-level contexts can insert into this table.
</issue_to_address>
### Comment 3
<location> `netlify/functions/game-session.js:64-73` </location>
<code_context>
});
-
+
if (response.ok) {
return await response.json();
} else {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Starting a game session silently falls back to a "demo" session while still returning success on Supabase write errors.
On Supabase insert failure, we still fabricate a `demo-session-*` session and return `success: true`, making it impossible for callers to know the session isn’t persisted and risking failures when resuming/updating later. Consider either returning `success: false` with an explicit error, or adding a clear `source`/`isDemo` flag so consumers can handle non-persisted sessions differently.
Suggested implementation:
```javascript
const response = await fetch(`${SUPABASE_URL}/rest/v1/game_sessions`, {
method: 'POST',
headers: getSupabaseHeaders(),
body: JSON.stringify(gameSession)
});
if (response.ok) {
const sessions = await response.json();
// If Supabase didn't actually persist a session row, treat this as a demo session
const isDemo = !Array.isArray(sessions) || sessions.length === 0;
const session = isDemo
? { ...gameSession, id: `demo-session-${Date.now()}`, isDemo: true }
: { ...sessions[0], isDemo: false };
return {
statusCode: 200,
headers: corsHeaders,
body: JSON.stringify({
success: !isDemo, // only report success when the session is persisted
session,
isDemo
})
};
} else {
// On Supabase write failure, do not silently fall back to a demo session
const errorText = await response.text().catch(() => null);
return {
statusCode: 500,
headers: corsHeaders,
body: JSON.stringify({
success: false,
error: 'Failed to create game session in Supabase',
details: errorText || null
})
};
```
If there are client-side callers that currently assume `success: true` and always-present `session`, they may need to be updated to:
1. Check `success` before using `session`.
2. Handle the `isDemo` flag when `success` is `false` or when they want to treat non-persisted sessions differently.
If you prefer to always return a session (even on Supabase failure) while still signaling non-persistence, you can instead keep a demo session in the `else` branch but set `success: false` and `isDemo: true`.
</issue_to_address>
### Comment 4
<location> `netlify.toml:13-19` </location>
<code_context>
+ directory = "netlify/functions"
+ node_bundler = "esbuild"
+
+# CORS headers for API functions
+[[headers]]
+ for = "/.netlify/functions/*"
+ [headers.values]
+ Access-Control-Allow-Origin = "*"
+ Access-Control-Allow-Methods = "GET, POST, PUT, PATCH, DELETE, OPTIONS"
+ Access-Control-Allow-Headers = "Content-Type, Authorization"
+
+# API redirects to Netlify Functions
</code_context>
<issue_to_address>
**🚨 issue (security):** Netlify-wide CORS header wildcard may conflict with per-function CORS logic that restricts origins.
Function-level CORS (`getCorsHeaders`) only sets `Access-Control-Allow-Origin` for allowed origins; otherwise it omits the header. This global `[[headers]]` rule then unconditionally adds `Access-Control-Allow-Origin = "*"` for all functions, which can negate the origin restrictions in your function code, depending on how Netlify merges headers. Please either remove or tighten this global rule, or align it with the same allowed-origins policy used in the functions.
</issue_to_address>
### Comment 5
<location> `package.json:10-13` </location>
<code_context>
+ "node": ">=18.0.0",
+ "npm": ">=9.0.0"
+ },
+ "scripts": {
+ "dev": "vite",
+ "build": "vite build",
</code_context>
<issue_to_address>
**suggestion (performance):** Running `npm install` as part of the build pipeline can significantly slow builds and complicate caching.
The `build` script calls `npm run install:all`, which re-installs dependencies in `apps/web` and `netlify/functions` even though CI/local runs `npm install` before `npm run build`. This breaks dependency caching and slows builds. Please move these installs to dedicated setup steps (e.g., CI workflow, onboarding scripts) or adopt a workspace/monorepo setup so a single root `npm install` is sufficient.
Suggested implementation:
```
"scripts": {
"dev": "vite",
"build": "vite build",
```
To fully implement your suggestion, you should:
1. Ensure your CI pipeline and any local onboarding scripts explicitly run dependency installation where needed (e.g., `npm install` at the root and in `apps/web` and `netlify/functions` if you keep them separate).
2. Optionally, if `install:all` is no longer used anywhere, remove the `install:all` script from `package.json` to avoid confusion.
3. Longer term, consider configuring npm/yarn/pnpm workspaces so a single root `npm install` handles all subprojects, simplifying your CI setup and caching.
</issue_to_address>
### Comment 6
<location> `apps/web/src/services/api.ts:85-86` </location>
<code_context>
- // Authentication API calls (Supabase-powered)
+ // Authentication API calls (Netlify Functions)
+ // Note: Current backend uses user-based sessions without JWT tokens
async signIn(email: string, password: string): Promise<SignInResponse> {
- const response = await this.request('/api/auth/signin', {
+ const response = await this.request('/auth-login', {
method: 'POST',
body: JSON.stringify({ email, password })
</code_context>
<issue_to_address>
**🚨 question (security):** The frontend still passes a password to `auth-login`, but the backend ignores it entirely.
`signIn` still posts `{ email, password }`, but `/auth-login` only reads `email` and does no password check. This makes the API contract misleading and obscures that login is effectively email-only. Once the intended auth flow (e.g., magic-link vs password-based) is confirmed, please either stop sending `password` or add proper password verification and update the types accordingly.
</issue_to_address>
### Comment 7
<location> `Project_documentation.md:298-307` </location>
<code_context>
+### **Port Architecture**
</code_context>
<issue_to_address>
**issue:** Large section appears duplicated in the documentation.
From this second `### **Port Architecture**` heading down to the footer, this section duplicates an earlier part of the file. Please remove or consolidate it to avoid confusion and keep the docs concise.
</issue_to_address>
### Comment 8
<location> `netlify/functions/achievements.js:52` </location>
<code_context>
`${SUPABASE_URL}/rest/v1/game_sessions?id=eq.${validatedSessionId}&select=user_id`,
</code_context>
<issue_to_address>
**security (javascript.aws-lambda.security.tainted-sql-string):** Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Security Fixes: - auth-signup.js: Use validateServiceKeyConfig and getSupabaseHeaders(true) (required for writing to users table) - netlify.toml: Remove CORS wildcard headers (functions handle CORS with origin validation) - auth-login.js: Add security documentation explaining simplified email-only auth - api.ts: Make password parameter optional, don't send unused data to backend Note: auth-login uses simplified email-lookup for demo purposes. Production should use Supabase Auth, OAuth, or custom password hashing.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries. (link)
General comments:
- The root
install:allscript (cd apps/web && npm install && cd ../../netlify/functions && npm install) resolves to a directory above the repo when run fromapps/web(ending up at../netlify/functionsfrom the parent of the repo); consider correcting the path (e.g.cd apps/web && npm install && cd ../.. && cd netlify/functions && npm install) so installs run in the intended functions folder. - In
AuthContextand theApiServiceyou now have duplicated logic for computing the Netlify Functions base URL fromVITE_BACKEND_URL/import.meta.env.MODE; consider centralizing this URL construction in a single helper to avoid drift between the two call sites. - The new auth flow stores
user.idas the "auth token" viasetAuthToken, which may be misleading and potentially unsafe if used as a bearer token later; consider either renaming this to make it clear it’s just a client-side session identifier or introducing a more robust session/token mechanism.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The root `install:all` script (`cd apps/web && npm install && cd ../../netlify/functions && npm install`) resolves to a directory above the repo when run from `apps/web` (ending up at `../netlify/functions` from the parent of the repo); consider correcting the path (e.g. `cd apps/web && npm install && cd ../.. && cd netlify/functions && npm install`) so installs run in the intended functions folder.
- In `AuthContext` and the `ApiService` you now have duplicated logic for computing the Netlify Functions base URL from `VITE_BACKEND_URL`/`import.meta.env.MODE`; consider centralizing this URL construction in a single helper to avoid drift between the two call sites.
- The new auth flow stores `user.id` as the "auth token" via `setAuthToken`, which may be misleading and potentially unsafe if used as a bearer token later; consider either renaming this to make it clear it’s just a client-side session identifier or introducing a more robust session/token mechanism.
## Individual Comments
### Comment 1
<location> `apps/web/src/services/api.ts:93-94` </location>
<code_context>
+
+ // Backend returns { success, user } without access_token
+ // Store user ID as simple session identifier if needed
+ if (response.success && response.user?.id) {
+ this.setAuthToken(response.user.id);
}
-
</code_context>
<issue_to_address>
**🚨 issue (security):** Using the raw user ID as an auth token is weak and conflates identity with authentication.
Previously we stored a backend-issued access token; now we pass `response.user.id` to `setAuthToken`, effectively using a public identifier as a credential and making user impersonation trivial.
If JWTs are being removed, the backend should still issue a session-specific opaque token (e.g., random UUID or Supabase session ID) that can be revoked/rotated, rather than using the user ID. It may also be worth renaming this to something like `setSessionToken` to make clear it’s not an auth credential.
</issue_to_address>
### Comment 2
<location> `netlify/functions/auth-login.js:11-20` </location>
<code_context>
+1. **Created Netlify Functions**
+ ```javascript
+ // Example: auth-login.js
+ exports.handler = async (event, context) => {
+ // Serverless authentication logic
+ // Replaced Flask route handling
</code_context>
<issue_to_address>
**🚨 issue (security):** Login endpoint ignores password and authenticates purely by email, which is a serious security flaw.
The handler currently authenticates users solely by matching `email` from the request body and creating a session, so anyone who knows a valid email can log in as that user.
If this is meant to be passwordless, the flow must be explicit (magic link, one-time code, OAuth, etc.) and the UI should not ask for a password. Otherwise, add proper password (or other secret) verification against a secure hash and reject invalid attempts. In practice, either:
- Restore secure password verification, or
- Redesign and rename this endpoint and UI as a passwordless flow that issues a magic link/one-time token instead of logging in directly.
</issue_to_address>
### Comment 3
<location> `netlify/functions/auth-signup.js:34-43` </location>
<code_context>
+ const SUPABASE_URL = getSupabaseUrl();
+
+ try {
+ const { email, username, full_name } = JSON.parse(event.body);
+
+ // Validate email format
+ if (!isValidEmail(email)) {
+ return createErrorResponse(400, ErrorMessages.INVALID_EMAIL, corsHeaders);
+ }
+
+ // Check if user already exists
+ const checkResponse = await fetch(`${SUPABASE_URL}/rest/v1/users?email=eq.${encodeURIComponent(email)}`, {
+ headers: getSupabaseHeaders()
+ });
+
+ if (checkResponse.ok) {
+ const existingUsers = await checkResponse.json();
+ if (existingUsers.length > 0) {
+ return createErrorResponse(400, ErrorMessages.USER_EXISTS, corsHeaders);
+ }
+ }
+
+ const userData = {
+ email: email,
+ username: username || email.split('@')[0],
</code_context>
<issue_to_address>
**🚨 issue (security):** Signup endpoint ignores password and creates accounts without any credential, which conflicts with the previous auth model.
The handler only reads `email`, `username`, and `full_name` and never accepts or stores any credential, while `auth-login` authenticates purely by email. Together, this effectively removes real authentication.
If passwordless is intentional, update the API/UI to remove password fields and implement an explicit passwordless flow (e.g., magic links). If not, this endpoint should accept and persist a password hash (or delegate to Supabase Auth) and be aligned with how `auth-login` verifies users.
</issue_to_address>
### Comment 4
<location> `netlify.toml:9-25` </location>
<code_context>
+ node_bundler = "esbuild"
+
+# CORS headers for API functions
+[[headers]]
+ for = "/.netlify/functions/*"
+ [headers.values]
+ Access-Control-Allow-Origin = "*"
+ Access-Control-Allow-Methods = "GET, POST, PUT, PATCH, DELETE, OPTIONS"
+ Access-Control-Allow-Headers = "Content-Type, Authorization"
+
+# API redirects to Netlify Functions
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Global `Access-Control-Allow-Origin: *` header can conflict with per-function CORS logic and credentials settings.
Netlify is setting a wildcard `Access-Control-Allow-Origin` for `/.netlify/functions/*`, while each function also sets its own CORS headers (including `Access-Control-Allow-Credentials: true` and specific origins). Depending on how Netlify merges headers, this can cause duplicate `Access-Control-Allow-Origin` values or `*` combined with `Allow-Credentials`, which browsers will reject.
To avoid this, use a single CORS strategy:
- Preferably remove or narrow the Netlify `[[headers]]` block for functions and let function code handle CORS, or
- Make the function-level CORS logic match the global `*` policy and omit `Allow-Credentials` in that case.
```suggestion
[functions]
directory = "netlify/functions"
node_bundler = "esbuild"
# NOTE: CORS for API functions is handled within each Netlify Function.
# Avoid setting global wildcard CORS headers here to prevent conflicts
# with per-function CORS (especially when using credentials and
# function-specific origins).
# API redirects to Netlify Functions
[[redirects]]
from = "/api/*"
to = "/.netlify/functions/:splat"
status = 200
```
</issue_to_address>
### Comment 5
<location> `Project_documentation.md:298` </location>
<code_context>
+◈ Loaded function quantum-measurements
+```
+
+### **Port Architecture**
+- **Port 8888**: Main development server (your access point)
+- **Port 3999**: Internal static file server (background)
</code_context>
<issue_to_address>
**issue:** The entire Port Architecture and subsequent sections appear duplicated at the end of the document.
From `### **Port Architecture**` to the footer, the document repeats sections that already appear earlier (Port Architecture, Technical Details, Troubleshooting, Deployment, Project Metrics, Next Steps, Additional Resources, Contributing). Please remove the duplicated block or consolidate so each section appears only once.
</issue_to_address>
### Comment 6
<location> `Installation_guide.md:53` </location>
<code_context>
+2. Installs backend deps in `netlify/functions/` (~10MB)
+3. Total: ~350MB of dependencies
+
+### **4. Setup Environment**
+```powershell
+# Copy example environment file
</code_context>
<issue_to_address>
**nitpick (typo):** Use "Set Up" (verb) instead of "Setup" (noun) in this instructional heading.
As this is an action step, please update the heading to `### **4. Set Up Environment**` to use the correct verb form.
```suggestion
### **4. Set Up Environment**
```
</issue_to_address>
### Comment 7
<location> `netlify/functions/achievements.js:52` </location>
<code_context>
`${SUPABASE_URL}/rest/v1/game_sessions?id=eq.${validatedSessionId}&select=user_id`,
</code_context>
<issue_to_address>
**security (javascript.aws-lambda.security.tainted-sql-string):** Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries. (link)
General comments:
- The email-only
auth-loginfunction is explicitly marked as demo/auth-lite but is always enabled; consider gating this behavior behind an environment flag (e.g.DEMO_AUTH_ENABLED) or restricting it to non-production environments to avoid it being accidentally used as the primary auth mechanism in production. - In
ApiService.signIn, the code now storesresponse.user.idviasetAuthTokenwhile still presenting aSignInResponseshape that used to carry JWTs; it may be worth renaming this token or separating a propersessionId/userIdfrom auth tokens to avoid downstream confusion where a JWT is expected. - The Google OAuth function (
auth-google.js) assumesGOOGLE_CLIENT_IDis present but doesn’t validate it like the Supabase config; adding an explicit check with a clear error path (similar tovalidateServiceKeyConfig) would make misconfiguration failures easier to diagnose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The email-only `auth-login` function is explicitly marked as demo/auth-lite but is always enabled; consider gating this behavior behind an environment flag (e.g. `DEMO_AUTH_ENABLED`) or restricting it to non-production environments to avoid it being accidentally used as the primary auth mechanism in production.
- In `ApiService.signIn`, the code now stores `response.user.id` via `setAuthToken` while still presenting a `SignInResponse` shape that used to carry JWTs; it may be worth renaming this token or separating a proper `sessionId`/`userId` from auth tokens to avoid downstream confusion where a JWT is expected.
- The Google OAuth function (`auth-google.js`) assumes `GOOGLE_CLIENT_ID` is present but doesn’t validate it like the Supabase config; adding an explicit check with a clear error path (similar to `validateServiceKeyConfig`) would make misconfiguration failures easier to diagnose.
## Individual Comments
### Comment 1
<location> `netlify/functions/auth-signup.js:34-43` </location>
<code_context>
- return;
- }
-
- try {
- await signInWithGoogle(credential);
- toast.success("Welcome to Quantum Quest!");
</code_context>
<issue_to_address>
**issue (bug_risk):** Using the anon key for `leaderboard_entries` writes may conflict with RLS and is inconsistent with other auth functions.
In `auth-signup`, the user is created with `getSupabaseHeaders(true)` (service key), but the initial `leaderboard_entries` insert uses `getSupabaseHeaders()` (anon key), whereas `auth-google` uses the service key for the same insert.
With RLS on `leaderboard_entries`, this mismatch can cause 401/403s or silent failures in the signup flow while Google auth continues to work. Please align this with `auth-google` by using `getSupabaseHeaders(true)` here as well, or clearly define RLS policies that allow anon writes to `leaderboard_entries` and document that expectation.
</issue_to_address>
### Comment 2
<location> `Project_documentation.md:298-307` </location>
<code_context>
+### **Port Architecture**
</code_context>
<issue_to_address>
**issue:** Large sections of the documentation are duplicated at the end of the file.
The block from `### **Port Architecture**` through `*Maintained by: QPlay Team*` is duplicated. Please remove one copy so this content appears only once in the file.
</issue_to_address>
### Comment 3
<location> `netlify/functions/achievements.js:52` </location>
<code_context>
`${SUPABASE_URL}/rest/v1/game_sessions?id=eq.${validatedSessionId}&select=user_id`,
</code_context>
<issue_to_address>
**security (javascript.aws-lambda.security.tainted-sql-string):** Detected user input used to manually construct a SQL string. This is usually bad practice because manual construction could accidentally result in a SQL injection. An attacker could use a SQL injection to steal or modify contents of the database. Instead, use a parameterized query which is available by default in most database engines. Alternatively, consider using an object-relational mapper (ORM) such as Sequelize which will protect your queries.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| try { | ||
| const { email, username, full_name } = JSON.parse(event.body); | ||
|
|
||
| // Validate email format | ||
| if (!isValidEmail(email)) { | ||
| return createErrorResponse(400, ErrorMessages.INVALID_EMAIL, corsHeaders); | ||
| } | ||
|
|
||
| // Check if user already exists | ||
| const checkResponse = await fetch(`${SUPABASE_URL}/rest/v1/users?email=eq.${encodeURIComponent(email)}`, { |
There was a problem hiding this comment.
issue (bug_risk): Using the anon key for leaderboard_entries writes may conflict with RLS and is inconsistent with other auth functions.
In auth-signup, the user is created with getSupabaseHeaders(true) (service key), but the initial leaderboard_entries insert uses getSupabaseHeaders() (anon key), whereas auth-google uses the service key for the same insert.
With RLS on leaderboard_entries, this mismatch can cause 401/403s or silent failures in the signup flow while Google auth continues to work. Please align this with auth-google by using getSupabaseHeaders(true) here as well, or clearly define RLS policies that allow anon writes to leaderboard_entries and document that expectation.
Adding this as a PR so we can get an overview and reviews by copilot
Summary by Sourcery
Migrate the project from a Flask-based backend to a Netlify Functions–backed monorepo, update the frontend to consume the new serverless APIs, and add comprehensive documentation and CI/CD for Netlify deployment.
New Features:
Enhancements:
Build:
CI:
Documentation:
Chores: