feat: Kanban Board MVP — column CRUD + board view#83
feat: Kanban Board MVP — column CRUD + board view#83rubenvdlinde wants to merge 6 commits intodevelopmentfrom
Conversation
- Add ColumnController with GET/POST/PUT/DELETE endpoints for columns - Add ColumnService with OpenRegister CRUD and project-membership checks - Add column API routes to appinfo/routes.php - Replace ProjectBoard.vue placeholder with full kanban board view showing columns left-to-right with task cards (title, assignee, priority, due date) - Add "Add column" button at right edge of board - Extend projects store with fetchColumns, fetchTasks, fetchBoard actions - Add /projects/:id/board route alias - Add ColumnControllerTest with 8 unit tests covering CRUD + auth - Copy OpenSpec change proposal into openspec/changes/spec/ Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace ternary operators with if/else blocks (PHPCS inline-IF rule) - Fix implicit true comparisons (PHPCS strict comparison rule) - Reorder @param before @NoAdminRequired in docblocks - Remove unused ViewColumnOutline component import - Fix max-attributes-per-line on kanban board container Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…created Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No description provided. |
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (215 total)
PHPUnit Tests
Code coverage: 64.1% (52 / 81 statements) Integration Tests (Newman)Newman integration tests were not enabled for this run. E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
rubenvdlinde
left a comment
rubenvdlinde
left a comment
There was a problem hiding this comment.
Clyde Barcode — Hydra Security Review
SAST scan results: Semgrep (p/security-audit, p/secrets, p/owasp-top-ten): 0 findings · Gitleaks: 0 findings · Trivy: not present
Inline findings are posted as line comments below. See the verdict JSON at the bottom of the PR discussion thread.
src/views/ProjectBoard.vue
Outdated
| <div class="kanban-card__header"> | ||
| <span | ||
| class="kanban-card__priority" | ||
| :class="'kanban-card__priority--' + (task.priority || 'normal')" |
There was a problem hiding this comment.
[WARNING] CSS Class Injection via unsanitised task.priority
:class="'kanban-card__priority--' + (task.priority || 'normal')"The task.priority value from the API is concatenated directly into a CSS class name without any allow-list check. Although Vue's :class binding calls classList.add() and therefore cannot inject HTML, a malicious value (e.g. one containing spaces) will silently add extra, unexpected class tokens to the element. If a future theme or shared stylesheet contains a class that leaks information via CSS counters or triggers unintended layout behaviour, this becomes an exploitable side-channel.
Recommendation: validate task.priority against a closed enum before rendering, e.g.:
const ALLOWED_PRIORITIES = ['low', 'normal', 'high', 'urgent']
const safePriority = ALLOWED_PRIORITIES.includes(task.priority) ? task.priority : 'normal'
lib/Service/ColumnService.php
Outdated
| * | ||
| * @return array The created column | ||
| */ | ||
| public function createColumn(array $data): array |
There was a problem hiding this comment.
[WARNING] No authorization check inside service layer (defense-in-depth gap)
createColumn(), updateColumn(), and deleteColumn() perform no authorization themselves — they trust that the calling controller has already validated project membership. This is a single-layer authorization model: if any future code path (event listener, CLI command, internal API) calls a service method directly, it will bypass the membership check entirely.
Recommendation: add a guard at the top of each mutating service method:
if ($this->isProjectMember($data['project'] ?? '')) === false) {
throw new \RuntimeException('Forbidden');
}Or, at minimum, add a @internal note that callers must perform the membership check before invoking.
| * @return string | ||
| */ | ||
| private function getCurrentUid(): string | ||
| { |
There was a problem hiding this comment.
[INFO] Unauthenticated callers produce an empty UID that could match stale data
getCurrentUid() returns an empty string '' when no user is logged in. The membership check in_array('', $members, true) will correctly return false for well-formed member arrays, but any project record that has inadvertently stored an empty-string member entry (e.g. due to a bug during member removal) would grant access to unauthenticated requests.
Recommendation: reject empty UIDs explicitly:
private function getCurrentUid(): string
{
$user = $this->userSession->getUser();
if ($user === null) {
throw new \RuntimeException('Unauthenticated');
}
return $user->getUID();
}Since all controller actions carry @NoAdminRequired (authentication still required), this should never trigger in practice — but the explicit guard makes the invariant clear.
| Http::STATUS_BAD_REQUEST | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
[INFO] No server-side allow-list validation on type and color fields
$data['type'] ?? 'active' and $data['color'] ?? null are passed through to storage without validation. Accepting arbitrary strings for type can cause the frontend to render unknown column states silently; accepting arbitrary strings for color bypasses the intent of a constrained palette.
Recommendation: add a minimal allow-list before building $columnData:
$allowedTypes = ['active', 'done', 'backlog'];
$type = in_array($data['type'] ?? '', $allowedTypes, true) ? $data['type'] : 'active';For color, validate it matches a hex colour pattern: /^#[0-9a-fA-F]{6}$/.
Clyde Barcode — Security Verdict{
"reviewer": "clyde-barcode",
"reviewed_at": "2026-04-06T11:43:00Z",
"pr": "https://github.com/ConductionNL/planix/pull/83",
"commit": "53fc6bacc018c6561976f40f37003e700727f0ac",
"verdict": "PASS_WITH_WARNINGS",
"sast": {
"semgrep": { "rulesets": ["p/security-audit","p/secrets","p/owasp-top-ten"], "findings": 0 },
"gitleaks": { "findings": 0 },
"trivy": { "status": "not_present" }
},
"findings": [
{
"id": "CB-001",
"severity": "WARNING",
"category": "CSS Class Injection",
"file": "src/views/ProjectBoard.vue",
"line": 99,
"title": "Unsanitised task.priority concatenated into CSS class name",
"detail": "task.priority is concatenated directly into a :class binding without an allow-list check. Vue's classList API prevents HTML injection, but arbitrary class tokens can be added. Validate against enum ['low','normal','high','urgent'] before rendering.",
"owasp": "A03:2021 – Injection"
},
{
"id": "CB-002",
"severity": "WARNING",
"category": "Missing Defense-in-Depth Authorization",
"file": "lib/Service/ColumnService.php",
"line": 168,
"title": "Service-layer mutation methods carry no independent authorization check",
"detail": "createColumn(), updateColumn(), and deleteColumn() trust the caller to have validated project membership. Any future direct invocation of the service bypasses the access check. Add a guard inside each mutating method or document the invariant explicitly.",
"owasp": "A01:2021 – Broken Access Control"
},
{
"id": "CB-003",
"severity": "INFO",
"category": "Unauthenticated Edge Case",
"file": "lib/Service/ColumnService.php",
"line": 76,
"title": "getCurrentUid() returns empty string for unauthenticated callers",
"detail": "An empty UID will silently fail the membership check rather than throwing. A project record with an empty-string member entry would grant access. Explicit rejection is safer.",
"owasp": "A07:2021 – Identification and Authentication Failures"
},
{
"id": "CB-004",
"severity": "INFO",
"category": "Missing Input Validation",
"file": "lib/Controller/ColumnController.php",
"line": 100,
"title": "No allow-list or format validation for column type and color fields",
"detail": "The type field accepts any string; color accepts any string. Recommend validating type against ['active','done','backlog'] and color against /^#[0-9a-fA-F]{6}$/.",
"owasp": "A03:2021 – Injection"
}
],
"summary": {
"critical": 0,
"warning": 2,
"info": 2,
"secrets_detected": false,
"xss_risk": false,
"sqli_risk": false,
"auth_bypass_risk": false
},
"notes": [
"All four controller actions are protected by @NoAdminRequired (authentication enforced by Nextcloud framework) and call isProjectMember() before any data operation — the authorization model is correct at the HTTP layer.",
"Vue template uses only text interpolation ({{ }}) and named :style/:class bindings — no v-html; XSS via template is not possible.",
"No hardcoded credentials, tokens, or secrets found in any changed file.",
"The client-side membership filter in fetchProjects() (projects.js:91-101) is a pre-existing information-disclosure pattern and is not introduced by this PR; it is flagged here for awareness only."
],
"merge_recommendation": "May merge after CB-001 and CB-002 are resolved."
} |
rubenvdlinde
left a comment
rubenvdlinde
left a comment
There was a problem hiding this comment.
Hydra Reviewer — Juan Claude van Damme
Reviewed commit 53fc6ba. Quality gates: PHPCS clean on new files (pre-existing violations in test bootstrap files are out of scope). ESLint clean on changed frontend files. PHPUnit skipped (requires Nextcloud environment).
Found 2 CRITICAL, 4 WARNING, and 3 SUGGESTION issues. See inline comments.
|
|
||
| return new JSONResponse(['success' => true]); | ||
|
|
||
| }//end destroy() |
There was a problem hiding this comment.
[CRITICAL] DELETE returns 200 instead of 204 No Content
destroy() returns new JSONResponse(['success' => true]) (HTTP 200) on success. According to the NL API strategie / Common Ground API standard, a successful DELETE MUST return HTTP 204 No Content with no response body.
Additionally, the return value of $this->columnService->deleteColumn($id) is never checked. If deletion fails (e.g. OpenRegister returns false), the controller silently returns 200 OK — a false success signal to the caller.
Fix:
$success = $this->columnService->deleteColumn($id);
if ($success === false) {
return new JSONResponse(
['error' => 'Column could not be deleted.'],
Http::STATUS_INTERNAL_SERVER_ERROR
);
}
return new JSONResponse(null, Http::STATUS_NO_CONTENT);
lib/Controller/ColumnController.php
Outdated
|
|
||
| $this->columnService->deleteColumn($id); | ||
|
|
||
| return new JSONResponse(['success' => true]); |
There was a problem hiding this comment.
[CRITICAL] DELETE returns HTTP 200 instead of 204 No Content — and silently swallows deletion failures
destroy() returns new JSONResponse(['success' => true]) (HTTP 200) on success. Per the NL API strategie / Common Ground API standard, a successful DELETE MUST return HTTP 204 No Content with no body.
A secondary bug: the return value of deleteColumn() is never checked. If OpenRegister fails to delete the object, the controller returns 200 OK — a false-success signal to the caller.
$deleted = $this->columnService->deleteColumn($id);
if ($deleted === false) {
return new JSONResponse(
['error' => 'Column could not be deleted.'],
Http::STATUS_INTERNAL_SERVER_ERROR
);
}
return new JSONResponse(null, Http::STATUS_NO_CONTENT);
lib/Controller/ColumnController.php
Outdated
| } | ||
|
|
||
| $columnData = [ | ||
| 'title' => $data['title'] ?? '', |
There was a problem hiding this comment.
[WARNING] Empty title is silently accepted
$data['title'] ?? '' falls back to an empty string when title is absent, and no subsequent validation rejects it. The spec's data model marks title as required. A column with an empty title will be created in OpenRegister with no error returned.
$title = trim($data['title'] ?? '');
if ($title === '') {
return new JSONResponse(
['error' => 'The title field is required.'],
Http::STATUS_BAD_REQUEST
);
}
// use $title in $columnData
appinfo/routes.php
Outdated
| // Column CRUD endpoints. | ||
| ['name' => 'column#index', 'url' => '/api/columns', 'verb' => 'GET'], | ||
| ['name' => 'column#create', 'url' => '/api/columns', 'verb' => 'POST'], | ||
| ['name' => 'column#update', 'url' => '/api/columns/{id}', 'verb' => 'PUT'], |
There was a problem hiding this comment.
[WARNING] PUT used for a partial-update operation — should be PATCH
The update() controller method only applies fields that are present in the request body (title, order, wipLimit, color, type are all optional). This is the HTTP definition of a partial update, which MUST use PATCH per RFC 7231 and the NL API strategie API design rules (API-30 / API-33). PUT semantics imply a full replacement of the resource.
Change this route to 'verb' => 'PATCH' and update the frontend store and any API consumers accordingly.
lib/Service/ColumnService.php
Outdated
| $objectService = $this->getObjectService(); | ||
| $project = $objectService->findObject(register: 'planix', schema: 'project', id: $projectId); | ||
|
|
||
| if ($project === null) { |
There was a problem hiding this comment.
[WARNING] Non-existent project returns false, surfaced as 403 instead of 404
When findObject() returns null for an unknown project ID, isProjectMember() returns false. The controller then responds with HTTP 403 Forbidden, misleading clients into thinking they lack permission rather than that the project doesn't exist.
Consider returning a tri-state (e.g. a custom result object or exception), or adding a separate projectExists() check in the controller before the membership check, so a missing project can return 404.
| self::assertSame(expected: Http::STATUS_CREATED, actual: $result->getStatus()); | ||
| self::assertSame(expected: 'col-new', actual: $result->getData()['id']); | ||
|
|
||
| }//end testCreateReturnsCreatedColumn() |
There was a problem hiding this comment.
[WARNING] Missing test cases for create() 400 and 403 paths
testCreateReturnsCreatedColumn() covers the happy path, but two error paths lack tests:
create()with no project field returns 400 — no test existscreate()by a non-member returns 403 — no test exists
Both paths exist in the controller. The 400 path is especially important since a valid spec entry requires a project ID. Please add:
testCreateReturnsBadRequestWithoutProject()testCreateReturnsForbiddenForNonMember()
| } | ||
|
|
||
| .kanban-card__priority--low { | ||
| background: #95a5a6; |
There was a problem hiding this comment.
[SUGGESTION] Priority dot colours use raw hex — use CSS custom properties instead
Hardcoded hex values like #95a5a6, #3498db, #f39c12, #e74c3c do not respect the Nextcloud theme, break dark-mode contrast, and differ from the palette used elsewhere in the app.
Consider mapping to semantic tokens, e.g.:
.kanban-card__priority--low { background: var(--color-text-maxcontrast); }
.kanban-card__priority--normal { background: var(--color-primary-element); }
.kanban-card__priority--high { background: var(--color-warning); }
.kanban-card__priority--urgent { background: var(--color-error); }
| :key="task.id" | ||
| class="kanban-card" | ||
| role="listitem" | ||
| tabindex="0"> |
There was a problem hiding this comment.
[SUGGESTION] Task cards have tabindex="0" but no keyboard activation handler
The cards are focusable via Tab, which is good for keyboard navigation, but pressing Enter/Space on a focused card does nothing. The spec mentions "Board is keyboard-navigable (WCAG AA)". Once card navigation to a detail view is wired up, add @keydown.enter="openTask(task)" alongside the focus styling already present.
| filters: ['project' => $projectId], | ||
| ); | ||
|
|
||
| usort( |
There was a problem hiding this comment.
[SUGGESTION] usort() closure indentation is visually inconsistent
The opening and closing parens of the usort() call sit at odd indentation levels compared to the rest of the file. While PHPCS passes, the multi-level indent inside the callback is harder to read than the flat-closure style used in similar projects.
Tidier form:
usort($columns, static fn(array $a, array $b): int => ($a['order'] ?? 0) <=> ($b['order'] ?? 0));(single line is fine given the simplicity of the comparator)
Hydra Reviewer — Verdict{
"reviewer": "Juan Claude van Damme",
"pr": "ConductionNL/planix#83",
"commit": "53fc6bacc018c6561976f40f37003e700727f0ac",
"verdict": "CHANGES_REQUESTED",
"quality_gates": {
"phpcs_new_files": "PASS",
"phpcs_pre_existing": "FAIL (out of scope — violations in tests/bootstrap.php pre-date this PR)",
"eslint": "PASS",
"phpunit": "SKIPPED (requires Nextcloud environment)"
},
"findings": [
{
"id": "F-01",
"severity": "CRITICAL",
"file": "lib/Controller/ColumnController.php",
"line": 217,
"title": "DELETE returns HTTP 200 instead of 204; deletion failures silently return 200 OK",
"detail": "destroy() returns JSONResponse(['success'=>true], 200). NL API strategie requires 204 No Content for successful DELETE. Return value of deleteColumn() is unchecked — a storage failure returns 200 to the client."
},
{
"id": "F-02",
"severity": "WARNING",
"file": "lib/Controller/ColumnController.php",
"line": 114,
"title": "Empty title accepted in create() — title is required per data model",
"detail": "$data['title'] ?? '' falls back to empty string; no subsequent guard rejects it. Spec marks title as required."
},
{
"id": "F-03",
"severity": "WARNING",
"file": "appinfo/routes.php",
"line": 16,
"title": "PUT used for partial-update semantics — should be PATCH",
"detail": "update() applies only provided fields (partial update). HTTP/RFC 7231 and NL API strategie require PATCH for partial updates; PUT implies full resource replacement."
},
{
"id": "F-04",
"severity": "WARNING",
"file": "lib/Service/ColumnService.php",
"line": 99,
"title": "Non-existent project returns false from isProjectMember(), surfaced as 403 not 404",
"detail": "GET /api/columns?projectId=nonexistent returns 403 Forbidden instead of 404 Not Found, misleading clients about the cause of failure."
},
{
"id": "F-05",
"severity": "WARNING",
"file": "tests/unit/Controller/ColumnControllerTest.php",
"line": 190,
"title": "create() error paths (400 missing project, 403 non-member) have no tests",
"detail": "Only the happy path is tested. Both error branches exist in the controller and should be covered."
},
{
"id": "F-06",
"severity": "SUGGESTION",
"file": "src/views/ProjectBoard.vue",
"line": 447,
"title": "Priority dot colours use raw hex values instead of Nextcloud CSS custom properties",
"detail": "Hard-coded hex breaks dark mode and theming. Use --color-primary-element, --color-warning, --color-error etc."
},
{
"id": "F-07",
"severity": "SUGGESTION",
"file": "src/views/ProjectBoard.vue",
"line": 95,
"title": "Task cards have tabindex=0 but no keyboard activation handler (WCAG AA gap)",
"detail": "Cards are focusable but Enter/Space does nothing. Add @keydown.enter when task detail navigation is implemented."
},
{
"id": "F-08",
"severity": "SUGGESTION",
"file": "lib/Service/ColumnService.php",
"line": 130,
"title": "usort() closure uses unusual multi-level indentation",
"detail": "Minor readability issue; consider fn arrow shorthand for this trivial comparator."
}
],
"summary": "The MVP scope (tasks.md) is fully implemented and PHPCS/ESLint are clean on all new files. Two blockers must be fixed before merge: the DELETE HTTP status / silent-failure bug (F-01), which violates the NL API strategie, and the four WARNINGs (F-02 through F-05) which are correctness or contract gaps. The three SUGGESTIONs are informational and may be deferred."
} |
Hydra Reviewer — Verdict```json |
F-01: destroy() now returns 204 No Content on success (NL API strategie) and
checks deleteColumn() return value — storage failures now surface as 500.
F-02: create() rejects empty title with 400 Bad Request; title is required per spec.
F-03: PATCH replaces PUT in routes.php — partial-update semantics per RFC 7231
and NL API strategie.
F-04: index() and create() now call findProject() first; non-existent project
returns 404 Not Found rather than 403 Forbidden.
F-05: Added testCreateReturnsBadRequestWithoutProject(),
testCreateReturnsBadRequestWithEmptyTitle(),
testCreateReturnsForbiddenForNonMember(), and
testIndexReturnsNotFoundForNonExistentProject(). Updated existing tests
for new findProject() call and 204 status on destroy.
CB-001: task.priority is now validated through sanitizePriority() against
allow-list ['low','normal','high','urgent'] before use as a CSS class token.
CB-002: createColumn(), updateColumn(), deleteColumn() each contain an
independent isProjectMember() guard for defense-in-depth; direct
service invocations cannot bypass the access check.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Hydra Builder — Fix iteration 1 Fixed findings:
Remaining SUGGESTIONs (not addressed — informational only):
Quality gates after fixes: PHPCS ✅ · ESLint ✅ · PHPUnit skipped (requires Nextcloud environment, consistent with original run) |
|
No description provided. |
Hydra Security Review — Finding #1 [WARNING]File: DescriptionBoth The exception message contains the raw project UUID: throw new \RuntimeException('Forbidden: caller is not a member of project '.$projectId);Depending on error handler configuration, this message may surface in server logs or API error responses, leaking an internal resource ID to an unauthorized caller. RiskAn unauthenticated or unauthorized caller who can trigger the race condition would receive a 500 response instead of a 403, and the project UUID may be exposed in the error payload. RecommendationCatch try {
$updated = $this->columnService->updateColumn($id, $updateData);
} catch (\RuntimeException $e) {
return new JSONResponse(['error' => 'Forbidden.'], Http::STATUS_FORBIDDEN);
}Apply the same pattern in Clyde Barcode — Hydra Security Reviewer |
Hydra Security Review — Finding #2 [WARNING]File: DescriptionThe // create() — no validation
'type' => $data['type'] ?? 'active',
// update() — no validation
if (isset($data['type']) === true) {
$updateData['type'] = $data['type'];
}An authenticated project member can store arbitrary strings (e.g. RiskStored arbitrary data in a schema-constrained field undermines data integrity and creates a latent XSS / CSS injection surface for any future consumer of the RecommendationAllowlist-validate $allowedTypes = ['active', 'done'];
$type = $data['type'] ?? 'active';
if (!in_array($type, $allowedTypes, true)) {
return new JSONResponse(['error' => 'Invalid type value.'], Http::STATUS_BAD_REQUEST);
}Clyde Barcode — Hydra Security Reviewer |
Hydra Security Review — Finding #3 [SUGGESTION]File: DescriptionThe $title = $data['title'] ?? '';
if (empty($title) === true) { ... }
// No max-length check followsSimilarly, RiskAn authenticated project member could store multi-megabyte strings in column metadata, potentially affecting storage performance and downstream rendering. This is a low-severity denial-of-service surface limited to project members. RecommendationAdd a reasonable max-length guard at the API boundary: if (mb_strlen($title) > 255) {
return new JSONResponse(['error' => 'Title must not exceed 255 characters.'], Http::STATUS_BAD_REQUEST);
}Clyde Barcode — Hydra Security Reviewer |
Hydra Security Review — Finding #4 [SUGGESTION]File: DescriptionThe 'color' => $data['color'] ?? null,The current frontend does not render this field via inline styles in this PR, but the data model documents it as a hex color. Any future component that renders it via RiskLatent CSS injection if RecommendationValidate the color as a 3- or 6-digit hex string at the API boundary: if ($data['color'] !== null && !preg_match('/^#[0-9a-fA-F]{3}(?:[0-9a-fA-F]{3})?$/', $data['color'])) {
return new JSONResponse(['error' => 'Color must be a valid hex value (e.g. #ff0000).'], Http::STATUS_BAD_REQUEST);
}Clyde Barcode — Hydra Security Reviewer |
Hydra Security Review — Finding #5 [SUGGESTION]File: DescriptionWhen a column is deleted, tasks are migrated to the backlog sequentially in a foreach ($tasks as $task) {
$objectService->saveObject(
register: 'planix',
schema: 'task',
object: ['id' => $task['id'], 'column' => null, 'columnOrder' => 0],
);
}
return $objectService->deleteObject(register: 'planix', schema: 'column', id: $id);If any RiskData loss / orphaned task records if column deletion fails partway through. No security exploit path, but can lead to user data being silently lost. RecommendationIf OpenRegister supports batch operations or transactions, prefer those. At minimum, log a warning if any individual task migration fails and consider aborting the column deletion if any task save fails: $failed = [];
foreach ($tasks as $task) {
$result = $objectService->saveObject(...);
if ($result === false) {
$failed[] = $task['id'];
}
}
if (!empty($failed)) {
$this->logger->error('Planix: column delete aborted — task migration failed', ['failed' => $failed]);
return false; // caller returns 500
}Clyde Barcode — Hydra Security Reviewer |
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (215 total)
PHPUnit Tests
Code coverage: 71.5% (73 / 102 statements) Integration Tests (Newman)Newman integration tests were not enabled for this run. E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
{
"review": "hydra-security",
"pr": 83,
"repo": "ConductionNL/planix",
"reviewer": "clyde-barcode",
"date": "2026-04-06",
"tools": {
"semgrep": { "version": "1.135.0", "findings": 0 },
"gitleaks": { "findings": 0 },
"trivy": { "status": "not_present" }
},
"findings": [
{
"id": "CSR-001",
"severity": "WARNING",
"title": "Unhandled RuntimeException leaks project UUID on TOCTOU race",
"file": "lib/Controller/ColumnController.php",
"locations": ["update()", "destroy()"],
"cwe": "CWE-209",
"summary": "Service-layer membership guard throws RuntimeException that the controller does not catch; exception message contains the raw project UUID and may surface in error responses or logs."
},
{
"id": "CSR-002",
"severity": "WARNING",
"title": "Column type field not validated against enum — stored XSS surface",
"file": "lib/Controller/ColumnController.php",
"locations": ["create()", "update()"],
"cwe": "CWE-20",
"summary": "The type field accepts arbitrary strings instead of enforcing the active/done enum, creating a latent stored XSS / data-integrity risk for future consumers of the field."
},
{
"id": "CSR-003",
"severity": "SUGGESTION",
"title": "No maximum length constraint on title and other string fields",
"file": "lib/Controller/ColumnController.php",
"locations": ["create()", "update()"],
"cwe": "CWE-400",
"summary": "Unbounded string inputs from authenticated project members could cause storage bloat; low-severity DoS surface."
},
{
"id": "CSR-004",
"severity": "SUGGESTION",
"title": "Unvalidated color field — latent CSS injection surface",
"file": "lib/Controller/ColumnController.php",
"locations": ["create()", "update()"],
"cwe": "CWE-20",
"summary": "The color field accepts arbitrary strings. Not currently rendered via inline styles, but if a future component binds it to v-bind:style without validation it would be a CSS injection vector."
},
{
"id": "CSR-005",
"severity": "SUGGESTION",
"title": "Non-atomic task migration on column deletion",
"file": "lib/Service/ColumnService.php",
"locations": ["deleteColumn()"],
"cwe": "CWE-362",
"summary": "Tasks are moved to backlog in a non-transactional loop; partial failure leaves orphaned task records referencing a deleted column."
}
],
"verdict": "CHANGES_REQUESTED",
"merge_recommendation": "Do not merge until CSR-001 and CSR-002 are resolved. CSR-003 through CSR-005 are informational and may be addressed in a follow-up.",
"positive_observations": [
"Defense-in-depth membership re-check in ColumnService (createColumn, updateColumn, deleteColumn) is a good pattern.",
"Controller correctly validates projectId presence before any authorization check.",
"sanitizePriority() in the frontend correctly allowlists priority values before CSS class binding.",
"No v-html usage in ProjectBoard.vue — all user data rendered via safe text interpolation.",
"No secrets, credentials, or tokens detected in the diff (Gitleaks: 0 findings).",
"CSRF protection is active on all state-changing endpoints (no @NoCSRFRequired annotation present).",
"EUPL-1.2 license headers present on all new PHP files."
]
} |
Hydra Security Review — Round 3 — Fix VerificationReviewer: Clyde Barcode (Hydra Security Reviewer, Conduction B.V.) What was fixed in fix iteration 2CSR-001 — RESOLVED ✅ Additionally, Remaining open findings |
Hydra Security Review — Finding CSR-002 [WARNING] — Round 3 (Still Open)File: DescriptionThe // create() — line 143
'type' => $data['type'] ?? 'active', // any string accepted
// update() — lines 202–204
if (isset($data['type']) === true) {
$updateData['type'] = $data['type']; // no allow-list check
}This was flagged in round 2 as a blocking WARNING. It has not been addressed in fix iteration 2. An authenticated project member can store an arbitrary string as the column type. Frontend components that branch on Required FixAdd an allow-list check before accepting the $allowedTypes = ['active', 'done'];
$type = $data['type'] ?? 'active';
if (in_array($type, $allowedTypes, true) === false) {
return new JSONResponse(
['error' => 'Invalid type. Allowed values: active, done.'],
Http::STATUS_BAD_REQUEST
);
}Apply the same guard in |
Hydra Security Review — Findings CSR-003/004/005 [SUGGESTION] — Round 3 (Still Open)These three suggestions were flagged in round 2 and have not been addressed in fix iteration 2. They remain informational; they do not block merge on their own, but are tracked here for completeness. CSR-003 — No maximum length constraint on CSR-004 — The CSR-005 — Task migration on column deletion is non-atomic ( |
{
"review": "hydra-security",
"round": 3,
"pr": 83,
"repo": "ConductionNL/planix",
"reviewer": "clyde-barcode",
"date": "2026-04-06",
"commit": "803cf835c86326b6e31c4d26acca53b19dcea6c0",
"tools": {
"semgrep": { "version": "1.135.0", "rulesets": ["p/security-audit","p/secrets","p/owasp-top-ten"], "findings": 0 },
"gitleaks": { "findings": 0 },
"trivy": { "status": "not_present" }
},
"resolved_since_round_2": [
{
"id": "CSR-001",
"title": "Unhandled RuntimeException leaks project UUID on TOCTOU race",
"resolution": "Both update() and destroy() now catch RuntimeException and return a generic 500. findColumn() also wraps getObjectService() in try/catch. CLOSED."
}
],
"findings": [
{
"id": "CSR-002",
"severity": "WARNING",
"status": "OPEN",
"title": "Column type field not validated against enum",
"file": "lib/Controller/ColumnController.php",
"locations": ["create() line 143", "update() lines 202-204"],
"cwe": "CWE-20",
"owasp": "A03:2021",
"summary": "The type field accepts arbitrary strings; only active and done are permitted per spec. Latent stored XSS surface and data-integrity risk. Fix: add in_array() allow-list check before accepting the value."
},
{
"id": "CSR-003",
"severity": "SUGGESTION",
"status": "OPEN",
"title": "No maximum length constraint on title and other string fields",
"file": "lib/Controller/ColumnController.php",
"cwe": "CWE-400",
"summary": "Authenticated project members can store arbitrarily long title strings. Add a max-length check (e.g. 255 chars)."
},
{
"id": "CSR-004",
"severity": "SUGGESTION",
"status": "OPEN",
"title": "Unvalidated color field — latent CSS injection surface",
"file": "lib/Controller/ColumnController.php",
"cwe": "CWE-20",
"summary": "The color field accepts arbitrary strings. Validate against /^#[0-9a-fA-F]{6}$/ before storing."
},
{
"id": "CSR-005",
"severity": "SUGGESTION",
"status": "OPEN",
"title": "Non-atomic task migration on column deletion",
"file": "lib/Service/ColumnService.php",
"locations": ["deleteColumn() lines 283-299"],
"cwe": "CWE-362",
"summary": "Task backlog migration is non-transactional; partial failure leaves orphaned task records. Address in a follow-up."
}
],
"verdict": "CHANGES_REQUESTED",
"merge_recommendation": "Do not merge until CSR-002 is resolved. CSR-003 through CSR-005 are informational and may be tracked as follow-up issues.",
"summary": {
"critical": 0,
"warning": 1,
"suggestion": 3,
"secrets_detected": false,
"xss_risk": false,
"sqli_risk": false,
"auth_bypass_risk": false
},
"positive_observations": [
"CSR-001 fully resolved: RuntimeException is now caught in both update() and destroy(); generic error message returned, UUID not leaked.",
"findColumn() now has exception guard consistent with findProject() — good defense in depth.",
"All SAST tools (Semgrep, Gitleaks) continue to return zero findings on this commit.",
"No secrets, credentials, or tokens in any changed file.",
"Authorization model (isProjectMember check before every mutation, plus service-layer re-check) remains correct.",
"No v-html usage in ProjectBoard.vue; sanitizePriority() allow-list in frontend remains intact.",
"CSRF protection active on all state-changing endpoints (no @NoCSRFRequired annotation present).",
"EUPL-1.2 license headers present on all new PHP files."
]
} |
rubenvdlinde
left a comment
rubenvdlinde
left a comment
There was a problem hiding this comment.
Hydra Code Review — Juan Claude van Damme
PR: feat: Kanban Board MVP — column CRUD + board view
Reviewer: Juan Claude van Damme (Hydra Reviewer)
Date: 2026-04-06
Quality checks run
| Check | Result |
|---|---|
composer cs:check (PHPCS PSR-12) |
✅ PASSED — no violations on lib/ |
composer psalm |
✅ PASSED — no errors (2 info-level notes) |
composer test:unit |
SettingsControllerTest. The test logic was reviewed manually and is structurally sound. |
Summary
The implementation ships a working Kanban board MVP with backend Column CRUD (ColumnController + ColumnService) and a read-only board view (ProjectBoard.vue). The code is well-structured, PSR-12 compliant, follows Conduction patterns, and includes meaningful test coverage. Two WARNING findings need to be addressed before merge.
| 'wipLimit' => $wipLimit, | ||
| 'color' => $data['color'] ?? null, | ||
| 'type' => $data['type'] ?? 'active', | ||
| ]; |
There was a problem hiding this comment.
[WARNING] Empty PATCH body may cause unintended data loss — lib/Controller/ColumnController.php line ~202-229
update() builds $updateData from recognised fields only. If the request body contains none of these keys (title, order, wipLimit, color, type), $updateData = [] and updateColumn($id, []) is called.
In ColumnService::updateColumn() this becomes:
$objectService->saveObject(register: 'planix', schema: 'column', object: ['id' => $id]);If OpenRegister's saveObject uses full-replace semantics (not merge/patch) this will silently erase all column fields. Even with merge semantics, accepting an empty PATCH violates RFC 5789 §2 (a PATCH request MUST contain a patch document).
Fix: add an early return before the service call:
if (empty($updateData) === true) {
return new JSONResponse(['error' => 'No updatable fields provided.'], Http::STATUS_BAD_REQUEST);
}
| { | ||
| $projectId = $data['project'] ?? ''; | ||
| if ($this->isProjectMember(projectId: $projectId) === false) { | ||
| throw new \RuntimeException('Forbidden: caller is not a member of project '.$projectId); |
There was a problem hiding this comment.
[WARNING] Redundant project lookup in createColumn()
The controller already verifies project membership before calling createColumn(). The service's defense-in-depth check calls isProjectMember($projectId) without passing the pre-fetched project, triggering another findProject() DB call — so every successful POST /api/columns makes two findProject round-trips.
Either pass the project object for the re-check:
if ($this->isProjectMember(projectId: $projectId, project: $projectData) === false) {…or document that the controller is the sole authorization gate and remove the service-level re-check.
The same pattern exists in updateColumn() and deleteColumn() (which re-call findColumn() after the controller already fetched it).
| /** | ||
| * Delete a column. Tasks in this column are moved to the backlog. | ||
| * | ||
| * @param string $id The column UUID |
There was a problem hiding this comment.
[SUGGESTION] Dead code — the $deleted === false branch is unreachable
deleteColumn() either returns true (success) or throws \RuntimeException (failure). The catch block above already handles the exception, so the if ($deleted === false) check can never be true — the variable will always be true at that point.
Safe to remove lines 278-283:
// These lines are unreachable:
if ($deleted === false) {
return new JSONResponse(['error' => 'Failed to delete column.'], Http::STATUS_INTERNAL_SERVER_ERROR);
}
| * | ||
| * @param {string} projectId Parent project ID | ||
| * @return {Promise<{columns: Array, tasks: Array}>} | ||
| */ |
There was a problem hiding this comment.
[SUGGESTION] Errors swallowed silently — no user feedback on board load failure
Both fetchColumns and fetchTasks catch all errors, log to console.error, and return []. The Vue component gets an empty board with no indication of whether the data is truly empty or whether loading failed.
Consider setting a store error state on failure so the board can show an error banner instead of an empty board:
} catch (err) {
console.error('fetchColumns error:', err)
this.columns = []
this.error = 'Failed to load board columns.' // surface to UI
return []
}
| @@ -0,0 +1,265 @@ | |||
| <?php | |||
There was a problem hiding this comment.
[SUGGESTION] SPDX license comment header missing
The Conduction Common Ground convention (CLAUDE.md) requires a machine-readable SPDX comment at the top of every new file:
// SPDX-License-Identifier: EUPL-1.2
// Copyright (C) 2026 Conduction B.V.The current @license PHPDoc tag is human-readable but not SPDX-compliant. The same applies to ColumnService.php. Existing files in the repo also lack this header, so this is consistent with the codebase today — but worth aligning in new files going forward.
{
"hydra_review": {
"reviewer": "Juan Claude van Damme",
"pr": "ConductionNL/planix#83",
"reviewed_at": "2026-04-06T12:18:00Z",
"commit": "803cf835c86326b6e31c4d26acca53b19dcea6c0",
"verdict": "REQUEST_CHANGES",
"quality_checks": {
"phpcs_psr12": "PASSED",
"psalm": "PASSED",
"phpunit": "SKIPPED — requires Nextcloud runtime (OC_App not available outside NC; same constraint as existing tests)"
},
"findings": [
{
"id": "W1",
"severity": "WARNING",
"file": "lib/Controller/ColumnController.php",
"line": 202,
"title": "Empty PATCH body may cause data loss",
"description": "update() passes $updateData = [] to updateColumn() when no recognised fields are in the request. If OpenRegister saveObject uses full-replace semantics, all column fields would be silently erased. Return 400 when $updateData is empty.",
"must_fix": true
},
{
"id": "W2",
"severity": "WARNING",
"file": "lib/Service/ColumnService.php",
"line": 501,
"title": "Redundant project lookup in createColumn()",
"description": "createColumn() calls isProjectMember() without the pre-fetched project, triggering an extra findProject() DB round-trip the controller already made. Pass the project object or remove the defence-in-depth re-check.",
"must_fix": true
},
{
"id": "S1",
"severity": "SUGGESTION",
"file": "lib/Controller/ColumnController.php",
"line": 278,
"title": "Dead code: destroy() $deleted === false check is unreachable",
"description": "deleteColumn() either returns true or throws RuntimeException. The if ($deleted === false) block after the try/catch can never be reached.",
"must_fix": false
},
{
"id": "S2",
"severity": "SUGGESTION",
"file": "src/store/projects.js",
"line": 820,
"title": "fetchColumns / fetchTasks swallow errors silently",
"description": "On fetch failure the store returns [] with no error state, making a load failure indistinguishable from an empty board.",
"must_fix": false
},
{
"id": "S3",
"severity": "SUGGESTION",
"file": "lib/Controller/ColumnController.php",
"line": 1,
"title": "SPDX license comment header missing from new PHP files",
"description": "Conduction standard requires // SPDX-License-Identifier: EUPL-1.2 and // Copyright (C) 2026 Conduction B.V. at the top of every new file. Consistent with existing codebase but deviates from CLAUDE.md spec.",
"must_fix": false
}
],
"positive_notes": [
"PHPCS PSR-12 clean across all new PHP files",
"Psalm reports zero errors on ColumnController and ColumnService",
"Authorization checks on all four endpoints (index, create, update, destroy)",
"Defense-in-depth membership re-check in service layer",
"Task-to-backlog migration on column delete is correctly implemented",
"14 unit tests cover all primary flows including 400/403/404/500 branches",
"Vue board component uses sanitizePriority() to prevent CSS injection from untrusted data",
"ARIA roles and keyboard focus on task cards (WCAG AA baseline)",
"Implementation deviation from spec is clearly documented in design.md and tasks.md"
]
}
} |
…nt project lookup - [WARNING] CSR-002: add allow-list check for column type field in create() and update() against ['active','done'] before storing; invalid values return 400 Bad Request - [WARNING] W1: update() now returns 400 Bad Request when the PATCH body contains no recognised fields (empty $updateData), preventing silent no-op or full-replace - [WARNING] W2: createColumn() now accepts an optional pre-fetched ?array $project=null parameter and passes it through to isProjectMember(), eliminating the redundant findProject() round-trip in the controller while preserving the defence-in-depth re-check for any future direct service invocations - Tests: added testCreateReturnsBadRequestWithInvalidType(), testUpdateReturnsBadRequestWithInvalidType(), and testUpdateReturnsBadRequestWithEmptyBody()
|
Hydra Builder — Fix iteration 3 Fixed findings:
New tests added:
Remaining SUGGESTIONs (not addressed — informational only):
Quality gates: PHPCS PSR-12 ✅ (0 violations on all changed files) · PHPUnit skipped (requires Nextcloud OCP environment, consistent with all prior runs) |
|
No description provided. |
|
[SECURITY] WARNING — File: The 'color' => $data['color'] ?? null,While Vue's Recommendation: Validate if ($data['color'] !== null && !preg_match('/^#[0-9a-fA-F]{3,8}$/', (string) $data['color'])) {
return new JSONResponse(['error' => 'Invalid color format. Use a hex colour (e.g. #ff0000).'], Http::STATUS_BAD_REQUEST);
}Severity: WARNING — not currently exploitable given the Vue frontend, but a defence-in-depth gap that should be closed before more views consume Hydra Security Reviewer — Clyde Barcode |
|
[SECURITY] WARNING — File: The WIP limit is cast to $wipLimit = (int) $data['wipLimit'];A caller can submit Recommendation: Reject non-positive wipLimit values explicitly: $wipLimit = (int) $data['wipLimit'];
if ($wipLimit < 0) {
return new JSONResponse(
['error' => 'wipLimit must be a non-negative integer.'],
Http::STATUS_BAD_REQUEST
);
}Severity: WARNING — no security exploit, but incorrect domain input is currently accepted and persisted. Hydra Security Reviewer — Clyde Barcode |
|
[SECURITY] WARNING — Column File: The title field is validated as non-empty but has no upper length bound: if (empty($title) === true) { ... }A request with a 10 MB title string will be accepted, stored in OpenRegister, and later returned in every Recommendation: Add a maximum length check (e.g. 255 characters is sufficient for a column name): if (mb_strlen($title) > 255) {
return new JSONResponse(
['error' => 'Title must not exceed 255 characters.'],
Http::STATUS_BAD_REQUEST
);
}Severity: WARNING — no direct RCE or data breach, but allows storage-level DoS against a project. Hydra Security Reviewer — Clyde Barcode |
|
[SECURITY] INFO — All projects over-fetched before client-side membership filter File:
// Fetch all projects; client-side filter below keeps only member projects.
// Note: server-side `members[]` array filter uses PostgreSQL jsonb syntax
// which is incompatible with MariaDB — do not pass it as a query param.
const results = await objectStore.fetchCollection(PROJECT_SCHEMA, params)
this.projects = uid
? results.filter((p) => Array.isArray(p.members) && p.members.includes(uid))
: resultsThe in-code comment acknowledges the MariaDB/PostgreSQL incompatibility, but the consequence is that every authenticated user receives every project's metadata over the wire before the filter is applied. A user who intercepts their own browser traffic can enumerate all project titles, IDs, member lists, and other metadata for projects they are not a member of. Recommendation: This should be addressed at the API / OpenRegister level with a proper server-side filter. As an intermediate mitigation, consider splitting the endpoint: one lightweight endpoint returns only the current user's member-project IDs; a separate endpoint returns full project detail by ID (already gated by membership in the column endpoints). Severity: INFO — no direct privilege escalation (write operations are properly gated), but constitutes an information disclosure that would not pass a GDPR or ISO 27001 review. Hydra Security Reviewer — Clyde Barcode |
{
"verdict": "PASS_WITH_WARNINGS",
"pr": "https://github.com/ConductionNL/planix/pull/83",
"reviewer": "Clyde Barcode — Hydra Security Reviewer",
"reviewed_at": "2026-04-06",
"sast": {
"semgrep": { "status": "clean", "findings": 0 },
"gitleaks": { "status": "clean", "findings": 0 },
"trivy": { "status": "not_present" }
},
"manual_findings": [
{
"id": "SEC-001",
"severity": "WARNING",
"title": "color field stored without format validation",
"file": "lib/Controller/ColumnController.php",
"locations": ["ColumnController::create ~L152", "ColumnController::update ~L207"],
"cwe": "CWE-20 (Improper Input Validation)",
"blocking": false
},
{
"id": "SEC-002",
"severity": "WARNING",
"title": "wipLimit accepts negative integers",
"file": "lib/Controller/ColumnController.php",
"locations": ["ColumnController::create ~L153", "ColumnController::update ~L216"],
"cwe": "CWE-20 (Improper Input Validation)",
"blocking": false
},
{
"id": "SEC-003",
"severity": "WARNING",
"title": "Column title has no maximum length limit",
"file": "lib/Controller/ColumnController.php",
"locations": ["ColumnController::create ~L129", "ColumnController::update ~L196"],
"cwe": "CWE-400 (Uncontrolled Resource Consumption)",
"blocking": false
},
{
"id": "SEC-004",
"severity": "INFO",
"title": "All projects over-fetched before client-side membership filter",
"file": "src/store/projects.js",
"locations": ["fetchProjects L91-101"],
"cwe": "CWE-359 (Exposure of Private Personal Information)",
"blocking": false,
"note": "Pre-existing pattern; comment in code acknowledges the workaround. Not introduced by this PR."
}
],
"positive_observations": [
"Authorization is properly enforced server-side with defence-in-depth (controller + service layer both check project membership)",
"CSRF protection is active for all state-changing endpoints (Nextcloud framework default; no @NoCSRFRequired annotation present)",
"No hardcoded credentials or secrets",
"No SQL injection surface — OpenRegister abstraction used throughout",
"XSS is mitigated: Vue template interpolation escapes by default; sanitizePriority() whitelists CSS class suffixes",
"Sensitive exceptions are caught and generic error messages are returned to clients",
"Task-to-backlog migration on column delete is atomic within the service and membership-checked"
],
"summary": "No critical or blocking security issues found. Three warnings relate to missing input validation hardening (color format, wipLimit bounds, title length) that should be addressed before the feature ships to production. One info finding documents a pre-existing over-fetch pattern that is not introduced by this PR but warrants a follow-up ticket."
} |
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (215 total)
PHPUnit Tests
Code coverage: 84.8% (112 / 132 statements) Integration Tests (Newman)Newman integration tests were not enabled for this run. E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
|
Hydra Code Reviewer — Finding W1 Severity: WARNING The This means:
The design.md explicitly documents that the intended architecture is direct objectStore access (no new PHP controllers), but Recommended fix: Either route frontend column operations through |
|
Hydra Code Reviewer — Finding W2 Severity: WARNING
If OpenRegister goes down after the project-membership check succeeds, the exception propagates as an unhandled 500, bypassing Nextcloud's standard error-response formatting and potentially leaking a stack trace. All other service methods in this file already follow the guard-and-return-null or catch-and-rethrow pattern. Recommended fix: public function listColumns(string $projectId): array
{
try {
$objectService = $this->getObjectService();
$columns = $objectService->findObjects(...);
usort($columns, ...);
return $columns;
} catch (\Throwable $e) {
$this->logger->warning('Planix: column list failed', ['exception' => $e->getMessage()]);
return [];
}
}Or catch in the controller and return a 503 response. |
|
Hydra Code Reviewer — Finding W3 Severity: WARNING
Recommended fix: try {
$column = $this->columnService->createColumn($columnData, $project);
} catch (\RuntimeException $e) {
return new JSONResponse(
['error' => 'Failed to create column.'],
Http::STATUS_INTERNAL_SERVER_ERROR
);
}
return new JSONResponse($column, Http::STATUS_CREATED); |
|
Hydra Code Reviewer — Finding S1 Severity: SUGGESTION Both
This is a minor performance concern (one extra network call per mutation) and is acceptable for the MVP, but worth noting for future refinement. |
|
Hydra Code Reviewer — Finding S2 Severity: SUGGESTION
This is a minor data integrity issue. An empty title is visually broken in the kanban board (the column header renders nothing). The fix is a one-liner: if (isset($data['title']) === true) {
if (empty($data['title']) === true) {
return new JSONResponse(['error' => 'Title cannot be empty.'], Http::STATUS_BAD_REQUEST);
}
$updateData['title'] = $data['title'];
} |
|
Hydra Code Reviewer — Finding S3 Severity: SUGGESTION The column endpoints are registered at Recommended convention: If the project intentionally defers versioning to a later change, add a |
|
Hydra Code Reviewer — Finding S4 Severity: SUGGESTION When async addColumn() {
const title = this.newColumnTitle.trim()
if (!title) return
// ...
await this.projectsStore.createNewColumn({ ... }) // ← failure silently ignored
this.newColumnTitle = ''
this.showAddColumn = false // ← hides form even on failure
},The user types a column title, presses Add, and the form disappears — but the column never appears on the board. This is confusing UX. Recommended fix: Check the return value and show a const column = await this.projectsStore.createNewColumn({ ... })
if (!column) {
showError(t('planix', 'Failed to create column — please try again'))
return
}
this.newColumnTitle = ''
this.showAddColumn = false |
{
"reviewer": "Juan Claude van Damme — Hydra Code Reviewer",
"pr": "https://github.com/ConductionNL/planix/pull/83",
"reviewed_at": "2026-04-06",
"verdict": "REQUEST_CHANGES",
"quality_gates": {
"phpcs": "PASS",
"phpstan": "PASS (0 errors)",
"eslint": "PASS",
"phpunit": "SKIP — requires Nextcloud runtime environment"
},
"findings": [
{
"id": "W1",
"severity": "WARNING",
"file": "src/store/projects.js",
"lines": "275-344",
"title": "ColumnController auth/validation bypassed by frontend",
"description": "fetchColumns() and createNewColumn() use objectStore directly, never routing through /api/columns. The membership checks and input validation in ColumnController are dead code from the UI perspective. Potential authorization bypass if OpenRegister does not enforce project ACLs independently."
},
{
"id": "W2",
"severity": "WARNING",
"file": "lib/Service/ColumnService.php",
"lines": "147-165",
"title": "listColumns() has no exception handling",
"description": "getObjectService() can throw RuntimeException when OpenRegister is unavailable. listColumns() has no try/catch, and ColumnController::index() does not wrap it either. Results in an unhandled 500 when OpenRegister is down post-membership-check. Inconsistent with all other methods in the same file."
},
{
"id": "W3",
"severity": "WARNING",
"file": "lib/Controller/ColumnController.php",
"lines": "155-157",
"title": "create() does not catch RuntimeException from createColumn()",
"description": "update() and destroy() wrap their service calls in try/catch(RuntimeException). create() does not. A defense-in-depth throw from createColumn() or a getObjectService() failure propagates as an unhandled 500."
},
{
"id": "S1",
"severity": "SUGGESTION",
"file": "lib/Service/ColumnService.php",
"lines": "235,269",
"title": "Double column lookup in updateColumn() and deleteColumn()",
"description": "Both methods re-fetch the column internally even though the controller already fetched it. Minor performance issue (two OpenRegister round-trips per mutation). The createColumn() method already solves the analogous project lookup with an optional pre-fetched parameter."
},
{
"id": "S2",
"severity": "SUGGESTION",
"file": "lib/Controller/ColumnController.php",
"lines": "191-193",
"title": "update() accepts empty string title",
"description": "PATCH with {title: ''} passes validation and stores an empty title. create() correctly rejects empty titles. update() should do the same."
},
{
"id": "S3",
"severity": "SUGGESTION",
"file": "appinfo/routes.php",
"lines": "14-17",
"title": "API routes lack version prefix",
"description": "NL API strategie requires versioned endpoints (/api/v1/columns). New API surface introduced by this PR should set the convention."
},
{
"id": "S4",
"severity": "SUGGESTION",
"file": "src/views/ProjectBoard.vue",
"lines": "278-295",
"title": "addColumn() silently swallows createNewColumn() failures",
"description": "On objectStore error, the form disappears without user feedback. The column never appears but the user sees no error."
}
],
"summary": "The implementation is solid overall: ColumnController is well-structured, has good auth layering, the test suite covers critical paths (12 test methods), all linters pass, and the sanitizePriority CSS-class guard is correctly applied. Three WARNING findings must be addressed before merge: the frontend bypasses the controller it was designed to protect (W1), listColumns() is missing exception handling (W2), and create() lacks the try/catch that update() and destroy() already have (W3). Four suggestions are informational and can be addressed in follow-up issues."
} |
1 participant
Summary
Implements the Kanban Board MVP for Planix. Adds a backend ColumnController with full CRUD endpoints (list, create, update, delete) for managing kanban columns per project, with project-membership authorization checks and automatic task-to-backlog migration on column deletion. Replaces the placeholder ProjectBoard.vue with a functional kanban board view displaying columns left-to-right with task cards showing title, assignee avatar, priority indicator, and due date.
Spec Reference
design.md
Changes
lib/Controller/ColumnController.php— new controller with GET/POST/PUT/DELETE endpoints for columns, including 403/404 handlinglib/Service/ColumnService.php— new service wrapping OpenRegister CRUD for columns with project-membership checks and task migration on deleteappinfo/routes.php— added column API routes (/api/columns,/api/columns/{id})src/views/ProjectBoard.vue— replaced "coming soon" placeholder with full kanban board: columns, task cards, add-column formsrc/store/projects.js— addedfetchColumns,fetchTasks,fetchBoard,createNewColumn,updateTaskactions and board statesrc/router/index.js— added/projects/:id/boardroute aliasopenspec/changes/spec/— copied spec files with all tasks marked completeTest Coverage
tests/unit/Controller/ColumnControllerTest.php— 8 test methods covering: list columns for member, 403 for non-member, 400 for missing projectId, create column, update 404, update 403, destroy 404, destroy success🤖 Generated with Claude Code