Skip to content

pr: [Nightly Fix] - Security - Restrict Public Edit Link#6

Open
jewel-claw wants to merge 1 commit intomasterfrom
nightly-fix/edit-link-capability
Open

pr: [Nightly Fix] - Security - Restrict Public Edit Link#6
jewel-claw wants to merge 1 commit intomasterfrom
nightly-fix/edit-link-capability

Conversation

@jewel-claw
Copy link

What

  • tighten the frontend edit-link check to require the actual admin capability instead of a truthy role slug

Why

  • the existing condition shows the edit link to any logged-in user because ninja_table_admin_role() returns a non-empty string
  • subscribers and customers should not see admin edit affordances on the public table output

Fix

  • replace the role-slug truthiness check with current_user_can(ninja_table_admin_role())

Confidence

  • linted app/Views/public/ninja-footable.php with php -l

@greptile-apps
Copy link

greptile-apps bot commented Mar 10, 2026

PR author is not in the allowed authors list.

@qodo-code-review
Copy link

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

Review Summary by Qodo

Restrict public edit link to users with admin capability

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Replace role-slug truthiness check with proper capability verification
• Prevent non-admin users from seeing edit link affordances
• Use current_user_can() to validate actual admin permissions
Diagram
flowchart LR
  A["is_user_logged_in() check"] --> B["Old: ninja_table_admin_role() truthiness"]
  B --> C["Shows edit link to any logged-in user"]
  A --> D["New: current_user_can(ninja_table_admin_role())"]
  D --> E["Shows edit link only to admin users"]
Loading

Grey Divider

File Changes

1. app/Views/public/ninja-footable.php 🐞 Bug fix +1/-1

Restrict edit link to users with admin capability

• Changed edit link visibility condition from ninja_table_admin_role() truthiness check to
 current_user_can(ninja_table_admin_role())
• Ensures only users with actual admin capability can see the edit table link
• Prevents subscribers and customers from viewing admin affordances on public table output

app/Views/public/ninja-footable.php


Grey Divider

Qodo Logo

@qodo-code-review
Copy link

qodo-code-review bot commented Mar 10, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Advisory comments

1. No-op permission tightening 🐞 Bug ✓ Correctness
Description
ninja_table_admin_role() already checks current_user_can() internally and returns false when
the user lacks the required role/cap, so wrapping it again with current_user_can(...) does not
further restrict who sees the edit link in this codebase. This makes the PR unlikely to fix the
stated issue unless ninja_table_admin_role() is overridden elsewhere at runtime.
Code

app/Views/public/ninja-footable.php[42]

+    <?php if (is_user_logged_in() && current_user_can(ninja_table_admin_role())): ?>
Evidence
In this repository, ninja_table_admin_role() only returns a non-empty string after verifying
current_user_can($role); otherwise it returns false. Therefore the previous condition
is_user_logged_in() && ninja_table_admin_role() already implied the same permission check as the
new wrapper.

boot/ninja-tables-global-function.php[102-119]
app/Views/public/ninja-footable.php[42-46]
app/Views/public/drag-and-drop-html.php[39-42]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
In this repo, `ninja_table_admin_role()` already performs the permission check (`current_user_can(...)`) and returns `false` when unauthorized, so wrapping it again with `current_user_can(ninja_table_admin_role())` does not change who can see the public edit link. If the goal is to harden against a truthy-but-unauthorized role slug, that hardening should be done by standardizing on a capability string getter and consistent gating.

## Issue Context
- `ninja_table_admin_role()` currently mixes two responsibilities: (1) *detecting* a configured admin role/capability and (2) *authorizing* the current user.
- The PR change only affects one template and does not change authorization behavior given the current helper implementation.

## Fix Focus Areas
- app/Views/public/ninja-footable.php[42-42]
- boot/ninja-tables-global-function.php[102-119]
- app/Views/public/drag-and-drop-html.php[39-42]

Potential implementation direction:
- Add a new helper that returns a capability string (no `current_user_can()` inside).
- Update templates/controllers to consistently call `current_user_can(&lt;capability&gt;)`.
- Keep `ninja_table_admin_role()` for backward compatibility or refactor call sites carefully if changing its semantics.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@kilo-code-bot
Copy link

kilo-code-bot bot commented Mar 10, 2026

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file)
  • app/Views/public/ninja-footable.php - Security fix verified

Change Analysis

The PR correctly addresses a security issue by replacing the implicit role check with explicit capability verification:

Before After
ninja_table_admin_role() truthiness current_user_can(ninja_table_admin_role())

Original Issue: The old code showed the "Edit Table" link to ANY logged-in user because ninja_table_admin_role() returns a role string (e.g., 'administrator') which is truthy in PHP, regardless of whether the user actually has that capability.

Fix Applied: The new code properly uses current_user_can() to verify the current user actually has the admin capability before showing the edit link.

This is a valid security fix that prevents non-admin users from seeing admin-only affordances in the public table output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant