Development #365
Development #365
Conversation
…n across controllers - Create SqlHelper utility with buildInClause method to safely construct parameterized SQL IN clauses - Replace unsafe string concatenation (flatString) with parameterized queries in project-categories-controller - Add comprehensive color code validation supporting both base colors and shade variations - Implement name validation in project category update endpoint - Update color constant reference from WorklenzColorCodes to WorklenzColorShades - Apply consistent formatting and parameter binding across multiple controllers (project-templates, workload, projects, reporting, schedule, tasks, socket.io) - Prevent SQL injection vulnerabilities by eliminating manual string interpolation - Improve data validation to reject invalid color codes and empty category names
Advisory fix 1
📝 WalkthroughWalkthroughThis PR introduces a SqlHelper utility and migrates many controllers to parameterized SQL, replaces ad-hoc IN/LIKE construction, hardens queries against injection, adds activity logging and time-log export features, refactors scheduling/workload/task query logic, improves validations and transactions, and pins Docker images with multi-tag build support. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProjectsController
participant SqlHelper
participant Database
participant ActivityLoggingService
Client->>ProjectsController: POST /projects (create)
ProjectsController->>ProjectsController: validate name, color_code
ProjectsController->>SqlHelper: buildWhereClause / buildInClause
SqlHelper-->>ProjectsController: { clause, params }
ProjectsController->>Database: INSERT project (parameterized)
Database-->>ProjectsController: project_id
ProjectsController->>ActivityLoggingService: log creation (i18n key)
ActivityLoggingService->>Database: INSERT activity log (parameterized)
ProjectsController-->>Client: 201 { project }
sequenceDiagram
participant Client
participant ScheduleController
participant SqlHelper
participant Database
participant DataTransformer
Client->>ScheduleController: GET /schedule/:projectId?start=&end=&members=
ScheduleController->>ScheduleController: buildQuery (validate, offsets)
ScheduleController->>SqlHelper: buildInClause(members, paramOffset)
SqlHelper-->>ScheduleController: { clause, params }
ScheduleController->>Database: SELECT tasks/time_logs (parameterized)
Database-->>ScheduleController: rows
ScheduleController->>DataTransformer: buildGroupedResponse(rows)
DataTransformer-->>ScheduleController: { groups, allTasks }
ScheduleController-->>Client: 200 { groups, allTasks, totalTasks }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
worklenz-backend/src/controllers/project-workload/workload-gannt-controller.ts (1)
534-626:⚠️ Potential issue | 🔴 CriticalMember filter params are never passed to
db.query— runtime error when filter is active.
getQuery(Lines 540-550) buildsqueryParamswith member IDs and incrementsparamOffset, generating placeholders like$2, $3, …in the SQL. However,getQueryonly returns the SQL string, notqueryParams.In
getList(Line 688), the params array is constructed as[req.params.id || null](just$1). The member-filter placeholders in the SQL reference$2,$3, etc. that have no corresponding values, causing a PostgreSQL error.The same issue exists in
getTasksOnly(Line 750).
getQueryneeds to return the accumulatedqueryParams(similar to howtasks-controller-v2.tsreturns{ query, params, isSubTasks }), and the callers need to merge them:Suggested approach
- private static getQuery(userId: string, options: ParsedQs) { + private static getQuery(userId: string, options: ParsedQs): { query: string; params: any[] } { const queryParams: any[] = []; let paramOffset = 1; // ... build query ... - return `SELECT ...`; + return { query: `SELECT ...`, params: queryParams }; }Then in callers:
- const q = WorkloadGanntController.getQuery(...); - const params = isSubTasks ? [req.params.id || null, req.query.parent_task] : [req.params.id || null]; + const { query: q, params: queryParams } = WorkloadGanntController.getQuery(...); + const params = isSubTasks + ? [req.params.id || null, req.query.parent_task, ...queryParams] + : [req.params.id || null, ...queryParams];Also applies to: 682-691
worklenz-backend/src/controllers/tasks-controller-v2.ts (1)
1360-1427:⚠️ Potential issue | 🔴 Critical
DO $$blocks do not support parameterized queries —$1won't bind.PostgreSQL anonymous
DOblocks cannot receive parameters. The$1on Line 1368 inside theDO $$block will not be bound to the[projectId]argument passed on Line 1423. This will cause a runtime error.You need to either:
- Convert this to a stored function/procedure that accepts a parameter, or
- Use a prepared function call, or
- Restructure as separate parameterized queries outside a DO block.
Option: Replace DO block with sequential parameterized statements
- const query = ` - DO $$ - DECLARE - v_project_id UUID := $1; - BEGIN - -- First, reset manual_progress flag ... - UPDATE tasks AS t - SET manual_progress = FALSE - WHERE project_id = v_project_id - ... - END $$; - `; - await db.query(query, [projectId]); + // Step 1: Reset manual_progress for tasks with subtasks + await db.query(` + UPDATE tasks AS t + SET manual_progress = FALSE + WHERE project_id = $1 + AND EXISTS ( + SELECT 1 FROM tasks WHERE parent_task_id = t.id AND archived IS FALSE + ) + `, [projectId]); + + // Step 2: Recalculate progress values bottom-up + await db.query(` + WITH RECURSIVE task_hierarchy AS ( + SELECT id, parent_task_id, 0 AS level + FROM tasks + WHERE project_id = $1 + AND NOT EXISTS ( + SELECT 1 FROM tasks AS sub WHERE sub.parent_task_id = tasks.id AND sub.archived IS FALSE + ) + AND archived IS FALSE + UNION ALL + SELECT t.id, t.parent_task_id, th.level + 1 + FROM tasks t + JOIN task_hierarchy th ON t.id = th.parent_task_id + WHERE t.archived IS FALSE + ) + UPDATE tasks + SET progress_value = (SELECT (get_task_complete_ratio(tasks.id)->>'ratio')::FLOAT) + FROM (SELECT id FROM task_hierarchy ORDER BY level) AS ordered_tasks + WHERE tasks.id = ordered_tasks.id + AND tasks.project_id = $1 + AND (manual_progress IS FALSE OR manual_progress IS NULL) + `, [projectId]);worklenz-backend/src/controllers/projects-controller.ts (3)
155-205:⚠️ Potential issue | 🟠 MajorRemaining SQL injection:
req.user?.idstill interpolated directly in inner SELECT.Lines 159 and 163 still use direct string interpolation:
WHERE user_id = '${req.user?.id}'This is the exact pattern the PR is removing elsewhere. While
req.user?.idcomes from the session (not direct user input), it's inconsistent with the PR's goals and still a risk if session data is ever compromised.The same issue exists in
get()at Lines 378, 382, and 420 ('${req.user?.team_member_id}').These should all use parameterized placeholders to be consistent with the rest of the refactoring.
1027-1213:⚠️ Potential issue | 🟡 Minor
getGroupedinner query uses.replace("projects.", "p2.")which is fragile.Lines 1178-1186 apply
.replace("projects.", "p2.")to multiple clause strings (categories, statuses, isArchived, isFavorites, filterByMember, searchQuery). JavaScript'sString.replaceonly replaces the first occurrence. If any clause containsprojects.more than once, subsequent occurrences won't be replaced, producing invalid SQL.Use
replaceAll(or a regex with the global flag):- ${categories.replace("projects.", "p2.")} + ${categories.replaceAll("projects.", "p2.")}Or ensure the clause strings never contain
projects.(which currently they don't, but this is fragile for future changes).
813-871:⚠️ Potential issue | 🔴 CriticalFix parameter offset arithmetic in
getAllTasks—userIdis incorrectly mapped to$1(team_id).The issue stems from
searchParamsalways being an empty array (thetoPaginationOptionsmethod does not returnsearchParams, onlysearchQuery). This causessearchParams.lengthto always be0.Lines 818 and 825 compute
userIdParam = searchParams.length + 1 = 0 + 1 = 1, mapping userId to$1. However, the actual query parameters (line 866-870) are:
$1=team_id$2=userId(when filterByMember or assignedToMe is true)$3=size$4=offsetThe code incorrectly uses
$1for userId (lines 818, 831) and miscalculates limitParam (line 835) as0 + (filterByMember ? 2 : 1) + 1, which doesn't align with actual positions. Use fixed parameter positions instead:Suggested fix
- const userIdParam = searchParams.length + 1; + const userIdParam = 2; - const limitParam = searchParams.length + (filterByMember ? 2 : 1) + 1; + const limitParam = 3;worklenz-backend/src/controllers/reporting/reporting-members-controller.ts (2)
940-998:⚠️ Potential issue | 🔴 CriticalSQL injection:
teamMemberId,teamId, anduserIdare interpolated directly into SQL.
getMemberProjectsDatastill uses string interpolation for critical identifiers:
- Line 943:
team_member_id = '${teamMemberId as string}'- Line 948:
'${teamMemberId}'- Line 950:
user_id = '${userId}'These bypass the parameterization effort entirely. Since
teamMemberIdandteamIdoriginate fromreq.query, they are attacker-controlled.
1388-1391:⚠️ Potential issue | 🟠 MajorSQL injection:
req.user?.idinterpolated in archived clause.Line 1390 uses
'${req.user?.id}'in the SQL string, same pattern as thegetMembersmethod. This should be parameterized for consistency with the refactoring effort.
🤖 Fix all issues with AI agents
In `@worklenz-backend/src/controllers/project-categories-controller.ts`:
- Around line 96-108: In getByMultipleTeams, guard against empty teamIds (result
of getTeamsByOrg) before calling SqlHelper.buildInClause: if teamIds.length ===
0 return res.status(200).send(new ServerResponse(true, [])); otherwise build the
clause and run the query; this prevents generating the invalid SQL "WHERE
team_id IN ()" (symbols: getByMultipleTeams, getTeamsByOrg, teamIds,
SqlHelper.buildInClause, q, db.query, ServerResponse).
- Around line 115-144: The update handler currently calls
req.body.color.toLowerCase() without checking type and also uses a different
field name than create; fix by accepting the same field used in create
(req.body.color_code) or normalizing both names, first ensure color exists and
is a string (e.g., const colorRaw = typeof req.body.color === "string" ?
req.body.color : req.body.color_code; if (!colorRaw) return 400), then compute
const color = colorRaw.trim().toLowerCase() and use that when validating against
WorklenzColorShades and when binding the query parameters (replace
req.body.color with the normalized color or original color_code as appropriate);
update references in this function (WorklenzColorShades check and db.query
parameter array) to use the normalized variable.
In `@worklenz-backend/src/controllers/project-templates/pt-tasks-controller.ts`:
- Around line 37-42: Remove the dead helper and its unused import: delete the
private static method getFilterByTemplatsWhereClosure from the
pt-tasks-controller class and remove the corresponding SqlHelper import since
SqlHelper is only referenced by that method; ensure no other code references
getFilterByTemplatsWhereClosure and run tests/TS compile to confirm no remaining
usages.
In
`@worklenz-backend/src/controllers/project-workload/workload-gannt-controller.ts`:
- Around line 332-334: In the for-loop over result.rows in
workload-gannt-controller.ts, getColor is being called with member.TaskName
which is undefined because the SQL selects name AS name; change the call in the
loop (the code around "for (const member of result.rows)") to pass member.name
to getColor instead of member.TaskName so getColor receives the actual task
name.
- Around line 469-491: getTaskDateRangeFilter and getLogDateRangeFilter (and the
other inline SQL that interpolates start_date/end_date) currently inject
user-controlled startDate/endDate directly into SQL; change them to build
parameterized filters (e.g., return "AND start_date >= $n" / "AND
twl.created_at::date <= $n" or named placeholders) and instead of returning raw
SQL strings, also return or accept an array to which the corresponding date
values are pushed so the caller can supply them as query parameters; update the
callers that currently interpolate start_date/end_date inline to use these
parameterized filters and pass the accumulated params to the DB query (ensure
the functions getTaskDateRangeFilter and getLogDateRangeFilter signatures are
adjusted to accept/return parameters or a params collector and remove any direct
string interpolation of req.query values).
In `@worklenz-backend/src/controllers/projects-controller.ts`:
- Around line 656-690: The activity log is written before the actual DELETE
executes in deleteById, so failures (e.g., FK constraints) produce false
deletion logs; move the ActivityLoggingService.logProjectDeleted call to after
the DELETE and only call it when the delete succeeded (check the delete query
result, e.g., result.rowCount or equivalent), preserving the same parameters
(team_id, project id, user id, project.name); optionally wrap the
SELECT+DELETE+log in a transaction if atomicity is required.
- Around line 236-274: validateAndMapSortField currently accepts dot-qualified
column names by short-circuiting to a sanitized regex check (see sortField and
sanitized), which bypasses the allowlist in fieldMapping; remove or disable the
dot-handling path so only mapped keys in fieldMapping are returned. Concretely,
delete or bypass the block that checks sortField.includes('.') / sanitized and
instead only map via fieldMapping[sortField]; if you must allow qualified names,
add explicit entries in fieldMapping (e.g., "projects.name": "projects.name")
and validate against that explicit whitelist rather than permitting arbitrary
dotted identifiers. Ensure the final fallback still returns
fieldMapping[defaultField] || 'name'.
In
`@worklenz-backend/src/controllers/reporting/projects/reporting-projects-controller.ts`:
- Around line 344-351: The "health" group case in
reporting-projects-controller.ts assigns groupJoin = "LEFT JOIN
sys_project_healths sph ON p.health_id = sph.id" which duplicates the
unconditional join already added for sph (used in the JSON_BUILD_OBJECT); remove
that assignment (or set groupJoin to an empty string) inside the case "health"
block so the sph table is only joined once, keeping groupField, groupName,
groupColor, groupByFields and groupOrderBy intact.
In `@worklenz-backend/src/controllers/reporting/reporting-members-controller.ts`:
- Around line 88-92: The archivedClause currently interpolates userId directly
into the SQL string (archivedClause, created near pagingClause), so change it to
use a parameter placeholder and add userId to queryParams using the current
paramOffset logic (increment paramOffset to compute the $N index), e.g. replace
`'${userId}'` with `$<paramIndex>` in archivedClause and push userId onto
queryParams where other query parameters are assembled (matching the pattern
used in reporting-projects-controller.ts); ensure any other occurrences around
lines 223/232 are updated the same way so all SQL uses parameterized userId
rather than string interpolation.
- Around line 1501-1537: Remove the dead assignment to rawDurationClause (the
call to this.getDateRangeClauseWithTimezone(...)) since its result is never
used; instead, keep the explicit fallback branch but amend each fallback
duration clause in getTimelogsFlat so they include an explicit upper-bound to
avoid including future-dated rows (mirror getDateRangeClauseMembers behavior).
Concretely, update the branches that set durationClause for
DATE_RANGES.LAST_WEEK, YESTERDAY, LAST_MONTH, LAST_QUARTER to add a "<
(CURRENT_DATE + INTERVAL '1 day')::TIMESTAMP" (or an equivalent upper-bound)
where appropriate so each clause uses both start and end bounds, and remove the
unused rawDurationClause variable and its call to
getDateRangeClauseWithTimezone.
- Around line 625-627: The header merge ranges currently extend to column M but
two new columns make the sheet span A–K (11 columns) — update the
worksheet.mergeCells calls that reference ranges ending in ":M1", ":M2" (and the
similar merge at the third header/summary) to use ":K1", ":K2", ":K3" (or the
appropriate row numbers) so the merged ranges match the actual columns created
by the keys billable_time and non_billable_time; search for mergeCells(...) in
reporting-members-controller.ts and replace the M-based ranges with K-based
ranges for all header/summary merges.
- Around line 1501-1608: getTimelogsFlat duplicates the SQL/parameter-building
logic found in getTimelogsFlatData; refactor getTimelogsFlat to call
getTimelogsFlatData instead of re-building the query. Specifically, remove the
inline parameter/SQL construction in getTimelogsFlat and call
ReportingMembersController.getTimelogsFlatData(req) (or adjust signature to
accept the same inputs: req.user?.id, req.body fields, and userTimezone) to
obtain the rows, then reuse the existing grouping/formatting and ServerResponse
creation; ensure getTimelogsFlatData returns raw rows (or adapt its return
shape) so secondsToReadable and the grouping code can operate unchanged and
preserve billable/team/member/search/duration behavior.
- Around line 93-119: paramOffset is computed as 2 + searchParams.length but
never accounts for teamIdsParams, causing placeholder collisions for downstream
clauses (assignClause, completedDurationClause, getActivityLogsOverdue,
getActivityLogsCreationClause, getTimeLogDateRangeClause). Fix by including the
number of teamIds parameters in the offset (either initialize paramOffset = 2 +
searchParams.length + teamIdsParams.length after teamsClause is built, or
immediately add paramOffset += teamIdsParams.length once teamIdsParams is
available) so that all calls creating clauses (memberAssignDurationFilter,
completedDurationFilter, getActivityLogsOverdue, getActivityLogsCreationClause,
getTimeLogDateRangeClause) receive the correct starting parameter index
consistent with the order in queryParams.
In `@worklenz-backend/src/controllers/schedule/schedule-controller.ts`:
- Around line 1055-1075: The task-distribution loop and updateMapByGroup are
missing handling for GroupBy.LABELS so tasks with labels end up in unmappedTasks
and label groups remain empty; update the distribution in the loop that builds
groupedResponse (where GroupBy.STATUS/PRIORITY/PHASE are checked) to add a
branch for GroupBy.LABELS that iterates task.labels (or task.label_ids) and
checks membership against groupedResponse keys, pushing the task into each
matching groupedResponse[labelId].tasks and setting taskAssigned=true; similarly
add the same labels-aware branch in updateMapByGroup so label groups receive
tasks based on their label associations (refer to getGroups(), GroupBy.LABELS,
groupedResponse, unmappedTasks, and updateMapByGroup to locate the spots to
change).
In `@worklenz-backend/src/controllers/tasks-controller-v2.ts`:
- Around line 468-501: The subtask filter IN clauses compute parameter start
positions by subtracting lengths from paramOffset, which miscalculates offsets;
change to record each filter's starting parameter index when you first build
that filter (e.g., capture priorityParamStart, labelParamStart, memberParamStart
at the moment you construct priorityResult.params / labelsResult.params /
membersResult.params) or recompute starts from scratch like the
hasFilteredChildren block does, then pass those recorded indices into
SqlHelper.buildInClause for subtaskFilters; update references to paramOffset in
the subtask block (and use the same unique symbols: subtaskFilters, paramOffset,
priorityResult, labelsResult, membersResult, SqlHelper.buildInClause) so bind
positions align with the original filter parameter ordering.
- Line 1: The file begins with a UTF-8 BOM character before the import causing
tooling issues; remove the invisible U+FEFF at the start of the file (i.e.,
delete the character immediately preceding the `import { ParsedQs } from "qs";`
line), ensure the file is saved as UTF-8 without BOM, and re-run
formatting/linting to confirm no remaining BOMs (or use your editor/IDE's "Save
with encoding: UTF-8 without BOM" option).
In `@worklenz-backend/src/shared/sql-helpers.ts`:
- Around line 236-242: The flatString function is vulnerable because it naively
interpolates values with `'${s}'` and doesn't escape single quotes; instead of
returning unsafe output, change flatString to immediately throw an Error (or
TypeError) indicating it's deprecated and unsafe and instructing callers to use
SqlHelper.buildInClause(), so callers are forced to migrate; update references
to flatString if needed and keep the console warning removal/replacement with
the thrown error in the flatString function.
- Around line 32-42: The buildInClause helper currently returns an empty clause
when values is empty which can produce a syntax error when callers interpolate
it into "IN (${clause})"; change buildInClause (the static method buildInClause)
to return a syntactically safe sentinel when values is empty (e.g., return {
clause: "NULL", params: [] } so callers using `IN (${clause})` become `IN
(NULL)` instead of `IN ()`), and update any related callsites or comments if you
prefer to throw on empty input instead (ensure callers like
getFilterByMembersWhereClosure and getByMultipleTeams still work as intended).
In `@worklenz-backend/src/socket.io/commands/on-task-timer-stop.ts`:
- Around line 25-61: The transaction is using db.query() (a pg.Pool) which can
check out different connections; change to acquire a dedicated client via
db.connect() (e.g., const client = await db.connect()) and run
client.query("BEGIN"), then execute the timerQuery and deleteQuery using
client.query(...), then client.query("COMMIT") on success and
client.query("ROLLBACK") on error, finally always call client.release() in a
finally block; update references in on-task-timer-stop.ts so timerQuery and
deleteQuery are executed on the same client rather than db.query().
🧹 Nitpick comments (10)
worklenz-backend/src/shared/sql-helpers.ts (3)
54-93:fieldandoperatorare interpolated without sanitization — document the trust boundary.
condition.fieldandcondition.operatorare interpolated directly into the SQL string (Lines 77, 81, 83). This is safe only when callers supply hardcoded field/operator values from code. If any caller ever passes user-controlled strings here, it becomes an injection vector.At minimum, add a JSDoc warning. Ideally, validate
operatoragainst an allow-list:Suggested operator validation
+ const ALLOWED_OPERATORS = new Set(["=", "!=", "<>", "<", ">", "<=", ">=", "IN", "IS NULL", "IS NOT NULL", "LIKE", "ILIKE"]); + conditions.forEach((condition, index) => { + if (!ALLOWED_OPERATORS.has(condition.operator.toUpperCase())) { + throw new Error(`Invalid SQL operator: ${condition.operator}`); + } const conjunction = index === 0 ? "" : ` ${condition.conjunction || "AND"} `;
185-193:escapeIdentifierrejects schema-qualified names (e.g.,schema.table_name).The regex
/^[a-zA-Z_][a-zA-Z0-9_]*$/rejects dots, so callers cannot passpublic.tasksor similar. This is fine as a deliberate restriction, but worth noting sincebuildUpdateQuerymight eventually need qualified table names. If that's intentional, consider adding a brief doc note.
198-227:tableinbuildUpdateQueryis interpolated raw — useescapeIdentifier.Line 218 interpolates
tabledirectly into the SQL string without any validation or escaping, despiteescapeIdentifierbeing available in the same class. Similarly, SET field names (line 215) are not escaped.Suggested fix
- let query = `UPDATE ${table} SET ${setClauses.join(", ")}`; + let query = `UPDATE ${SqlHelper.escapeIdentifier(table)} SET ${setClauses.join(", ")}`;And for SET fields:
const setClauses = setEntries.map(([field, value]) => { params.push(value); - return `${field} = $${paramOffset++}`; + return `${SqlHelper.escapeIdentifier(field)} = $${paramOffset++}`; });worklenz-backend/src/controllers/project-categories-controller.ts (1)
25-57: Duplicated color validation logic betweencreateandupdate.The color validation (building
validColorsarray fromWorklenzColorShades) is duplicated betweencreate(Lines 30-42) andupdate(Lines 116-122). Consider extracting a shared helper:private static isValidColor(color: string): boolean { const validColors = [ ...Object.keys(WorklenzColorShades), ...Object.values(WorklenzColorShades).flat(), ].map(c => c.toLowerCase()); return validColors.includes(color.toLowerCase()); }Also, in
create, theallColorsarray (Lines 38-41) is reconstructed just to find the original-case version. You could simplify:Suggested refactor
- // Validate color - accept both base colors and all shade variations - const validColors = [ - ...Object.keys(WorklenzColorShades), - ...Object.values(WorklenzColorShades).flat(), - ].map((c) => c.toLowerCase()); - - const providedColor = req.body.color_code.trim().toLowerCase(); - if (validColors.includes(providedColor)) { - // Find the original case color from the valid colors - const allColors = [ - ...Object.keys(WorklenzColorShades), - ...Object.values(WorklenzColorShades).flat(), - ]; - colorCode = allColors.find(c => c.toLowerCase() === providedColor) || providedColor; + const allColors = [ + ...Object.keys(WorklenzColorShades), + ...Object.values(WorklenzColorShades).flat(), + ]; + const providedColor = req.body.color_code.trim().toLowerCase(); + const matched = allColors.find(c => c.toLowerCase() === providedColor); + if (matched) { + colorCode = matched;worklenz-backend/src/controllers/tasks-controller-v2.ts (1)
55-108: Recursive CTEs in correlated subqueries — potential performance concern.Each of
getFilterByPriorityWhereClosure,getFilterByLabelsWhereClosure, andgetFilterByMembersWhereClosuregenerates anEXISTS (WITH RECURSIVE ...)subquery that's evaluated per row in the outer query. For large task tables, this means a recursive CTE is executed for every task being evaluated, which can be very expensive.Consider materializing the set of matching ancestor task IDs via a single CTE at the top of the query instead:
WITH RECURSIVE matching_ancestors AS ( SELECT DISTINCT parent_task_id AS id FROM tasks WHERE priority_id IN (...) AND archived IS FALSE UNION SELECT t.parent_task_id FROM tasks t JOIN matching_ancestors ma ON t.id = ma.id WHERE t.parent_task_id IS NOT NULL AND t.archived IS FALSE ) SELECT ... FROM tasks t WHERE t.priority_id IN (...) OR t.id IN (SELECT id FROM matching_ancestors)This evaluates the recursion once rather than per-row.
worklenz-backend/src/controllers/reporting/projects/reporting-projects-controller.ts (2)
259-456: Significant code duplication betweenget()andgetGrouped().The filter-building logic (lines 272–326) is nearly identical to
get()(lines 27–81), differing only in the initialparamOffset. Consider extracting the shared filter construction into a private helper method to reduce maintenance burden and divergence risk.
360-366: Performance concern: repeated correlated subqueries inmanagerGROUP BY.The
groupField,groupName, andgroupByFieldsfor the"manager"case each contain multiple correlated subqueries that hitproject_members,project_access_levels, andteam_member_info_viewper row. These same subqueries are also repeated ingroupByFields(line 364). For large datasets, this will be very expensive.Consider using a CTE or an additional
LEFT JOIN(e.g., joiningproject_membersfiltered toPROJECT_MANAGERaccess level) to compute the manager once and reference it inGROUP BY,SELECT, andORDER BY.worklenz-backend/src/controllers/schedule/schedule-controller.ts (2)
846-857: Date range overlap filter is logically correct but has a subtle readability issue.The parameters are pushed in reverse order (
endDatefirst at line 854, thenstartDateat line 855), while the SQL placeholders read$paramOffsetthen$paramOffset + 1. The overlap logicstart_date <= endDate AND end_date >= startDateis correct, but the swapped push order may confuse future maintainers.Consider naming the variables or reordering to match the placeholder order:
♻️ Suggested improvement for clarity
dateRangeFilter = `( (start_date IS NOT NULL AND end_date IS NOT NULL) AND - (start_date <= $${paramOffset} AND end_date >= $${paramOffset + 1}) + (start_date <= $${paramOffset}::DATE AND end_date >= $${paramOffset + 1}::DATE) )`; - queryParams.push(options.endDate as string); - queryParams.push(options.startDate as string); + queryParams.push(options.endDate as string); // $paramOffset: range end + queryParams.push(options.startDate as string); // $paramOffset+1: range start
371-393: The date union subquery block is duplicated 4 times across this file.The same date union subquery (computing
min_date/max_datefromproject_member_allocationsandtasks) appears at lines 371–393, 496–518, 556–578, and 620–642. Extract this into a reusable SQL fragment builder or a database view/function to reduce maintenance overhead and risk of the copies drifting apart.worklenz-backend/src/controllers/reporting/reporting-members-controller.ts (1)
1203-1210: Archived clause parameter index calculation looks correct but is fragile.Line 1208 computes the archived parameter position as
$${params.length + 3}, relying on the assumption thatparamsdoesn't change after this point and that$1=team_id,$2=team_member_id. This is correct for the current call sites but is brittle — any future caller passing different base params will break it.Consider accepting
paramOffsetexplicitly (like the other filter methods) rather than computing it fromparams.length.
| // Validate color - accept both base colors and all shade variations | ||
| const validColors = [ | ||
| ...Object.keys(WorklenzColorShades), | ||
| ...Object.values(WorklenzColorShades).flat(), | ||
| ].map((c) => c.toLowerCase()); | ||
| if (!validColors.includes(req.body.color.toLowerCase())) { | ||
| return res.status(400).send(new ServerResponse(false, "Invalid color")); | ||
| } | ||
|
|
||
| // Validate name | ||
| const name = | ||
| typeof req.body.name === "string" ? req.body.name.trim() : undefined; | ||
| if (!name || name.length === 0) { | ||
| return res | ||
| .status(400) | ||
| .send(new ServerResponse(false, "Category name is required.")); | ||
| } | ||
|
|
||
| const q = ` | ||
| UPDATE project_categories | ||
| SET color_code = $2 | ||
| SET name = $2, color_code = $3 | ||
| WHERE id = $1 | ||
| AND team_id = $3; | ||
| AND team_id = $4; | ||
| `; | ||
|
|
||
| if (!WorklenzColorCodes.includes(req.body.color)) | ||
| return res.status(400).send(new ServerResponse(false, null)); | ||
|
|
||
| const result = await db.query(q, [req.params.id, req.body.color, req.user?.team_id]); | ||
| const result = await db.query(q, [ | ||
| req.params.id, | ||
| name, | ||
| req.body.color, | ||
| req.user?.team_id, | ||
| ]); |
There was a problem hiding this comment.
Missing null/type check on req.body.color — will throw on undefined.
Line 120 calls req.body.color.toLowerCase() without checking if req.body.color exists or is a string. If the client omits the color field, this throws a TypeError.
Also note the field name inconsistency: create() reads req.body.color_code (Line 28) while update() reads req.body.color (Line 120). This means the client must use different field names for create vs. update.
Suggested fix
+ if (!req.body.color || typeof req.body.color !== 'string') {
+ return res.status(400).send(new ServerResponse(false, "Color is required"));
+ }
+
const validColors = [
...Object.keys(WorklenzColorShades),
...Object.values(WorklenzColorShades).flat(),
].map((c) => c.toLowerCase());
if (!validColors.includes(req.body.color.toLowerCase())) {
return res.status(400).send(new ServerResponse(false, "Invalid color"));
}📝 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.
| // Validate color - accept both base colors and all shade variations | |
| const validColors = [ | |
| ...Object.keys(WorklenzColorShades), | |
| ...Object.values(WorklenzColorShades).flat(), | |
| ].map((c) => c.toLowerCase()); | |
| if (!validColors.includes(req.body.color.toLowerCase())) { | |
| return res.status(400).send(new ServerResponse(false, "Invalid color")); | |
| } | |
| // Validate name | |
| const name = | |
| typeof req.body.name === "string" ? req.body.name.trim() : undefined; | |
| if (!name || name.length === 0) { | |
| return res | |
| .status(400) | |
| .send(new ServerResponse(false, "Category name is required.")); | |
| } | |
| const q = ` | |
| UPDATE project_categories | |
| SET color_code = $2 | |
| SET name = $2, color_code = $3 | |
| WHERE id = $1 | |
| AND team_id = $3; | |
| AND team_id = $4; | |
| `; | |
| if (!WorklenzColorCodes.includes(req.body.color)) | |
| return res.status(400).send(new ServerResponse(false, null)); | |
| const result = await db.query(q, [req.params.id, req.body.color, req.user?.team_id]); | |
| const result = await db.query(q, [ | |
| req.params.id, | |
| name, | |
| req.body.color, | |
| req.user?.team_id, | |
| ]); | |
| // Validate color - accept both base colors and all shade variations | |
| if (!req.body.color || typeof req.body.color !== 'string') { | |
| return res.status(400).send(new ServerResponse(false, "Color is required")); | |
| } | |
| const validColors = [ | |
| ...Object.keys(WorklenzColorShades), | |
| ...Object.values(WorklenzColorShades).flat(), | |
| ].map((c) => c.toLowerCase()); | |
| if (!validColors.includes(req.body.color.toLowerCase())) { | |
| return res.status(400).send(new ServerResponse(false, "Invalid color")); | |
| } | |
| // Validate name | |
| const name = | |
| typeof req.body.name === "string" ? req.body.name.trim() : undefined; | |
| if (!name || name.length === 0) { | |
| return res | |
| .status(400) | |
| .send(new ServerResponse(false, "Category name is required.")); | |
| } | |
| const q = ` | |
| UPDATE project_categories | |
| SET name = $2, color_code = $3 | |
| WHERE id = $1 | |
| AND team_id = $4; | |
| `; | |
| const result = await db.query(q, [ | |
| req.params.id, | |
| name, | |
| req.body.color, | |
| req.user?.team_id, | |
| ]); |
🤖 Prompt for AI Agents
In `@worklenz-backend/src/controllers/project-categories-controller.ts` around
lines 115 - 144, The update handler currently calls req.body.color.toLowerCase()
without checking type and also uses a different field name than create; fix by
accepting the same field used in create (req.body.color_code) or normalizing
both names, first ensure color exists and is a string (e.g., const colorRaw =
typeof req.body.color === "string" ? req.body.color : req.body.color_code; if
(!colorRaw) return 400), then compute const color =
colorRaw.trim().toLowerCase() and use that when validating against
WorklenzColorShades and when binding the query parameters (replace
req.body.color with the normalized color or original color_code as appropriate);
update references in this function (WorklenzColorShades check and db.query
parameter array) to use the normalized variable.
| private static getFilterByTemplatsWhereClosure(text: string, paramOffset: number): { clause: string; params: string[] } { | ||
| if (!text) return { clause: "", params: [] }; | ||
| const templateIds = text.split(" ").filter(id => id.trim()); | ||
| const { clause } = SqlHelper.buildInClause(templateIds, paramOffset); | ||
| return { clause: `template_id IN (${clause})`, params: templateIds }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all usages of getFilterByTemplatsWhereClosure in the entire codebase
rg -n "getFilterByTemplatsWhereClosure" --type tsRepository: Worklenz/worklenz
Length of output: 263
🏁 Script executed:
#!/bin/bash
# Read the file to check other usages of SqlHelper and file context
head -50 worklenz-backend/src/controllers/project-templates/pt-tasks-controller.ts | cat -nRepository: Worklenz/worklenz
Length of output: 2476
🏁 Script executed:
#!/bin/bash
# Check total file length and search for all SqlHelper usages
wc -l worklenz-backend/src/controllers/project-templates/pt-tasks-controller.ts
echo "---"
rg -n "SqlHelper" worklenz-backend/src/controllers/project-templates/pt-tasks-controller.tsRepository: Worklenz/worklenz
Length of output: 279
Remove unreferenced method and unused import.
The getFilterByTemplatsWhereClosure method (lines 37-42) is never called anywhere in the codebase and qualifies as dead code. Additionally, the SqlHelper import on line 8 is only used within this unreferenced method and should also be removed.
🤖 Prompt for AI Agents
In `@worklenz-backend/src/controllers/project-templates/pt-tasks-controller.ts`
around lines 37 - 42, Remove the dead helper and its unused import: delete the
private static method getFilterByTemplatsWhereClosure from the
pt-tasks-controller class and remove the corresponding SqlHelper import since
SqlHelper is only referenced by that method; ensure no other code references
getFilterByTemplatsWhereClosure and run tests/TS compile to confirm no remaining
usages.
worklenz-backend/src/controllers/project-workload/workload-gannt-controller.ts
Show resolved
Hide resolved
| private static getTaskDateRangeFilter(startDate?: string, endDate?: string): string { | ||
| if (!startDate && !endDate) return ""; | ||
| const conditions: string[] = []; | ||
| if (startDate) { | ||
| conditions.push(`start_date >= '${startDate}'`); | ||
| } | ||
| if (endDate) { | ||
| conditions.push(`end_date <= '${endDate}'`); | ||
| } | ||
| return closure; | ||
| return conditions.length > 0 ? `AND ${conditions.join(" AND ")}` : ""; | ||
| } | ||
|
|
||
| private static getFilterByMembersWhereClosure(text: string) { | ||
| return text | ||
| ? `id IN (SELECT task_id FROM tasks_assignees WHERE team_member_id IN (${this.flatString(text)}))` | ||
| : ""; | ||
| private static getLogDateRangeFilter(startDate?: string, endDate?: string): string { | ||
| if (!startDate && !endDate) return ""; | ||
| const conditions: string[] = []; | ||
| if (startDate) { | ||
| conditions.push(`twl.created_at::date >= '${startDate}'`); | ||
| } | ||
| if (endDate) { | ||
| conditions.push(`twl.created_at::date <= '${endDate}'`); | ||
| } | ||
| return conditions.length > 0 ? `AND ${conditions.join(" AND ")}` : ""; | ||
| } |
There was a problem hiding this comment.
SQL injection: startDate and endDate are interpolated directly from request query params.
getTaskDateRangeFilter and getLogDateRangeFilter directly interpolate startDate/endDate into SQL strings using string literals. These values originate from req.query.start_date and req.query.end_date (Lines 236-237), which are user-controlled. An attacker can craft a malicious date string like '; DROP TABLE tasks; -- to inject arbitrary SQL.
This is ironic given the PR's stated goal of eliminating string interpolation. The same pattern appears inline at Lines 315-316.
These must be parameterized:
Suggested fix
- private static getTaskDateRangeFilter(startDate?: string, endDate?: string): string {
- if (!startDate && !endDate) return "";
- const conditions: string[] = [];
- if (startDate) {
- conditions.push(`start_date >= '${startDate}'`);
- }
- if (endDate) {
- conditions.push(`end_date <= '${endDate}'`);
- }
- return conditions.length > 0 ? `AND ${conditions.join(" AND ")}` : "";
- }
+ private static getTaskDateRangeFilter(startDate?: string, endDate?: string, paramOffset = 1): { clause: string; params: string[] } {
+ if (!startDate && !endDate) return { clause: "", params: [] };
+ const conditions: string[] = [];
+ const params: string[] = [];
+ if (startDate) {
+ conditions.push(`start_date >= $${paramOffset}`);
+ params.push(startDate);
+ paramOffset++;
+ }
+ if (endDate) {
+ conditions.push(`end_date <= $${paramOffset}`);
+ params.push(endDate);
+ }
+ return { clause: conditions.length > 0 ? `AND ${conditions.join(" AND ")}` : "", params };
+ }Apply the same pattern to getLogDateRangeFilter and the inline interpolation at Lines 315-316.
🤖 Prompt for AI Agents
In
`@worklenz-backend/src/controllers/project-workload/workload-gannt-controller.ts`
around lines 469 - 491, getTaskDateRangeFilter and getLogDateRangeFilter (and
the other inline SQL that interpolates start_date/end_date) currently inject
user-controlled startDate/endDate directly into SQL; change them to build
parameterized filters (e.g., return "AND start_date >= $n" / "AND
twl.created_at::date <= $n" or named placeholders) and instead of returning raw
SQL strings, also return or accept an array to which the corresponding date
values are pushed so the caller can supply them as query parameters; update the
callers that currently interpolate start_date/end_date inline to use these
parameterized filters and pass the accumulated params to the DB query (ensure
the functions getTaskDateRangeFilter and getLogDateRangeFilter signatures are
adjusted to accept/return parameters or a params collector and remove any direct
string interpolation of req.query values).
| @@ -1,4 +1,4 @@ | |||
| import { ParsedQs } from "qs"; | |||
| import { ParsedQs } from "qs"; | |||
There was a problem hiding this comment.
BOM character in file.
Line 1 starts with a BOM ( / U+FEFF) before the import statement. This can cause issues with some tools and should be removed.
🤖 Prompt for AI Agents
In `@worklenz-backend/src/controllers/tasks-controller-v2.ts` at line 1, The file
begins with a UTF-8 BOM character before the import causing tooling issues;
remove the invisible U+FEFF at the start of the file (i.e., delete the character
immediately preceding the `import { ParsedQs } from "qs";` line), ensure the
file is saved as UTF-8 without BOM, and re-run formatting/linting to confirm no
remaining BOMs (or use your editor/IDE's "Save with encoding: UTF-8 without BOM"
option).
| // Build filtered subtask count query - apply same filters to subtasks | ||
| const subtaskFilters = []; | ||
|
|
||
| // Always filter by archived status for subtasks | ||
| subtaskFilters.push(archivedFilter); | ||
|
|
||
| // Apply status filter to subtasks if present | ||
| if (statusesResult.clause) { | ||
| subtaskFilters.push(statusesResult.clause.replace(/\bt\./g, 'subtask.')); | ||
| } | ||
|
|
||
| // Apply priority filter to subtasks if present (reuse parameters) | ||
| if (options.priorities && priorityResult.clause) { | ||
| const priorityIds = (options.priorities as string).split(" ").filter(id => id.trim()); | ||
| const priorityParamStart = paramOffset - priorityResult.params.length; | ||
| const { clause: inClause } = SqlHelper.buildInClause(priorityIds, priorityParamStart); | ||
| subtaskFilters.push(`subtask.priority_id IN (${inClause})`); | ||
| } | ||
|
|
||
| // Apply labels filter to subtasks if present (reuse parameters) | ||
| if (options.labels && labelsResult.clause) { | ||
| const labelIds = (options.labels as string).split(" ").filter(id => id.trim()); | ||
| const labelParamStart = paramOffset - labelsResult.params.length - priorityResult.params.length; | ||
| const { clause: inClause } = SqlHelper.buildInClause(labelIds, labelParamStart); | ||
| subtaskFilters.push(`subtask.id IN (SELECT task_id FROM task_labels WHERE label_id IN (${inClause}))`); | ||
| } | ||
|
|
||
| // Apply members filter to subtasks if present (reuse parameters) | ||
| if (options.members && membersResult.clause) { | ||
| const memberIds = (options.members as string).split(" ").filter(id => id.trim()); | ||
| const memberParamStart = paramOffset - membersResult.params.length - projectsResult.params.length; | ||
| const { clause: inClause } = SqlHelper.buildInClause(memberIds, memberParamStart); | ||
| subtaskFilters.push(`subtask.id IN (SELECT task_id FROM tasks_assignees WHERE team_member_id IN (${inClause}))`); | ||
| } |
There was a problem hiding this comment.
Subtask filter parameter offsets are computed incorrectly — will reference wrong bind values.
The parameter start positions for subtask filters are calculated by subtracting from the final paramOffset, but the arithmetic doesn't account for all intermediate increments.
For example, Line 490:
const labelParamStart = paramOffset - labelsResult.params.length - priorityResult.params.length;After all filters, paramOffset = base + status + labels + members + projects + priority. Subtracting labels + priority yields base + status + members + projects, but labels actually start at base + status. The offset is wrong by members.length + projects.length.
Similarly, Line 498 for members:
const memberParamStart = paramOffset - membersResult.params.length - projectsResult.params.length;This yields base + status + labels + priority, but members start at base + status + labels.
The hasFilteredChildren section (Lines 519-591) correctly recalculates from scratch — use the same approach here or record the offsets when they're first computed:
Suggested fix: Record offsets when building filters
+ let statusParamStart = paramOffset;
const statusesResult = TasksControllerV2.getFilterByStatusWhereClosure(
options.statuses as string,
paramOffset
);
if (statusesResult.params.length > 0) {
queryParams.push(...statusesResult.params);
paramOffset += statusesResult.params.length;
}
+ let labelsParamStart = paramOffset;
const labelsResult = TasksControllerV2.getFilterByLabelsWhereClosure(
options.labels as string,
paramOffset
);
// ...
+ let membersParamStart = paramOffset;
const membersResult = TasksControllerV2.getFilterByMembersWhereClosure(
options.members as string,
paramOffset
);
// ...
+ let projectsParamStart = paramOffset;
// ...
+ let priorityParamStart = paramOffset;
const priorityResult = TasksControllerV2.getFilterByPriorityWhereClosure(
options.priorities as string,
paramOffset
);Then use these recorded positions directly:
- const priorityParamStart = paramOffset - priorityResult.params.length;
+ // use the already-recorded priorityParamStart
- const labelParamStart = paramOffset - labelsResult.params.length - priorityResult.params.length;
+ // use the already-recorded labelsParamStart
- const memberParamStart = paramOffset - membersResult.params.length - projectsResult.params.length;
+ // use the already-recorded membersParamStart🤖 Prompt for AI Agents
In `@worklenz-backend/src/controllers/tasks-controller-v2.ts` around lines 468 -
501, The subtask filter IN clauses compute parameter start positions by
subtracting lengths from paramOffset, which miscalculates offsets; change to
record each filter's starting parameter index when you first build that filter
(e.g., capture priorityParamStart, labelParamStart, memberParamStart at the
moment you construct priorityResult.params / labelsResult.params /
membersResult.params) or recompute starts from scratch like the
hasFilteredChildren block does, then pass those recorded indices into
SqlHelper.buildInClause for subtaskFilters; update references to paramOffset in
the subtask block (and use the same unique symbols: subtaskFilters, paramOffset,
priorityResult, labelsResult, membersResult, SqlHelper.buildInClause) so bind
positions align with the original filter parameter ordering.
| static buildInClause(values: any[], paramOffset = 1): { clause: string; params: any[] } { | ||
| if (!values || values.length === 0) { | ||
| return { clause: "", params: [] }; | ||
| } | ||
|
|
||
| const placeholders = values.map((_, index) => `$${paramOffset + index}`).join(", "); | ||
| return { | ||
| clause: placeholders, | ||
| params: values, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Empty clause return can produce IN () if callers don't guard.
When values is empty, this returns { clause: "", params: [] }. If a caller naively does `WHERE id IN (${clause})` without checking for an empty clause, the resulting SQL WHERE id IN () is a syntax error. Several callers in this PR (e.g., getFilterByMembersWhereClosure in workload-gannt-controller.ts Line 462, getByMultipleTeams in project-categories-controller.ts Line 104) interpolate the clause directly into IN (...) without guarding against the empty case.
Consider either throwing on empty input or returning a clause that is always syntactically valid (e.g., a NULL sentinel):
Suggested approach
static buildInClause(values: any[], paramOffset = 1): { clause: string; params: any[] } {
if (!values || values.length === 0) {
- return { clause: "", params: [] };
+ // Return a clause that is syntactically safe inside IN()
+ return { clause: "NULL", params: [] };
}🤖 Prompt for AI Agents
In `@worklenz-backend/src/shared/sql-helpers.ts` around lines 32 - 42, The
buildInClause helper currently returns an empty clause when values is empty
which can produce a syntax error when callers interpolate it into "IN
(${clause})"; change buildInClause (the static method buildInClause) to return a
syntactically safe sentinel when values is empty (e.g., return { clause: "NULL",
params: [] } so callers using `IN (${clause})` become `IN (NULL)` instead of `IN
()`), and update any related callsites or comments if you prefer to throw on
empty input instead (ensure callers like getFilterByMembersWhereClosure and
getByMultipleTeams still work as intended).
| export function flatString(text: string): string { | ||
| console.warn("flatString() is deprecated and unsafe. Use SqlHelper.buildInClause() instead."); | ||
| return (text || "") | ||
| .split(" ") | ||
| .map((s) => `'${s}'`) | ||
| .join(","); | ||
| } |
There was a problem hiding this comment.
Deprecated flatString is still vulnerable to SQL injection.
The naive '${s}' quoting (Line 241) does not escape single quotes within values, so any value containing ' can break out of the string literal. Since this is deprecated, consider making it throw an error instead of continuing to produce unsafe output, to force migration.
Suggested approach
export function flatString(text: string): string {
- console.warn("flatString() is deprecated and unsafe. Use SqlHelper.buildInClause() instead.");
- return (text || "")
- .split(" ")
- .map((s) => `'${s}'`)
- .join(",");
+ throw new Error("flatString() is removed. Use SqlHelper.buildInClause() instead.");
}🤖 Prompt for AI Agents
In `@worklenz-backend/src/shared/sql-helpers.ts` around lines 236 - 242, The
flatString function is vulnerable because it naively interpolates values with
`'${s}'` and doesn't escape single quotes; instead of returning unsafe output,
change flatString to immediately throw an Error (or TypeError) indicating it's
deprecated and unsafe and instructing callers to use SqlHelper.buildInClause(),
so callers are forced to migrate; update references to flatString if needed and
keep the console warning removal/replacement with the thrown error in the
flatString function.
fix: Address multiple SQL injection vulnerabilities, introduce a new …
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
worklenz-backend/src/controllers/reporting/reporting-members-controller.ts (3)
1412-1414:⚠️ Potential issue | 🟠 MajorSQL injection remnant:
userIdis still string-interpolated ingetMemberTaskStats.Line 1414 embeds
req.user?.iddirectly into SQL via'${req.user?.id}'. This is inconsistent with the parameterization effort applied elsewhere in this file (e.g.,getMembersnow correctly parameterizesuserId). Parameterize it using the existingparamOffsetpattern.🛡️ Sketch of fix direction
- 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 archivedClauseParams: any[] = []; + const archivedClause = includeArchived + ? "" + : (() => { + archivedClauseParams = [req.user?.id]; + return `AND t.project_id NOT IN (SELECT project_id FROM archived_projects WHERE project_id = t.project_id AND archived_projects.user_id = $${paramOffset++})`; + })();Then include
archivedClauseParamsinqueryParamsat line 1481.
1874-1876:⚠️ Potential issue | 🟠 MajorSQL injection remnant:
userIdis still string-interpolated ingetSingleMemberProjects.Same issue as
getMemberTaskStats—req.user?.idis directly embedded in the SQL string at line 1876. Parameterize it for consistency and safety.
680-682:⚠️ Potential issue | 🟠 MajorContent-Disposition filename not sanitized or quoted — potential header injection.
fileNameon line 682 includesteamNamefromreq.query, which is user-controlled. Special characters (e.g.,\r\n,",;) in the team name could break the header or enable HTTP response splitting. The same pattern appears in other export methods (lines 805, 940, 2027).Note that the newer
exportTimelogsFlatCSV(line 1674) andexportTimelogsFlatExcel(line 1852) correctly use quoted filenames. Apply the same quoting and sanitize the name in the older exports.🛡️ Proposed fix for line 682 (apply same pattern to 805, 940, 2027)
- res.setHeader("Content-Disposition", `attachment; filename=${fileName}.xlsx`); + const safeName = (fileName || "export").replace(/[^a-zA-Z0-9_\-. ]/g, "_"); + res.setHeader("Content-Disposition", `attachment; filename="${safeName}.xlsx"`);manage.sh (1)
1131-1137:⚠️ Potential issue | 🟡 Minor
build_and_push_imagesresults in double prompts for push and version.
build_imagesalready prompts "Do you want to push images to Docker Hub?" (line 922) and may push them. Thenbuild_and_push_imagescallspush_imageswhich prompts for versions again and pushes again. This creates a confusing UX where the user can end up being asked to push twice with potentially different version inputs.Consider either removing the push prompt from
build_imageswhen called frombuild_and_push_images, or havingbuild_and_push_imagesskip the redundant call.worklenz-backend/src/controllers/project-workload/workload-gannt-controller.ts (1)
534-626:⚠️ Potential issue | 🔴 CriticalCritical: parameterized member-filter placeholders are built but never passed to
db.query().
getQueryaccumulatesqueryParams(lines 540–548) for the member IN-clause, but it only returns the SQL string — never the params. The callers (getListat line 688 andgetTasksOnlyat line 750) pass[req.params.id]or[req.params.id, req.query.parent_task]as query parameters, completely ignoring the member-filter values.Furthermore,
paramOffsetstarts at1(line 541), so the member filter generates$1, $2, …placeholders that collide withproject_id = $1on line 623. This means:
- If a member filter is active, the query has mismatched placeholder-to-param mapping, producing wrong results or a runtime error.
- The member IDs are never bound, defeating the parameterization.
The function needs to return both the SQL string and the accumulated params, and callers must merge them with the fixed params (projectId, etc.), adjusting the offset so
$1remains reserved forproject_id.Sketch of the fix
- private static getQuery(userId: string, options: ParsedQs) { + private static getQuery(userId: string, options: ParsedQs): { sql: string; params: any[] } { const searchField = options.search ? "t.name" : "sort_order"; const { searchQuery, sortField } = WorkloadGanntController.toPaginationOptions(options, searchField); const isSubTasks = !!options.parent_task; const queryParams: any[] = []; - let paramOffset = 1; + // $1 = project_id, $2 = parent_task_id (if subtask) + let paramOffset = isSubTasks ? 3 : 2; // ... build member filter with correct offset ... - return `SELECT ... WHERE ${filters} ${searchQuery} AND project_id = $1 ...`; + return { + sql: `SELECT ... WHERE ${filters} ${searchQuery} AND project_id = $1 ...`, + params: queryParams + }; }Then in callers:
- const q = WorkloadGanntController.getQuery(req.user?.id as string, req.query); - const params = isSubTasks ? [req.params.id, req.query.parent_task] : [req.params.id]; - const result = await db.query(q, params); + const { sql, params: filterParams } = WorkloadGanntController.getQuery(req.user?.id as string, req.query); + const baseParams = isSubTasks ? [req.params.id, req.query.parent_task] : [req.params.id]; + const result = await db.query(sql, [...baseParams, ...filterParams]);
🤖 Fix all issues with AI agents
In `@manage.sh`:
- Around line 1078-1093: In push_images (manage.sh) you must verify the exact
tag exists locally before attempting docker push: check that the image
"${docker_username}/worklenz-backend:${backend_version}" is present (e.g., via
docker image inspect or docker images lookup) and if absent log a clear error
and return non-zero, then proceed to push the version and latest; also validate
the "latest" image exists before pushing to avoid cryptic docker errors (use the
same presence check for "${docker_username}/worklenz-backend:latest").
In `@worklenz-backend/src/controllers/project-categories-controller.ts`:
- Around line 108-112: The SQL WHERE clause is missing the IN (...) wrapper:
SqlHelper.buildInClause returns a placeholder list (assigned to clause) and
params, but the query string q interpolates clause directly after "team_id"
producing invalid SQL; fix by changing the query construction to include
"team_id IN (" + clause + ")" (or equivalent string/template) so the final SQL
is "WHERE team_id IN ($1, $2, ...)", then pass params to db.query as before
(ensure you update the q variable used in the db.query call).
- Line 24: The create() handler is calling req.body.name.trim() unguarded which
will throw if name is missing; update the create() function in
project-categories-controller to validate and sanitize name the same way as
update() does: check that req.body.name exists and is a non-empty string, trim
it only after confirming it's present, and return a 400 error if validation
fails; reference the req.body.name usage in create() and mirror the validation
logic from the update() method to avoid the TypeError.
In
`@worklenz-backend/src/controllers/reporting/projects/reporting-projects-controller.ts`:
- Around line 28-34: The SQL filter construction incorrectly nests an extra "IN"
because SqlHelper.buildInClause already returns an IN clause (e.g., "IN
($4,$5)"); update each filter construction (e.g., where you set statusesClause,
and the analogous clauses for other filters in both get and getGrouped) to
either use the returned clause directly (statusesClause = `AND p.status_id
${clause}`) or replace buildInClause with buildOptionalInClause which returns
the full "AND column IN (...)" string, and ensure filterParams and paramOffset
are adjusted the same way for all 10 occurrences.
In `@worklenz-backend/src/socket.io/commands/on-task-timer-stop.ts`:
- Around line 7-17: on_task_timer_stop currently doesn't check that
getLoggedInUserIdFromSocket returned a non-null value; add an early bail similar
to the task_id validation: after const userId =
getLoggedInUserIdFromSocket(socket) check if userId is null, and if so call
socket.emit(SocketEvents.TASK_TIMER_STOP.toString(), null) and return; this
prevents running parameterized queries with a null $1 and ensures
unauthenticated sockets are handled consistently.
- Around line 7-9: Move the db.pool.connect() call inside the try block (or wrap
it in its own try) within the on_task_timer_stop function so connection
acquisition errors are caught; ensure any acquired client is released in the
finally block (use client.release() guarded by a null/defined check) and that
socket.emit error response is sent from the catch so the client always receives
an error reply if connection or query fails.
In `@worklenz-backend/src/utils/sql-helper.ts`:
- Around line 64-77: The buildOptionalInClause function directly interpolates
columnName, creating an injection risk; validate or sanitize it before use by
calling the shared SqlHelper.escapeIdentifier (or check columnName against an
allowlist of permitted column names) and throw or return an empty clause if
invalid. Update buildOptionalInClause to transform columnName via
SqlHelper.escapeIdentifier (or assert membership in allowedColumns) and only
then build the `AND ${columnName} IN (...)` clause so the returned clause uses a
safe, escaped identifier while leaving the parameter placeholders and params
logic unchanged.
- Around line 1-157: There are two conflicting SqlHelper classes; consolidate
into one canonical module by merging/exporting a single SqlHelper (keep the
existing shared/sql-helpers.ts API to avoid breaking eight controllers) and
remove the new utils/sql-helper.ts file; ensure the merged SqlHelper implements
buildInClause with the existing callers' behavior (returning empty clause for
empty arrays if that is what controllers expect or preserve throwing only if
callers were updated), make buildOrderByClause behavior compatible with shared
usage (accept or preserve defaultField behavior used by callers), and harden
buildOptionalInClause by validating columnName against an allowlist or strict
identifier regex before interpolating (refer to the SqlHelper class and the
methods buildInClause, buildOptionalInClause, and buildOrderByClause to locate
code to change).
🧹 Nitpick comments (10)
worklenz-backend/src/controllers/reporting/projects/reporting-projects-controller.ts (4)
260-263: Unused destructured variables fromtoPaginationOptions.
sortField,sortOrder,size, andoffsetare destructured at line 262 but never used ingetGrouped. This is misleading — readers will expect pagination/sorting to be applied. Either remove the unused bindings or apply them to the grouped query.♻️ Suggested fix
- const { searchQuery, searchParams, sortField, sortOrder, size, offset } = this.toPaginationOptions(req.query, ["p.name"], false, 2); + const { searchQuery, searchParams } = this.toPaginationOptions(req.query, ["p.name"], false, 2);
352-358:COALESCEon a string literal is a no-op.Line 355:
COALESCE('#1890ff', '#888')will always return'#1890ff'since it's never NULL. Same applies to line 363. Use a plain literal instead.♻️ Suggested fix
- groupColor = "COALESCE('#1890ff', '#888')"; + groupColor = "'#1890ff'";
360-366: Heavy correlated subqueries repeated ingroupField,groupName, andgroupByFields.The same
SELECT pm.team_member_id FROM project_members pm WHERE …subquery appears 6 times across these three assignments, and it will be evaluated per-row in the outer query. Consider using a CTE or a lateral join to compute the manager once per project.
269-326: Filter-building logic is duplicated verbatim betweengetandgetGrouped.Lines 27–81 and 269–326 are near-identical blocks constructing
statusesClause,healthsClause,categoriesClause,projectManagersClause,teamsClause, andarchivedClause. Extract a shared helper (e.g.,buildFilterClauses(req, startParamOffset)) that returns the clauses, accumulated params, and the next param offset.worklenz-backend/src/controllers/reporting/reporting-members-controller.ts (2)
1040-1063:tableNameis interpolated directly into SQL ingetMinMaxDates.Line 1060 embeds
${tableName}into the query string. All current callers pass hardcoded literals ("task_work_log","task_activity_logs"), so this is safe today. Consider adding a whitelist check or a type-level constraint to prevent accidental misuse if this method is reused with dynamic input in the future.
1635-1637: Preferconstfor destructured query parameters.Lines 1636 and 1801 use
letfor the destructuredreq.queryvalues, but none are reassigned. Useconstfor correctness.♻️ Proposed fix
- let { team_member_id, duration, date_range, billable, search } = req.query; + const { team_member_id, duration, date_range, billable, search } = req.query;Apply at both line 1636 and line 1801.
Also applies to: 1800-1801
worklenz-backend/src/controllers/project-categories-controller.ts (1)
26-50: DuplicatevalidColorsarray construction — extract to a shared helper.The same array of all valid colors (base keys + shade values, lowercased) is built in
create()(Lines 30-33) and again inupdate()(Lines 127-130). Additionally, withincreate()itself, the un-lowercased variant is built a second time (Lines 38-41) just to recover the original casing.Consider extracting this to a private static field or helper method to avoid the repeated work and keep validation consistent.
Example refactor
export default class ProjectCategoriesController extends WorklenzControllerBase { + + private static readonly validColorsLower: string[] = [ + ...Object.keys(WorklenzColorShades), + ...Object.values(WorklenzColorShades).flat(), + ].map((c) => c.toLowerCase()); + + private static readonly allColorsOriginal: string[] = [ + ...Object.keys(WorklenzColorShades), + ...Object.values(WorklenzColorShades).flat(), + ]; +worklenz-backend/src/controllers/project-workload/workload-gannt-controller.ts (2)
245-252: Deeply nested correlated subqueries for org settings — consider a JOIN instead.Each row executes two correlated subqueries with nested sub-selects to fetch
org_working_hoursandorg_working_days. For projects with many members, this produces N × 2 extra round-trips through the planner. A single JOIN throughteam_members → teams → organizations(and a LEFT JOIN toorganization_working_days) would be more efficient and readable.
141-164:req.query.start_date/req.query.end_dateare used unvalidated to construct moment dates.When the branch on line 145 is taken, the raw query-string values are passed directly to
moment(). While this is less severe than SQL injection (these aren't interpolated into SQL here), invalid or malicious date strings could produce unexpectedInvalid Datemoments that propagate through the date-range logic silently. Consider validating the date format (e.g.,YYYY-MM-DD) before use.worklenz-backend/src/controllers/worklenz-controller-base.ts (1)
57-64: Sort field validation improves but still lacks allowlist protection.The added guards against
"null","undefined", and empty strings strengthen the validation. However,sortFieldremains user-controlled input that gets interpolated directly intoORDER BYclauses. The codebase already providesSqlHelper.validateSortField()(documented as preventing ORDER BY SQL injection) andSqlHelper.validateSortOrder()— consider using these at the caller sites to enforce allowlist-based validation, particularly inclients-controller.ts,job-titles-controller.ts,team-members-controller.ts, and other controllers that usetoPaginationOptions.
worklenz-backend/src/controllers/project-categories-controller.ts
Outdated
Show resolved
Hide resolved
worklenz-backend/src/controllers/reporting/projects/reporting-projects-controller.ts
Show resolved
Hide resolved
| static buildOptionalInClause(values: any[], columnName: string, startIndex: number): SqlClauseResult { | ||
| if (!Array.isArray(values) || values.length === 0) { | ||
| return { | ||
| clause: '', | ||
| params: [] | ||
| }; | ||
| } | ||
|
|
||
| const placeholders = values.map((_, index) => `$${startIndex + index}`).join(', '); | ||
| return { | ||
| clause: `AND ${columnName} IN (${placeholders})`, | ||
| params: values | ||
| }; | ||
| } |
There was a problem hiding this comment.
columnName parameter is not validated — potential SQL injection vector.
buildOptionalInClause interpolates columnName directly into the SQL string without any sanitization. If a caller passes user-controlled input as columnName, this could enable SQL injection. Consider validating it with escapeIdentifier (available in the shared SqlHelper) or an allowlist check.
static buildOptionalInClause(values: any[], columnName: string, startIndex: number): SqlClauseResult {
+ // Validate column name contains only safe identifier characters
+ if (!/^[a-zA-Z_][a-zA-Z0-9_.]*$/.test(columnName)) {
+ throw new Error(`Invalid column name: ${columnName}`);
+ }
if (!Array.isArray(values) || values.length === 0) {📝 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.
| static buildOptionalInClause(values: any[], columnName: string, startIndex: number): SqlClauseResult { | |
| if (!Array.isArray(values) || values.length === 0) { | |
| return { | |
| clause: '', | |
| params: [] | |
| }; | |
| } | |
| const placeholders = values.map((_, index) => `$${startIndex + index}`).join(', '); | |
| return { | |
| clause: `AND ${columnName} IN (${placeholders})`, | |
| params: values | |
| }; | |
| } | |
| static buildOptionalInClause(values: any[], columnName: string, startIndex: number): SqlClauseResult { | |
| // Validate column name contains only safe identifier characters | |
| if (!/^[a-zA-Z_][a-zA-Z0-9_.]*$/.test(columnName)) { | |
| throw new Error(`Invalid column name: ${columnName}`); | |
| } | |
| if (!Array.isArray(values) || values.length === 0) { | |
| return { | |
| clause: '', | |
| params: [] | |
| }; | |
| } | |
| const placeholders = values.map((_, index) => `$${startIndex + index}`).join(', '); | |
| return { | |
| clause: `AND ${columnName} IN (${placeholders})`, | |
| params: values | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@worklenz-backend/src/utils/sql-helper.ts` around lines 64 - 77, The
buildOptionalInClause function directly interpolates columnName, creating an
injection risk; validate or sanitize it before use by calling the shared
SqlHelper.escapeIdentifier (or check columnName against an allowlist of
permitted column names) and throw or return an empty clause if invalid. Update
buildOptionalInClause to transform columnName via SqlHelper.escapeIdentifier (or
assert membership in allowedColumns) and only then build the `AND ${columnName}
IN (...)` clause so the returned clause uses a safe, escaped identifier while
leaving the parameter placeholders and params logic unchanged.
… and updating affected controllers
fix: Address SQL injection vulnerabilities by refactoring SQL helpers…
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manage.sh (1)
1141-1147:⚠️ Potential issue | 🟡 Minor
build_and_push_imagescan double-push and double-prompt for versions.
build_imagesalready asks "Do you want to push images to Docker Hub?" (line 922) and pushes if the user says yes. Thenbuild_and_push_imagesunconditionally callspush_images, which re-prompts for version numbers and pushes again. This leads to redundant pushes and a confusing user experience.Consider either:
- Skipping the push prompt inside
build_imageswhen called frombuild_and_push_images, or- Having
build_and_push_imagesonly callpush_imagesifbuild_imagesdidn't already push.
🤖 Fix all issues with AI agents
In `@manage.sh`:
- Around line 1108-1117: The frontend push block lacks a local-image existence
check; before calling docker push for
"${docker_username}/worklenz-frontend:${frontend_version}" (and only when
frontend_version != "latest"), run the same verification used in the backend
path: use docker image inspect on the same image tag and detect non-zero exit to
print an error and return 1 if the image is missing, then proceed to docker
push; update the block around the variables frontend_version and docker_username
to mirror the backend's verification flow.
- Around line 1317-1329: The push commands inside install_worklenz currently
call docker push for backend and frontend tags but do not check exit status;
update the block that pushes
"${docker_username}/worklenz-backend:${backend_version}",
"${docker_username}/worklenz-backend:latest",
"${docker_username}/worklenz-frontend:${frontend_version}", and
"${docker_username}/worklenz-frontend:latest" to verify each docker push's exit
code (like other functions do with $? ), and on failure call print_error with a
descriptive message and exit 1 (or return non-zero) so failures are not reported
as successful; keep using print_info/print_error helpers and preserve the
conditional checks for "latest".
In
`@worklenz-backend/src/controllers/reporting/projects/reporting-projects-controller.ts`:
- Around line 16-83: The issue is that get(...) passes dot-qualified column
names like 'p.status_id' into SqlHelper.buildOptionalInClause which fails
because escapeIdentifier rejects dots; fix by using unqualified column names
(e.g., 'status_id', 'health_id', 'category_id', 'team_id') in the
buildOptionalInClause calls inside the get method (the blocks setting
statusesClause, healthsClause, categoriesClause, teamsClause) and keep the table
alias scope in the SQL templates (e.g., still use p.<col> where you build the
final clause or when composing teamFilterClause); do not change paramOffset
logic or the nested project_managers buildInClause usage—only replace the
dot-qualified identifiers passed into buildOptionalInClause.
- Around line 132-155: The ALL_TIME branch in getMinMaxDates hardcodes "$1"
which ignores paramOffset; change the SQL to use the dynamic placeholder
`$${paramOffset}` (e.g., WHERE project_id = $${paramOffset}) instead of $1, and
update the contract so callers supply the projectId value at that parameter
position (thread projectId into the params array when calling getMinMaxDates or
append it to the returned params in the caller such as getProjectTimeLogs) to
ensure the placeholder is bound correctly.
In `@worklenz-backend/src/shared/sql-helpers.ts`:
- Around line 68-84: buildOptionalInClause calls this.escapeIdentifier with
dot-qualified names like "p.status_id", but escapeIdentifier currently rejects
dots; update escapeIdentifier to accept and correctly escape qualified
identifiers by splitting the input on '.' and validating/escaping each segment
individually (apply the existing /^[a-zA-Z_][a-zA-Z0-9_]*$/ check per segment
and then rejoin with the identifier separator), or alternatively change
buildOptionalInClause to split columnName into segments and call
escapeIdentifier on each segment before rejoining; reference the methods
buildOptionalInClause and escapeIdentifier and ensure callers such as
reporting-projects-controller.ts that pass "p.status_id" continue to work
without throwing.
🧹 Nitpick comments (6)
manage.sh (1)
906-916: Output summary assumes backend and frontend versions are always in sync.The conditional on line 906 only checks
backend_version, but the user can specify different values for backend and frontend (e.g., backend=latest, frontend=2.1.5). When they differ, the else-branch would hide the frontend version tag, or the if-branch would display both even if one islatest-only.Consider checking each component independently:
- if [ "$backend_version" != "latest" ]; then - echo -e " ${GREEN}Backend Images:${NC}" - echo -e " - ${docker_username}/worklenz-backend:${backend_version}" - echo -e " - ${docker_username}/worklenz-backend:latest" - echo -e " ${GREEN}Frontend Images:${NC}" - echo -e " - ${docker_username}/worklenz-frontend:${frontend_version}" - echo -e " - ${docker_username}/worklenz-frontend:latest" - else - echo -e " ${GREEN}Backend Image:${NC} ${docker_username}/worklenz-backend:latest" - echo -e " ${GREEN}Frontend Image:${NC} ${docker_username}/worklenz-frontend:latest" - fi + echo -e " ${GREEN}Backend Image(s):${NC}" + if [ "$backend_version" != "latest" ]; then + echo -e " - ${docker_username}/worklenz-backend:${backend_version}" + fi + echo -e " - ${docker_username}/worklenz-backend:latest" + echo -e " ${GREEN}Frontend Image(s):${NC}" + if [ "$frontend_version" != "latest" ]; then + echo -e " - ${docker_username}/worklenz-frontend:${frontend_version}" + fi + echo -e " - ${docker_username}/worklenz-frontend:latest"The same pattern applies to the push summary at lines 1130–1136.
worklenz-backend/src/shared/sql-helpers.ts (3)
96-135:fieldandoperatorinbuildWhereClauseare interpolated without validation.The
fieldandoperatorvalues from each condition are embedded directly in the SQL string (lines 119, 123, 125). While these are presumably developer-provided, the helper's purpose is to prevent injection — inconsistent to leave these unsanitized. Consider validatingfieldviaescapeIdentifierandoperatoragainst an allowlist (e.g.,=,!=,<,>,<=,>=,IN,IS NULL,IS NOT NULL,LIKE,ILIKE).
240-269:tableand SETfieldnames inbuildUpdateQueryare not escaped.Line 260 interpolates
tabledirectly, and line 257 interpolates the field name fromsetentries — both without callingescapeIdentifier. This undermines the safety guarantees the helper is designed to provide.Suggested fix
+ const safeTable = this.escapeIdentifier(table); + const setClauses = setEntries.map(([field, value]) => { + const safeField = this.escapeIdentifier(field); params.push(value); - return `${field} = $${paramOffset++}`; + return `${safeField} = $${paramOffset++}`; }); - let query = `UPDATE ${table} SET ${setClauses.join(", ")}`; + let query = `UPDATE ${safeTable} SET ${setClauses.join(", ")}`;
140-162:searchTerminbuildLikeClauseis not sanitized for LIKE metacharacters.If
searchTermcontains%or_, these are interpreted as LIKE wildcards, allowing callers to inadvertently match unintended patterns. Consider escaping LIKE metacharacters before wrapping with%:Suggested fix
static buildLikeClause( field: string, searchTerm: string, paramOffset = 1, options: { caseSensitive?: boolean; prefix?: boolean; suffix?: boolean; } = {} ): { clause: string; params: string[] } { const { caseSensitive = false, prefix = true, suffix = true } = options; - let pattern = searchTerm; + // Escape LIKE metacharacters + let pattern = searchTerm.replace(/[%_\\]/g, '\\$&'); if (prefix) pattern = `%${pattern}`; if (suffix) pattern = `${pattern}%`;worklenz-backend/src/controllers/project-categories-controller.ts (1)
34-58: Color validation logic is duplicated betweencreateandupdate.Lines 38–50 (create) and lines 135–141 (update) both build the same
validColorsarray fromWorklenzColorShades. Consider extracting this to a private static helper method (e.g.,isValidColor(color: string): string | null) to keep it DRY.Also note the field name inconsistency:
create()readsreq.body.color_code(line 36) whileupdate()readsreq.body.color(line 130). Clients need to use different field names for the same concept.worklenz-backend/src/controllers/reporting/projects/reporting-projects-controller.ts (1)
259-456: NewgetGroupedmethod: well-organized, but a few concerns.The grouped query structure is solid. A few observations:
"manager" group-by uses correlated subqueries in
GROUP BYandSELECT(lines 361–365). Each project row triggers multiple subqueries againstproject_membersandteam_member_info_view. For large datasets this can be significantly slow. Consider a CTE or JOIN-based approach similar to the other group-by cases.Line 355:
COALESCE('#1890ff', '#888')always returns'#1890ff'— theCOALESCEis a no-op on two non-null literals.Lines 262:
sizeandoffsetare destructured fromtoPaginationOptionsbut never used (per comment at line 436). This is fine functionally but could confuse future readers.
worklenz-backend/src/controllers/reporting/projects/reporting-projects-controller.ts
Show resolved
Hide resolved
| protected static getMinMaxDates(key: string, dateRange: string[], paramOffset = 1): { clause: string; params: any[] } { | ||
| if (dateRange.length === 2) { | ||
| // Use parameterized queries for dates | ||
| const start = moment(dateRange[0]).format("YYYY-MM-DD"); | ||
| const end = moment(dateRange[1]).format("YYYY-MM-DD"); | ||
| return `,(SELECT '${start}'::DATE )AS start_date, (SELECT '${end}'::DATE )AS end_date`; | ||
| return { | ||
| clause: `,(SELECT $${paramOffset}::DATE )AS start_date, (SELECT $${paramOffset + 1}::DATE )AS end_date`, | ||
| params: [start, end] | ||
| }; | ||
| } | ||
|
|
||
| if (key === DATE_RANGES.YESTERDAY) | ||
| return ",(SELECT (CURRENT_DATE - INTERVAL '1 day')::DATE) AS start_date, (SELECT (CURRENT_DATE)::DATE) AS end_date"; | ||
| return { clause: ",(SELECT (CURRENT_DATE - INTERVAL '1 day')::DATE) AS start_date, (SELECT (CURRENT_DATE)::DATE) AS end_date", params: [] }; | ||
| if (key === DATE_RANGES.LAST_WEEK) | ||
| return ",(SELECT (CURRENT_DATE - INTERVAL '1 week')::DATE) AS start_date, (SELECT (CURRENT_DATE)::DATE) AS end_date"; | ||
| return { clause: ",(SELECT (CURRENT_DATE - INTERVAL '1 week')::DATE) AS start_date, (SELECT (CURRENT_DATE)::DATE) AS end_date", params: [] }; | ||
| if (key === DATE_RANGES.LAST_MONTH) | ||
| return ",(SELECT (CURRENT_DATE - INTERVAL '1 month')::DATE) AS start_date, (SELECT (CURRENT_DATE)::DATE) AS end_date"; | ||
| return { clause: ",(SELECT (CURRENT_DATE - INTERVAL '1 month')::DATE) AS start_date, (SELECT (CURRENT_DATE)::DATE) AS end_date", params: [] }; | ||
| if (key === DATE_RANGES.LAST_QUARTER) | ||
| return ",(SELECT (CURRENT_DATE - INTERVAL '3 months')::DATE) AS start_date, (SELECT (CURRENT_DATE)::DATE) AS end_date"; | ||
| return { clause: ",(SELECT (CURRENT_DATE - INTERVAL '3 months')::DATE) AS start_date, (SELECT (CURRENT_DATE)::DATE) AS end_date", params: [] }; | ||
| if (key === DATE_RANGES.ALL_TIME) | ||
| return ",(SELECT (MIN(task_work_log.created_at)::DATE) FROM task_work_log WHERE task_id IN (SELECT id FROM tasks WHERE project_id = $1)) AS start_date, (SELECT (MAX(task_work_log.created_at)::DATE) FROM task_work_log WHERE task_id IN (SELECT id FROM tasks WHERE project_id = $1)) AS end_date"; | ||
| return { clause: ",(SELECT (MIN(task_work_log.created_at)::DATE) FROM task_work_log WHERE task_id IN (SELECT id FROM tasks WHERE project_id = $1)) AS start_date, (SELECT (MAX(task_work_log.created_at)::DATE) FROM task_work_log WHERE task_id IN (SELECT id FROM tasks WHERE project_id = $1)) AS end_date", params: [] }; | ||
|
|
||
| return ""; | ||
| return { clause: "", params: [] }; | ||
| } |
There was a problem hiding this comment.
ALL_TIME branch hardcodes $1 regardless of paramOffset.
Line 152 uses $1 directly in the subquery (WHERE project_id = $1), ignoring the paramOffset parameter. This works today because the sole caller (getProjectTimeLogs) places projectId at $1, but it breaks the method's contract and will silently produce wrong results if any future caller has a different parameter layout.
Suggested fix
- if (key === DATE_RANGES.ALL_TIME)
- return { clause: ",(SELECT (MIN(task_work_log.created_at)::DATE) FROM task_work_log WHERE task_id IN (SELECT id FROM tasks WHERE project_id = $1)) AS start_date, (SELECT (MAX(task_work_log.created_at)::DATE) FROM task_work_log WHERE task_id IN (SELECT id FROM tasks WHERE project_id = $1)) AS end_date", params: [] };
+ if (key === DATE_RANGES.ALL_TIME)
+ return { clause: `,(SELECT (MIN(task_work_log.created_at)::DATE) FROM task_work_log WHERE task_id IN (SELECT id FROM tasks WHERE project_id = $${paramOffset})) AS start_date, (SELECT (MAX(task_work_log.created_at)::DATE) FROM task_work_log WHERE task_id IN (SELECT id FROM tasks WHERE project_id = $${paramOffset})) AS end_date`, params: [] };Note: You'd also need to thread projectId through the params if you use a new offset, or document that the caller must supply projectId at the indicated param position.
🤖 Prompt for AI Agents
In
`@worklenz-backend/src/controllers/reporting/projects/reporting-projects-controller.ts`
around lines 132 - 155, The ALL_TIME branch in getMinMaxDates hardcodes "$1"
which ignores paramOffset; change the SQL to use the dynamic placeholder
`$${paramOffset}` (e.g., WHERE project_id = $${paramOffset}) instead of $1, and
update the contract so callers supply the projectId value at that parameter
position (thread projectId into the params array when calling getMinMaxDates or
append it to the returned params in the caller such as getProjectTimeLogs) to
ensure the placeholder is bound correctly.
…helpers and refactoring controllers
fix: Address SQL injection vulnerabilities by introducing secure SQL …
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manage.sh (1)
1146-1152:⚠️ Potential issue | 🟠 Major
build_and_push_imagescan double-push and double-prompt for versions.
build_images()already asks the user "Do you want to push images to Docker Hub?" (Line 922) and handles the push internally. Whenbuild_and_push_imagesthen callspush_images(), the user is prompted for version numbers again and images may be pushed a second time.Either remove the push prompt from
build_imageswhen called via this path, or skip thepush_imagescall here. The simplest fix:Proposed fix
build_and_push_images() { - build_images - if [ $? -eq 0 ]; then - echo "" - push_images - fi + # Delegate entirely to build_images, which already offers to push + build_images }
🤖 Fix all issues with AI agents
In `@manage.sh`:
- Around line 906-916: The display logic incorrectly couples backend and
frontend version checks: update the conditional blocks that render image lines
so backend and frontend are checked independently (use separate checks on
variables backend_version and frontend_version rather than a single if for
backend_version), e.g., render backend image lines based on backend_version and
docker_username and render frontend image lines based on frontend_version and
docker_username; apply the same independent-check fix to the other repeated
sections referenced (the blocks around the occurrences at the later locations)
so each service’s "latest" vs version-specific output is determined by its own
variable.
- Around line 1123-1128: Add the same local-image verification used in the
backend path (the docker image inspect check) before pushing the frontend latest
tag: run docker image inspect for "${docker_username}/worklenz-frontend:latest",
and if it fails call print_error with a clear message and return 1 instead of
attempting docker push; keep the existing print_info, docker push, and error
handling (print_error/return 1) for the push step intact so the push only runs
when the image exists.
In `@worklenz-backend/src/shared/sql-helpers.ts`:
- Around line 258-287: buildUpdateQuery currently interpolates the table name
and SET field keys directly and allows empty where arrays (risking full-table
UPDATEs); update buildUpdateQuery to escape the table and column identifiers
using the existing escapeIdentifier helper when constructing the query (use
escaped table name for the UPDATE clause and escape each set field name when
building setClauses) and preserve parameter numbering logic, and change the
signature to accept an optional allowUpdateAll boolean (default false) or simply
throw an Error if where.length === 0 — enforce that buildWhereClause is invoked
only when where has conditions and throw a clear error (or require
allowUpdateAll: true) to prevent accidental full-table updates.
🧹 Nitpick comments (3)
manage.sh (2)
894-898: Greedy.*in sed pattern may over-match if the YAML line contains additional braces.The pattern
\${BACKEND_VERSION:-.*}uses.*}which greedily matches to the last}on the line. If the YAML line ever contains additional}characters (e.g., in a comment or nested interpolation), the substitution could corrupt it. A safer pattern would use a non-greedy equivalent:- sed -i.bak "s|image: .*/worklenz-backend:\${BACKEND_VERSION:-.*}|image: ${docker_username}/worklenz-backend:\${BACKEND_VERSION:-${backend_version}}|" "$DOCKER_COMPOSE_FILE" + sed -i.bak "s|image: .*/worklenz-backend:\${BACKEND_VERSION:-[^}]*}|image: ${docker_username}/worklenz-backend:\${BACKEND_VERSION:-${backend_version}}|" "$DOCKER_COMPOSE_FILE"Same applies to the frontend sed on Line 898, and the duplicates on Lines 1308–1309.
1255-1257: No interactive version prompt during install — intentional?Unlike
build_images()which prompts for version numbers,install_worklenz()silently defaults tolatest(or the env value). This may be intentional for a streamlined install experience, but worth confirming that users who want versioned builds during install know to setBACKEND_VERSION/FRONTEND_VERSIONin the environment beforehand.worklenz-backend/src/shared/sql-helpers.ts (1)
113-129:condition.fieldis interpolated withoutescapeIdentifier, inconsistent withbuildOptionalInClause.
buildOptionalInClausevalidatescolumnNameviaescapeIdentifier, butbuildWhereClausedirectly interpolatescondition.field(lines 119, 123, 125) into SQL. While these fields typically come from application code, applying the same validation would be consistent and would harden the helper against misuse.Additionally, line 119 delegates to
buildInClauseforINconditions — ifcondition.valueresolves to an empty array, the empty clause bug (flagged above) propagates here as well, producingfield IN ().
| if [ "$backend_version" != "latest" ]; then | ||
| echo -e " ${GREEN}Backend Images:${NC}" | ||
| echo -e " - ${docker_username}/worklenz-backend:${backend_version}" | ||
| echo -e " - ${docker_username}/worklenz-backend:latest" | ||
| echo -e " ${GREEN}Frontend Images:${NC}" | ||
| echo -e " - ${docker_username}/worklenz-frontend:${frontend_version}" | ||
| echo -e " - ${docker_username}/worklenz-frontend:latest" | ||
| else | ||
| echo -e " ${GREEN}Backend Image:${NC} ${docker_username}/worklenz-backend:latest" | ||
| echo -e " ${GREEN}Frontend Image:${NC} ${docker_username}/worklenz-frontend:latest" | ||
| fi |
There was a problem hiding this comment.
Display logic incorrectly couples backend and frontend version checks.
The condition on Line 906 (if [ "$backend_version" != "latest" ]) controls the display for both backend and frontend images. If backend is latest but frontend has a specific version (or vice versa), the output is misleading — it will either hide or incorrectly show version tags.
The same pattern repeats at Lines 978–980 and Lines 1135–1141.
Proposed fix for Lines 906–916
- if [ "$backend_version" != "latest" ]; then
- echo -e " ${GREEN}Backend Images:${NC}"
- echo -e " - ${docker_username}/worklenz-backend:${backend_version}"
- echo -e " - ${docker_username}/worklenz-backend:latest"
- echo -e " ${GREEN}Frontend Images:${NC}"
- echo -e " - ${docker_username}/worklenz-frontend:${frontend_version}"
- echo -e " - ${docker_username}/worklenz-frontend:latest"
- else
- echo -e " ${GREEN}Backend Image:${NC} ${docker_username}/worklenz-backend:latest"
- echo -e " ${GREEN}Frontend Image:${NC} ${docker_username}/worklenz-frontend:latest"
- fi
+ echo -e " ${GREEN}Backend Images:${NC}"
+ if [ "$backend_version" != "latest" ]; then
+ echo -e " - ${docker_username}/worklenz-backend:${backend_version}"
+ fi
+ echo -e " - ${docker_username}/worklenz-backend:latest"
+ echo -e " ${GREEN}Frontend Images:${NC}"
+ if [ "$frontend_version" != "latest" ]; then
+ echo -e " - ${docker_username}/worklenz-frontend:${frontend_version}"
+ fi
+ echo -e " - ${docker_username}/worklenz-frontend:latest"📝 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.
| if [ "$backend_version" != "latest" ]; then | |
| echo -e " ${GREEN}Backend Images:${NC}" | |
| echo -e " - ${docker_username}/worklenz-backend:${backend_version}" | |
| echo -e " - ${docker_username}/worklenz-backend:latest" | |
| echo -e " ${GREEN}Frontend Images:${NC}" | |
| echo -e " - ${docker_username}/worklenz-frontend:${frontend_version}" | |
| echo -e " - ${docker_username}/worklenz-frontend:latest" | |
| else | |
| echo -e " ${GREEN}Backend Image:${NC} ${docker_username}/worklenz-backend:latest" | |
| echo -e " ${GREEN}Frontend Image:${NC} ${docker_username}/worklenz-frontend:latest" | |
| fi | |
| echo -e " ${GREEN}Backend Images:${NC}" | |
| if [ "$backend_version" != "latest" ]; then | |
| echo -e " - ${docker_username}/worklenz-backend:${backend_version}" | |
| fi | |
| echo -e " - ${docker_username}/worklenz-backend:latest" | |
| echo -e " ${GREEN}Frontend Images:${NC}" | |
| if [ "$frontend_version" != "latest" ]; then | |
| echo -e " - ${docker_username}/worklenz-frontend:${frontend_version}" | |
| fi | |
| echo -e " - ${docker_username}/worklenz-frontend:latest" |
🤖 Prompt for AI Agents
In `@manage.sh` around lines 906 - 916, The display logic incorrectly couples
backend and frontend version checks: update the conditional blocks that render
image lines so backend and frontend are checked independently (use separate
checks on variables backend_version and frontend_version rather than a single if
for backend_version), e.g., render backend image lines based on backend_version
and docker_username and render frontend image lines based on frontend_version
and docker_username; apply the same independent-check fix to the other repeated
sections referenced (the blocks around the occurrences at the later locations)
so each service’s "latest" vs version-specific output is determined by its own
variable.
| print_info "Pushing ${docker_username}/worklenz-frontend:latest..." | ||
| docker push "${docker_username}/worklenz-frontend:latest" | ||
| if [ $? -ne 0 ]; then | ||
| print_error "Frontend push failed!" | ||
| print_error "Frontend latest push failed!" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
Missing local image verification for frontend latest tag before push.
The backend path verifies the :latest image exists locally via docker image inspect (Lines 1093–1097) before pushing. The frontend path at Line 1123 pushes :latest without the same check, which will produce a cryptic Docker error if the image is missing.
Proposed fix
+ # Verify latest image exists locally
+ if ! docker image inspect "${docker_username}/worklenz-frontend:latest" >/dev/null 2>&1; then
+ print_error "Image ${docker_username}/worklenz-frontend:latest not found locally. Please build it first."
+ return 1
+ fi
print_info "Pushing ${docker_username}/worklenz-frontend:latest..."
docker push "${docker_username}/worklenz-frontend:latest"📝 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.
| print_info "Pushing ${docker_username}/worklenz-frontend:latest..." | |
| docker push "${docker_username}/worklenz-frontend:latest" | |
| if [ $? -ne 0 ]; then | |
| print_error "Frontend push failed!" | |
| print_error "Frontend latest push failed!" | |
| return 1 | |
| fi | |
| # Verify latest image exists locally | |
| if ! docker image inspect "${docker_username}/worklenz-frontend:latest" >/dev/null 2>&1; then | |
| print_error "Image ${docker_username}/worklenz-frontend:latest not found locally. Please build it first." | |
| return 1 | |
| fi | |
| print_info "Pushing ${docker_username}/worklenz-frontend:latest..." | |
| docker push "${docker_username}/worklenz-frontend:latest" | |
| if [ $? -ne 0 ]; then | |
| print_error "Frontend latest push failed!" | |
| return 1 | |
| fi |
🤖 Prompt for AI Agents
In `@manage.sh` around lines 1123 - 1128, Add the same local-image verification
used in the backend path (the docker image inspect check) before pushing the
frontend latest tag: run docker image inspect for
"${docker_username}/worklenz-frontend:latest", and if it fails call print_error
with a clear message and return 1 instead of attempting docker push; keep the
existing print_info, docker push, and error handling (print_error/return 1) for
the push step intact so the push only runs when the image exists.
| static buildUpdateQuery(options: { | ||
| table: string; | ||
| set: Record<string, any>; | ||
| where: Array<{ field: string; operator: string; value: any; conjunction?: "AND" | "OR" }>; | ||
| }): ParameterizedQuery { | ||
| const { table, set, where } = options; | ||
|
|
||
| const setEntries = Object.entries(set); | ||
| if (setEntries.length === 0) { | ||
| throw new Error("UPDATE query must have at least one field to set"); | ||
| } | ||
|
|
||
| const params: any[] = []; | ||
| let paramOffset = 1; | ||
|
|
||
| const setClauses = setEntries.map(([field, value]) => { | ||
| params.push(value); | ||
| return `${field} = $${paramOffset++}`; | ||
| }); | ||
|
|
||
| let query = `UPDATE ${table} SET ${setClauses.join(", ")}`; | ||
|
|
||
| if (where.length > 0) { | ||
| const { where: whereClause, params: whereParams } = this.buildWhereClause(where, paramOffset); | ||
| query += ` WHERE ${whereClause}`; | ||
| params.push(...whereParams); | ||
| } | ||
|
|
||
| return { query, params }; | ||
| } |
There was a problem hiding this comment.
table and SET field names are not escaped; empty where silently updates all rows.
Two concerns:
-
table(line 278) and SETfieldkeys (line 275) are interpolated raw — unlikebuildOptionalInClause, they bypassescapeIdentifier. A caller passing an untrusted or malformed name gets no protection. -
If
whereis an empty array, the query omits theWHEREclause entirely, executing an unrestrictedUPDATEon the whole table. Consider requiring at least oneWHEREcondition or adding an explicit opt-in flag (e.g.,allowUpdateAll: true) to prevent accidental full-table updates.
Proposed fix
static buildUpdateQuery(options: {
table: string;
set: Record<string, any>;
where: Array<{ field: string; operator: string; value: any; conjunction?: "AND" | "OR" }>;
+ allowUpdateAll?: boolean;
}): ParameterizedQuery {
- const { table, set, where } = options;
+ const { table, set, where, allowUpdateAll = false } = options;
+
+ if (!where || where.length === 0) {
+ if (!allowUpdateAll) {
+ throw new Error("UPDATE query must have a WHERE clause. Pass allowUpdateAll: true to override.");
+ }
+ }
const setEntries = Object.entries(set);
if (setEntries.length === 0) {
throw new Error("UPDATE query must have at least one field to set");
}
const params: any[] = [];
let paramOffset = 1;
+ const safeTable = this.escapeIdentifier(table);
const setClauses = setEntries.map(([field, value]) => {
params.push(value);
- return `${field} = $${paramOffset++}`;
+ return `${this.escapeIdentifier(field)} = $${paramOffset++}`;
});
- let query = `UPDATE ${table} SET ${setClauses.join(", ")}`;
+ let query = `UPDATE ${safeTable} SET ${setClauses.join(", ")}`;
if (where.length > 0) {🤖 Prompt for AI Agents
In `@worklenz-backend/src/shared/sql-helpers.ts` around lines 258 - 287,
buildUpdateQuery currently interpolates the table name and SET field keys
directly and allows empty where arrays (risking full-table UPDATEs); update
buildUpdateQuery to escape the table and column identifiers using the existing
escapeIdentifier helper when constructing the query (use escaped table name for
the UPDATE clause and escape each set field name when building setClauses) and
preserve parameter numbering logic, and change the signature to accept an
optional allowUpdateAll boolean (default false) or simply throw an Error if
where.length === 0 — enforce that buildWhereClause is invoked only when where
has conditions and throw a clear error (or require allowUpdateAll: true) to
prevent accidental full-table updates.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores