chore(security): ADR-005 — stop leaking exception messages from 3 controllers#80
Merged
rubenvdlinde merged 1 commit intodevelopmentfrom May 1, 2026
Merged
Conversation
…ADR-005) 12 sites in 3 controllers were forwarding `$e->getMessage()` from DoesNotExistException / InvalidArgumentException / generic Exception straight into the response body. Replaced each with a fixed safe string matching the HTTP status semantics: - 4 × `DoesNotExistException` → 404 → `'Dashboard not found'` - 4 × generic `Exception` → 403 → `'Forbidden'` - 3 × `InvalidArgumentException` → 400 → `'Invalid request'` - 1 × `InvalidArgumentException` (groups payload) → 400 → `'Invalid groups payload'` Kept 2 sites unchanged: `PersonalDashboardsDisabledException` returns its own `getErrorCode()` + a safe translated message designed for clients. Also kept logger->warning/error sites with `$e->getMessage()` — those are server-side log context, never reach the client. Affected files: - lib/Controller/AdminController.php - lib/Controller/DashboardApiController.php - lib/Controller/DashboardShareApiController.php
rubenvdlinde
added
the
ready-for-code-review
Contributor
Quality Report — ConductionNL/mydash @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 342/342 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-01 07:52 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
May 1, 2026
fix: re-apply JSONResponse swap (reverted by #80) + phpcs nit + apexcharts license
This was referenced May 1, 2026
rubenvdlinde
added a commit
that referenced
this pull request
May 3, 2026
…ADR-005) (#80) 12 sites in 3 controllers were forwarding `$e->getMessage()` from DoesNotExistException / InvalidArgumentException / generic Exception straight into the response body. Replaced each with a fixed safe string matching the HTTP status semantics: - 4 × `DoesNotExistException` → 404 → `'Dashboard not found'` - 4 × generic `Exception` → 403 → `'Forbidden'` - 3 × `InvalidArgumentException` → 400 → `'Invalid request'` - 1 × `InvalidArgumentException` (groups payload) → 400 → `'Invalid groups payload'` Kept 2 sites unchanged: `PersonalDashboardsDisabledException` returns its own `getErrorCode()` + a safe translated message designed for clients. Also kept logger->warning/error sites with `$e->getMessage()` — those are server-side log context, never reach the client. Affected files: - lib/Controller/AdminController.php - lib/Controller/DashboardApiController.php - lib/Controller/DashboardShareApiController.php
rubenvdlinde
added a commit
that referenced
this pull request
May 3, 2026
Three small fixes to clear the failing checks on PR #87 (release development → beta). DashboardShareApiController — re-apply the DataResponse → JSONResponse swap from #79. PR #80 (security) was opened from a base before #79 landed; when #80 merged, its diff base contained the old DataResponse code, silently reverting the swap. Brings the 21 phpstan errors in this controller back to zero, same as the already-merged #79 fix. AdminTemplateService:328 — collapse a stray double blank line after pickFirstMatch() into a single blank line. Single phpcs error introduced by #79's generateUuid() addition (added \n\n between methods instead of \n). .license-overrides.json — add apexcharts@5.10.6. license-checker flags it as `Custom: https://apexcharts.com/media/apexcharts-logo.png` because it picks up a stray HTTP URL from the package's README; the actual project is MIT-licensed (LICENSE file in the repo). It arrives as a transitive dep through @conduction/nextcloud-vue, so this is a generic fix not tied to any one feature. NOT included: the eslint Nc* import/named errors. Those are blocked on nextcloud-vue cutting a release from development (PR #102 merged into ncvue/development, not yet propagated to ncvue/main where semantic-release publishes from). Verified locally: - composer phpstan → [OK] No errors (was 21) - composer phpcs → clean (was 1) - composer test:unit → 354/354
rubenvdlinde
added a commit
that referenced
this pull request
May 3, 2026
fix: re-apply JSONResponse swap (reverted by #80) + phpcs nit + apexcharts license
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ADR-005 follow-up audit. Found 12 sites in lib/Controller/ where my agents' generated catch blocks were forwarding raw
\$e->getMessage()to clients. Replaced each with a fixed safe string per HTTP-status semantics.Kept (intentional, not leaks):
DashboardApiController.php× 2 sites withPersonalDashboardsDisabledException— hasgetErrorCode()+ a translated safe message designed for client displaylogger->warning/errorsites with\$e->getMessage()— server-side log context onlyADR-014 audit also done — no action required. The repo's
REUSE.toml(added in #34) provides external SPDX coverage for all PHP files, so per-file SPDX-License-Identifier lines are intentionally NOT required (REUSE.toml says so explicitly). My agents' new files have inline SPDX anyway, which is over-spec but harmless.