Conversation
The global Computer::canView() only checks Session::haveRight('computer',
READ) — it performs no entity check. Any authenticated user with the
standard computer-read right could iterate computers_id to dump inventory
from entities they have no access to.
Replace with the per-object Computer::can(\, READ) which calls
canViewItem() -> checkEntity() -> Session::haveAccessToEntity(), enforcing
the entity boundary before any data is read.
Also change the exception class from BadRequestHttpException (400) to
AccessDeniedHttpException (403), which is the correct HTTP semantics for
an authorization failure.
Fixes: HIGH / broken-access-control (security review 2026-08-27)
Capacity::getLinkedVMs() resolved a VM's inventory UUID to a Computer row
without scoping the lookup to the current session's entities. A VM migrated
between two entities (or a shared hypervisor whose guests are recorded under
a different customer entity) would cause a storage figure computed from a
Computer in an inaccessible entity to appear in the VM breakdown table of
a user who should not see it.
Fix: add getEntitiesRestrictCriteria('glpi_computers') to the WHERE clause
so the lookup only matches Computer rows in entities the session can access.
Also add is_template => 0 to exclude template records, which are never
real inventory hosts.
When no accessible Computer matches the UUID the storage column falls back
to 0 (the existing no-match path), displayed as '0 GB'.
Fixes: LOW / cross-entity-data-access (security review 2026-08-27)
…mment In GLPI 11, all plugin routes that are not explicitly declared stateless are authenticated by the framework before the PHP script is executed. Session::checkLoginUser() therefore does nothing and was misleadingly documented as the security control for this endpoint. Remove the call and update the file-level block comment to accurately describe the real security controls: framework authentication and the per-object Computer::can(\, READ) check added in the previous commit. Quality note from security review 2026-08-27.
Twig auto-escapes every {{ }} expression by default. Calling
htmlspecialchars() on values before passing them to Twig causes
double-encoding: a computer named 'R&D-host01' renders in the
browser as 'R&D-host01'.
Remove the htmlspecialchars() calls from:
- CapacityRenderer::render() for \, \, \
- front/report.php for the \ argument to Html::header()
(Html::header() accepts plain text, not HTML, so pre-escaping
was doubly wrong there)
Let Twig handle HTML-context escaping, which is the documented
GLPI 10+/11 contract for plugin templates.
Quality note from security review 2026-08-27.
getVmAllocations() issued two separate DB->request() calls against glpi_itemvirtualmachines with identical FROM and WHERE clauses, fetching SUM(ram) and SUM(vcpu) in separate round-trips. Replace with a single query that selects both aggregates together. No behaviour change — the returned array shape is unchanged. Quality note from security review 2026-08-27.
…on system Several user-facing strings were hardcoded in English directly inside Twig templates, bypassing the plugin's own __() / _n() translation infrastructure and breaking any future localisation effort: capacity_tab.html.twig:25 'Capacity Report — %s' capacity_tab.html.twig:31 'Generated: %s' metric_cards.html.twig:68 'drive' / 'drives' metric_cards.html.twig:81 '% used' metric_cards.html.twig:84 'No volume data' metric_cards.html.twig:84 ' GB free' (reused existing i18n.free) storage_tables.html.twig:107 'used' (reused existing i18n.used) Add the missing keys (capacity_report_title, generated_at_label, drive_singular, drive_plural, pct_used_suffix, no_volume_data, used_label) to CapacityRenderer::buildI18n() and replace the raw strings in the templates with i18n bag references. No behaviour change for English users; enables correct translation for all other locales. Quality note from security review 2026-08-27.
The excludePaths list contained the literal string 'node_modules (?)' which is not a valid path or glob pattern. PHPStan silently ignored it, but the entry was misleading — suggesting the config had been validated against a project that contains node_modules. This plugin has no JS build system and no node_modules directory. Remove the invalid entry. Quality note from security review 2026-08-27.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
|
@copilot review |
Copilot stopped work on behalf of
tahir242 due to an error
August 27, 2026 11:53
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.
Security fixes & quality improvements (v1.0.0 review)
Fixes all findings from the 2026-08-27 security review.
Security
front/report.php: Replace globalcanView()with per-objectcan($id, READ),enforcing entity boundaries on the standalone report endpoint.
src/Capacity.php: Restrict VM UUID→Computer resolution to the current session'saccessible entities via
getEntitiesRestrictCriteria().Quality
Session::checkLoginUser()(GLPI 11 framework handles auth)htmlspecialchars()pre-escaping (Twig auto-escapes)getVmAllocations()queries into one round-trip__()/ i18n bagnode_modules (?)fromphpstan.neon