Skip to content

N°8606 - Check user permissions in search operation of ajax.render.php#836

Open
Lenaick wants to merge 1 commit intosupport/3.2from
issue/8606-unauthorized-access-to-object
Open

N°8606 - Check user permissions in search operation of ajax.render.php#836
Lenaick wants to merge 1 commit intosupport/3.2from
issue/8606-unauthorized-access-to-object

Conversation

@Lenaick
Copy link
Contributor

@Lenaick Lenaick commented Mar 10, 2026

Base information

Question Answer
Related to a SourceForge thread / Another PR / Combodo ticket? N°8606
Type of change? Bug fix

Symptom

Users can access unauthorized objects via the search operation in ajax.render.php

Cause

User permissions are not checked before returning object

Proposed solution

Add a permission check before returning objects

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested all changes I made on an iTop instance
  • I have added a unit test, otherwise I have explained why I couldn't
  • Is the PR clear and detailed enough so anyone can understand digging in the code?

@Lenaick Lenaick self-assigned this Mar 10, 2026
@CombodoApplicationsAccount CombodoApplicationsAccount added the internal Work made by Combodo label Mar 10, 2026
@Lenaick Lenaick added this to the 3.2.3 milestone Mar 10, 2026
@Lenaick Lenaick requested review from bdalsass and steffunky March 10, 2026 10:55
@Lenaick Lenaick force-pushed the issue/8606-unauthorized-access-to-object branch from c4d74c4 to 2b6fee3 Compare March 10, 2026 11:08
@Lenaick Lenaick requested a review from Copilot March 10, 2026 11:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a security vulnerability (N°8606) where users could access unauthorized objects via the search operation in ajax.render.php. The fix adds permission checks using UserRights::IsActionAllowed() in the GetDataForTable method before returning object data.

Changes:

  • Adds a UserRights import and two UR_ACTION_READ permission checks in AjaxRenderController::GetDataForTable() — one for the main query class and one for each alias class in join queries — throwing an exception with a localized error message if the user lacks read access.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +105 to +108
// N°8606 : Check user permissions on the current class
if (UserRights::IsActionAllowed($sClass, UR_ACTION_READ, $oSet) !== UR_ALLOWED_YES) {
throw new Exception(Dict::Format('UI:Error:ReadNotAllowedOn_Class', $sClass));
}
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The permission check for alias classes is placed inside the while ($aObject = $oSet->FetchAssoc()) loop, meaning UserRights::IsActionAllowed is called once per row per alias. Since the check is class-based (not object-based), the result is the same on every iteration. While GetUserActionGrant has internal caching that reduces the cost, this check should be moved before the while loop (alongside the main class check at line 77) for clarity and to avoid unnecessary function call overhead on every row. You could iterate over $aClassAliases before the loop and check each class once.

Copilot uses AI. Check for mistakes.
@Combodo Combodo deleted a comment from greptile-apps bot Mar 10, 2026
@Combodo Combodo deleted a comment from greptile-apps bot Mar 10, 2026
@Combodo Combodo deleted a comment from greptile-apps bot Mar 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Work made by Combodo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants