Feature: Security Redirect Page by DawoudIO · Pull Request #7669 · ChurchCRM/CRM

DawoudIO · 2025-11-29T23:25:22Z

What Changed

Fixes #

Type

Screenshots

Security Check

Introduces new input validation
Modifies authentication/authorization
Affects data privacy/GDPR

Code Quality

Database: Propel ORM only, no raw SQL
No deprecated attributes (align, valign, nowrap, border, cellpadding, cellspacing, bgcolor)
Bootstrap CSS classes used
All CSS bundled via webpack

Pre-Merge

Tested locally
No new warnings
Build passes
Backward compatible (or migration documented)

- Create /v2/access-denied route with role query parameter - Add user-friendly access-denied.php template with: - Clean card-based layout (Bootstrap 4.6.2/AdminLTE) - Human-readable permission descriptions - Clear messaging for non-technical users - Dashboard navigation button - Supports i18n with gettext() wrapped strings Part of fix for issue #6975

Update AuthenticationManager::redirectHomeIfFalse() to accept optional missingRole parameter that triggers securityRedirect() with role info. Updated 34 files to pass role names: - Finance: FinancialReports, ManageEnvelopes, PledgeDetails, eGive, Reports/(AdvancedDeposit, FamilyPledgeSummary, PledgeSummary, PrintDeposit, ReminderReport, TaxReport, ZeroGivers) - ManageGroups: GroupEditor, GroupPropsFormEditor, GroupPropsFormRowOps, MemberRoleChange, CartToEvent, CartToGroup, OptionManager(RowOps) - MenuOptions: PropertyEditor, PropertyTypeEditor, PropertyTypeDelete, PropertyDelete, OptionManager(RowOps) - DeleteRecords: SelectDelete, PledgeDelete, VolunteerOpportunityEditor - EditRecords: GroupPropsEditor - Notes: NoteDelete, NoteEditor - CreateDirectory: DirectoryReports, Reports/DirectoryReport - AddRecords: CartToFamily - Admin: TaxReport (with bCSVAdminOnly check) Fixes #6975

Redirect to /v2/access-denied?role=X instead of dashboard, providing users with clear feedback about missing permissions.

- Add isBrowserRequest() to detect browser vs API requests - Add redirectToAccessDenied() for proper redirect handling - Add getRoleName() to all middleware subclasses - Browser requests now redirect to /v2/access-denied instead of JSON - API requests continue to return JSON for programmatic clients

…RM/CRM into feature-securityRedirect

Copilot

Pull request overview

This PR implements a user-friendly security redirect system by introducing a dedicated /v2/access-denied page that displays which permission is required when users attempt to access restricted features. This improves UX over the previous behavior of silently redirecting to the dashboard, helping users understand why they were denied access and who to contact.

Key changes:

New access-denied template with role-specific error messages and contact guidance
Enhanced AuthenticationManager::redirectHomeIfFalse() to accept optional role parameter
Updated RedirectUtils::securityRedirect() to redirect to access-denied page with role context
Consistent role parameter additions across 40+ legacy PHP files

Reviewed changes

Copilot reviewed 49 out of 49 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`src/v2/templates/common/access-denied.php`	New user-facing error page with role descriptions, using Bootstrap 4.6.2 and i18n
`src/v2/routes/root.php`	Route handler for `/access-denied` endpoint extracting role from query params
`src/ChurchCRM/utils/RedirectUtils.php`	Updated `securityRedirect()` to redirect to access-denied with role parameter
`src/ChurchCRM/Authentication/AuthenticationManager.php`	Added optional `$missingRole` parameter to `redirectHomeIfFalse()`
`src/eGive.php`	Added 'Finance' role parameter to redirect call
`src/VolunteerOpportunityEditor.php`	Added 'DeleteRecords' role parameter (2 locations)
`src/TaxReport.php`	Added 'Admin' role parameter (contains logic error)
`src/SelectDelete.php`	Added 'DeleteRecords' role parameter
`src/Reports/ZeroGivers.php`	Updated to use `securityRedirect('Admin')`
`src/Reports/TaxReport.php`	Updated to use `securityRedirect('Admin')`
`src/Reports/ReminderReport.php`	Updated to use `securityRedirect('Admin')`
`src/Reports/PrintDeposit.php`	Updated to use `securityRedirect('Admin')` (contains undefined variable bug)
`src/Reports/PledgeSummary.php`	Updated to use `securityRedirect('Admin')`
`src/Reports/FamilyPledgeSummary.php`	Updated to use `securityRedirect('Admin')`
`src/Reports/EnvelopeReport.php`	Updated to use `securityRedirect('Admin')`
`src/Reports/DirectoryReport.php`	Added 'CreateDirectory' role parameter
`src/Reports/AdvancedDeposit.php`	Updated to use `securityRedirect('Admin')`
`src/ReminderReport.php`	Updated to use `securityRedirect('Admin')`
`src/PropertyTypeEditor.php`	Added 'MenuOptions' role parameter
`src/PropertyTypeDelete.php`	Added 'MenuOptions' role parameter
`src/PropertyEditor.php`	Added 'MenuOptions' role parameter
`src/PropertyDelete.php`	Added 'MenuOptions' role parameter
`src/PledgeDetails.php`	Added 'Finance' role parameter
`src/PledgeDelete.php`	Added 'DeleteRecords' role parameter
`src/OptionManagerRowOps.php`	Added 'MenuOptions' and 'ManageGroups' role parameters
`src/OptionManager.php`	Added 'MenuOptions' and 'ManageGroups' role parameters
`src/NoteEditor.php`	Added 'Notes' role parameter
`src/NoteDelete.php`	Added 'Notes' role parameter
`src/MemberRoleChange.php`	Added 'ManageGroups' role parameter
`src/ManageEnvelopes.php`	Added 'Finance' role parameter
`src/GroupPropsFormRowOps.php`	Added 'ManageGroups' role parameter
`src/GroupPropsFormEditor.php`	Added 'ManageGroups' role parameter
`src/GroupPropsEditor.php`	Added 'EditRecords' role parameter
`src/GroupEditor.php`	Added 'ManageGroups' role parameter
`src/FinancialReports.php`	Added 'Finance' role parameter
`src/FamilyEditor.php`	Updated to use `securityRedirect()` with 'EditRecords' and 'AddRecords'
`src/DirectoryReports.php`	Added 'CreateDirectory' role parameter
`src/DepositSlipEditor.php`	Updated to use `securityRedirect('Finance')`
`src/CartToGroup.php`	Added 'ManageGroups' role parameter
`src/CartToFamily.php`	Added 'AddRecords' role parameter
`src/CartToEvent.php`	Added 'ManageGroups' role parameter

Copilot · 2025-11-29T23:30:11Z

src/TaxReport.php


 // If CSVAdminOnly option is enabled and user is not admin, redirect to the menu.
-AuthenticationManager::redirectHomeIfFalse(AuthenticationManager::getCurrentUser()->isAdmin() && SystemConfig::getValue('bCSVAdminOnly'));
+AuthenticationManager::redirectHomeIfFalse(AuthenticationManager::getCurrentUser()->isAdmin() && SystemConfig::getValue('bCSVAdminOnly'), 'Admin');


The logic is incorrect here. This should check if both conditions are true (admin user AND CSVAdminOnly enabled) before blocking access. Currently, it uses && which means it will only redirect if the user is admin AND the setting is enabled, which is backwards.

This should likely be:

AuthenticationManager::redirectHomeIfFalse(!(SystemConfig::getValue('bCSVAdminOnly') && !AuthenticationManager::getCurrentUser()->isAdmin()), 'Admin');

Or more clearly:

if (!AuthenticationManager::getCurrentUser()->isAdmin() && SystemConfig::getValue('bCSVAdminOnly')) { RedirectUtils::securityRedirect('Admin'); }

@copilot open a new pull request to apply changes based on this feedback

src/v2/routes/root.php

src/v2/templates/common/access-denied.php

src/v2/routes/root.php

src/Reports/PrintDeposit.php

Copilot · 2025-11-30T00:13:08Z

@DawoudIO I've opened a new pull request, #7676, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2025-11-30T00:13:32Z

@DawoudIO I've opened a new pull request, #7677, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2025-11-30T00:13:54Z

@DawoudIO I've opened a new pull request, #7678, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2025-11-30T00:14:06Z

@DawoudIO I've opened a new pull request, #7679, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>

…re (#7676) - [x] Fix undefined variable `$bCSVAdminOnly` in `src/Reports/PrintDeposit.php` - Changed to `SystemConfig::getValue('bCSVAdminOnly')` - Added missing `use ChurchCRM\dto\SystemConfig;` import - [x] Run code review - [x] Run security analysis  --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs.

Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>

## What Changed Fixed inverted logic in `TaxReport.php` security check. The original code redirected admins when `bCSVAdminOnly` was enabled and allowed non-admins access — exactly backwards. **Before (wrong):** ```php AuthenticationManager::redirectHomeIfFalse( AuthenticationManager::getCurrentUser()->isAdmin() && SystemConfig::getValue('bCSVAdminOnly'), 'Admin' ); ``` **After (correct):** ```php if (!AuthenticationManager::getCurrentUser()->isAdmin() && SystemConfig::getValue('bCSVAdminOnly')) { RedirectUtils::securityRedirect('Admin'); } ``` Now matches the pattern used in `ReminderReport.php` and other files with the same check. Fixes review feedback from #7669 ## Type - [ ] ✨ Feature - [x] 🐛 Bug fix - [ ] ♻️ Refactor - [ ] 🏗️ Build/Infrastructure - [ ] Security ## Testing 1. Enable `bCSVAdminOnly` in system settings 2. As non-admin: navigate to TaxReport.php → should redirect to access-denied 3. As admin: navigate to TaxReport.php → should load normally 4. Disable `bCSVAdminOnly` → both users should have access ## Screenshots N/A - no UI changes ## Security Check - [ ] Introduces new input validation - [x] Modifies authentication/authorization - [ ] Affects data privacy/GDPR ### Code Quality - [x] Database: Propel ORM only, no raw SQL - [x] No deprecated attributes (align, valign, nowrap, border, cellpadding, cellspacing, bgcolor) - [x] Bootstrap CSS classes used - [x] All CSS bundled via webpack ## Pre-Merge - [ ] Tested locally - [x] No new warnings - [ ] Build passes - [x] Backward compatible (or migration documented)  --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs.

## What Changed Adds comprehensive Cypress UI test coverage for the access-denied page security feature introduced in #7669. Tests verify: - Page displays correctly with all default elements - All 11 role types show correct permission descriptions (Admin, Finance, ManageGroups, EditRecords, DeleteRecords, AddRecords, MenuOptions, Notes, CreateDirectory, AddEvent, CSVExport) - Unknown roles fall back to generic "Required permission" message - "Go to Dashboard" button navigation works - Standard users are redirected to access-denied with correct role parameter when accessing admin pages ## Type - [x] ✨ Feature - [ ] 🐛 Bug fix - [ ] ♻️ Refactor - [ ] 🏗️ Build/Infrastructure - [ ] 🔒 Security ## Testing ```bash npx cypress run --e2e --config-file docker/cypress.config.ts --spec "cypress/e2e/ui/session/access-denied.spec.js" ``` ## Screenshots N/A - Test-only changes ## Security Check - [ ] Introduces new input validation - [ ] Modifies authentication/authorization - [ ] Affects data privacy/GDPR ### Code Quality - [x] Database: Propel ORM only, no raw SQL - [x] No deprecated attributes (align, valign, nowrap, border, cellpadding, cellspacing, bgcolor) - [x] Bootstrap CSS classes used - [x] All CSS bundled via webpack ## Pre-Merge - [x] Tested locally - [x] No new warnings - [x] Build passes - [x] Backward compatible (or migration documented)  --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).

## What Changed Validates the `role` query parameter on the access-denied page against an allowlist of known role codes per [review feedback](#7669 (comment)). Prevents potential XSS/injection by rejecting unknown values. - **`src/v2/routes/root.php`**: Added allowlist validation for `role` parameter - **`src/v2/templates/common/access-denied.php`**: Added `Authentication` to role descriptions for consistency ```php $allowedRoles = ['Admin', 'Finance', 'ManageGroups', 'EditRecords', ...]; $missingRole = in_array($queryParams['role'] ?? '', $allowedRoles, true) ? $queryParams['role'] : ''; ``` ## Type - [ ] ✨ Feature - [ ] 🐛 Bug fix - [ ] ♻️ Refactor - [ ] 🏗️ Build/Infrastructure - [x] 🔒 Security ## Testing 1. Visit `/v2/access-denied?role=Admin` → Shows "Administrator privileges" 2. Visit `/v2/access-denied?role=<script>alert(1)</script>` → Role info hidden (invalid value rejected) 3. Visit `/v2/access-denied?role=InvalidRole` → Role info hidden ## Screenshots N/A - No visual changes; defense-in-depth validation ## Security Check - [x] Introduces new input validation - [ ] Modifies authentication/authorization - [ ] Affects data privacy/GDPR ### Code Quality - [x] Database: Propel ORM only, no raw SQL - [x] No deprecated attributes (align, valign, nowrap, border, cellpadding, cellspacing, bgcolor) - [x] Bootstrap CSS classes used - [x] All CSS bundled via webpack ## Pre-Merge - [x] Tested locally - [x] No new warnings - [x] Build passes - [x] Backward compatible (or migration documented)  --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs.

This PR updates the locale translation strings extracted from source code. ## Triggered By Commit: `218cedefb25b93191d7e9f147be63c09d2e14940` Message: Feature: Security Redirect Page (#7669) ## What Changed  Fixes # ## Type  - [x] ✨ Feature - [ ] 🐛 Bug fix - [ ] ♻️ Refactor - [ ] 🏗️ Build/Infrastructure - [x] 🔒 Security ## Screenshots  <img width="2575" height="742" alt="image" src="https://github.com/user-attachments/assets/039c8bfb-6e82-4963-adca-9640be5ce6c7" /> ## Security Check  - [ ] Introduces new input validation - [x] Modifies authentication/authorization - [ ] Affects data privacy/GDPR ### Code Quality - [ ] Database: Propel ORM only, no raw SQL - [ ] No deprecated attributes (align, valign, nowrap, border, cellpadding, cellspacing, bgcolor) - [ ] Bootstrap CSS classes used - [ ] All CSS bundled via webpack ## Pre-Merge - [x] Tested locally - [ ] No new warnings - [ ] Build passes - [ ] Backward compatible (or migration documented) ## Changes - Updated `locale/messages.po` with new/modified translatable strings - Updated JSON locale keys ## Next Steps - Review the new strings that need translation - Merge this PR to update the base locale file - Upload to POEditor for translation if needed

DawoudIO added 4 commits November 29, 2025 15:12

Update securityRedirect to use access-denied page

ed5c7fe

Redirect to /v2/access-denied?role=X instead of dashboard, providing users with clear feedback about missing permissions.

RedirectUtils::securityRedirect

f25fe2f

DawoudIO added this to the 6.3.0 milestone Nov 29, 2025

DawoudIO requested a review from a team as a code owner November 29, 2025 23:25

DawoudIO requested review from DAcodedBEAT, MrClever, bigtigerku, Copilot, grayeul and respencer and removed request for a team November 29, 2025 23:25

DawoudIO added the Security label Nov 29, 2025

Copilot started reviewing on behalf of DawoudIO November 29, 2025 23:25 View session

DawoudIO added 3 commits November 29, 2025 15:26

Merge branch 'master' into feature-securityRedirect

b28586e

Merge branch 'feature-securityRedirect' of https://github.com/ChurchC…

7e69902

…RM/CRM into feature-securityRedirect

DawoudIO mentioned this pull request Nov 29, 2025

Enhancement: Use securityRedirect whenever we redirect because of a missing user role #6975

Closed

DawoudIO linked an issue Nov 29, 2025 that may be closed by this pull request

Enhancement: Use securityRedirect whenever we redirect because of a missing user role #6975

Closed

Copilot finished reviewing on behalf of DawoudIO November 29, 2025 23:29

Copilot AI reviewed Nov 29, 2025

View reviewed changes

Initial plan

8e69bf9

Copilot AI mentioned this pull request Nov 30, 2025

[WIP] WIP addressing review feedback for security redirect page feature #7676

Merged

3 tasks

Initial plan

e95dc5d

Copilot AI mentioned this pull request Nov 30, 2025

Add Cypress UI tests for access-denied security redirect page #7677

Merged

16 tasks

Initial plan

188b21e

Copilot AI mentioned this pull request Nov 30, 2025

Add role parameter validation to prevent XSS/injection attacks #7678

Merged

16 tasks

Initial plan

fe59db0

Copilot AI mentioned this pull request Nov 30, 2025

Fix incorrect CSVAdminOnly logic in TaxReport.php #7679

Merged

16 tasks

Merge branch 'master' into feature-securityRedirect

698908f

Copilot AI and others added 10 commits November 30, 2025 00:14

Fix undefined variable $bCSVAdminOnly in PrintDeposit.php

4edb49a

Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>

Fix incorrect CSVAdminOnly logic in TaxReport.php

aec3fb9

Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>

Add role parameter validation to prevent XSS/injection attacks

cb7c8d7

Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>

Add Cypress UI tests for access-denied page

bd0ad5d

Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>

Merge branch 'master' into feature-securityRedirect

b476de3

fixed unknown role

12d6d20

DawoudIO merged commit 218cede into master Nov 30, 2025
7 checks passed

DawoudIO deleted the feature-securityRedirect branch November 30, 2025 06:26

github-actions bot mentioned this pull request Nov 30, 2025

Update locale strings #7690

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature: Security Redirect Page#7669

Feature: Security Redirect Page#7669
DawoudIO merged 22 commits intomasterfrom
feature-securityRedirect

DawoudIO commented Nov 29, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 29, 2025

Uh oh!

DawoudIO Nov 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Nov 30, 2025

Uh oh!

Copilot AI commented Nov 30, 2025

Uh oh!

Copilot AI commented Nov 30, 2025

Uh oh!

Copilot AI commented Nov 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

DawoudIO commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Changed

Type

Screenshots

Security Check

Code Quality

Pre-Merge

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

DawoudIO Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Nov 30, 2025

Uh oh!

Copilot AI commented Nov 30, 2025

Uh oh!

Copilot AI commented Nov 30, 2025

Uh oh!

Copilot AI commented Nov 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DawoudIO commented Nov 29, 2025 •

edited

Loading