Merged
Conversation
…visory templates and backend fixes.
…llers, update SSL certificates, and introduce an activity logging service.
Fix/sql injection vulnerabilities
📝 WalkthroughWalkthroughThis PR introduces type-safety improvements across reporting controllers, adds a new activity logging service for project events, enhances pagination with parameterized search handling, extends the reporting controller base with team lead filtering capabilities, and refactors the reporting members controller with parameter wiring updates. Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
worklenz-backend/src/controllers/reporting/reporting-members-controller.ts (2)
1543-1557:⚠️ Potential issue | 🟡 MinorUnused variable
rawDurationClause— dead code from incomplete refactoring.
rawDurationClauseis computed at line 1545 but never referenced. The if/else chain below reconstructs the duration clause manually, making the assignment pointless. This duplicates the clause-building logic and leaves a confusing artifact from incomplete refactoring.Either remove the dead assignment or use the value returned by
getDateRangeClauseWithTimezonedirectly, removing the redundant if/else chain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@worklenz-backend/src/controllers/reporting/reporting-members-controller.ts` around lines 1543 - 1557, The variable rawDurationClause is computed by calling getDateRangeClauseWithTimezone but never used, leaving dead code alongside a duplicated if/else that builds durationClause; either remove the rawDurationClause assignment or replace the entire manual if/else with the value from getDateRangeClauseWithTimezone (assign durationClause = rawDurationClause) and drop the redundant branches; update references to DATE_RANGES and userTimezone as needed so durationClause is sourced only from getDateRangeClauseWithTimezone or delete the unused rawDurationClause entirely.
88-254:⚠️ Potential issue | 🔴 CriticalSQL parameter binding mismatch:
teamsClauseplaceholders reference wrong parameter values when archived filtering is active.In
getReportingMembers(line 557),teamsClauseis built withSqlHelper.buildInClause(teamIds, 2 + searchParams.length), generating placeholders like$3, $4, ...(assuming 1 search param). However, insidegetMembers, thequeryParamsarray (line 254) is constructed as:[teamId, ...searchParams, ...archivedParams, ...teamIdsParams, ...]When
includeArchivedisfalse,archivedParamscontains theuserId. This shifts allteamIdsParamsone position to the right. TheteamsClauseplaceholders ($3, $4, ...) now point toarchivedParams[0](userId) instead ofteamIdsParams[0](team ID), producing incorrect query results.This bug only manifests when both the archived filter and teams filter are active simultaneously.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@worklenz-backend/src/controllers/reporting/reporting-members-controller.ts` around lines 88 - 254, The teamsClause placeholders are misaligned when includeArchived is false because archivedParams (userId) is inserted before teamIdsParams in queryParams; update getMembers so queryParams orders teamIdsParams before archivedParams (i.e., [teamId, ...searchParams, ...teamIdsParams, ...archivedParams, ...assignParams, ...completedParams, ...overdueParams, ...activityLogParams, ...timeLogParams, ...projectParams]) to match how teamsClause was constructed in getReportingMembers, or alternatively rebuild teamsClause in getReportingMembers after archivedParams is known using the correct param offset; adjust the code around teamsClause, archivedParams, and queryParams accordingly to ensure placeholders refer to the intended values.
🧹 Nitpick comments (5)
worklenz-backend/src/controllers/reporting/reporting-members-controller.ts (1)
1522-1628: Near-identical query building duplicated betweengetTimelogsFlatandgetTimelogsFlatData.
getTimelogsFlat(reads fromreq.body) andgetTimelogsFlatData(reads from function params) construct the same SQL query and clause-building logic independently. Consider refactoringgetTimelogsFlatto normalize its inputs and delegate togetTimelogsFlatData, eliminating ~80 lines of duplication and the risk of the two paths diverging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@worklenz-backend/src/controllers/reporting/reporting-members-controller.ts` around lines 1522 - 1628, getTimelogsFlat duplicates the entire SQL/params construction found in getTimelogsFlatData; instead normalize inputs from req (team_member_id, duration, date_range, billable, search, req.user.team_id) and delegate to getTimelogsFlatData to remove duplication. Update getTimelogsFlat to: fetch userTimezone via getUserTimezone(req.user?.id), assemble a simple args object (teamId, team_member_id, duration, date_range, billable, search, userTimezone) and call getTimelogsFlatData with those normalized args (or adjust getTimelogsFlatData signature to accept the args object), then return its ServerResponse; keep helper calls like buildBillableQuery and secondsToReadable inside getTimelogsFlatData so all SQL/params logic lives only in that function. Ensure behavior is preserved for team filter and search param handling when delegating.worklenz-backend/src/controllers/worklenz-controller-base.ts (1)
30-30: Return type ofsortFieldshould be documented or narrowed.
toPaginationOptionsreturnssortFieldasstring | string[]depending on the input. Multiple call sites now usesortField as stringcasts to work around this. Consider returning a normalizedstring(e.g., joining arrays with,) to reduce the need for casts at every call site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@worklenz-backend/src/controllers/worklenz-controller-base.ts` at line 30, The toPaginationOptions method returns sortField as either string or string[] which forces callers to cast; normalize and document sortField as a plain string by converting any array input into a single string (e.g., join with ", ") and update the method's return type/signature accordingly; inside toPaginationOptions (use the function name toPaginationOptions and reference the searchField/queryParams inputs) perform the normalization before returning so all callers can treat sortField as string and remove the ad-hoc `as string` casts.worklenz-backend/src/controllers/schedule/schedule-controller.ts (1)
834-834:sortField as stringcast is safe here but fragile.
toPaginationOptionsreturnssortFieldtyped asstring | string[]. In this call site,searchFieldis always a single string, so the cast is correct. However, ifsearchFieldis ever changed to an array, this cast will silently produce"t.name,sort_order".replace(...)instead of failing loudly.Consider narrowing the type upstream or adding a runtime guard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@worklenz-backend/src/controllers/schedule/schedule-controller.ts` at line 834, The current cast of sortField to string in the calculation of sortFields is fragile because toPaginationOptions returns string | string[]; update the call site in schedule-controller by adding a runtime guard that ensures sortField is a string (e.g., if Array.isArray(sortField) then handle explicitly — pick the first element, join with a separator, or throw a clear error) before performing .replace; alternatively narrow the type upstream in toPaginationOptions so sortField is guaranteed string; reference the sortFields variable and the sortField value returned from toPaginationOptions when implementing the guard.worklenz-backend/src/services/activity-logging.service.ts (1)
12-105: Reduce duplication across the four specific log methods.
logProjectCreated,logProjectUpdated,logProjectDeleted, andlogProjectArchivedare identical except for thelog_typestring and the verb in the description. Consider a shared private helper:Suggested refactor
+ private static async logProjectEvent( + teamId: string, + projectId: string, + userId: string, + projectName: string, + logType: string, + verb: string + ): Promise<void> { + try { + const q = `INSERT INTO project_logs (team_id, project_id, user_id, description, log_type) + VALUES ($1, $2, $3, $4, $5) + ON CONFLICT DO NOTHING`; + await db.query(q, [teamId, projectId, userId, `Project "${projectName}" ${verb}`, logType]); + } catch (e) { + log_error(e); + } + } + public static async logProjectCreated(...) { - try { ... } catch (e) { log_error(e); } + return this.logProjectEvent(teamId, projectId, userId, projectName, 'project_created', 'created'); } // Same for updated, deleted, archived🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@worklenz-backend/src/services/activity-logging.service.ts` around lines 12 - 105, The four methods logProjectCreated, logProjectUpdated, logProjectDeleted and logProjectArchived are duplicated; introduce a single private helper (e.g., private static async logProjectEvent(teamId: string, projectId: string, userId: string, projectName: string, logType: string, verb: string): Promise<void>) that builds the SQL and description and calls db.query, then refactor each public method to call logProjectEvent with the appropriate logType ('project_created'|'project_updated'|'project_deleted'|'project_archived') and verb ('created'|'updated'|'deleted'|'archived'); keep existing error handling (log_error) inside the helper so the public methods become one-line delegations and leave logProjectActivity unchanged.worklenz-backend/src/controllers/team-members-controller.ts (1)
842-842:sortField as stringmay mask an array value at runtime.
toPaginationOptionsis called with an arraysearchField(["tmiv.name", "tmiv.email", "u.name"]) on line 839. WhenqueryParams.fieldis absent,sortFieldremains astring[]. Theas stringcast silences TypeScript but doesn't change the runtime value — when interpolated into the SQLORDER BYclause, JavaScript's array-to-string coercion producestmiv.name,tmiv.email,u.name, which happens to be valid multi-column SQL.This works by coincidence. A safer approach would be to normalize
sortFieldexplicitly:Suggested fix
- const teamMembers = await this.getTeamMemberInsightData(teamId as string, start, end, project, status, searchQuery, sortField as string, sortOrder, size, offset, req.query.all as string); + const resolvedSortField = Array.isArray(sortField) ? sortField.join(", ") : sortField; + const teamMembers = await this.getTeamMemberInsightData(teamId as string, start, end, project, status, searchQuery, resolvedSortField, sortOrder, size, offset, req.query.all as string);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@worklenz-backend/src/controllers/team-members-controller.ts` at line 842, The call passes sortField with a blind `as string` cast but sortField can be a string[] from toPaginationOptions (via searchField), so explicitly normalize it before calling getTeamMemberInsightData: check Array.isArray(sortField) and convert it to the intended string form (e.g. either pick a single field like sortField[0] or join into a comma-separated string with sortField.join(', ')), remove the unsafe `as string` cast, and then pass the normalized string into getTeamMemberInsightData (referencing sortField, toPaginationOptions, getTeamMemberInsightData, and queryParams.field to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@worklenz-backend/src/controllers/project-workload/workload-gannt-controller.ts`:
- Around line 226-227: The condition uses the wrong property name: replace the
camelCase dateRange.endDate with the snake_case dateRange.end_date so the moment
check compares logsRange.max_date against the actual end date; update the
expression inside the if that calls moment(logsRange.max_date).isAfter(...) to
use dateRange.end_date and ensure the subsequent assignment sets
dateRange.end_date = logsRange.max_date (referencing the dateRange and logsRange
variables and the end_date field).
In
`@worklenz-backend/src/controllers/reporting/overview/reporting-overview-controller.ts`:
- Line 93: The sortField coming from toPaginationOptions can be a string or
string[] which causes a runtime type mismatch when passed into
ReportingControllerBase.getProjectsByTeam and used in ORDER BY; update the code
so sortField is always a string: either modify toPaginationOptions to normalize
its sortField return value (if sortField is an array, return the selected field
or the first element as a string) or, if you prefer the minimal change, add a
defensive normalization before the call at reporting-overview-controller.ts
(e.g., if Array.isArray(sortField) pick the first element or a safe default like
"p.name"); ensure the normalized string is what you pass into getProjectsByTeam
and used in SQL templating so ORDER BY always receives a string.
In
`@worklenz-backend/src/controllers/reporting/overview/reporting-overview-export-controller.ts`:
- Around line 419-421: Remove the dead variable by either deleting `archived`
from the destructuring or use it to compute `includeArchived`; specifically
update the block where `const { duration, date_range, only_single_member,
archived } = req.query;` and `const includeArchived = req.query.archived ===
"true";` are defined so that `includeArchived` is derived from the destructured
`archived` (e.g., `includeArchived` computed from `archived`) or remove
`archived` from the destructure and keep the current `includeArchived`
assignment; modify the code around the `includeArchived` computation in
reporting-overview-export-controller.ts accordingly.
In `@worklenz-backend/src/controllers/reporting/reporting-controller-base.ts`:
- Around line 169-210: buildProjectFilterForTeamLead currently injects userId
directly into the SQL fragment (user_id = '${userId}'), causing a SQL injection
risk; change the method (buildProjectFilterForTeamLead) to return a
parameterized clause plus its params (e.g. { clause: string, params: any[] })
instead of a raw string, replace the interpolated userId with a positional
placeholder ($n) and push userId into the returned params array, and ensure
callers spread those params into their filterParams and advance their
paramOffset (follow the pattern used by SqlHelper.buildInClause for parameter
numbering/offset handling).
In `@worklenz-backend/src/controllers/reporting/reporting-members-controller.ts`:
- Around line 1870-1872: The archivedClause builds SQL with direct interpolation
of req.user?.id causing SQL injection (same issue as in getMemberTaskStats);
change archivedClause to use a parameter placeholder (e.g., $/userId/ or ?
depending on the DB library) instead of embedding req.user?.id, then pass the
actual req.user.id value in the parameter object/array when executing the query
(also update the query execution that concatenates archivedClause so it receives
the parameter), ensuring the parameter name matches the placeholder and that
getMemberTaskStats and this reporting query use the same safe parameterized
approach.
- Around line 1925-1952: The code assumes data[0] exists and will crash when
result.rows is empty; update the logic around the db.query result handling (the
data variable and the body construction used with ServerResponse) to guard
against empty arrays: only iterate/transform rows if data.length > 0 and set
team_member_name to a safe default when data is empty (e.g., null or derived
from another source) instead of accessing data[0].team_member_name; ensure
downstream uses (projects: data) remain valid for an empty array.
- Around line 1408-1410: The archivedClause currently interpolates req.user?.id
directly causing SQL injection; change it to use a parameterized placeholder
(e.g., `AND t.project_id NOT IN (SELECT project_id FROM archived_projects WHERE
project_id = t.project_id AND archived_projects.user_id = $<n>)` or your
project's placeholder style) and push the corresponding user id value into
archivedParams, then append archivedParams into the method's queryParams (the
array used by the query) before executing; update references to archivedClause,
includeArchived, archivedParams and queryParams in
reporting-members-controller.ts so the user id is passed as a parameter rather
than concatenated.
In `@worklenz-backend/src/controllers/worklenz-controller-base.ts`:
- Around line 42-55: The parameterized-search branch is incorrectly pre-escaping
single quotes (escapedSearch = search.replace(/'/g, "''")) causing values like
O'Brien to become O''Brien when pushed into searchParams; in the branch where
paramOffset is defined (the code building s and pushing to searchParams), remove
the replacement and push the raw search value (e.g., `%${search}%`) into
searchParams for both the string and Array.isArray(searchField) cases so the DB
driver can handle escaping automatically; keep any existing escaping only for
the non-parameterized/raw-SQL code path.
In `@worklenz-backend/src/services/activity-logging.service.ts`:
- Around line 24-26: The INSERT in activity-logging.service.ts (variable q) is
inserting non-existent columns user_id and log_type and using a meaningless ON
CONFLICT; fix by aligning the SQL with the actual schema: change the statement
to INSERT INTO project_logs (team_id, project_id, description) VALUES ($1, $2,
$3) (omit user_id and log_type) and remove the ON CONFLICT clause, or
alternatively if you need deduplication add a migration to create a log_type
column and a UNIQUE constraint (e.g., on (project_id, log_type)) and then update
the INSERT to include log_type and use ON CONFLICT (...) DO NOTHING. Ensure the
parameter ordering and the values supplied where q is used match the revised
column list.
---
Outside diff comments:
In `@worklenz-backend/src/controllers/reporting/reporting-members-controller.ts`:
- Around line 1543-1557: The variable rawDurationClause is computed by calling
getDateRangeClauseWithTimezone but never used, leaving dead code alongside a
duplicated if/else that builds durationClause; either remove the
rawDurationClause assignment or replace the entire manual if/else with the value
from getDateRangeClauseWithTimezone (assign durationClause = rawDurationClause)
and drop the redundant branches; update references to DATE_RANGES and
userTimezone as needed so durationClause is sourced only from
getDateRangeClauseWithTimezone or delete the unused rawDurationClause entirely.
- Around line 88-254: The teamsClause placeholders are misaligned when
includeArchived is false because archivedParams (userId) is inserted before
teamIdsParams in queryParams; update getMembers so queryParams orders
teamIdsParams before archivedParams (i.e., [teamId, ...searchParams,
...teamIdsParams, ...archivedParams, ...assignParams, ...completedParams,
...overdueParams, ...activityLogParams, ...timeLogParams, ...projectParams]) to
match how teamsClause was constructed in getReportingMembers, or alternatively
rebuild teamsClause in getReportingMembers after archivedParams is known using
the correct param offset; adjust the code around teamsClause, archivedParams,
and queryParams accordingly to ensure placeholders refer to the intended values.
---
Duplicate comments:
In
`@worklenz-backend/src/controllers/reporting/overview/reporting-overview-export-controller.ts`:
- Line 27: The call to ReportingControllerBase.getProjectsByTeam casts sortField
with a risky "as string" (same issue as in reporting-overview-controller.ts);
update the code so toPaginationOptions returns a guaranteed normalized string or
validate/normalize sortField before calling getProjectsByTeam (e.g., check
typeof sortField === "string" and fallback to a safe default or coerce via
String(sortField)) and pass that validated value to getProjectsByTeam instead of
using the unchecked "sortField as string".
---
Nitpick comments:
In `@worklenz-backend/src/controllers/reporting/reporting-members-controller.ts`:
- Around line 1522-1628: getTimelogsFlat duplicates the entire SQL/params
construction found in getTimelogsFlatData; instead normalize inputs from req
(team_member_id, duration, date_range, billable, search, req.user.team_id) and
delegate to getTimelogsFlatData to remove duplication. Update getTimelogsFlat
to: fetch userTimezone via getUserTimezone(req.user?.id), assemble a simple args
object (teamId, team_member_id, duration, date_range, billable, search,
userTimezone) and call getTimelogsFlatData with those normalized args (or adjust
getTimelogsFlatData signature to accept the args object), then return its
ServerResponse; keep helper calls like buildBillableQuery and secondsToReadable
inside getTimelogsFlatData so all SQL/params logic lives only in that function.
Ensure behavior is preserved for team filter and search param handling when
delegating.
In `@worklenz-backend/src/controllers/schedule/schedule-controller.ts`:
- Line 834: The current cast of sortField to string in the calculation of
sortFields is fragile because toPaginationOptions returns string | string[];
update the call site in schedule-controller by adding a runtime guard that
ensures sortField is a string (e.g., if Array.isArray(sortField) then handle
explicitly — pick the first element, join with a separator, or throw a clear
error) before performing .replace; alternatively narrow the type upstream in
toPaginationOptions so sortField is guaranteed string; reference the sortFields
variable and the sortField value returned from toPaginationOptions when
implementing the guard.
In `@worklenz-backend/src/controllers/team-members-controller.ts`:
- Line 842: The call passes sortField with a blind `as string` cast but
sortField can be a string[] from toPaginationOptions (via searchField), so
explicitly normalize it before calling getTeamMemberInsightData: check
Array.isArray(sortField) and convert it to the intended string form (e.g. either
pick a single field like sortField[0] or join into a comma-separated string with
sortField.join(', ')), remove the unsafe `as string` cast, and then pass the
normalized string into getTeamMemberInsightData (referencing sortField,
toPaginationOptions, getTeamMemberInsightData, and queryParams.field to locate
the code).
In `@worklenz-backend/src/controllers/worklenz-controller-base.ts`:
- Line 30: The toPaginationOptions method returns sortField as either string or
string[] which forces callers to cast; normalize and document sortField as a
plain string by converting any array input into a single string (e.g., join with
", ") and update the method's return type/signature accordingly; inside
toPaginationOptions (use the function name toPaginationOptions and reference the
searchField/queryParams inputs) perform the normalization before returning so
all callers can treat sortField as string and remove the ad-hoc `as string`
casts.
In `@worklenz-backend/src/services/activity-logging.service.ts`:
- Around line 12-105: The four methods logProjectCreated, logProjectUpdated,
logProjectDeleted and logProjectArchived are duplicated; introduce a single
private helper (e.g., private static async logProjectEvent(teamId: string,
projectId: string, userId: string, projectName: string, logType: string, verb:
string): Promise<void>) that builds the SQL and description and calls db.query,
then refactor each public method to call logProjectEvent with the appropriate
logType
('project_created'|'project_updated'|'project_deleted'|'project_archived') and
verb ('created'|'updated'|'deleted'|'archived'); keep existing error handling
(log_error) inside the helper so the public methods become one-line delegations
and leave logProjectActivity unchanged.
Comment on lines
+226
to
227
| if (moment(logsRange.max_date).isAfter(dateRange.endDate)) | ||
| dateRange.end_date = logsRange.max_date; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the specific lines around 226 to confirm the issue
sed -n '220,230p' worklenz-backend/src/controllers/project-workload/workload-gannt-controller.ts | cat -nRepository: Worklenz/worklenz
Length of output: 518
🏁 Script executed:
# Search for all dateRange property accesses to confirm naming convention
rg 'dateRange\.' worklenz-backend/src/controllers/project-workload/workload-gannt-controller.ts -A 0 -B 0Repository: Worklenz/worklenz
Length of output: 1356
🏁 Script executed:
# Check if dateRange object has endDate property defined anywhere
rg 'endDate' worklenz-backend/src/controllers/project-workload/workload-gannt-controller.tsRepository: Worklenz/worklenz
Length of output: 2047
🌐 Web query:
moment.js isAfter(undefined) behavior current version
💡 Result:
In the current Moment.js latest release (2.30.1), moment#isAfter(undefined) behaves the same as calling isAfter() with no argument: the undefined gets parsed into a Moment representing “now”, and the comparison is done against that current time. [1][2]
Concretely:
someMoment.isAfter(undefined)⇒ comparessomeMomentto now (at the timeisAfterruns). [1][2]moment().isAfter(undefined)⇒ typicallyfalse, because it compares “now” (created slightly earlier) to a new “now” created insideisAfter. [2]
This happens because isAfter internally turns non-Moment inputs into a Moment via createLocal(input) (and createLocal(undefined) becomes “now”). [2]
Sources:
[1] Moment.js site showing current version 2.30.1
[2] Moment.js docs + isAfter implementation (parsing input via createLocal(input); moment(undefined) treated as moment())
dateRange.endDate is a non-existent property — should be dateRange.end_date.
Line 226 uses dateRange.endDate (camelCase), which is undefined at runtime. Every other access to this object uses the snake_case property end_date (lines 214–215, 227, and throughout the file). As a result, moment(logsRange.max_date).isAfter(undefined) never evaluates as intended: in moment.js, isAfter(undefined) treats undefined as the current time, so the condition becomes "is logsRange.max_date after now?" — which is false for typical past log dates. The log-range max date is silently never applied to dateRange.end_date.
🐛 Proposed fix
- if (moment(logsRange.max_date).isAfter(dateRange.endDate))
+ if (moment(logsRange.max_date).isAfter(dateRange.end_date))
dateRange.end_date = logsRange.max_date;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (moment(logsRange.max_date).isAfter(dateRange.endDate)) | |
| dateRange.end_date = logsRange.max_date; | |
| if (moment(logsRange.max_date).isAfter(dateRange.end_date)) | |
| dateRange.end_date = logsRange.max_date; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@worklenz-backend/src/controllers/project-workload/workload-gannt-controller.ts`
around lines 226 - 227, The condition uses the wrong property name: replace the
camelCase dateRange.endDate with the snake_case dateRange.end_date so the moment
check compares logsRange.max_date against the actual end date; update the
expression inside the if that calls moment(logsRange.max_date).isAfter(...) to
use dateRange.end_date and ensure the subsequent assignment sets
dateRange.end_date = logsRange.max_date (referencing the dateRange and logsRange
variables and the end_date field).
| const teamFilterClause = `p.team_id = $1`; | ||
|
|
||
| const result = await ReportingControllerBase.getProjectsByTeam(teamId, size, offset, searchQuery, sortField, sortOrder, "", "", "", archivedClause, teamFilterClause, ""); | ||
| const result = await ReportingControllerBase.getProjectsByTeam(teamId, size, offset, searchQuery, sortField as string, sortOrder, "", "", "", archivedClause, teamFilterClause, ""); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find toPaginationOptions definition
find . -type f -name "*.ts" | xargs grep -l "toPaginationOptions" | head -20Repository: Worklenz/worklenz
Length of output: 1389
🏁 Script executed:
#!/bin/bash
# Locate and inspect toPaginationOptions implementation
rg -n --type=ts "toPaginationOptions" -A 15 worklenz-backend/src/controllers/Repository: Worklenz/worklenz
Length of output: 50373
🏁 Script executed:
#!/bin/bash
# Get the full toPaginationOptions function from worklenz-controller-base.ts
wc -l worklenz-backend/src/controllers/worklenz-controller-base.tsRepository: Worklenz/worklenz
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Read the toPaginationOptions function implementation
sed -n '30,100p' worklenz-backend/src/controllers/worklenz-controller-base.tsRepository: Worklenz/worklenz
Length of output: 2264
🏁 Script executed:
#!/bin/bash
# Find the specific line 93 in reporting-overview-controller.ts and surrounding context
sed -n '80,100p' worklenz-backend/src/controllers/reporting/overview/reporting-overview-controller.tsRepository: Worklenz/worklenz
Length of output: 1158
🏁 Script executed:
#!/bin/bash
# Find getProjectsByTeam implementation and see how it uses sortField
rg -n "getProjectsByTeam" worklenz-backend/src/controllers/ -A 5 | head -40Repository: Worklenz/worklenz
Length of output: 4508
🏁 Script executed:
#!/bin/bash
# Check the ReportingControllerBase class
find . -type f -name "*reporting*controller*base*" -o -name "*base*controller*" | grep reportingRepository: Worklenz/worklenz
Length of output: 222
🏁 Script executed:
#!/bin/bash
# Read the getProjectsByTeam function in reporting-controller-base.ts
sed -n '230,280p' worklenz-backend/src/controllers/reporting/reporting-controller-base.tsRepository: Worklenz/worklenz
Length of output: 2140
🏁 Script executed:
#!/bin/bash
# Read more of the getProjectsByTeam function to find the ORDER BY clause
sed -n '230,330p' worklenz-backend/src/controllers/reporting/reporting-controller-base.tsRepository: Worklenz/worklenz
Length of output: 5743
🏁 Script executed:
#!/bin/bash
# Search for ORDER BY in the getProjectsByTeam function
sed -n '230,400p' worklenz-backend/src/controllers/reporting/reporting-controller-base.ts | grep -n "ORDER BY"Repository: Worklenz/worklenz
Length of output: 245
🏁 Script executed:
#!/bin/bash
# Get the complete file size and read the end portion
wc -l worklenz-backend/src/controllers/reporting/reporting-controller-base.ts
tail -200 worklenz-backend/src/controllers/reporting/reporting-controller-base.ts | head -100Repository: Worklenz/worklenz
Length of output: 6041
🏁 Script executed:
#!/bin/bash
# Read lines around 370-380 where ORDER BY ${sortField} is used
sed -n '360,390p' worklenz-backend/src/controllers/reporting/reporting-controller-base.tsRepository: Worklenz/worklenz
Length of output: 2383
🏁 Script executed:
#!/bin/bash
# Check the return type annotation of toPaginationOptions
sed -n '30,78p' worklenz-backend/src/controllers/worklenz-controller-base.ts | tail -5Repository: Worklenz/worklenz
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Look for where sortField is used in other controllers after toPaginationOptions
rg "toPaginationOptions.*sortField" worklenz-backend/src/controllers/ -A 3 | head -50Repository: Worklenz/worklenz
Length of output: 43
Verify that toPaginationOptions always normalizes sortField to a guaranteed string.
The toPaginationOptions function can return sortField as either a string or string[] depending on the searchField parameter passed to it. At line 93, toPaginationOptions is called with ["p.name"] (an array), and when no explicit field query parameter is provided, sortField defaults to this array. The subsequent as string cast is a compile-time assertion that does not enforce a runtime check—if searchField is an array and no query field is provided, sortField will be passed to getProjectsByTeam as an array despite the cast, leading to a type mismatch in the SQL template literal ORDER BY ${sortField} (which would rely on JavaScript's implicit toString() conversion).
Ensure toPaginationOptions normalizes sortField to always be a single string value before returning, or validate/map the array to a safe default string field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@worklenz-backend/src/controllers/reporting/overview/reporting-overview-controller.ts`
at line 93, The sortField coming from toPaginationOptions can be a string or
string[] which causes a runtime type mismatch when passed into
ReportingControllerBase.getProjectsByTeam and used in ORDER BY; update the code
so sortField is always a string: either modify toPaginationOptions to normalize
its sortField return value (if sortField is an array, return the selected field
or the first element as a string) or, if you prefer the minimal change, add a
defensive normalization before the call at reporting-overview-controller.ts
(e.g., if Array.isArray(sortField) pick the first element or a safe default like
"p.name"); ensure the normalized string is what you pass into getProjectsByTeam
and used in SQL templating so ORDER BY always receives a string.
Comment on lines
+419
to
421
| const { duration, date_range, only_single_member, archived } = req.query; | ||
|
|
||
| const includeArchived = req.query.archived === "true"; |
There was a problem hiding this comment.
archived is destructured but never used — dead variable.
archived is added to the destructuring on line 419, but includeArchived on line 421 re-reads req.query.archived directly. The destructured binding is never referenced.
Either remove archived from the destructuring, or use it to compute includeArchived:
🔧 Proposed fix
- const { duration, date_range, only_single_member, archived } = req.query;
+ const { duration, date_range, only_single_member } = req.query;
const includeArchived = req.query.archived === "true";Or, alternatively, use the destructured variable and remove the redundant req.query access:
const { duration, date_range, only_single_member, archived } = req.query;
- const includeArchived = req.query.archived === "true";
+ const includeArchived = archived === "true";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { duration, date_range, only_single_member, archived } = req.query; | |
| const includeArchived = req.query.archived === "true"; | |
| const { duration, date_range, only_single_member } = req.query; | |
| const includeArchived = req.query.archived === "true"; |
Suggested change
| const { duration, date_range, only_single_member, archived } = req.query; | |
| const includeArchived = req.query.archived === "true"; | |
| const { duration, date_range, only_single_member, archived } = req.query; | |
| const includeArchived = archived === "true"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@worklenz-backend/src/controllers/reporting/overview/reporting-overview-export-controller.ts`
around lines 419 - 421, Remove the dead variable by either deleting `archived`
from the destructuring or use it to compute `includeArchived`; specifically
update the block where `const { duration, date_range, only_single_member,
archived } = req.query;` and `const includeArchived = req.query.archived ===
"true";` are defined so that `includeArchived` is derived from the destructured
`archived` (e.g., `includeArchived` computed from `archived`) or remove
`archived` from the destructure and keep the current `includeArchived`
assignment; modify the code around the `includeArchived` computation in
reporting-overview-export-controller.ts accordingly.
Comment on lines
+169
to
+210
| /** | ||
| * Build project filter clause for Team Leads | ||
| * Team Leads can only see projects they are assigned to as project managers | ||
| */ | ||
| public static async buildProjectFilterForTeamLead(req: IWorkLenzRequest): Promise<string> { | ||
| // Check if user is a Team Lead (not Admin or Owner) | ||
| const userId = req.user?.id; | ||
| const teamId = req.user?.team_id; | ||
|
|
||
| if (!userId || !teamId) return ""; | ||
|
|
||
| // Check user's role | ||
| const roleQuery = ` | ||
| SELECT r.key | ||
| FROM roles r | ||
| JOIN team_members tm ON tm.role_id = r.id | ||
| WHERE tm.user_id = $1 AND tm.team_id = $2 | ||
| `; | ||
| const roleResult = await db.query(roleQuery, [userId, teamId]); | ||
|
|
||
| if (roleResult.rows.length === 0) return ""; | ||
|
|
||
| const roleKey = roleResult.rows[0].key; | ||
|
|
||
| // Only apply filter for Team Leads | ||
| if (roleKey === 'TEAM_LEAD') { | ||
| // Team Leads can only see projects they manage | ||
| return `AND p.id IN ( | ||
| SELECT pm.project_id | ||
| FROM project_members pm | ||
| WHERE pm.team_member_id IN ( | ||
| SELECT id FROM team_members WHERE user_id = '${userId}' | ||
| ) | ||
| AND pm.project_access_level_id = ( | ||
| SELECT id FROM project_access_levels WHERE key = 'PROJECT_MANAGER' | ||
| ) | ||
| )`; | ||
| } | ||
|
|
||
| // Admins and Owners can see all projects | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
SQL injection: userId is interpolated directly into the returned clause.
Line 200 embeds userId via string interpolation (user_id = '${userId}'), bypassing parameterized queries. Although userId originates from the authenticated session, this contradicts the parameterization effort in this PR and is a security anti-pattern. If this method is ever called with unsanitized input, it becomes exploitable.
Return both a clause with placeholders and the corresponding params, similar to SqlHelper.buildInClause:
Proposed fix
- public static async buildProjectFilterForTeamLead(req: IWorkLenzRequest): Promise<string> {
+ public static async buildProjectFilterForTeamLead(req: IWorkLenzRequest, paramOffset: number): Promise<{ clause: string; params: any[] }> {
const userId = req.user?.id;
const teamId = req.user?.team_id;
- if (!userId || !teamId) return "";
+ if (!userId || !teamId) return { clause: "", params: [] };
const roleQuery = `
SELECT r.key
FROM roles r
JOIN team_members tm ON tm.role_id = r.id
WHERE tm.user_id = $1 AND tm.team_id = $2
`;
const roleResult = await db.query(roleQuery, [userId, teamId]);
- if (roleResult.rows.length === 0) return "";
+ if (roleResult.rows.length === 0) return { clause: "", params: [] };
const roleKey = roleResult.rows[0].key;
if (roleKey === 'TEAM_LEAD') {
- return `AND p.id IN (
+ return {
+ clause: `AND p.id IN (
SELECT pm.project_id
FROM project_members pm
WHERE pm.team_member_id IN (
- SELECT id FROM team_members WHERE user_id = '${userId}'
+ SELECT id FROM team_members WHERE user_id = $${paramOffset}
)
AND pm.project_access_level_id = (
SELECT id FROM project_access_levels WHERE key = 'PROJECT_MANAGER'
)
- )`;
+ )`,
+ params: [userId]
+ };
}
- return "";
+ return { clause: "", params: [] };
}Callers would then spread the returned params into their filterParams array and advance paramOffset accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@worklenz-backend/src/controllers/reporting/reporting-controller-base.ts`
around lines 169 - 210, buildProjectFilterForTeamLead currently injects userId
directly into the SQL fragment (user_id = '${userId}'), causing a SQL injection
risk; change the method (buildProjectFilterForTeamLead) to return a
parameterized clause plus its params (e.g. { clause: string, params: any[] })
instead of a raw string, replace the interpolated userId with a positional
placeholder ($n) and push userId into the returned params array, and ensure
callers spread those params into their filterParams and advance their
paramOffset (follow the pattern used by SqlHelper.buildInClause for parameter
numbering/offset handling).
Comment on lines
1408
to
+1410
| const archivedClause = includeArchived | ||
| ? "" | ||
| : `AND t.project_id NOT IN (SELECT project_id FROM archived_projects WHERE project_id = t.project_id AND archived_projects.user_id = '${req.user?.id}')`; | ||
| ? "" | ||
| : `AND t.project_id NOT IN (SELECT project_id FROM archived_projects WHERE project_id = t.project_id AND archived_projects.user_id = '${req.user?.id}')`; |
There was a problem hiding this comment.
SQL injection: req.user?.id is interpolated directly into the query string.
Other methods in this file (e.g., getMembers at Line 99, memberTimeLogsData at Line 1181) correctly use parameterized placeholders for the archived clause. This method bypasses that pattern and concatenates the user ID directly into SQL, which is a SQL injection vector.
Use a parameterized placeholder instead:
🔒 Proposed fix
- const archivedClause = includeArchived
- ? ""
- : `AND t.project_id NOT IN (SELECT project_id FROM archived_projects WHERE project_id = t.project_id AND archived_projects.user_id = '${req.user?.id}')`;
+ let archivedClause = "";
+ const archivedParams: any[] = [];
+ if (!includeArchived) {
+ archivedClause = `AND t.project_id NOT IN (SELECT project_id FROM archived_projects WHERE project_id = t.project_id AND archived_projects.user_id = $${paramOffset})`;
+ archivedParams.push(req.user?.id);
+ paramOffset++;
+ }Then append ...archivedParams into the queryParams array at Line 1477.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@worklenz-backend/src/controllers/reporting/reporting-members-controller.ts`
around lines 1408 - 1410, The archivedClause currently interpolates req.user?.id
directly causing SQL injection; change it to use a parameterized placeholder
(e.g., `AND t.project_id NOT IN (SELECT project_id FROM archived_projects WHERE
project_id = t.project_id AND archived_projects.user_id = $<n>)` or your
project's placeholder style) and push the corresponding user id value into
archivedParams, then append archivedParams into the method's queryParams (the
array used by the query) before executing; update references to archivedClause,
includeArchived, archivedParams and queryParams in
reporting-members-controller.ts so the user id is passed as a parameter rather
than concatenated.
Comment on lines
+1870
to
+1872
| const archivedClause = includeArchived | ||
| ? "" | ||
| : `AND projects.id NOT IN (SELECT project_id FROM archived_projects WHERE project_id = projects.id AND archived_projects.user_id = '${req.user?.id}')`; |
There was a problem hiding this comment.
SQL injection: same direct interpolation of req.user?.id as in getMemberTaskStats.
🔒 Proposed fix — use a parameterized query
- const archivedClause = includeArchived
- ? ""
- : `AND projects.id NOT IN (SELECT project_id FROM archived_projects WHERE project_id = projects.id AND archived_projects.user_id = '${req.user?.id}')`;
+ let archivedClause = "";
+ const archivedParams: any[] = [];
+ if (!includeArchived) {
+ archivedClause = `AND projects.id NOT IN (SELECT project_id FROM archived_projects WHERE project_id = projects.id AND archived_projects.user_id = $2)`;
+ archivedParams.push(req.user?.id);
+ }Then update the query execution at Line 1925:
- const result = await db.query(q, [team_member_id]);
+ const result = await db.query(q, [team_member_id, ...archivedParams]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@worklenz-backend/src/controllers/reporting/reporting-members-controller.ts`
around lines 1870 - 1872, The archivedClause builds SQL with direct
interpolation of req.user?.id causing SQL injection (same issue as in
getMemberTaskStats); change archivedClause to use a parameter placeholder (e.g.,
$/userId/ or ? depending on the DB library) instead of embedding req.user?.id,
then pass the actual req.user.id value in the parameter object/array when
executing the query (also update the query execution that concatenates
archivedClause so it receives the parameter), ensuring the parameter name
matches the placeholder and that getMemberTaskStats and this reporting query use
the same safe parameterized approach.
Comment on lines
+1925
to
+1952
| const result = await db.query(q, [team_member_id]); | ||
| const data = result.rows; | ||
|
|
||
| for (const row of data) { | ||
| row.estimated_time = int(row.estimated_time); | ||
| row.actual_time = int(row.actual_time); | ||
| row.estimated_time_string = this.convertMinutesToHoursAndMinutes(int(row.estimated_time)); | ||
| row.actual_time_string = this.convertSecondsToHoursAndMinutes(int(row.actual_time)); | ||
| row.days_left = this.getDaysLeft(row.end_date); | ||
| row.is_overdue = this.isOverdue(row.end_date); | ||
| if (row.days_left && row.is_overdue) { | ||
| row.days_left = row.days_left.toString().replace(/-/g, ""); | ||
| } | ||
| row.is_today = this.isToday(row.end_date); | ||
| if (row.project_manager) { | ||
| row.project_manager.name = row.project_manager.project_manager_info.name; | ||
| row.project_manager.avatar_url = row.project_manager.project_manager_info.avatar_url; | ||
| row.project_manager.color_code = getColor(row.project_manager.name); | ||
| } | ||
| row.project_health = row.health_name ? row.health_name : null; | ||
| } | ||
| row.project_health = row.health_name ? row.health_name : null; | ||
| } | ||
|
|
||
| const body = { | ||
| team_member_name: data[0].team_member_name, | ||
| projects: data | ||
| }; | ||
| const body = { | ||
| team_member_name: data[0].team_member_name, | ||
| projects: data | ||
| }; | ||
|
|
||
| return res.status(200).send(new ServerResponse(true, body)); | ||
| return res.status(200).send(new ServerResponse(true, body)); |
There was a problem hiding this comment.
Crash if the member has no projects: data[0] is undefined when result.rows is empty.
When a team member has no project memberships, data will be an empty array, causing data[0].team_member_name at Line 1948 to throw a TypeError. Add a guard before accessing the first element.
🐛 Proposed fix
const result = await db.query(q, [team_member_id]);
const data = result.rows;
+ if (!data.length) {
+ return res.status(200).send(new ServerResponse(true, { team_member_name: null, projects: [] }));
+ }
+
for (const row of data) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const result = await db.query(q, [team_member_id]); | |
| const data = result.rows; | |
| for (const row of data) { | |
| row.estimated_time = int(row.estimated_time); | |
| row.actual_time = int(row.actual_time); | |
| row.estimated_time_string = this.convertMinutesToHoursAndMinutes(int(row.estimated_time)); | |
| row.actual_time_string = this.convertSecondsToHoursAndMinutes(int(row.actual_time)); | |
| row.days_left = this.getDaysLeft(row.end_date); | |
| row.is_overdue = this.isOverdue(row.end_date); | |
| if (row.days_left && row.is_overdue) { | |
| row.days_left = row.days_left.toString().replace(/-/g, ""); | |
| } | |
| row.is_today = this.isToday(row.end_date); | |
| if (row.project_manager) { | |
| row.project_manager.name = row.project_manager.project_manager_info.name; | |
| row.project_manager.avatar_url = row.project_manager.project_manager_info.avatar_url; | |
| row.project_manager.color_code = getColor(row.project_manager.name); | |
| } | |
| row.project_health = row.health_name ? row.health_name : null; | |
| } | |
| row.project_health = row.health_name ? row.health_name : null; | |
| } | |
| const body = { | |
| team_member_name: data[0].team_member_name, | |
| projects: data | |
| }; | |
| const body = { | |
| team_member_name: data[0].team_member_name, | |
| projects: data | |
| }; | |
| return res.status(200).send(new ServerResponse(true, body)); | |
| return res.status(200).send(new ServerResponse(true, body)); | |
| const result = await db.query(q, [team_member_id]); | |
| const data = result.rows; | |
| if (!data.length) { | |
| return res.status(200).send(new ServerResponse(true, { team_member_name: null, projects: [] })); | |
| } | |
| for (const row of data) { | |
| row.estimated_time = int(row.estimated_time); | |
| row.actual_time = int(row.actual_time); | |
| row.estimated_time_string = this.convertMinutesToHoursAndMinutes(int(row.estimated_time)); | |
| row.actual_time_string = this.convertSecondsToHoursAndMinutes(int(row.actual_time)); | |
| row.days_left = this.getDaysLeft(row.end_date); | |
| row.is_overdue = this.isOverdue(row.end_date); | |
| if (row.days_left && row.is_overdue) { | |
| row.days_left = row.days_left.toString().replace(/-/g, ""); | |
| } | |
| row.is_today = this.isToday(row.end_date); | |
| if (row.project_manager) { | |
| row.project_manager.name = row.project_manager.project_manager_info.name; | |
| row.project_manager.avatar_url = row.project_manager.project_manager_info.avatar_url; | |
| row.project_manager.color_code = getColor(row.project_manager.name); | |
| } | |
| row.project_health = row.health_name ? row.health_name : null; | |
| } | |
| const body = { | |
| team_member_name: data[0].team_member_name, | |
| projects: data | |
| }; | |
| return res.status(200).send(new ServerResponse(true, body)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@worklenz-backend/src/controllers/reporting/reporting-members-controller.ts`
around lines 1925 - 1952, The code assumes data[0] exists and will crash when
result.rows is empty; update the logic around the db.query result handling (the
data variable and the body construction used with ServerResponse) to guard
against empty arrays: only iterate/transform rows if data.length > 0 and set
team_member_name to a safe default when data is empty (e.g., null or derived
from another source) instead of accessing data[0].team_member_name; ensure
downstream uses (projects: data) remain valid for an empty array.
Comment on lines
+42
to
+55
| if (search && paramOffset !== undefined) { | ||
| // Use parameterized queries when paramOffset is provided | ||
| const escapedSearch = search.replace(/'/g, "''"); | ||
|
|
||
|
|
||
| let s = ""; | ||
| if (typeof searchField === "string") { | ||
| s = ` ${searchField} ILIKE $${paramOffset}`; | ||
| searchParams.push(`%${escapedSearch}%`); | ||
| } else if (Array.isArray(searchField)) { | ||
| s = searchField.map((field, idx) => { | ||
| searchParams.push(`%${escapedSearch}%`); | ||
| return ` ${field} ILIKE $${paramOffset + idx}`; | ||
| }).join(" OR "); | ||
| } |
There was a problem hiding this comment.
Bug: Single-quote escaping breaks parameterized search containing apostrophes.
Line 44 applies search.replace(/'/g, "''") before passing the value as a query parameter. Parameterized queries handle escaping automatically via the database driver — the doubled single quotes will be sent literally, so a search for O'Brien becomes a parameter value %O''Brien%, which won't match the intended row.
Remove the escaping in the parameterized branch:
Proposed fix
if (search && paramOffset !== undefined) {
- // Use parameterized queries when paramOffset is provided
- const escapedSearch = search.replace(/'/g, "''");
-
let s = "";
if (typeof searchField === "string") {
- s = ` ${searchField} ILIKE $${paramOffset}`;
- searchParams.push(`%${escapedSearch}%`);
+ s = ` ${searchField} ILIKE $${paramOffset}`;
+ searchParams.push(`%${search}%`);
} else if (Array.isArray(searchField)) {
s = searchField.map((field, idx) => {
- searchParams.push(`%${escapedSearch}%`);
+ searchParams.push(`%${search}%`);
return ` ${field} ILIKE $${paramOffset + idx}`;
}).join(" OR ");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@worklenz-backend/src/controllers/worklenz-controller-base.ts` around lines 42
- 55, The parameterized-search branch is incorrectly pre-escaping single quotes
(escapedSearch = search.replace(/'/g, "''")) causing values like O'Brien to
become O''Brien when pushed into searchParams; in the branch where paramOffset
is defined (the code building s and pushing to searchParams), remove the
replacement and push the raw search value (e.g., `%${search}%`) into
searchParams for both the string and Array.isArray(searchField) cases so the DB
driver can handle escaping automatically; keep any existing escaping only for
the non-parameterized/raw-SQL code path.
Comment on lines
+24
to
+26
| const q = `INSERT INTO project_logs (team_id, project_id, user_id, description, log_type) | ||
| VALUES ($1, $2, $3, $4, 'project_created') | ||
| ON CONFLICT DO NOTHING`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the project_logs table schema for unique constraints
echo "=== Searching for project_logs table definition ==="
rg -n 'project_logs' --type=sql -C 3
echo ""
echo "=== Searching for project_logs in migration files ==="
fd -e sql --exec grep -l 'project_logs' {}
echo ""
echo "=== Searching for CREATE TABLE project_logs ==="
rg -nU 'CREATE TABLE.*project_logs' --type=sql -A 20Repository: Worklenz/worklenz
Length of output: 7773
🏁 Script executed:
#!/bin/bash
# Check for indexes on project_logs
echo "=== Searching for indexes on project_logs ==="
rg -n 'project_logs' --type=sql | grep -i 'index\|constraint'
echo ""
echo "=== Full schema extraction for project_logs ==="
sed -n '676,810p' worklenz-backend/database/sql/1_tables.sql
echo ""
echo "=== Check if user_id or log_type columns are added elsewhere ==="
rg -n 'ALTER TABLE project_logs' --type=sql -A 3Repository: Worklenz/worklenz
Length of output: 7749
🏁 Script executed:
#!/bin/bash
# View the actual code being reviewed
echo "=== Code being reviewed ==="
sed -n '20,35p' worklenz-backend/src/services/activity-logging.service.ts
echo ""
echo "=== Search for ALTER TABLE project_logs in migration files ==="
fd -e sql -exec grep -l 'ALTER TABLE project_logs' {} \;
echo ""
echo "=== Search for user_id and log_type in project_logs ==="
rg -n 'user_id|log_type' worklenz-backend/database/ --type=sql | grep -i 'project_logs\|ALTER'Repository: Worklenz/worklenz
Length of output: 1285
🏁 Script executed:
#!/bin/bash
# Search more carefully for any schema modifications to project_logs that add user_id or log_type
echo "=== Check all SQL files for user_id or log_type additions to project_logs ==="
rg 'ADD COLUMN.*user_id|ADD COLUMN.*log_type' worklenz-backend/database/ --type=sql
echo ""
echo "=== Check migration files for project_logs schema changes ==="
find worklenz-backend/database/migrations -name "*.sql" -exec grep -l 'project_logs' {} \;
echo ""
echo "=== View migration files content if any ==="
find worklenz-backend/database/migrations -name "*.sql" -exec grep -A 5 'ALTER TABLE project_logs\|CREATE TABLE project_logs' {} +
echo ""
echo "=== Full table schema from 1_tables.sql around project_logs ==="
sed -n '676,690p' worklenz-backend/database/sql/1_tables.sqlRepository: Worklenz/worklenz
Length of output: 1167
🏁 Script executed:
#!/bin/bash
# Double-check: search all SQL files for any reference to user_id or log_type in project_logs context
echo "=== Search entire database directory for user_id or log_type in project_logs ==="
rg -C 10 'project_logs.*user_id|project_logs.*log_type|user_id.*project_logs|log_type.*project_logs' worklenz-backend/database/ --type=sql
echo ""
echo "=== List all migration files with dates ==="
ls -la worklenz-backend/database/migrations/ | head -20
echo ""
echo "=== Check the two migration files that reference project_logs ==="
head -100 worklenz-backend/database/migrations/20250423000000-subtask-manual-progress.sql | tail -50Repository: Worklenz/worklenz
Length of output: 3973
🏁 Script executed:
#!/bin/bash
# Verify the exact error: the columns being inserted don't exist in the table
echo "=== Confirm project_logs table schema (final check) ==="
rg -A 10 'CREATE TABLE IF NOT EXISTS project_logs' worklenz-backend/database/sql/1_tables.sql
echo ""
echo "=== Verify no ALTER statements add user_id or log_type ==="
rg 'ADD COLUMN.*user_id|ADD COLUMN.*log_type' worklenz-backend/database/ --type=sql -C 3Repository: Worklenz/worklenz
Length of output: 697
The INSERT statement references columns that don't exist in the project_logs table.
The table only has columns: id, team_id, project_id, description, created_at. The code attempts to insert user_id and log_type, which are not defined in the schema and will cause a database error at runtime.
Additionally, even if those columns existed, the ON CONFLICT DO NOTHING clause would be ineffective because it only applies to PRIMARY KEY (id) conflicts. With an auto-generated UUID, this clause is meaningless and provides no deduplication. To prevent duplicate events, define a unique constraint on the relevant column combination (e.g., (project_id, log_type)).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@worklenz-backend/src/services/activity-logging.service.ts` around lines 24 -
26, The INSERT in activity-logging.service.ts (variable q) is inserting
non-existent columns user_id and log_type and using a meaningless ON CONFLICT;
fix by aligning the SQL with the actual schema: change the statement to INSERT
INTO project_logs (team_id, project_id, description) VALUES ($1, $2, $3) (omit
user_id and log_type) and remove the ON CONFLICT clause, or alternatively if you
need deduplication add a migration to create a log_type column and a UNIQUE
constraint (e.g., on (project_id, log_type)) and then update the INSERT to
include log_type and use ON CONFLICT (...) DO NOTHING. Ensure the parameter
ordering and the values supplied where q is used match the revised column list.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor