SS1214#104
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughIntroduces comprehensive activity tracking enhancements: timezone auto-detection and storage, birthday webhook notifications with cron execution, idle-time configuration, activity dashboard refactoring with tabbed UI components, profile information editing with timezone and member linking, session pattern editing workflow, leaderboard style selection (list/podium), quotas page redesign, and supporting API endpoints and database schema extensions. Changes
Sequence DiagramssequenceDiagram
actor User
participant Frontend as Client/UI
participant API as API Handler
participant Discord as Discord Webhook
participant Database as Database
User->>Frontend: Test Birthday Webhook
Frontend->>API: POST /settings/general/birthdays/test<br/>(webhookUrl)
activate API
API->>API: Validate webhook URL format
API->>Discord: POST embed payload
activate Discord
Discord-->>API: 200 OK / Error
deactivate Discord
alt Success
API-->>Frontend: { success: true }
Frontend-->>User: Toast: "Test sent successfully"
else Failed
API-->>Frontend: { error: "..." }
Frontend-->>User: Toast: "Test failed"
end
deactivate API
sequenceDiagram
actor User
participant Frontend as Edit Session UI
participant Dialog as PatternEditDialog
participant API as API Handler
participant Database as Database
User->>Frontend: Click Edit on Recurring Session
alt Session has scheduleId (pattern)
Frontend->>Dialog: Open PatternEditDialog
activate Dialog
User->>Dialog: Select Scope<br/>(single/future/all)
Dialog-->>Frontend: onConfirm(scope)
deactivate Dialog
Frontend->>API: PUT /sessions/[sid]/update-pattern<br/>(scope, changes)
activate API
alt Scope: single
API->>Database: Update this session only
else Scope: future
API->>Database: Update this + future sessions<br/>in same pattern
else Scope: all
API->>Database: Update all sessions<br/>in pattern
end
Database-->>API: Updated
API-->>Frontend: { updatedCount, success }
deactivate API
Frontend-->>User: Toast: "Updated (scope)"
else No pattern
Frontend->>API: Standard session edit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| try { | ||
| if (skip) { | ||
| await axios.post(`/api/workspace/${workspaceId}/birthday`, { day: 0, month: 0 }); | ||
| await axios.post(`/api/workspace/${workspaceId}/birthday`, { day: 0, month: 0, timezone }); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix this vulnerability, we should validate the workspaceId before using it to construct API request URLs. Specifically, we should:
- Accept only
workspaceIdvalues that conform to an expected pattern and are valid for the use-case. If workspace IDs are numeric, ensure they are numbers. If they are UUIDs or have a fixed format, strictly check that the input matches. - If
workspaceIdis expected to be a numeric ID, coerce toNumberand ensure it is a finite positive integer (not NaN, Infinity, or negative), otherwise bail out. - If alphanumeric or UUID, use a regex to ensure the value matches the expected format.
- Reject (do not make the request, or show an error/return null) if the value does not pass validation, preventing attempts to forge paths, perform path traversal, or inject unexpected values. That way, even if the user provides a malicious
id, no sensitive or unexpected backend route is accessed via the frontend.
The changes are to be implemented in components/bdayprompt.tsx:
- Before using
workspaceIdin any backend API call (in theuseEffectandsavemethods), validate its value per above. - Only proceed with fetch/post requests if the value is valid; otherwise, skip with a comment or log.
We do not modify backend code, nor do we change the way props are set from layouts/workspace.tsx—we simply validate at the point of use.
| @@ -15,13 +15,27 @@ | ||
| const [loading, setLoading] = useState(false); | ||
| const [initialLoaded, setInitialLoaded] = useState(false); | ||
|
|
||
| // Helper for validating workspaceId: allow only numbers or UUIDs. Adjust pattern as needed. | ||
| const isValidWorkspaceId = (id: string | number): boolean => { | ||
| if (typeof id === "number") { | ||
| return Number.isInteger(id) && id > 0; | ||
| } | ||
| if (typeof id === "string") { | ||
| // Accept only positive integer string or canonical UUID; adjust as needed for your system | ||
| const intPattern = /^[1-9][0-9]*$/; | ||
| const uuidPattern = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; | ||
| return intPattern.test(id) || uuidPattern.test(id); | ||
| } | ||
| return false; | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| const detectedTimezone = Intl.DateTimeFormat().resolvedOptions().timeZone; | ||
| setTimezone(detectedTimezone); | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (!workspaceId) return; | ||
| if (!workspaceId || !isValidWorkspaceId(workspaceId)) return; | ||
| let cancelled = false; | ||
| (async () => { | ||
| try { | ||
| @@ -58,6 +66,11 @@ | ||
| const save = async (skip = false) => { | ||
| setLoading(true); | ||
| try { | ||
| if (!isValidWorkspaceId(workspaceId)) { | ||
| // Silently ignore or notify user; at minimum, avoid possibly malicious request | ||
| setOpen(false); | ||
| return; | ||
| } | ||
| if (skip) { | ||
| await axios.post(`/api/workspace/${workspaceId}/birthday`, { day: 0, month: 0, timezone }); | ||
| } else { |
| await axios.post(`/api/workspace/${workspaceId}/birthday`, { day: 0, month: 0, timezone }); | ||
| } else { | ||
| await axios.post(`/api/workspace/${workspaceId}/birthday`, { day: Number(day), month: Number(month) }); | ||
| await axios.post(`/api/workspace/${workspaceId}/birthday`, { day: Number(day), month: Number(month), timezone }); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix the SSRF/path traversal risk, we should validate (or sanitize) workspaceId before including it in the URL. Since the allowed values are likely to be either numeric or slug/UUID identifiers for existing workspaces, we should constrain the allowed set as strictly as possible. For example, if IDs are always numbers, allow only numbers; if they're UUIDs, validate the UUID pattern; if they're slugs, allow only alphanumerics (and possibly -/_). This can be achieved with simple regex checks or type coercion. If the identifier fails validation, do not make the API call (or raise an error/UI notification).
The ideal place for this validation is just before making the API request—namely, in components/bdayprompt.tsx. We'll add a function to check that workspaceId is valid, and ensure the API call is only made if so. We'll apply validation in both the useEffect (GET) and in the save (POST).
If not valid, the component should not interact with the API.
| @@ -7,6 +7,23 @@ | ||
| visible?: boolean; | ||
| } | ||
|
|
||
| // Only allow numeric IDs or UUID format; adjust regex as needed for your ID convention | ||
| function isValidWorkspaceId(id: unknown): boolean { | ||
| // Numeric ID | ||
| if (typeof id === "number") { | ||
| return Number.isInteger(id) && id > 0; | ||
| } | ||
| if (typeof id === "string") { | ||
| // Allow integer strings | ||
| if (/^\d+$/.test(id)) return true; | ||
| // Allow simple slug (alphanumeric, dash, underscore, at least 3 chars) | ||
| if (/^[a-zA-Z0-9-_]{3,}$/.test(id)) return true; | ||
| // Allow UUIDs (version 1-5, optional dashes) | ||
| if (/^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(id)) return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| export const WorkspaceBirthdayPrompt: React.FC<WorkspaceBirthdayPromptProps> = ({ workspaceId, visible }) => { | ||
| const [open, setOpen] = useState(false); | ||
| const [month, setMonth] = useState(''); | ||
| @@ -21,7 +38,7 @@ | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (!workspaceId) return; | ||
| if (!isValidWorkspaceId(workspaceId)) return; | ||
| let cancelled = false; | ||
| (async () => { | ||
| try { | ||
| @@ -56,6 +73,10 @@ | ||
| const days = month ? Array.from({ length: daysInMonth(Number(month)) }, (_, i) => i + 1) : []; | ||
|
|
||
| const save = async (skip = false) => { | ||
| if (!isValidWorkspaceId(workspaceId)) { | ||
| setLoading(false); | ||
| return; | ||
| } | ||
| setLoading(true); | ||
| try { | ||
| if (skip) { | ||
| @@ -69,7 +90,7 @@ | ||
| } | ||
| }; | ||
|
|
||
| if (!open || !initialLoaded) return null; | ||
| if (!isValidWorkspaceId(workspaceId) || !open || !initialLoaded) return null; | ||
|
|
||
| return ( | ||
| <div className="fixed inset-0 flex items-center justify-center bg-black/50 z-50 p-4"> |
| const fetchConfig = async () => { | ||
| if (id) { | ||
| try { | ||
| const res = await axios.get(`/api/workspace/${id}/settings/activity/getConfig`); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix this problem while preserving existing frontend functionality, we should ensure that the user-provided id parameter cannot be abused to create unexpected or unauthorized backend API calls. The best fix, given only the shown code, is to strictly validate id before inserting it into the URL. Typically, workspace IDs have a fixed expected format, such as an alphanumeric string, UUID, or database object ID. We should ensure that id matches the allowed pattern (e.g., /^[a-zA-Z0-9_-]+$/ for alphanumeric + dashes/underscores) before making the API call. If it does not, we should avoid making the request, show an error, and possibly log the event.
The change should be applied within the file components/profile/activity.tsx, specifically within the fetchConfig function inside the useEffect on line 128–140. This involves validating id before calling axios, and handling the error scenario. No third-party validation package is required; a simple regex suffices.
| @@ -127,13 +127,16 @@ | ||
|
|
||
| useEffect(() => { | ||
| const fetchConfig = async () => { | ||
| if (id) { | ||
| if (id && typeof id === "string" && /^[a-zA-Z0-9_-]+$/.test(id)) { | ||
| try { | ||
| const res = await axios.get(`/api/workspace/${id}/settings/activity/getConfig`); | ||
| setIdleTimeEnabled(res.data.idleTimeEnabled ?? true); | ||
| } catch (error) { | ||
| console.error("Failed to fetch activity config:", error); | ||
| } | ||
| } else if (id) { | ||
| // Invalid id, do not send request | ||
| console.error("Invalid workspace id provided:", id); | ||
| } | ||
| }; | ||
| fetchConfig(); |
| const { data, status } = await axios.get( | ||
| `/api/workspace/${id}/activity/${sessionId}` | ||
| ); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix this SSRF (and potential path traversal) risk, the user-supplied id value should be validated before being used to construct API endpoint paths. The recommended approach is to explicitly validate id against an allow-list of acceptable values (if feasible), or at minimum sanitize the value to ensure it only contains permitted characters (e.g., a UUID, numeric string, or alphanumeric slug). For this component, assuming id should be a string consisting of standard URL-safe characters (such as a UUID, alphanumerics, or slug), we should validate it before it is used.
A safe and straightforward patch, within the shown code, is to check that id matches an expected pattern (such as a UUID, or whitelist of workspace IDs if available). If invalid, do not make the API request and show an error. This validation can be done at the start of fetchSession.
Changes needed:
- Before using
idwhen forming the API path, validateidmatches an expected pattern (e.g.,/^[a-zA-Z0-9_-]+$/for slugs, or a stricter UUID regex if appropriate). - If
idis invalid, do not proceed with the API request, and show a user-friendly error. - No new packages are needed; regex validation is standard.
- Only code inside the
components/profile/activityoverview.tsxfile (as shown) needs to be edited.
| @@ -175,6 +175,12 @@ | ||
| }, [liveSessionTimer]); | ||
|
|
||
| const fetchSession = async (sessionId: string) => { | ||
| // Validate the 'id' to allow only workspace slugs (alphanumeric, -, _), adjust regex as needed | ||
| const idPattern = /^[a-zA-Z0-9_-]+$/; | ||
| if (!id || typeof id !== "string" || !idPattern.test(id)) { | ||
| toast.error("Invalid workspace ID."); | ||
| return; | ||
| } | ||
| setLoading(true); | ||
| setIsOpen(true); | ||
| setConcurrentUsers([]); |
| const concurrentResponse = await axios.get( | ||
| `/api/workspace/${id}/activity/concurrent?sessionId=${sessionId}&startTime=${data.message.startTime}&endTime=${data.message.endTime}` | ||
| ); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix the problem, user-supplied data from router.query.id must be validated before being interpolated into the API endpoint URL. The best-practice way is to define a whitelist or strict validation for workspace IDs, ensuring it follows expected format—for example, a UUID, alphanumeric string, or similar—before using it in any API call. This can be implemented in the React component with a simple validation check, for example:
- Accept only alphanumeric/known-safe characters for workspace IDs.
- If invalid, refuse to make the request or replace with a safe fallback.
The safest edit is to define a function to check that id is a string and matches the expected pattern before interpolating it in any outgoing API request. If not, show an error and return early. This logic belongs directly before the lines where the risky call is made—within the code shown, in the relevant async function.
We also need to ensure that validation is DRY and clear, so a helper like isValidWorkspaceId(id) can be introduced (above/beside the component definition, or in a relevant spot within the file). The validation can be simple (e.g., /^[a-zA-Z0-9_-]+$/ for slugs/IDs, or a UUID regex for UUIDs).
Edits needed:
- Add a function to validate
id. - Before using
/api/workspace/${id}/activity/concurrent?sessionId=..., check thatidis valid. - If not valid, do not send request and notify the user.
| @@ -93,6 +93,11 @@ | ||
| avatar: string; | ||
| }; | ||
|
|
||
| // Validate workspace IDs: Only allow alphanumeric, hyphens, or underscores (adjust as needed) | ||
| function isValidWorkspaceId(id: unknown): id is string { | ||
| return typeof id === "string" && /^[a-zA-Z0-9_-]+$/.test(id); | ||
| } | ||
|
|
||
| export function ActivityOverview({ | ||
| data, | ||
| displayMinutes, | ||
| @@ -200,6 +205,11 @@ | ||
| }); | ||
|
|
||
| if (data.message?.startTime && data.message?.endTime) { | ||
| if (!isValidWorkspaceId(id)) { | ||
| toast.error("Invalid workspace ID."); | ||
| setLoading(false); | ||
| return; | ||
| } | ||
| try { | ||
| const concurrentResponse = await axios.get( | ||
| `/api/workspace/${id}/activity/concurrent?sessionId=${sessionId}&startTime=${data.message.startTime}&endTime=${data.message.endTime}` |
| await axios.patch( | ||
| `/api/workspace/${workspaceId}/profile/${user.userid}/member-info`, | ||
| { | ||
| department, | ||
| lineManagerId: selectedManager?.userid || null, | ||
| timezone: selectedTimezone || null, | ||
| birthdayDay: birthdayDay ? parseInt(birthdayDay as string) : null, | ||
| birthdayMonth: birthdayMonth ? parseInt(birthdayMonth as string) : null, | ||
| discordId: discordId || null, | ||
| } | ||
| ); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix this SSRF concern, the value of workspaceId (taken from router.query.id) should be validated or sanitized before it's interpolated into the API endpoint URL. The best practice is to:
- Whitelist or validate the format of
workspaceId, ensuring it only contains allowed characters (e.g., alphanumerics, hyphens, or whatever your workspace IDs are guaranteed to be). - Reject or sanitize (return or handle gracefully) if the value does not conform.
- The validation should be performed just after assigning
workspaceIdand before using it in requests. - For safety, avoid any
/,\, or other path traversal characters.
To implement this:
- Add a function to validate workspace IDs (e.g., only allow slugs like
/^[a-zA-Z0-9-_]+$/). - In
handleSave, before proceeding, checkworkspaceIdagainst this function; if invalid, don't proceed (show error and return). - If using TypeScript, optionally cast/parse to the expected type.
- No external dependencies are needed; use a simple regex.
Edit to make:
- Insert a
validateWorkspaceIdhelper (if not already present). - Check and return early in
handleSaveif invalid.
| @@ -139,6 +139,12 @@ | ||
| const [localTime, setLocalTime] = useState(""); | ||
| const [isNight, setIsNight] = useState(false); | ||
|
|
||
|
|
||
| // Only allow alphanumerics, dashes, underscores (adjust as needed) | ||
| function validateWorkspaceId(id: string): boolean { | ||
| return typeof id === "string" && /^[a-zA-Z0-9_-]+$/.test(id); | ||
| } | ||
|
|
||
| const workspaceId = router.query.id as string; | ||
| const canEdit = canEditMembers && (isUser || isAdmin); | ||
|
|
||
| @@ -157,7 +163,10 @@ | ||
| }, [workspaceMember?.timezone]); | ||
|
|
||
| const handleSave = async () => { | ||
| if (!workspaceId) return; | ||
| if (!workspaceId || !validateWorkspaceId(workspaceId)) { | ||
| toast.error("Invalid workspace ID"); | ||
| return; | ||
| } | ||
| setLoading(true); | ||
| try { | ||
| await axios.patch( |
| axios | ||
| .get( | ||
| `/api/workspace/${router.query.id}/settings/general/birthdays/hook` | ||
| ) |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To address potential SSRF or routing manipulation, we should strictly validate or sanitize router.query.id before including it in a constructed URL. If the workspace id should be a UUID, restrict it to valid UUIDs; if numeric, restrict to valid integer IDs. In this code, before using router.query.id in the axios calls, we can perform basic validation to ensure it conforms to the expected id format. If it doesn’t, we should refuse to issue the backend API request and optionally show an error. The minimal change is to add a validation function (e.g., isValidId(id)) and only proceed with data fetching and requests if id passes validation.
The changes will go in the file components/settings/general/bhooks.tsx. We need to:
- Define a validation function for workspace IDs.
- Use this function to guard outgoing axios requests (on
.getas well as other places usingrouter.query.id).
No external dependencies are needed unless the ID format is complex (for simple UUID or numeric, no new packages are required).
| @@ -14,8 +14,15 @@ | ||
| const [loading, setLoading] = useState(false); | ||
| const [testing, setTesting] = useState(false); | ||
|
|
||
| // Accept only UUIDs as workspace IDs | ||
| function isValidWorkspaceId(id: string | string[] | undefined): boolean { | ||
| // If your IDs are not UUIDs, adjust this regex accordingly | ||
| if (typeof id !== 'string') return false; | ||
| return /^[0-9a-fA-F\-]{36}$/.test(id); | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| if (router.query.id) { | ||
| if (router.query.id && isValidWorkspaceId(router.query.id)) { | ||
| axios | ||
| .get( | ||
| `/api/workspace/${router.query.id}/settings/general/birthdays/hook` | ||
| @@ -29,10 +35,17 @@ | ||
| .catch((err) => { | ||
| console.error("Error fetching birthday webhook config:", err); | ||
| }); | ||
| } else if (router.query.id) { | ||
| toast.error("Invalid workspace ID"); | ||
| } | ||
| }, [router.query.id]); | ||
|
|
||
| const handleSave = async () => { | ||
| // Validate workspace id before saving | ||
| if (!isValidWorkspaceId(router.query.id)) { | ||
| toast.error("Invalid workspace ID"); | ||
| return; | ||
| } | ||
| setLoading(true); | ||
| try { | ||
| await axios.patch( | ||
| @@ -56,6 +65,11 @@ | ||
| toast.error("Please enter a webhook URL first"); | ||
| return; | ||
| } | ||
| // Validate workspace id before testing | ||
| if (!isValidWorkspaceId(router.query.id)) { | ||
| toast.error("Invalid workspace ID"); | ||
| return; | ||
| } | ||
|
|
||
| setTesting(true); | ||
| try { |
| await axios.patch( | ||
| `/api/workspace/${router.query.id}/settings/general/birthdays/hook`, | ||
| { | ||
| enabled, | ||
| url: webhookUrl, | ||
| } | ||
| ); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix this problem, constrain router.query.id to only allow safe values before using it in the URL for the API call. Typically, this involves validating that id matches the expected format (such as a UUID, numeric ID, or a string of specific permitted characters), thus preventing arbitrary path injection, path traversal, or other abuses. The best single fix is to extract id from router.query, validate it against a strict pattern (for example, a RegExp that matches only allowed formats), and only use it if validation passes. Otherwise, display an error or abort the operation. This can be done inline before using the value in string interpolation for the URL.
What is needed:
- Import or define a function or regex to validate the allowed format for
id(e.g., only alphanumeric, or UUID). - Add a check before making the
axios.patchcall to ensure thatrouter.query.idis valid. - Do the same validation in the
useEffectthat makes anaxios.getcall to the same dynamic endpoint. - Display an error or otherwise abort if the value is invalid.
| @@ -15,10 +15,14 @@ | ||
| const [testing, setTesting] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| if (router.query.id) { | ||
| const id = router.query.id; | ||
| // Restrict id to a valid pattern (e.g., UUID v4, or only alphanumerics/hyphen/underscore) | ||
| // Adjust the regex as appropriate for your IDs | ||
| const validIdPattern = /^[a-zA-Z0-9-_]{1,64}$/; | ||
| if (typeof id === "string" && validIdPattern.test(id)) { | ||
| axios | ||
| .get( | ||
| `/api/workspace/${router.query.id}/settings/general/birthdays/hook` | ||
| `/api/workspace/${id}/settings/general/birthdays/hook` | ||
| ) | ||
| .then((res) => { | ||
| if (res.data.value) { | ||
| @@ -29,14 +31,23 @@ | ||
| .catch((err) => { | ||
| console.error("Error fetching birthday webhook config:", err); | ||
| }); | ||
| } else if (id) { | ||
| console.error("Invalid workspace ID format."); | ||
| } | ||
| }, [router.query.id]); | ||
|
|
||
| const handleSave = async () => { | ||
| setLoading(true); | ||
| const id = router.query.id; | ||
| const validIdPattern = /^[a-zA-Z0-9-_]{1,64}$/; | ||
| if (typeof id !== "string" || !validIdPattern.test(id)) { | ||
| toast.error("Invalid workspace ID."); | ||
| setLoading(false); | ||
| return; | ||
| } | ||
| try { | ||
| await axios.patch( | ||
| `/api/workspace/${router.query.id}/settings/general/birthdays/hook`, | ||
| `/api/workspace/${id}/settings/general/birthdays/hook`, | ||
| { | ||
| enabled, | ||
| url: webhookUrl, |
| const response = await axios.post( | ||
| `/api/workspace/${router.query.id}/settings/general/birthdays/test`, | ||
| { url: webhookUrl } | ||
| ); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
The solution is to sanitize and validate the router.query.id value before incorporating it into the API request URL. If the backend expects a specific format or set of workspace IDs (UUIDs, numerical IDs, etc.), the frontend should confirm the value matches the allowed pattern before making the request. Ideally, an allow-list or regex match can enforce this. If invalid, the action should be halted and the user notified. This fix concerns the usages of router.query.id in /api/workspace/${router.query.id}/settings/general/birthdays/test (in handleTest), as well as /api/workspace/${router.query.id}/settings/general/birthdays/hook (in useEffect and handleSave), to consistently prevent any malformed or malicious input from being sent to the backend API route.
Specifically, add a validation helper (e.g., isValidId) above or within the component that only allows workspace IDs matching an expected pattern (such as /^[a-zA-Z0-9_-]+$/). Before every API request that uses router.query.id, check that router.query.id is valid; if not, do not proceed (perhaps show an error toast). This requires a new function definition and checks at all API-call points where router.query.id is interpolated.
| @@ -6,6 +6,11 @@ | ||
| import { useRouter } from "next/router"; | ||
| import { IconGift, IconCheck, IconX } from "@tabler/icons-react"; | ||
|
|
||
| // Helper to validate workspace id (allow only alphanumerics, '-', '_' for example) | ||
| function isValidId(id: any) { | ||
| return typeof id === 'string' && /^[a-zA-Z0-9_-]+$/.test(id); | ||
| } | ||
|
|
||
| function BirthdayWebhook() { | ||
| const [workspace, setWorkspace] = useRecoilState(workspacestate); | ||
| const router = useRouter(); | ||
| @@ -15,7 +20,7 @@ | ||
| const [testing, setTesting] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| if (router.query.id) { | ||
| if (router.query.id && isValidId(router.query.id)) { | ||
| axios | ||
| .get( | ||
| `/api/workspace/${router.query.id}/settings/general/birthdays/hook` | ||
| @@ -29,10 +34,16 @@ | ||
| .catch((err) => { | ||
| console.error("Error fetching birthday webhook config:", err); | ||
| }); | ||
| } else if (router.query.id) { | ||
| toast.error("Invalid workspace ID"); | ||
| } | ||
| }, [router.query.id]); | ||
|
|
||
| const handleSave = async () => { | ||
| if (!isValidId(router.query.id)) { | ||
| toast.error("Invalid workspace ID"); | ||
| return; | ||
| } | ||
| setLoading(true); | ||
| try { | ||
| await axios.patch( | ||
| @@ -56,6 +63,10 @@ | ||
| toast.error("Please enter a webhook URL first"); | ||
| return; | ||
| } | ||
| if (!isValidId(router.query.id)) { | ||
| toast.error("Invalid workspace ID"); | ||
| return; | ||
| } | ||
|
|
||
| setTesting(true); | ||
| try { |
| const response = await fetch(webhookConfig.url, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify(webhookBody), | ||
| }); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
General approach:
Validate all webhook URLs strictly against an allow-list of domains and URL structure before making outgoing HTTP requests. Do not rely solely on validation at the configuration-input endpoints. Specifically, before making the fetch request in pages/api/cron/birthdays.ts, ensure the loaded webhook URL is a valid HTTPS Discord webhook endpoint (using a strict regex).
Detailed description:
- In
pages/api/cron/birthdays.ts, before callingfetch(webhookConfig.url, ...), check thatwebhookConfig.urlis a string and matches a strict regular expression for Discord webhook URLs (e.g.,^https:\/\/discord\.com\/api\/webhooks\/\d+\/[\w\-]+). - If the check fails, skip sending the request for that workspace (just as the code skips if the URL isn't present).
- No changes are needed to input or config-writing endpoints, since the key fix is to prevent SSRF via runtime validation at the request sink.
- No additional dependencies are required to use regex matching.
- Add a helper function, e.g.,
isValidDiscordWebhookURL(url)above the handler, and use this to guard the request.
What to change:
- In
pages/api/cron/birthdays.ts:- Add the validator function.
- Change the webhook sending section to check for URL validity before sending.
| @@ -2,6 +2,12 @@ | ||
| import prisma from "@/utils/database"; | ||
| import { getConfig } from "@/utils/configEngine"; | ||
|
|
||
| // Only allow real Discord webhook URLs | ||
| function isValidDiscordWebhookURL(url: any): boolean { | ||
| if (typeof url !== 'string') return false; | ||
| return /^https:\/\/discord\.com\/api\/webhooks\/\d+\/[\w\-]+/i.test(url); | ||
| } | ||
|
|
||
| export default async function handler( | ||
| req: NextApiRequest, | ||
| res: NextApiResponse | ||
| @@ -29,7 +35,12 @@ | ||
| try { | ||
| const webhookConfig = await getConfig("birthday_webhook", workspace.groupId); | ||
|
|
||
| if (!webhookConfig || !webhookConfig.enabled || !webhookConfig.url) { | ||
| if ( | ||
| !webhookConfig || | ||
| !webhookConfig.enabled || | ||
| !webhookConfig.url || | ||
| !isValidDiscordWebhookURL(webhookConfig.url) | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pages/api/admin/instance-config.ts (1)
21-40:robloxClientSecretis stored and returned in plaintext — tighten secret handling before shipping.Even with owner-only access, returning the secret in GET (Line 38) and persisting it raw increases accidental exposure risk (UI/devtools, logs, backups). Consider: don’t return it after initial save (return masked
****+ “isSet” flag), and encrypt-at-rest (KMS/app-level) if it must be stored.Also applies to: 47-55
components/settings/general/color.tsx (1)
39-43: Sync effect won’t handle “clear theme” cases (empty string).
Because Line 40 checks truthiness, a valid “reset to no theme” ("") won’t updateselectedColor. If clearing is supported, set unconditionally (or check forundefinedinstead).- useEffect(() => { - if (workspace?.groupTheme) { - setSelectedColor(workspace.groupTheme); - } - }, [workspace?.groupTheme]); + useEffect(() => { + // Sync even when theme is intentionally cleared to "" + setSelectedColor(workspace?.groupTheme ?? ""); + }, [workspace?.groupTheme]);pages/workspace/[id]/policies.tsx (1)
210-221: Update thepagePropstype to includeisOwnerRole.The
rolestype definition doesn't includeisOwnerRole, but it's used for filtering at lines 1580 and 2231. This causes a type mismatch.type pageProps = { isUserView?: boolean; documents?: (document & { owner: { username: string; picture: string }; roles: Array<{ id: string; name: string }>; acknowledgments: Array<{ user: { userid: string; username: string; picture: string }; acknowledgedAt: string; }>; })[]; - roles?: Array<{ id: string; name: string }>; + roles?: Array<{ id: string; name: string; isOwnerRole?: boolean }>; };prisma/migrations/20251011203302_hw_25/migration.sql (1)
49-57: Some code paths do not setjoinDate, creating members with NULL values.The
joinDatecolumn now has no default, butpages/api/workspace/[id]/home/upcoming.tsandpages/api/workspace/[id]/profile/[uid]/member-info.tscreate workspace members without explicitly settingjoinDate. This results in NULL values instead of the current timestamp.Fix these operations to include
joinDate: new Date()in their create blocks, or document whether NULL joinDate is intentional.pages/api/workspace/[id]/profile/[uid].ts (1)
167-234: FixsessionsLogged.byType: you're readingsession.typebut didn't select it
allSessionParticipationsselects{ id, date, sessionType { ... } }, so(p.session as any).typewill be undefined and everything buckets into"other".include: { session: { select: { id: true, date: true, + type: true, sessionType: { select: { slots: true, name: true, }, }, }, }, },
🟠 Major comments (17)
components/settings/general/color.tsx-45-48 (1)
45-48: Guardworkspace.groupIdbefore PATCHes (avoid calling/api/workspace/undefined/...).
With settings pages, it’s common for workspace to load async;updateColor/updateSessionColorassumeworkspace.groupIdexists. Consider disabling the controls untilworkspace?.groupIdis truthy, or early-return with a toast.Also applies to: 67-82, 95-107
pages/workspace/[id]/wall.tsx-55-66 (1)
55-66: Merge permissions from all user roles.The system supports users having multiple roles in a single workspace, but this code extracts permissions from only the first role. Users with additional roles will lose access to permissions granted by those other roles, causing authorization failures (e.g., a user with
manage_wallin a secondary role won't be able to delete posts).Change:
- const userPermissions = user?.roles?.[0]?.permissions || []; + const userPermissions = user?.roles?.flatMap(role => role.permissions) || [];components/profile/activityoverview.tsx-177-220 (1)
177-220: Potential Server-Side Request Forgery (SSRF) vulnerability.The
idparameter fromrouter.queryis used directly in API URLs without validation. While this is a client-side component, the pattern could be exploited if an attacker can control theidquery parameter to make requests to unintended endpoints.Consider validating the
idparameter before using it in API calls:const fetchSession = async (sessionId: string) => { + // Validate workspace ID format + if (!id || typeof id !== 'string' || !/^\d+$/.test(id)) { + toast.error("Invalid workspace ID"); + return; + } + setLoading(true); setIsOpen(true); setConcurrentUsers([]);components/profile/activityoverview.tsx-147-167 (1)
147-167: Potential infinite loop in useEffect due toliveSessionTimerin dependency array.Including
liveSessionTimerin the dependency array while also setting it inside the effect can cause re-renders. Additionally, on Line 163, accessingliveSessionTimerafter it might have changed can lead to clearing the wrong timer.Consider using a ref to store the timer instead:
-const [liveSessionTimer, setLiveSessionTimer] = useState<NodeJS.Timeout | null>(null); +const liveSessionTimerRef = useRef<NodeJS.Timeout | null>(null); useEffect(() => { const hasLiveSessions = timeline.some( (item) => item.__type === "session" && item.active && !item.endTime ); if (hasLiveSessions) { const timer = setInterval(() => { setTimeline((prev) => [...prev]); }, 60000); - setLiveSessionTimer(timer); + liveSessionTimerRef.current = timer; return () => { clearInterval(timer); - setLiveSessionTimer(null); + liveSessionTimerRef.current = null; }; - } else if (liveSessionTimer) { - clearInterval(liveSessionTimer); - setLiveSessionTimer(null); + } else if (liveSessionTimerRef.current) { + clearInterval(liveSessionTimerRef.current); + liveSessionTimerRef.current = null; } -}, [timeline, liveSessionTimer]); +}, [timeline]);Committable suggestion skipped: line range outside the PR's diff.
pages/api/workspace/[id]/allies/[aid]/visits/[vid].ts-97-105 (1)
97-105: Add the sametime/participantsvalidation on PATCH to prevent 500s.
Current code will throw on invalidparticipantsshapes and can write an Invalid Date.if(!req.body.name || !req.body.time) return res.status(400).json({ success: false, error: 'Missing data' }) + const parsedTime = new Date(req.body.time) + if (Number.isNaN(parsedTime.getTime())) { + return res.status(400).json({ success: false, error: 'Invalid time' }) + } + if (req.body.participants !== undefined && !Array.isArray(req.body.participants)) { + return res.status(400).json({ success: false, error: 'Invalid participants' }) + } const updateData: any = { name: req.body.name, - time: new Date(req.body.time) + time: parsedTime }; if (req.body.participants !== undefined) { - updateData.participants = req.body.participants.map((p: number) => BigInt(p)); + updateData.participants = req.body.participants.map((p: unknown) => { + if (typeof p !== 'number' || !Number.isSafeInteger(p)) throw new Error('Invalid participant id') + return BigInt(p) + }); }Also applies to: 112-113
pages/api/workspace/[id]/activity/stats.ts-7-8 (1)
7-8: GuardworkspaceIdparsing; don’t passNaNinto Prisma/getConfig.
This endpoint now usesreq.query.idearlier, so a bad/missing id becomes a 500 risk.- const workspaceId = parseInt(req.query.id as string); + const rawId = req.query.id + if (typeof rawId !== 'string' || !/^\d+$/.test(rawId)) { + return res.status(400).json({ success: false, error: 'Invalid workspace id' }) + } + const workspaceId = parseInt(rawId, 10);Also applies to: 24-31, 38-50
pages/api/workspace/[id]/allies/[aid]/visits/index.ts-75-76 (1)
75-76: Validatetimeandparticipantsbefore coercion to avoid 500s / bad writes.
Right nowparticipants.map(BigInt)will throw if body shape is wrong (or values aren’t integers), andnew Date(time)can silently become Invalid Date.- const { name, time, participants } = req.body + const { name, time, participants } = req.body ?? {} if(!name || !time) return res.status(400).json({ success: false, error: 'Missing content' }) + const parsedTime = new Date(time) + if (Number.isNaN(parsedTime.getTime())) { + return res.status(400).json({ success: false, error: 'Invalid time' }) + } + if (participants !== undefined && !Array.isArray(participants)) { + return res.status(400).json({ success: false, error: 'Invalid participants' }) + } const visit = await prisma.allyVisit.create({ data: { hostId: req.session.userid, allyId: req.query.aid, name: name, - time: new Date(time), - participants: participants ? participants.map((p: number) => BigInt(p)) : [] + time: parsedTime, + participants: Array.isArray(participants) + ? participants.map((p: unknown) => { + if (typeof p !== 'number' || !Number.isSafeInteger(p)) throw new Error('Invalid participant id') + return BigInt(p) + }) + : [] } })Also applies to: 85-87
pages/api/workspace/[id]/birthday.ts-20-43 (1)
20-43: Don’t overwritejoinDateon birthday updates; validatetimezoneand ensureuserIdtype matches Prisma (BigInt vs number).
SettingjoinDate: nowin theupdatepath will mutate historical join data on every birthday edit. Also, storing arbitrarytimezonestrings will create downstream parsing bugs, anduserId: useridshould match your Prisma scalar type (notablycreatewswritesBigInt(req.session.userid)).- const now = new Date(); + const now = new Date(); await prisma.workspaceMember.upsert({ where: { workspaceGroupId_userId: { workspaceGroupId, userId: userid } }, update: { birthdayDay: day, birthdayMonth: month, ...(timezone && { timezone }), - joinDate: now }, create: { workspaceGroupId, userId: userid, birthdayDay: day, birthdayMonth: month, timezone: timezone || 'UTC', joinDate: now }, });pages/api/workspace/[id]/settings/activity/setIdleTime.ts-27-31 (1)
27-31: Handle null config to prevent TypeError.If no existing activity config exists,
getConfigreturnsnull(perconfigEngine.ts). Spreadingnullwith...activityconfigwill throwTypeError: Cannot convert undefined or null to object.const activityconfig = await getConfig('activity', parseInt(req.query.id as string)); const newconfig = { - ...activityconfig, + ...(activityconfig || {}), idleTimeEnabled: req.body.enabled };pages/api/workspace/[id]/activity/reset.ts-184-201 (1)
184-201: Bug:session.typeis not selected in the participation query.The
allSessionParticipationsquery (lines 131-155) selects onlyidandsessionTypefromsession, but line 194 accesses(p.session as any).type. This will always beundefined, causing all participation sessions to be categorized as'other'insessionsByType.Add
typeto the session select:const allSessionParticipations = await prisma.sessionUser.findMany({ where: { userid: userId, session: { sessionType: { workspaceGroupId }, date: { gte: periodStart, lte: periodEnd, }, }, }, include: { session: { select: { id: true, + type: true, sessionType: { select: { slots: true, name: true, }, }, }, }, }, });Committable suggestion skipped: line range outside the PR's diff.
pages/api/cron/birthdays.ts-38-41 (1)
38-41: Birthday “today” should be timezone-aware (otherwise you’ll fire on the wrong day)Using server
new Date()means “today” depends on where the server runs, not the workspace/member timezone. Since this PR adds timezone storage, consider computing “today” per workspace (or per member viaworkspaceMember.timezone) and filtering accordingly. A low-effort improvement is: store a workspace timezone config and use it here; the higher-correctness approach is per-member timezone.Also applies to: 42-49
pages/api/workspace/[id]/settings/general/birthdays/test.ts-12-33 (1)
12-33: Alignuseridtyping with the rest of the API (likely BigInt)Other endpoints treat session
useridas string and convert viaBigInt(...). Here it's passed directly to Prisma; if your schema usesBigInt, this can silently fail auth/permission checks.- const userId = req.session.userid; + const userIdRaw = req.session.userid; ... - if (!userId || isNaN(workspaceId)) { + if (!userIdRaw || isNaN(workspaceId)) { return res.status(400).json({ success: false, error: "Invalid request" }); } + const userId = typeof userIdRaw === "bigint" ? userIdRaw : BigInt(userIdRaw); ... - const user = await prisma.user.findFirst({ - where: { userid: userId }, + const user = await prisma.user.findFirst({ + where: { userid: userId },pages/api/workspace/[id]/settings/general/session-colors.ts-21-23 (1)
21-23: Missing session middleware wrapper - authentication may fail.Unlike
birthdays/hook.tswhich useswithSessionRoute, this handler directly accesses(req as any).sessionwithout ensuring the session is initialized. This could result insessionbeingundefinedand authentication silently failing.Wrap the handler with
withSessionRoutefor consistent session handling:+import { withSessionRoute } from "@/lib/withSession"; -export default handler; +export default withSessionRoute(handler); async function handler(req: NextApiRequest, res: NextApiResponse<Data>) { // ... - const userId = (req as any).session?.userid; + const userId = req.session.userid;Also applies to: 54-57
pages/workspace/[id]/quotas.tsx-55-66 (1)
55-66:getRandomColorusesMath.random()which causes hydration mismatches.This function is called during render for role badges (lines 635, 725), but
Math.random()produces different values on server vs client, causing React hydration warnings and visual flicker.Use the deterministic
getRandomBgfunction (which uses a hash) instead, or pass a stable identifier.-const getRandomColor = () => { - const colors = [ - "bg-blue-500", - "bg-green-500", - "bg-yellow-500", - "bg-red-500", - "bg-purple-500", - "bg-pink-500", - "bg-indigo-500", - ]; - return colors[Math.floor(Math.random() * colors.length)]; -}; +function getRoleColor(roleId: string) { + const colors = [ + "bg-blue-500", + "bg-green-500", + "bg-yellow-500", + "bg-red-500", + "bg-purple-500", + "bg-pink-500", + "bg-indigo-500", + ]; + let hash = 5381; + for (let i = 0; i < roleId.length; i++) { + hash = ((hash << 5) - hash) ^ roleId.charCodeAt(i); + } + return colors[(hash >>> 0) % colors.length]; +}Then update usages:
-className={`${qr.role.color || getRandomColor()} text-white ...`} +className={`${qr.role.color || getRoleColor(qr.role.id)} text-white ...`}pages/api/workspace/[id]/activity/users.ts-189-212 (1)
189-212: Clamp idle-time math to prevent negative “active time” + avoid O(n²) aggregation.
IfidleTimeMs > sessionDuration,effectiveTimegoes negative (Line 201) and can invert rankings. AlsocombinedMinutes.find(...)per session is O(n²) as sessions grow.- const combinedMinutes: CombinedObj[] = []; - sessions.forEach((session) => { + const combinedMinutesByUserId = new Map<number, number[]>(); + sessions.forEach((session) => { if (!session.endTime) return; - const found = combinedMinutes.find( - (x) => x.userId == Number(session.userId) - ); + const userId = Number(session.userId); + const bucket = combinedMinutesByUserId.get(userId) ?? []; const sessionDuration = session.endTime.getTime() - session.startTime.getTime(); const idleTimeMs = idleTimeEnabled && session.idleTime ? Number(session.idleTime) * 60000 : 0; - const effectiveTime = sessionDuration - idleTimeMs; - - if (found) { - found.ms.push(effectiveTime); - } else { - combinedMinutes.push({ - userId: Number(session.userId), - ms: [effectiveTime], - }); - } + const effectiveTime = Math.max(0, sessionDuration - Math.max(0, idleTimeMs)); + bucket.push(effectiveTime); + combinedMinutesByUserId.set(userId, bucket); }); + + const combinedMinutes: CombinedObj[] = Array.from( + combinedMinutesByUserId, + ([userId, ms]) => ({ userId, ms }) + );pages/api/workspace/[id]/activity/users.ts-161-188 (1)
161-188: Dedup/cross-filter is currently broken (object identity); dedupe byuserId.
indexOf(v)andx == ycompare object references, so duplicates won’t be removed and inactive users won’t be removed when also active (Line 181-187).- activeUsers = activeUsers.filter((v, i, a) => a.indexOf(v) == i); - inactiveUsers = inactiveUsers.filter((v, i, a) => a.indexOf(v) == i); - - inactiveUsers = inactiveUsers.filter((x) => { - if (activeUsers.find((y) => x == y)) return false; - return true; - }); + const activeUserIdSet = new Set<number>(); + activeUsers = activeUsers.filter((u) => { + if (activeUserIdSet.has(u.userId)) return false; + activeUserIdSet.add(u.userId); + return true; + }); + + const inactiveUserIdSet = new Set<number>(); + inactiveUsers = inactiveUsers.filter((u) => { + if (activeUserIdSet.has(u.userId)) return false; + if (inactiveUserIdSet.has(u.userId)) return false; + inactiveUserIdSet.add(u.userId); + return true; + });pages/api/workspace/[id]/activity/users.ts-59-65 (1)
59-65: Fix property name mismatch and harden config typing (leaderboardRolevslRole).The code reads
leaderboardRolefrom config, butsetLRole.tsstores it aslRole. This meansleaderboardRankwill always beundefined, preventing the rank filter from ever working. Additionally, since theconfig.valuecolumn is untyped JSON, values could be stored as strings (e.g.,"0"or"false"), and the truthy checks on lines 120, 243, and 264 would incorrectly filter out rank 0 or treat string-valued booleans as true.Fix by either:
- Aligning property names (
lRole→leaderboardRoleeverywhere), or- Reading the correct property (
activityConfig?.lRoleinstead ofleaderboardRole)Then apply defensive coercion:
- const leaderboardRank = activityConfig?.leaderboardRole; + const leaderboardRankRaw = activityConfig?.leaderboardRole; + const leaderboardRank = + leaderboardRankRaw == null ? null : Number(leaderboardRankRaw); - const idleTimeEnabled = activityConfig?.idleTimeEnabled ?? true; + const idleTimeEnabledRaw = activityConfig?.idleTimeEnabled; + const idleTimeEnabled = + typeof idleTimeEnabledRaw === "boolean" + ? idleTimeEnabledRaw + : typeof idleTimeEnabledRaw === "string" + ? idleTimeEnabledRaw.toLowerCase() === "true" + : true;Committable suggestion skipped: line range outside the PR's diff.
🟡 Minor comments (15)
components/sticky-note-announcement.tsx-61-61 (1)
61-61: Fix grammatical error in birthday webhook description.The verb tense is incorrect. Since this describes a current capability, it should use present tense.
Apply this diff:
- We have added the ability for you to add a webhook which announced + We have added the ability for you to add a webhook which announces the birthdays of people on your workspace.utils/randomText.ts-18-18 (1)
18-18: Inconsistent spacing around hyphens used as em dash replacements.The replacement of em dashes with hyphens shows inconsistent spacing:
- Lines 18, 21, 22, 24, 54, 60, 64, 66, 75, 77: No spaces around hyphens (e.g., "time-deck", "tasks-let's")
- Lines 31, 44: Spaces around hyphens (e.g., "through - keep")
When using hyphens to replace em dashes (for separating clauses), standard practice is to include spaces on both sides. Without spaces, they create compound words that may be confusing.
Consider standardizing the punctuation:
Option 1 (Preferred): Use consistent spacing with hyphens:
- `It's go time-deck those halls, ${name} 🎄`, + `It's go time - deck those halls, ${name} 🎄`, - `New festive tasks-let's sleigh them, ${name} 🛷`, + `New festive tasks - let's sleigh them, ${name} 🛷`, - `Winter sun & Christmas lights-glow on, ${name} 🎄`, + `Winter sun & Christmas lights - glow on, ${name} 🎄`, - `Don't just wake-make spirits bright, ${name} 🔔` + `Don't just wake - make spirits bright, ${name} 🔔`Option 2: Use proper em dashes (—) if the platform supports Unicode:
- `It's go time-deck those halls, ${name} 🎄`, + `It's go time—deck those halls, ${name} 🎄`,Apply similar changes to the other affected lines (54, 60, 64, 66, 75, 77) to maintain consistency throughout the file.
Also applies to: 21-22, 24-24, 31-31, 41-41, 44-44, 54-54, 60-60, 64-64, 66-66, 75-75, 77-77
components/sessionpatterns.tsx-9-9 (1)
9-9: Missing null check forsession.datecould cause runtime error.If
session.dateis undefined or null,new Date(session.date)will create an "Invalid Date" object, causinggetDay()to returnNaNand subsequent operations to fail silently.Add a defensive check:
+if (!session?.date) { + return null; +} + const dayNames = ["Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"]; const sessionDate = new Date(session.date); +if (isNaN(sessionDate.getTime())) { + return null; +}Also applies to: 26-26
pages/workspace/[id]/sessions/edit/[sid].tsx-378-385 (1)
378-385: Delete without confirmation when scope is pre-set could be unexpected.When coming from the pattern dialog with a scope already set, clicking Delete immediately executes the deletion without showing the confirmation modal. This could lead to accidental deletions.
Consider always showing a confirmation for destructive actions:
onPress={() => { - if (scope) { - deleteSession(); - } else { - setShowDeleteModal(true); - } + setShowDeleteModal(true); }}Alternatively, show a simpler confirmation that mentions the pre-selected scope.
pages/workspace/[id]/sessions/edit/[sid].tsx-208-208 (1)
208-208: Validatescopequery parameter.The
scopeparameter fromrouter.queryis used directly without validation. A malicious or malformed value could cause unexpected behavior.-const { scope } = router.query; +const rawScope = router.query.scope; +const scope = typeof rawScope === 'string' && ['single', 'future', 'all'].includes(rawScope) + ? rawScope as 'single' | 'future' | 'all' + : undefined;components/profile/activityoverview.tsx-86-87 (1)
86-87:noticesprop is declared but never used.The
noticesprop is defined in thePropstype but is not destructured or used in the component.Either use the
noticesprop or remove it from the type definition:type Props = { data: any; displayMinutes: number; messages: number; idleTime: number; sessionsHosted: number; sessionsAttended: number; idleTimeEnabled: boolean; - notices: inactivityNotice[]; adjustments: any[]; sessions: (ActivitySession & {components/profile/activityoverview.tsx-186-186 (1)
186-186: Non-2xx status codes won't reach the conditional check.Axios throws an error for non-2xx responses by default, so
status !== 200will never be true here. The error will be caught in the catch block instead.Remove the redundant check or use
validateStatusin axios config:-if (status !== 200) return toast.error("Could not fetch session.");components/settings/permissions/roles.tsx-72-78 (1)
72-78: Bug:findIndexreturns-1when not found, notnull.The guard
if (index === null) returnwill never trigger becausefindIndexreturns-1when the element is not found. This mirrors a pre-existing bug inupdateRole,togglePermission, andtoggleGroupRole. Note thatsaveRolecorrectly checksindex === -1.const updateRoleColor = async (color: string, id: string) => { const index = roles.findIndex((role: any) => role.id === id); - if (index === null) return; + if (index === -1) return; const rroles = Object.assign(([] as typeof roles), roles); rroles[index].color = color; setRoles(rroles); };pages/api/cron/birthdays.ts-68-119 (1)
68-119: Fix 1-second delay to avoid sleeping after the last sendCurrent logic sleeps even after the final birthday in the workspace. Use an index.
- for (const member of membersWithBirthdays) { + for (let i = 0; i < membersWithBirthdays.length; i++) { + const member = membersWithBirthdays[i]; ... - if (membersWithBirthdays.length > 1) { + if (i < membersWithBirthdays.length - 1) { await new Promise((resolve) => setTimeout(resolve, 1000)); } }components/settings/general/leaderboard.tsx-39-65 (1)
39-65: Missing error handling for API call.The
updateLeaderboardfunction doesn't wrap theaxios.patchcall in a try/catch block, unlikehandleStyleChangeinLeaderboardStyleSelector. If the request fails, it will throw an unhandled promise rejection.const updateLeaderboard = async (enabled?: boolean, style?: string) => { + try { const res = await axios.patch( `/api/workspace/${workspace.groupId}/settings/general/leaderboard`, { enabled: enabled !== undefined ? enabled : workspace.settings.leaderboardEnabled, style: style || leaderboardStyle, } ); if (res.status === 200) { // ... existing success handling } else { triggerToast.error("Failed to update leaderboard."); } + } catch (error) { + triggerToast.error("Failed to update leaderboard."); + } };pages/workspace/[id]/activity/index.tsx-125-156 (1)
125-156: Switch case declarations without block scope.Variable declarations (
const hostedCount,let loggedCount) inside switch cases without block scoping can be accessed by fall-through cases, leading to potential bugs.Wrap each case body in a block:
case "sessions_hosted": + { const hostedCount = quota.sessionType && quota.sessionType !== "all" ? profileData.sessionsLogged?.byType[quota.sessionType] || 0 : sessionsHosted; currentValue = hostedCount; percentage = (hostedCount / quota.value) * 100; - break; + break; + } case "sessions_logged": + { let loggedCount = 0; // ... rest of logic currentValue = loggedCount; percentage = (loggedCount / quota.value) * 100; - break; + break; + }pages/workspace/[id]/views.tsx-279-293 (1)
279-293: Potential issue with session type access.The code accesses
(p.session as any).typeon line 284, but thesessionincluded fromallSessionParticipationsonly selectsidandsessionType(lines 233-240). Thetypefield may not be available on this object.Verify that
p.session.typeexists. Based on the include query, it appears onlysessionType(the relation) is selected, not thetypefield. You may need to update the include:include: { session: { select: { id: true, + type: true, sessionType: { select: { slots: true, name: true, }, }, }, }, },Committable suggestion skipped: line range outside the PR's diff.
pages/workspace/[id]/quotas.tsx-398-398 (1)
398-398: Missing dependency inuseMemohook.The
useMemohas an empty dependency array but useslogin.displayname. This means the value won't update if the display name changes.- const text = useMemo(() => randomText(login.displayname), []); + const text = useMemo(() => randomText(login.displayname), [login.displayname]);pages/workspace/[id]/quotas.tsx-285-316 (1)
285-316: Wrap switch case declarations in blocks to prevent accidental access.The static analysis tool correctly identifies that
hostedCount(line 291) andloggedCount(line 306) are declared without block scoping, making them technically accessible from other switch clauses.case "sessions_hosted": - const hostedCount = quota.sessionType && quota.sessionType !== "all" + { + const hostedCount = quota.sessionType && quota.sessionType !== "all" ? sessionsLogged.filter( (s) => s.ownerId?.toString() === userId && s.type === quota.sessionType ).length : sessionsHosted; currentValue = hostedCount; percentage = (hostedCount / quota.value) * 100; break; + } case "sessions_attended": currentValue = sessionsAttended; percentage = (sessionsAttended / quota.value) * 100; break; case "sessions_logged": - const loggedCount = quota.sessionType && quota.sessionType !== "all" + { + const loggedCount = quota.sessionType && quota.sessionType !== "all" ? sessionsLogged.filter((s) => s.type === quota.sessionType).length : totalSessionsLogged; currentValue = loggedCount; percentage = (loggedCount / quota.value) * 100; break; + }Committable suggestion skipped: line range outside the PR's diff.
pages/workspace/[id]/profile/[uid].tsx-407-426 (1)
407-426: Add pagination to theallMembersquery to handle workspaces with many members.The
workspaceMember.findMany()query at lines 407-426 fetches all workspace members without pagination. For workspaces with large member counts, this causes unnecessary data transfer and renders many DOM elements in the line manager selection dropdown. Addtakeandskipparameters, similar to pagination patterns used elsewhere in the codebase (e.g.,/api/workspace/[id]/activity/history.ts).
🧹 Nitpick comments (38)
pages/api/admin/instance-config.ts (1)
47-55: Validate / coerce request body types before writing to DB.
req.bodyvalues may be non-strings; add minimal validation (e.g., ensuretypeof value === 'string', trim, enforce max length; reject invalid URL for redirect URI).Also applies to: 57-65
components/settings/general/color.tsx (1)
45-48: PreferuseCallback+ depend on the callback (avoids stale closure / exhaustive-deps noise).
Right now the effect depends onworkspace?.groupIdbut calls a recreatedloadSessionColorsthat closes overworkspace. Making the callback explicit tightens correctness and intent.-import { useEffect, useState } from "react"; +import { useCallback, useEffect, useState } from "react"; - useEffect(() => { - loadSessionColors(); - }, [workspace?.groupId]); - - const loadSessionColors = async () => { - if (!workspace?.groupId) return; + const loadSessionColors = useCallback(async () => { + if (!workspace?.groupId) return; try { setIsLoadingSessionColors(true); const response = await axios.get( `/api/workspace/${workspace.groupId}/settings/general/session-colors` ); if (response.data.success && response.data.colors) { setSessionColors(response.data.colors); } } catch (error) { console.error("Failed to load session colors:", error); } finally { setIsLoadingSessionColors(false); } - }; + }, [workspace?.groupId]); + + useEffect(() => { + loadSessionColors(); + }, [loadSessionColors]);pages/api/workspace/[id]/home/upcoming.ts (1)
34-34: Consider removing redundant filter.The database query already filters for non-null
birthdayDayandbirthdayMonthvalues (lines 22-25), making this in-memory filter redundant. Removing it would improve code clarity.Apply this diff:
const filtered = memberBirthdays - .filter((m: typeof memberBirthdays[number]) => (m.user.birthdayDay ?? 0) > 0 && (m.user.birthdayMonth ?? 0) > 0) .map((m: typeof memberBirthdays[number]) => {pages/workspace/[id]/wall.tsx (1)
413-429: The permission logic is correct.The delete authorization correctly allows both post authors and users with
manage_wallpermission to delete posts. TheString()conversion provides good type safety.However, the IIFE pattern adds unnecessary complexity to the JSX. Consider this refactor for better readability:
- {(() => { - const isAuthor = String(post.authorId) === String(login.userId); - const hasManageWall = userPermissions.includes("manage_wall"); - const canDelete = isAuthor || hasManageWall; - - return canDelete ? ( - <button - onClick={() => { - setPostToDelete(post.id); - setShowDeleteModal(true); - }} - className="p-2 text-red-500 hover:bg-red-50 dark:hover:bg-red-900/20 rounded-lg transition-colors" - > - <IconTrash size={18} /> - </button> - ) : null; - })()} + {(String(post.authorId) === String(login.userId) || + userPermissions.includes("manage_wall")) && ( + <button + onClick={() => { + setPostToDelete(post.id); + setShowDeleteModal(true); + }} + className="p-2 text-red-500 hover:bg-red-50 dark:hover:bg-red-900/20 rounded-lg transition-colors" + > + <IconTrash size={18} /> + </button> + )}Or extract to a helper function above the return statement:
const canDeletePost = (post: any) => { const isAuthor = String(post.authorId) === String(login.userId); const hasManageWall = userPermissions.includes("manage_wall"); return isAuthor || hasManageWall; };pages/api/workspace/[id]/settings/roles/[roleid]/update.ts (1)
35-36: Prefer nullish coalescing operator (??) over logical OR (||).Using
||treats all falsy values (including empty string""and0) as triggers for the default. Use??to only default onnullorundefined.- groupRoles: req.body.groupRoles || [], - color: req.body.color || null + groupRoles: req.body.groupRoles ?? [], + color: req.body.color ?? nullpages/api/workspace/[id]/settings/general/leaderboard.ts (1)
29-29: Prefer nullish coalescing operator (??) over logical OR (||).Using
||treats all falsy values (including empty string) as triggers for the default 'list'. Use??to only default onnullorundefined.- const after = { enabled: req.body.enabled, style: req.body.style || 'list' }; + const after = { enabled: req.body.enabled, style: req.body.style ?? 'list' };pages/api/workspace/[id]/activity/quotas/new.ts (1)
34-48: Consider adding type safety and validation forsessionType.Using
anytype forquotaDatabypasses TypeScript's type checking. Additionally,sessionTypeis conditionally included without validation.- const quotaData: any = { + const quotaData = { name, type, value: parseInt(value), workspaceGroupId: parseInt(req.query.id as string), description: description || null, + ...(sessionType && { sessionType: String(sessionType) }), }; - - if (sessionType) { - quotaData.sessionType = sessionType; - } const quota = await prisma.quota.create({ data: quotaData });If
sessionTypeshould only accept specific values, consider validating it against allowed session types before saving.pages/api/workspace/[id]/sessions/create-scheduled.ts (1)
99-106: Consider wrapping schedule and session creation in a transaction.If an error occurs after
patternScheduleis created but before sessions are created (e.g., at line 167), an orphan schedule record will remain in the database. Using a Prisma transaction ensures atomicity.+ const result = await prisma.$transaction(async (tx) => { const patternSchedule = await prisma.schedule.create({ data: { Days: selectedDays, Hour: hours, Minute: minutes, sessionTypeId: sessionType.id, }, }); + // ... rest of session creation logic using tx instead of prisma + return { patternSchedule, createdSessions }; + });This prevents orphan schedule records if subsequent operations fail.
components/profile/activitysubsection.tsx (1)
1-1: Remove unusedFragmentimport.
Fragmentis imported but not used in this component.-import React, { Fragment } from "react"; +import React from "react";components/settings/general/bhooks.tsx (3)
7-7: Remove unused importIconX.-import { IconGift, IconCheck, IconX } from "@tabler/icons-react"; +import { IconGift, IconCheck } from "@tabler/icons-react";
10-10: Remove unusedsetWorkspacefrom destructuring.- const [workspace, setWorkspace] = useRecoilState(workspacestate); + const [workspace] = useRecoilState(workspacestate);
54-78: Add URL validation before sending test webhook.While the server should validate the URL, adding client-side validation improves UX by catching invalid URLs early.
const handleTest = async () => { if (!webhookUrl) { toast.error("Please enter a webhook URL first"); return; } + if (!webhookUrl.startsWith("https://discord.com/api/webhooks/")) { + toast.error("Please enter a valid Discord webhook URL"); + return; + } + setTesting(true);pages/api/workspace/[id]/sessions/[sid]/update-pattern.ts (1)
94-106: Consider using a transaction for bulk updates.When updating multiple sessions with time changes, each session is updated individually in a loop. For large patterns, this could be slow and leaves the database in an inconsistent state if an error occurs mid-update.
+ await prisma.$transaction(async (tx) => { for (const session of filteredSessions) { const sessionDate = new Date(session.date); sessionDate.setHours(hours, minutes, 0, 0); const updateData: any = { date: sessionDate }; if (newDuration !== undefined) updateData.duration = newDuration; if (newName !== undefined) updateData.name = newName; - await prisma.session.update({ + await tx.session.update({ where: { id: session.id }, data: updateData, }); } + });The same pattern applies to lines 151-163 in the "all" scope.
components/profile/activityoverview.tsx (1)
76-76: Excessive use ofanytype reduces type safety.Multiple instances of
anytype are used where more specific types would improve maintainability and catch errors at compile time.Consider defining proper interfaces for:
TimelineItemadjustment type (Line 76)Props.data(Line 79)Props.adjustments(Line 87)dialogDatastate (Line 125)Also applies to: 79-79, 87-87, 125-125
components/sessionpatterns.tsx (1)
5-10: Consider typingsessionprop more strictly.Using
anyfor thesessionprop reduces type safety. At minimum, the expected shape should be documented.+interface SessionData { + date: string | Date; + name: string; + scheduleId?: string | null; +} + interface PatternEditDialogProps { isOpen: boolean; onClose: () => void; onConfirm: (scope: "single" | "future" | "all") => void; - session: any; + session: SessionData; }pages/workspace/[id]/sessions.tsx (1)
33-60: DuplicatedBG_COLORSarray andgetRandomBgfunction.This code is duplicated in
activityoverview.tsxandedit/[sid].tsx. Consider extracting to a shared utility.Create a shared utility file:
// utils/avatarColors.ts export const BG_COLORS = [ "bg-red-200", "bg-green-200", // ... rest of colors ]; export function getRandomBg(userid: string, username?: string): string { const key = `${userid ?? ""}:${username ?? ""}`; let hash = 5381; for (let i = 0; i < key.length; i++) { hash = ((hash << 5) - hash) ^ key.charCodeAt(i); } const index = (hash >>> 0) % BG_COLORS.length; return BG_COLORS[index]; }pages/api/createws.ts (1)
92-110: Consider making the creator-member insert idempotent (retry/double-submit safe).
If the POST is retried after partial failure,tx.workspaceMember.createcould throw on a unique constraint (if one exists).pages/workspace/[id]/index.tsx (1)
148-159: Consider preventing redundant timezone POST requests.This
useEffectwill fire on every render/remount when bothworkspace.groupIdandlogin.userIdare truthy. Without deduplication, multiple requests may be sent unnecessarily. Consider using a ref to track if the timezone was already submitted this session.+const timezoneSubmittedRef = useRef(false); + useEffect(() => { - if (workspace?.groupId && login?.userId) { + if (workspace?.groupId && login?.userId && !timezoneSubmittedRef.current) { + timezoneSubmittedRef.current = true; const detectedTimezone = Intl.DateTimeFormat().resolvedOptions().timeZone; fetch(`/api/workspace/${workspace.groupId}/timezone`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ timezone: detectedTimezone }), }).catch(() => { // no errors its gonan work amazing trust - famous last words }); } }, [workspace?.groupId, login?.userId])pages/api/workspace/[id]/profile/[uid]/member-info.ts (1)
39-47: Redundant conditional and missing input validation.The ternary
birthdayDay !== undefined ? birthdayDay : undefinedis a no-op. Additionally,birthdayDayandbirthdayMonthshould be validated as integers within valid ranges (day: 1-31, month: 1-12) to prevent invalid data.await prisma.user.update({ where: { userid: BigInt(userId), }, data: { - birthdayDay: birthdayDay !== undefined ? birthdayDay : undefined, - birthdayMonth: birthdayMonth !== undefined ? birthdayMonth : undefined, + ...(birthdayDay !== undefined && { birthdayDay: Number(birthdayDay) }), + ...(birthdayMonth !== undefined && { birthdayMonth: Number(birthdayMonth) }), }, });pages/api/cron/birthdays.ts (1)
13-16: Harden cron auth header parsing + avoid timing leaks
req.headers["x-cron-secret"]can bestring | string[] | undefined, so the current compare can fail unexpectedly. Also consider constant-time compare for a shared secret.+import crypto from "crypto"; ... - const cronSecret = req.headers["x-cron-secret"]; - if (cronSecret !== process.env.CRON_SECRET) { + const cronSecretHeader = req.headers["x-cron-secret"]; + const cronSecret = + typeof cronSecretHeader === "string" + ? cronSecretHeader + : Array.isArray(cronSecretHeader) + ? cronSecretHeader[0] + : undefined; + + const expected = process.env.CRON_SECRET; + if (!cronSecret || !expected) { return res.status(401).json({ error: "Unauthorized" }); } + + const a = Buffer.from(cronSecret); + const b = Buffer.from(expected); + if (a.length !== b.length || !crypto.timingSafeEqual(a, b)) { + return res.status(401).json({ error: "Unauthorized" }); + }pages/api/workspace/[id]/profile/[uid].ts (1)
235-260: Usecount()instead offindMany()when only returningallianceVisitsCountThis avoids loading rows + joins just to do
.length.- const allianceVisits = await prisma.allyVisit.findMany({ - where: { - ally: { - workspaceGroupId: workspaceGroupId, - }, - time: { - gte: startDate, - lte: currentDate, - }, - OR: [ - { hostId: userId }, - { participants: { has: userId } } - ] - }, - include: { - ally: { - select: { - id: true, - name: true, - } - } - } - }); - - const allianceVisitsCount = allianceVisits.length; + const allianceVisitsCount = await prisma.allyVisit.count({ + where: { + ally: { workspaceGroupId }, + time: { gte: startDate, lte: currentDate }, + OR: [{ hostId: userId }, { participants: { has: userId } }], + }, + });components/profile/sessions.tsx (1)
8-30: Consider responsive layout + avoid unused destructured props if lintedIf you run no-unused-vars/eslint, destructuring
sessions,notices,adjustments,avatar,idleTimeEnabledwithout using them may warn. Also,grid-cols-2could be cramped on mobile.- <div className="grid grid-cols-2 gap-4 mb-6"> + <div className="grid grid-cols-1 sm:grid-cols-2 gap-4 mb-6">Also applies to: 34-78
pages/api/workspace/[id]/timezone.ts (1)
20-22: Validate timezone as an IANA time zone (not just a string)A simple server-side check:
if (!timezone || typeof timezone !== "string") { return res.status(400).json({ error: "Invalid timezone" }); } + try { + Intl.DateTimeFormat("en-US", { timeZone: timezone }).format(new Date()); + } catch { + return res.status(400).json({ error: "Invalid timezone" }); + }pages/api/workspace/[id]/settings/general/birthdays/test.ts (1)
47-54: Prefer URL parsing + host allowlist; also use HTTPS for avatar_urlRegex-only validation is easier to bypass than parsing+allowlist, and
avatar_urlshould be https.- if (!url.match(/^https:\/\/discord\.com\/api\/webhooks\/\d+\/.+/)) { + let parsed: URL; + try { parsed = new URL(url); } catch { parsed = null as any; } + const isAllowed = + parsed && + parsed.protocol === "https:" && + (parsed.hostname === "discord.com" || parsed.hostname === "canary.discord.com") && + /^\/api\/webhooks\/\d+\/.+/.test(parsed.pathname); + if (!isAllowed) { return res.status(400).json({ success: false, error: "Invalid Discord webhook URL format", }); } ... - avatar_url: `http://cdn.planetaryapp.us/brand/planetary.png`, + avatar_url: `https://cdn.planetaryapp.us/brand/planetary.png`,Also applies to: 82-94
pages/api/workspace/[id]/settings/general/birthdays/hook.ts (2)
25-27: Permission check relies on array ordering assumption.The code assumes the first role after ordering by
isOwnerRole: "desc"is the most privileged. This works correctly, but if a user has multiple roles with different permissions, only the first role's permissions are checked.Consider checking if any role has admin permission for more comprehensive authorization:
- const userRole = user?.roles?.[0]; - const hasAdminPermission = - userRole?.permissions?.includes("admin") || userRole?.isOwnerRole; + const hasAdminPermission = user?.roles?.some( + (role) => role.permissions?.includes("admin") || role.isOwnerRole + );
58-63: Webhook URL regex may not cover all valid Discord webhook formats.The current regex
/^https:\/\/discord\.com\/api\/webhooks\/\d+\/.+/is reasonable but may miss webhooks usingdiscordapp.comorptb.discord.com/canary.discord.comsubdomains.Consider a more comprehensive pattern:
- if (enabled && url && !url.match(/^https:\/\/discord\.com\/api\/webhooks\/\d+\/.+/)) { + if (enabled && url && !url.match(/^https:\/\/(discord\.com|discordapp\.com|ptb\.discord\.com|canary\.discord\.com)\/api\/webhooks\/\d+\/.+/)) {components/settings/general/leaderboard.tsx (1)
21-37: Duplicate fetch logic between components.Both
LeaderboardandLeaderboardStyleSelectorhave nearly identicaluseEffecthooks for fetching leaderboard style. This violates DRY principles and can lead to inconsistent behavior if modified independently.Consider extracting the shared logic into a custom hook:
function useLeaderboardStyle(workspaceId: number | undefined) { const [style, setStyle] = useState<"list" | "podium">("list"); useEffect(() => { if (!workspaceId) return; axios.get(`/api/workspace/${workspaceId}/settings/general/leaderboard`) .then((res) => { if (res.data?.value?.style) setStyle(res.data.value.style); }) .catch((error) => console.error("Failed to fetch leaderboard style:", error)); }, [workspaceId]); return [style, setStyle] as const; }Also applies to: 107-123
pages/api/workspace/[id]/settings/general/session-colors.ts (2)
69-70: Permission check pattern identical to birthdays/hook.ts.Same observation as in
hook.ts- only the first role's permissions are checked. Consider checking if any role has admin permission.- const userRole = user?.roles?.[0]; - const hasAdminPermission = userRole?.permissions?.includes('admin') || userRole?.isOwnerRole; + const hasAdminPermission = user?.roles?.some( + (role) => role.permissions?.includes('admin') || role.isOwnerRole + );
99-103: Audit logging errors are silently swallowed.The empty catch block suppresses any audit logging failures without logging them. While audit failures shouldn't break the request, silent failures make debugging difficult.
try { const actorId = (req as any).session?.userid ? Number((req as any).session.userid) : null; const actorUsername = actorId ? await getUsername(actorId).catch(() => null) : null; await logAudit(workspaceId, actorId, 'settings.general.sessionColors.update', 'sessionColors', { before, after: colors, actorUsername }); - } catch (e) {} + } catch (e) { + console.error("Failed to log audit:", e); + }pages/workspace/[id]/activity/index.tsx (1)
222-253: Dual polling intervals may increase server load.User data polls every 30 seconds (line 215-218) while leaderboard data polls every 60 seconds (line 250). With many users, this could create significant load.
Consider:
- Aligning the intervals or combining the fetches
- Using a visibility-based approach (pause polling when tab is not visible)
- Using WebSockets or server-sent events for real-time updates
components/profile/information.tsx (1)
311-322: Consider validating day based on selected month.The day selector always shows 31 days regardless of the selected month. While server-side validation can catch invalid dates like February 30th, consider adjusting the available days based on the selected month for better UX.
+ const daysInMonth = birthdayMonth + ? new Date(2000, parseInt(birthdayMonth as string), 0).getDate() + : 31; <select value={birthdayDay} onChange={(e) => setBirthdayDay(e.target.value)} className="flex-1 px-2 py-1 text-sm rounded-lg bg-white dark:bg-zinc-900 border border-zinc-300 dark:border-zinc-600 text-zinc-900 dark:text-white focus:outline-none focus:ring-2 focus:ring-[#ff0099]/50" > <option value="">Day</option> - {Array.from({ length: 31 }, (_, i) => i + 1).map((day) => ( + {Array.from({ length: daysInMonth }, (_, i) => i + 1).map((day) => ( <option key={day} value={day}> {day} </option> ))} </select>pages/workspace/[id]/views.tsx (1)
166-409: Consider extracting duplicated user computation logic.The logic for computing user activity data (minutes, idle time, sessions, alliance visits, etc.) is largely duplicated between the main user loop (lines 166-409) and the fallback loop for users not in
computedUsers(lines 417-586). This increases maintenance burden and risk of inconsistencies.Extract the shared computation logic into a helper function to reduce duplication and ensure consistent behavior:
async function computeUserActivity( userId: bigint, allActivity: any[], workspaceGroupId: number, startDate: Date, currentDate: Date, idleTimeEnabled: boolean ) { // ... shared computation logic return { minutes, idleMinutes, sessionsHosted, sessionsAttended, allianceVisits, ... }; }Also applies to: 417-586
components/profile/quotas.tsx (2)
21-43:getQuotaPercentagecan returnundefinedfor unknown quota types.The switch statement lacks a
defaultcase, so ifquota.typedoesn't match any case andquota.percentageis undefined, the function returnsundefined. This could cause the progress bar to display incorrectly (though the|| 0fallback on line 127 mitigates runtime issues).Additionally,
Quota | anydefeats type safety—consider using a discriminated union or the augmented type from Props.- const getQuotaPercentage = (quota: Quota | any) => { + const getQuotaPercentage = (quota: Quota & { currentValue?: number; percentage?: number }) => { if (quota.percentage !== undefined) { return quota.percentage; } switch (quota.type) { case "mins": { return (displayMinutes / quota.value) * 100; } case "sessions_hosted": { return (sessionsHosted / quota.value) * 100; } case "sessions_attended": { return (sessionsAttended / quota.value) * 100; } case "sessions_logged": { const totalLogged = sessionsHosted + sessionsAttended; return (totalLogged / quota.value) * 100; } case "alliance_visits": { return (allianceVisits / quota.value) * 100; } + default: + return 0; } };
45-73:getQuotaProgresscan also returnundefinedfor unknown quota types.Similar to
getQuotaPercentage, this function lacks adefaultcase. If the tooltip receivesundefined, it may display poorly.case "alliance_visits": { return `${allianceVisits} / ${quota.value} alliance visits`; } + default: + return `${quota.currentValue ?? 0} / ${quota.value}`; } };pages/workspace/[id]/quotas.tsx (2)
140-144: Hardcoded fallback date should be documented or made configurable.The hardcoded date
2024-11-30T00:00:00Zappears in multiple places (here and in the profile page). Consider extracting this to a shared constant with documentation explaining its purpose.- // Use last reset date, or November 30th 2024, whichever is more recent - const nov30 = new Date("2024-11-30T00:00:00Z"); + // Fallback date for activity tracking start - represents the launch date of the quota system + const QUOTA_SYSTEM_LAUNCH_DATE = new Date("2024-11-30T00:00:00Z"); + const startDate = lastReset?.resetAt + ? (lastReset.resetAt > QUOTA_SYSTEM_LAUNCH_DATE ? lastReset.resetAt : QUOTA_SYSTEM_LAUNCH_DATE) + : QUOTA_SYSTEM_LAUNCH_DATE;
482-493: Consider adding a confirmation dialog before quota deletion.The delete action immediately calls the API without user confirmation. For destructive operations, a confirmation dialog prevents accidental data loss.
pages/workspace/[id]/profile/[uid].tsx (1)
795-849: Consider extracting the timezone clock to a separate component.The IIFE pattern
(() => { ... })()in JSX works but reduces readability. Extracting this to a dedicated component would improve maintainability and allow for memoization.Also,
parseInt(userHour)on line 801 could returnNaNif the locale format differs unexpectedly. Consider adding a fallback.+// Extract as a separate component or helper +function TimezoneClock({ timezone }: { timezone: string }) { + const [currentTime, setCurrentTime] = useState(new Date()); + + useEffect(() => { + const timer = setInterval(() => setCurrentTime(new Date()), 1000); + return () => clearInterval(timer); + }, []); + + const hour = parseInt( + currentTime.toLocaleString('en-US', { timeZone: timezone, hour: 'numeric', hour12: false }), + 10 + ); + const isDay = !isNaN(hour) && hour >= 6 && hour < 18; + // ... rest of render +}pages/api/workspace/[id]/activity/users.ts (1)
66-104: Use a single “now” Date and reuseworkspaceIdto avoid tiny TOCTOU inconsistencies.
You callnew Date()multiple times inside the inactivity query (Line 88-92) and re-parsereq.query.id(Line 93, 135). This can produce small mismatches and is harder to reason about.- const currentDate = new Date(); + const currentDate = new Date(); + const nowDate = currentDate; ... - endTime: { - gt: new Date(), - }, - startTime: { - lt: new Date(), - }, - workspaceGroupId: parseInt(req.query.id as string), + endTime: { gt: nowDate }, + startTime: { lt: nowDate }, + workspaceGroupId: workspaceId,
| const handleSave = async () => { | ||
| if (!workspaceId) return; | ||
| setLoading(true); | ||
| try { | ||
| await axios.patch( | ||
| `/api/workspace/${workspaceId}/profile/${user.userid}/member-info`, | ||
| { | ||
| department, | ||
| lineManagerId: selectedManager?.userid || null, | ||
| timezone: selectedTimezone || null, | ||
| birthdayDay: birthdayDay ? parseInt(birthdayDay as string) : null, | ||
| birthdayMonth: birthdayMonth ? parseInt(birthdayMonth as string) : null, | ||
| discordId: discordId || null, | ||
| } | ||
| ); | ||
|
|
||
| toast.success("Information updated!"); | ||
| setEditing(false); | ||
| router.replace(router.asPath); | ||
| } catch (e) { | ||
| toast.error("Failed to update information"); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: PlanetaryOrbit/orbit
Length of output: 184
🏁 Script executed:
# Search for the member-info API endpoint
rg -n "member-info" --type ts --type tsxRepository: PlanetaryOrbit/orbit
Length of output: 91
🏁 Script executed:
# Search for workspace API routes to find authorization patterns
fd -t f -e ts -e tsx "api" | grep -E "(workspace|profile)" | head -20Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Look for the specific API route
rg -n "/api/workspace.*profile.*member-info" --type ts --type tsxRepository: PlanetaryOrbit/orbit
Length of output: 91
🏁 Script executed:
find . -name "*.ts" -o -name "*.js" | grep -i "api" | head -30Repository: PlanetaryOrbit/orbit
Length of output: 1081
🏁 Script executed:
# Search for member-info in any file type
grep -r "member-info" . --include="*.ts" --include="*.js" 2>/dev/null | head -20Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Look for workspace API routes directory structure
find . -type d -name "api" 2>/dev/null | head -10Repository: PlanetaryOrbit/orbit
Length of output: 101
🏁 Script executed:
# Search for handleSave and related authorization patterns
grep -r "workspace.*member-info\|member-info.*authorization\|validateUser\|checkPermission" . --include="*.ts" --include="*.js" 2>/dev/null | head -20Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Find workspace API routes
find ./pages/api -name "*workspace*" -type f 2>/dev/null | head -20Repository: PlanetaryOrbit/orbit
Length of output: 93
🏁 Script executed:
# Search for profile endpoint
find ./pages/api -name "*profile*" -type f 2>/dev/null | head -20Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Look for files in workspace directory structure
find ./pages/api -type d 2>/dev/null | grep workspaceRepository: PlanetaryOrbit/orbit
Length of output: 2215
🏁 Script executed:
# List all files in pages/api directory to understand structure
ls -la ./pages/api/ 2>/dev/null | head -40Repository: PlanetaryOrbit/orbit
Length of output: 1255
🏁 Script executed:
# List files in the profile directory
find ./pages/api/workspace/[id]/profile -type f 2>/dev/nullRepository: PlanetaryOrbit/orbit
Length of output: 163
🏁 Script executed:
# List files in the profile/[uid] directory
find "./pages/api/workspace" -path "*/profile/\[uid\]*" -type f 2>/dev/nullRepository: PlanetaryOrbit/orbit
Length of output: 163
🏁 Script executed:
# Try to list all files under workspace/[id]/profile
ls -la ./pages/api/workspace/[id]/profile/ 2>/dev/nullRepository: PlanetaryOrbit/orbit
Length of output: 292
🏁 Script executed:
# Search for member-info more broadly
rg "member-info" . 2>/dev/null | head -20Repository: PlanetaryOrbit/orbit
Length of output: 176
🏁 Script executed:
# Read the member-info API endpoint to check authorization
cat "./pages/api/workspace/[id]/profile/[uid]/member-info.ts"Repository: PlanetaryOrbit/orbit
Length of output: 1936
🏁 Script executed:
# Also check the parent profile endpoint for any shared authorization logic
cat "./pages/api/workspace/[id]/profile/[uid].ts" | head -100Repository: PlanetaryOrbit/orbit
Length of output: 2638
🏁 Script executed:
# Check what withSessionRoute does - is there any authorization built in?
cat ./lib/withSession.ts 2>/dev/null | head -50Repository: PlanetaryOrbit/orbit
Length of output: 1784
🏁 Script executed:
# Also check if there's any middleware that might protect this endpoint
rg -A 5 "withSessionRoute.*member-info" . 2>/dev/nullRepository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Look for any authorization checks in the profile directory
rg "withPermissionCheck\|checkAuthorization\|isAuthorized" "./pages/api/workspace/[id]/profile/" 2>/dev/nullRepository: PlanetaryOrbit/orbit
Length of output: 46
Add authorization validation to the member-info endpoint.
The PATCH endpoint at /pages/api/workspace/[id]/profile/[uid]/member-info.ts is missing critical authorization checks. Currently it only uses withSessionRoute (which verifies the user is logged in) but does not verify:
- That the requesting user is editing their own profile or has permission to edit others
- That the user belongs to the workspace
- That the workspace and user IDs form a valid relationship
Any authenticated user can modify any user's profile in any workspace. The parent endpoint [uid].ts demonstrates the correct authorization pattern with isOwnProfile checks and role-based permissions—apply the same approach to the member-info endpoint.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 163-173: Server-side request forgery
The URL of this request depends on a user-provided value.
🤖 Prompt for AI Agents
In components/profile/information.tsx around lines 159 to 183, the client calls
the PATCH member-info endpoint without ensuring the server enforces
authorization; update the server handler at
pages/api/workspace/[id]/profile/[uid]/member-info.ts to mirror the parent
[uid].ts checks: extract session user, verify the session user belongs to the
workspace, confirm the workspaceId and target uid form a valid relationship,
allow the update only if isOwnProfile(sessionUser, uid) or the sessionUser has
an elevated role/permission to edit other members, and return 401/403 for
unauthorized requests; ensure any missing or invalid workspace/user lookups
return appropriate not-found/forbidden responses and use the same role-check
helpers as the parent endpoint.
| const webhookConfig = await getConfig("birthday_webhook", workspace.groupId); | ||
|
|
||
| if (!webhookConfig || !webhookConfig.enabled || !webhookConfig.url) { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "birthday_webhook" -SRepository: PlanetaryOrbit/orbit
Length of output: 421
🏁 Script executed:
rg -n "getConfig\(\"birthday_webhook\"" -SRepository: PlanetaryOrbit/orbit
Length of output: 314
🏁 Script executed:
fd -n "birthdays.ts" --type fRepository: PlanetaryOrbit/orbit
Length of output: 294
🏁 Script executed:
wc -l pages/api/cron/birthdays.tsRepository: PlanetaryOrbit/orbit
Length of output: 95
🏁 Script executed:
cat -n pages/api/cron/birthdays.tsRepository: PlanetaryOrbit/orbit
Length of output: 5174
🏁 Script executed:
cat -n pages/api/workspace/[id]/settings/general/birthdays/hook.tsRepository: PlanetaryOrbit/orbit
Length of output: 3165
🏁 Script executed:
rg -n "discord" pages/api/cron/birthdays.tsRepository: PlanetaryOrbit/orbit
Length of output: 195
🏁 Script executed:
rg -n "webhook" pages/api/cron/birthdays.ts -iRepository: PlanetaryOrbit/orbit
Length of output: 450
🏁 Script executed:
rg -n "canary.discord" --type-list | head -20Repository: PlanetaryOrbit/orbit
Length of output: 453
Add request timeout to prevent webhook fetch from hanging
The fetch(webhookConfig.url, ...) call lacks a timeout mechanism, which could cause the cron job to hang indefinitely if the webhook endpoint is unresponsive. While Discord webhook URL validation already exists in the config endpoint (hook.ts:58), add an AbortController with a timeout to ensure cron executions complete reliably:
const response = await fetch(webhookConfig.url, {
+ signal: AbortSignal.timeout(10_000),
method: "POST",
headers: {
"Content-Type": "application/json",
},
body: JSON.stringify(webhookBody),
});This applies to line 98-104 where the webhook is invoked.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pages/api/cron/birthdays.ts around lines 30-35 and where the webhook is
invoked at lines ~98-104, the fetch call for webhookConfig.url has no timeout
and can hang; add an AbortController, start a timer (e.g., 5000 ms or
configurable) and pass controller.signal to fetch, clear the timer after fetch
completes, and catch AbortError to log a timeout-specific message and continue
so the cron job doesn't stall. Ensure the controller's timer is cleaned up in
both success and error paths to avoid leaks and keep behavior unchanged for
non-timeout errors.
| export default withPermissionCheck(handler, "view_entire_groups_activity"); | ||
|
|
||
| export async function handler(req: NextApiRequest, res: NextApiResponse<Data>) { | ||
| if (req.method !== "GET") | ||
| return res | ||
| .status(405) | ||
| .json({ success: false, error: "Method not allowed" }); | ||
| if (!req.session.userid) | ||
| return res.status(401).json({ success: false, error: "Not logged in" }); | ||
|
|
||
| const workspaceId = parseInt(req.query.id as string); | ||
| const cacheKey = `activity_users_${workspaceId}`; | ||
| const now = Date.now(); | ||
| const cached = activityUsersCache.get(cacheKey); | ||
| if (cached && now - cached.timestamp < ACTIVITY_CACHE_DURATION) { | ||
| return res.status(200).json({ success: true, message: cached.data }); | ||
| } |
There was a problem hiding this comment.
Fix auth regression: handler’s session check breaks withPermissionCheck service-key bypass + validate workspaceId.
withPermissionCheck(...) allows calling the handler with the x-planetary-cloud-service-key header even if there’s no session, but the handler rejects when req.session.userid is missing (Line 39-40). Also, parseInt(req.query.id as string) can yield NaN if the handler is invoked directly (tests/other callers).
export async function handler(req: NextApiRequest, res: NextApiResponse<Data>) {
if (req.method !== "GET")
return res
.status(405)
.json({ success: false, error: "Method not allowed" });
- if (!req.session.userid)
- return res.status(401).json({ success: false, error: "Not logged in" });
-
- const workspaceId = parseInt(req.query.id as string);
+ const workspaceId = Number.parseInt(req.query.id as string, 10);
+ if (Number.isNaN(workspaceId)) {
+ return res.status(400).json({ success: false, error: "Invalid workspace id" });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default withPermissionCheck(handler, "view_entire_groups_activity"); | |
| export async function handler(req: NextApiRequest, res: NextApiResponse<Data>) { | |
| if (req.method !== "GET") | |
| return res | |
| .status(405) | |
| .json({ success: false, error: "Method not allowed" }); | |
| if (!req.session.userid) | |
| return res.status(401).json({ success: false, error: "Not logged in" }); | |
| const workspaceId = parseInt(req.query.id as string); | |
| const cacheKey = `activity_users_${workspaceId}`; | |
| const now = Date.now(); | |
| const cached = activityUsersCache.get(cacheKey); | |
| if (cached && now - cached.timestamp < ACTIVITY_CACHE_DURATION) { | |
| return res.status(200).json({ success: true, message: cached.data }); | |
| } | |
| export default withPermissionCheck(handler, "view_entire_groups_activity"); | |
| export async function handler(req: NextApiRequest, res: NextApiResponse<Data>) { | |
| if (req.method !== "GET") | |
| return res | |
| .status(405) | |
| .json({ success: false, error: "Method not allowed" }); | |
| const workspaceId = Number.parseInt(req.query.id as string, 10); | |
| if (Number.isNaN(workspaceId)) { | |
| return res.status(400).json({ success: false, error: "Invalid workspace id" }); | |
| } | |
| const cacheKey = `activity_users_${workspaceId}`; | |
| const now = Date.now(); | |
| const cached = activityUsersCache.get(cacheKey); | |
| if (cached && now - cached.timestamp < ACTIVITY_CACHE_DURATION) { | |
| return res.status(200).json({ success: true, message: cached.data }); | |
| } |
| const adjustments = await prisma.activityAdjustment.findMany({ | ||
| where: { | ||
| workspaceGroupId: workspaceId, | ||
| createdAt: { | ||
| gte: startDate, | ||
| lte: currentDate, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| adjustments.forEach((adjustment: any) => { | ||
| const found = combinedMinutes.find( | ||
| (x) => x.userId == Number(adjustment.userId) | ||
| ); | ||
| const adjustmentMs = adjustment.minutes * 60000; | ||
| if (found) { | ||
| found.ms.push(adjustmentMs); | ||
| } else { | ||
| combinedMinutes.push({ | ||
| userId: Number(adjustment.userId), | ||
| ms: [adjustmentMs], | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| const topStaff: TopStaff[] = []; | ||
| const processedUserIds = new Set<number>(); | ||
| for (const min of combinedMinutes) { | ||
| const minSum = min.ms.reduce((partial, a) => partial + a, 0); | ||
| const found = users.find((x) => x.userid === BigInt(min.userId)); | ||
| if (leaderboardRank && found) { | ||
| const userRank = (found as any).ranks?.[0]?.rankId; | ||
| if (!userRank || Number(userRank) < leaderboardRank) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if (found) { | ||
| topStaff.push({ | ||
| userId: min.userId, | ||
| username: found?.username || "Unknown", | ||
| ms: minSum, | ||
| picture: found?.picture || "Unknown", | ||
| }); | ||
| processedUserIds.add(min.userId); | ||
| } | ||
| } | ||
| for (const user of users) { | ||
| const userId = Number(user.userid); | ||
| if (processedUserIds.has(userId)) continue; | ||
|
|
||
| if (leaderboardRank) { | ||
| const userRank = (user as any).ranks?.[0]?.rankId; | ||
| if (!userRank || Number(userRank) < leaderboardRank) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| topStaff.push({ | ||
| userId: userId, | ||
| username: user.username || "Unknown", | ||
| ms: 0, | ||
| picture: user.picture || "Unknown", | ||
| }); | ||
| } | ||
|
|
||
| const bestStaff = topStaff.sort((a, b) => { | ||
| if (b.ms !== a.ms) { | ||
| return b.ms - a.ms; | ||
| } | ||
| return a.username.localeCompare(b.username); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, get the file size and structure
wc -l pages/api/workspace/[id]/activity/users.tsRepository: PlanetaryOrbit/orbit
Length of output: 110
🏁 Script executed:
# Check the full file to see all Number() conversions and context
cat -n pages/api/workspace/[id]/activity/users.tsRepository: PlanetaryOrbit/orbit
Length of output: 9502
🏁 Script executed:
# Search for userId/userid type definitions in Prisma schema
fd -t f "schema.prisma" -o prisma -x cat -n {}Repository: PlanetaryOrbit/orbit
Length of output: 195
🏁 Script executed:
# Search for userId/userid field definitions across the codebase
rg -n "userid.*BigInt|userId.*BigInt" --type ts --type tsxRepository: PlanetaryOrbit/orbit
Length of output: 91
🏁 Script executed:
# Find the Prisma schema file
fd -t f "schema.prisma"Repository: PlanetaryOrbit/orbit
Length of output: 84
🏁 Script executed:
# Search for userid/userId field definitions in schema.prisma
rg -A 3 -B 1 "userid|userId" prisma/schema.prisma | head -100Repository: PlanetaryOrbit/orbit
Length of output: 3422
🏁 Script executed:
# Also check if there's documentation or constraints on user ID ranges
rg -n "userid.*Int|userId.*Int" -A 2 -B 2Repository: PlanetaryOrbit/orbit
Length of output: 50376
Store user IDs as BigInt or string; do not convert database BigInt IDs to Number.
Lines 164, 172, 193, 207, 225, 232, 261 convert BigInt IDs to Number, causing precision loss for large user IDs. Line 242's lookup users.find((x) => x.userid === BigInt(min.userId)) cannot recover the lost precision when comparing back to BigInt, resulting in silent lookup failures. Additionally, Set<number> deduplication at line 239 will not work correctly for IDs exceeding Number.MAX_SAFE_INTEGER. Use BigInt throughout (matching the codebase pattern seen in utils/permissionsManager.ts and auth handlers), or adjust types to handle string IDs if the API contract requires it.
🤖 Prompt for AI Agents
pages/api/workspace/[id]/activity/users.ts lines 213-285: the code converts DB
BigInt user IDs to Number (e.g., Number(adjustment.userId), Number(user.userid))
causing precision loss and broken lookups; change all ID handling to BigInt (or
to strings consistently): keep adjustment.userId and user.userid as BigInt (use
BigInt(adjustment.userId) if needed), use Set<bigint> for processedUserIds,
compare BigInts directly (user.userid === min.userId as BigInt), and avoid any
Number(...) conversions; if the external API expects string IDs, convert to
string only at the final output (e.g., userId: min.userId.toString()) rather
than mid-flow.
| user: { | ||
| birthdayDay: { not: null }, | ||
| birthdayMonth: { not: null } | ||
| } | ||
| }, | ||
| include: { user: { select: { userid: true, username: true, picture: true } } } | ||
| include: { user: { select: { userid: true, username: true, picture: true, birthdayDay: true, birthdayMonth: true } } } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Prisma schema file
find . -name "schema.prisma" -o -name "*.prisma" | head -20Repository: PlanetaryOrbit/orbit
Length of output: 86
🏁 Script executed:
# Look at the file structure to understand the project
git ls-files | grep -E "(prisma|pages/api)" | head -30Repository: PlanetaryOrbit/orbit
Length of output: 973
🏁 Script executed:
# Inspect the Prisma schema for user and workspaceMember models
cat ./prisma/schema.prismaRepository: PlanetaryOrbit/orbit
Length of output: 16714
🏁 Script executed:
# Also get the actual file under review
cat -n pages/api/workspace/[id]/home/upcoming.ts | head -60Repository: PlanetaryOrbit/orbit
Length of output: 2614
🏁 Script executed:
# Check if there are migration files that show the schema changes
find . -path "./prisma/migrations" -type dRepository: PlanetaryOrbit/orbit
Length of output: 83
🏁 Script executed:
# Look for any database migration files or migration history
ls -la prisma/migrations/ 2>/dev/null | head -30Repository: PlanetaryOrbit/orbit
Length of output: 1961
🏁 Script executed:
# Search for birthday-related migrations
grep -r "birthdayDay\|birthdayMonth" prisma/migrations/ | head -20Repository: PlanetaryOrbit/orbit
Length of output: 397
🏁 Script executed:
# Check the most recent migrations for schema changes
ls -lt prisma/migrations/ | head -10 | awk '{print $NF}' | while read dir; do
if [ -n "$dir" ] && [ "$dir" != "total" ]; then
echo "=== Migration: $dir ==="
cat "prisma/migrations/$dir/migration.sql" 2>/dev/null | head -20
fi
doneRepository: PlanetaryOrbit/orbit
Length of output: 4860
🏁 Script executed:
# Read the birthday migration file
cat prisma/migrations/20250613_birthdays/migration.sqlRepository: PlanetaryOrbit/orbit
Length of output: 152
🏁 Script executed:
# Read the recent hw_25 migration that also mentions birthdays
cat prisma/migrations/20251011203302_hw_25/migration.sqlRepository: PlanetaryOrbit/orbit
Length of output: 3553
🏁 Script executed:
# Check if there are any other references to workspaceMember birthday fields in the codebase
rg "workspaceMember.*birthday|birthday.*workspaceMember" --type ts --type jsRepository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Check all API files that might use birthday data to see if any still reference workspaceMember
rg "birthdayDay|birthdayMonth" pages/api/ --type ts --type js -B 2 -A 2Repository: PlanetaryOrbit/orbit
Length of output: 9062
Critical: Birthday field migration is incomplete—multiple endpoints still write to workspaceMember instead of user.
The code reads birthdayDay and birthdayMonth from the user relation, but other endpoints continue to write these fields to workspaceMember:
pages/api/workspace/[id]/birthday.tswrites to workspaceMemberpages/api/workspace/[id]/birthday/[userId].tswrites to workspaceMemberpages/api/workspace/[id]/profile/[uid]/member-info.tswrites to workspaceMember
This split implementation means birthdays updated via those endpoints won't be visible in the upcoming.ts query. Migrate all write operations to use user instead of workspaceMember, or revert this query to read from workspaceMember to match the write operations.
🤖 Prompt for AI Agents
In pages/api/workspace/[id]/home/upcoming.ts around lines 22–27 the query reads
birthdayDay and birthdayMonth from the user relation while other endpoints still
persist those fields on workspaceMember, causing inconsistent data; update the
write endpoints to persist birthdayDay and birthdayMonth on the user record
(load user id from workspaceMember and update User model) so all reads use user,
or revert this query to select birthdays from workspaceMember instead to match
current writers—pick one approach and make all read/write code consistent across
the four listed files.
| async function handler(req: NextApiRequest, res: NextApiResponse) { | ||
| const workspaceId = Number(req.query.id as string); | ||
| const userId = String(req.query.uid as string); | ||
|
|
||
| if (!workspaceId || !userId) { | ||
| return res.status(400).json({ success: false, error: "Missing workspace ID or user ID" }); | ||
| } |
There was a problem hiding this comment.
Critical: Missing authorization check.
This endpoint allows any authenticated user to modify any other user's profile data (department, timezone, birthday, etc.) without verifying they have permission. Consider using withPermissionCheck or adding explicit authorization logic to verify the requesting user is either the profile owner or has appropriate admin permissions.
-import { withSessionRoute } from "@/lib/withSession";
+import { withPermissionCheck } from "@/utils/permissionsManager";
async function handler(req: NextApiRequest, res: NextApiResponse) {
const workspaceId = Number(req.query.id as string);
const userId = String(req.query.uid as string);
+ const requestingUserId = (req as any).session?.userid;
if (!workspaceId || !userId) {
return res.status(400).json({ success: false, error: "Missing workspace ID or user ID" });
}
+ // Allow if user is editing their own profile or has admin permission
+ const isSelf = requestingUserId && BigInt(requestingUserId) === BigInt(userId);
+ if (!isSelf) {
+ // Permission check is handled by withPermissionCheck wrapper for non-self edits
+ // This path requires 'manage_members' or 'admin' permission
+ }
+
if (req.method === "PATCH") {At the bottom:
-export default withSessionRoute(handler);
+export default withPermissionCheck(handler, 'manage_members');Alternatively, implement self-edit logic that bypasses permission checks when userId === requestingUserId.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pages/api/workspace/[id]/profile/[uid]/member-info.ts around lines 5 to 11,
there is no authorization check so any authenticated user can modify another
user's profile; add an authorization gate that retrieves the requesting user
(from the session/JWT), then allow the operation only if requestingUserId ===
userId (self-edit) OR the requesting user has the workspace admin/appropriate
permission (use existing withPermissionCheck helper if available or check
workspace role/permissions explicitly); if neither condition is met return
res.status(403) with an error, and ensure this check runs before any profile
modifications are applied.
| const { id } = req.query; | ||
| const { timezone } = req.body; | ||
|
|
||
| if (!timezone || typeof timezone !== "string") { | ||
| return res.status(400).json({ error: "Invalid timezone" }); | ||
| } | ||
|
|
||
| try { | ||
| const existingMember = await prisma.workspaceMember.findUnique({ | ||
| where: { | ||
| workspaceGroupId_userId: { | ||
| workspaceGroupId: parseInt(id as string), | ||
| userId: BigInt(userId), | ||
| }, | ||
| }, | ||
| select: { | ||
| timezone: true, | ||
| }, | ||
| }); | ||
| if (!existingMember?.timezone) { | ||
| await prisma.workspaceMember.update({ | ||
| where: { | ||
| workspaceGroupId_userId: { | ||
| workspaceGroupId: parseInt(id as string), | ||
| userId: BigInt(userId), | ||
| }, | ||
| }, | ||
| data: { | ||
| timezone: timezone, | ||
| }, | ||
| }); | ||
|
|
||
| return res.status(200).json({ success: true, updated: true }); | ||
| } | ||
|
|
||
| return res.status(200).json({ success: true, updated: false }); | ||
| } catch (error) { |
There was a problem hiding this comment.
Handle “member not found” explicitly (current code will 500)
If findUnique returns null, !existingMember?.timezone is true and update() will throw “Record to update not found”.
const { id } = req.query;
const { timezone } = req.body;
+ const workspaceGroupId = parseInt(id as string);
+ if (Number.isNaN(workspaceGroupId)) {
+ return res.status(400).json({ error: "Invalid workspace id" });
+ }
...
const existingMember = await prisma.workspaceMember.findUnique({
where: {
workspaceGroupId_userId: {
- workspaceGroupId: parseInt(id as string),
+ workspaceGroupId,
userId: BigInt(userId),
},
},
select: {
timezone: true,
},
});
- if (!existingMember?.timezone) {
+ if (!existingMember) {
+ return res.status(404).json({ error: "Workspace member not found" });
+ }
+ if (!existingMember.timezone) {
await prisma.workspaceMember.update({
where: {
workspaceGroupId_userId: {
- workspaceGroupId: parseInt(id as string),
+ workspaceGroupId,
userId: BigInt(userId),
},
},
data: {
timezone: timezone,
},
});🤖 Prompt for AI Agents
In pages/api/workspace/[id]/timezone.ts around lines 17 to 53, the code treats a
null result from prisma.workspaceMember.findUnique as if a row exists and then
calls update (causing a "Record to update not found" 500). Explicitly check if
existingMember is null and return a 404 (or appropriate 4xx) JSON error before
attempting any update; otherwise, compare existingMember.timezone to the new
timezone and only call prisma.workspaceMember.update when the member exists and
the timezone actually needs changing, then return the appropriate
success/updated response.
| // If we have a scope from the pattern dialog, use it instead of asking again | ||
| const deleteScope = scope || (deleteAll ? "all" : "single"); | ||
|
|
||
| await axios.delete( | ||
| `/api/workspace/${workspace.groupId}/sessions/${session.id}/delete`, | ||
| { | ||
| data: { deleteAll }, | ||
| data: { | ||
| deleteAll: deleteScope === "all", | ||
| deleteScope: deleteScope, // Pass the scope for future/single distinction | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, search for deleteScope references to understand the scope handling
rg -n "deleteScope" --type ts --type tsx -A 3 -B 2Repository: PlanetaryOrbit/orbit
Length of output: 91
🏁 Script executed:
# Search for the backend delete endpoint implementation
rg -n "delete.*sessions.*delete" --type ts --type tsx --type js --type jsx -A 5 -B 2Repository: PlanetaryOrbit/orbit
Length of output: 91
🏁 Script executed:
# Look for the delete modal implementation to see what options are offered
rg -n "deleteAll|deleteModal|delete.*modal" --type ts --type tsx --type jsx --type js -A 3 -B 2 | head -100Repository: PlanetaryOrbit/orbit
Length of output: 91
🏁 Script executed:
# Search for "future" scope references to see if it's used elsewhere
rg -n "future" --type ts --type tsx -i | grep -i "scope\|delete"Repository: PlanetaryOrbit/orbit
Length of output: 91
🏁 Script executed:
# Search for deleteScope references without file type restriction
rg -n "deleteScope" -A 3 -B 2Repository: PlanetaryOrbit/orbit
Length of output: 1767
🏁 Script executed:
# Search for the delete endpoint in API routes
find . -type f -name "*.ts" -o -name "*.js" -o -name "*.tsx" -o -name "*.jsx" | xargs rg -l "delete.*sessions.*delete\|deleteScope" | head -20Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Look for API route handlers - typically in /pages/api or /src/pages/api
fd -e ts -e js -e tsx -e jsx "api" --type dRepository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Search for the delete modal or scope selection logic
rg -n "deleteAll.*single.*all\|scope.*future\|single.*all.*delete" -A 3 -B 2 | head -50Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Search for API routes - look for pages/api structure
find . -path "*/pages/api*" -type f \( -name "*.ts" -o -name "*.js" \) | head -20Repository: PlanetaryOrbit/orbit
Length of output: 690
🏁 Script executed:
# Search for the delete endpoint in API
rg -n "sessions.*delete\|/api.*delete" --glob "*/pages/api/**" -A 5 -B 2Repository: PlanetaryOrbit/orbit
Length of output: 212
🏁 Script executed:
# Search for where scope parameter comes from - the pattern dialog
rg -n "scope.*=\|setScope\|pattern.*dialog" -A 2 -B 2 | head -80Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Look for deleteAll parameter usage and modal implementation
rg -n "deleteAll.*=\|setDeleteAll\|openDeleteModal\|DeleteModal" -A 3 -B 2 | head -100Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Search for workspace API routes
find . -path "*/pages/api/workspace*" -type fRepository: PlanetaryOrbit/orbit
Length of output: 6237
🏁 Script executed:
# Search for files containing "delete" and "workspace"
rg -l "workspace.*delete\|delete.*workspace" pages/api/Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Search entire codebase for the delete endpoint handler
rg -n "req.method.*DELETE\|DELETE.*sessions" -A 10 | head -100Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Search for the delete modal or scope selection dialog
rg -n "Modal\|Dialog\|scope.*\[" pages/workspace -A 3 -B 2 | head -150Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Look for the specific file with the delete handler
find . -type f \( -name "*.ts" -o -name "*.js" \) -exec grep -l "sessions.*delete\|deleteScope" {} \; 2>/dev/nullRepository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Read the delete endpoint that's called from the edit page
cat -n ./pages/api/workspace/[id]/sessions/[sid]/delete.tsRepository: PlanetaryOrbit/orbit
Length of output: 2852
🏁 Script executed:
# Also check the other delete endpoint to understand the difference
cat -n ./pages/api/workspace/[id]/sessions/manage/[sid]/delete.tsRepository: PlanetaryOrbit/orbit
Length of output: 1510
🏁 Script executed:
# Search for where scope comes from in the edit page - look for pattern dialog
rg -n "scope\|pattern.*dialog\|Dialog" pages/workspace/[id]/sessions/edit/ -A 2 -B 2Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Search for "scope" in the edit page more broadly
cat -n pages/workspace/[id]/sessions/edit/[sid].tsx | head -350 | tail -100Repository: PlanetaryOrbit/orbit
Length of output: 4017
🏁 Script executed:
# Look for pattern dialog or scope-related state
rg -n "pattern\|scope\|deleteAll" pages/workspace/[id]/sessions/edit/ --type-add='tsx:*.tsx' -A 2 -B 2Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Search for Dialog, Modal, or where scope state is set
rg -n "useState.*scope\|setScope\|const scope" pages/workspace/[id]/sessions/edit/ --type-add='tsx:*.tsx'Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Search for delete modal/dialog implementation
find pages/workspace -name "*delete*" -o -name "*modal*" -o -name "*dialog*" | head -20Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Look for the pattern dialog in the file - search for Dialog or Modal components
rg -n "Dialog\|Modal\|<dialog\|<modal" pages/workspace/[id]/sessions/edit/[sid].tsx -A 5 -B 2Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Search for the pattern dialog component definition
find pages/workspace -type f -name "*.tsx" -o -name "*.ts" | xargs rg -l "PatternDialog\|pattern.*dialog" 2>/dev/nullRepository: PlanetaryOrbit/orbit
Length of output: 48
🏁 Script executed:
# Look at the complete file to find scope initialization
wc -l pages/workspace/[id]/sessions/edit/[sid].tsxRepository: PlanetaryOrbit/orbit
Length of output: 112
🏁 Script executed:
# Get the beginning of the file to see scope declaration
head -100 pages/workspace/[id]/sessions/edit/[sid].tsxRepository: PlanetaryOrbit/orbit
Length of output: 2557
🏁 Script executed:
# Search for scope variable declaration and usage throughout the file
rg -n "scope\|Pattern" pages/workspace/[id]/sessions/edit/[sid].tsx | head -50Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Look around the delete function for the pattern dialog
sed -n '200,400p' pages/workspace/[id]/sessions/edit/[sid].tsx | cat -nRepository: PlanetaryOrbit/orbit
Length of output: 8414
🏁 Script executed:
# Search for where deleteAll is set and the delete modal
rg -n "deleteAll\|DeleteModal\|showDeleteModal" pages/workspace/[id]/sessions/edit/[sid].tsx -A 2 -B 2Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Search for all useState calls
rg -n "useState" pages/workspace/[id]/sessions/edit/[sid].tsxRepository: PlanetaryOrbit/orbit
Length of output: 656
🏁 Script executed:
# Search for the delete modal implementation
rg -n "showDeleteModal\|DeleteModal" pages/workspace/[id]/sessions/edit/[sid].tsx -A 20 | head -80Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Look for where the modal offers delete options
sed -n '400,700p' pages/workspace/[id]/sessions/edit/[sid].tsx | cat -nRepository: PlanetaryOrbit/orbit
Length of output: 20361
🏁 Script executed:
# Search for what the modal buttons/options are
rg -n "deleteAll.*true\|setDeleteAll\|Delete all\|Delete this" pages/workspace/[id]/sessions/edit/[sid].tsx -A 2 -B 2Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Search for the update-pattern API implementation
cat -n ./pages/api/workspace/[id]/sessions/[sid]/update-pattern.ts | head -100Repository: PlanetaryOrbit/orbit
Length of output: 4028
🏁 Script executed:
# Check if update-pattern API handles "future" properly
rg -n "future\|updateScope" ./pages/api/workspace/[id]/sessions/[sid]/update-pattern.ts -A 3 -B 2Repository: PlanetaryOrbit/orbit
Length of output: 46
🏁 Script executed:
# Check the complete delete endpoint to confirm deleteScope is not used
cat -n ./pages/api/workspace/[id]/sessions/[sid]/delete.ts | grep -A 30 "deleteAll"Repository: PlanetaryOrbit/orbit
Length of output: 2164
Delete scope logic doesn't properly handle "future" scope.
The delete endpoint at /api/workspace/${workspace.groupId}/sessions/${session.id}/delete only accepts a deleteAll boolean parameter and completely ignores the deleteScope parameter sent by the frontend. When deleteScope is set to "future", the code calculates deleteAll: false (line 331), which causes the backend to delete only the single session instead of deleting this session and all future sessions on the same day of week. However, the frontend displays a success message stating "This and future sessions deleted successfully" (lines 339-340), creating a mismatch between the user's expectation and the actual backend behavior.
To fix this, the delete endpoint needs to implement the same scope handling logic currently used in the update-pattern API (update-pattern.ts), which properly supports "single", "future", and "all" scopes by filtering sessions based on schedule and day of week.
2 participants
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.