Conversation
Implements centralized routing architecture with multi-layer security: - Add home.php as main front controller for centralized routing - Add .htaccess with Apache rewrite rules for request routing - Add security-check.php as auto-prepended .inc.php blocker - Block direct access to .inc.php files (403 Forbidden) - Deny access to sensitive paths (/portal/patient/fwk/libs/, /sites/*/documents/) - Preserve existing inner front controllers (/apis/, /portal/, /oauth2/, /interface/*) - Add OPENEMR_ENABLE_FRONT_CONTROLLER feature flag (disabled by default) - Add extension hooks for early/late custom logic injection - Add multisite support with automatic site detection - Add path traversal prevention with realpath() validation - Add admin path detection for telemetry integration - Add comprehensive test suite (PHPUnit + shell scripts) - Add README-FRONT-CONTROLLER.md with deployment guide Security improvements: - Three-layer .inc.php blocking (.htaccess → security-check.php → home.php) - Request validation before routing - Error logging for security events - Debug mode via OPENEMR_FC_LOG_LEVEL Backward compatibility: - Feature disabled by default (opt-in via environment variable) - No breaking changes to existing routing - All existing front controllers preserved - Static assets passed through unchanged Files added: - .htaccess (Apache routing rules) - home.php (main front controller) - security-check.php (PHP prepend security layer) - README-FRONT-CONTROLLER.md (documentation) - custom/.gitkeep (extension directory) - tests/FrontController/SecurityTest.php - tests/FrontController/CompatibilityTest.php - tests/scripts/test_security.sh - tests/scripts/test_compatibility.sh - tests/scripts/test_performance.sh Files modified: - .env.example (feature flag documentation)
…tation ARCHITECTURAL IMPROVEMENTS: - Extracted routing logic from home.php into dedicated Router class (src/OpenCoreEMR/FrontController/Router.php) - Created RouteConfig class for centralized route configuration (src/OpenCoreEMR/FrontController/RouteConfig.php) - Delegated security validation to SecurityValidator class - Implemented event system hooks for module extensibility - Removed global variables, improved testability and maintainability ROUTING SYSTEM: - Route patterns now centralized in RouteConfig with clear categorization - Admin paths, forbidden paths, bypass patterns all configurable - Event system allows modules to extend routing rules dynamically - Removed hardcoded path matching from home.php CI INTEGRATION: - Integrated FrontController tests with PHPUnit CI pipeline (phpunit.xml) - Created dedicated 'frontcontroller' test suite for targeted testing - Added tests to main 'openemr' suite for comprehensive coverage - Enabled CLI test execution via OPENEMR_TEST_URL environment variable TEST QUALITY IMPROVEMENTS: - Replaced curl with Guzzle HTTP client in CompatibilityTest.php - Fixed HTTP assertions to be specific per endpoint (200, 302, 401, 400) - Moved tests to PSR-4 compliant location (tests/Tests/FrontController/) - Added proper client setup with base_uri and error handling configuration SHELL SCRIPT QUALITY (POSIX compliance): - Replaced ANSI escape sequences with tput commands for portability - Changed echo -n/-e to printf for POSIX compliance - Replaced [ ] with [[ ]] for consistency and safety - Changed == to = for string comparisons (POSIX compliant) - Consolidated echo statements into compound commands - Used herestrings instead of echo | pipe for efficiency - Removed auto-install logic for security best practices DOCUMENTATION: - Updated architecture diagram showing Router/RouteConfig delegation - Added extensible configuration examples with event system - Updated file counts (6→8 core files, correct test paths) - Added troubleshooting section (.htaccess flags, performance, CLI) - Documented CI integration commands and test execution - Added FAQ about extending routing rules via Event System SECURITY: - Maintained all security validations (path traversal, .inc.php blocking) - Preserved feature flag controls and multisite selection - No changes to security-check.php auto-prepend mechanism - All security patterns now in RouteConfig for easier auditing BACKWARD COMPATIBILITY: - Zero functional changes to routing behavior - All existing front controllers preserved (/apis/, /portal/, /oauth2/) - Static assets still bypass routing - Extension hooks remain optional Addresses Client Review Comments #1-21:
… the disabled condition.
The `.htaccess` routing is designed to work in both states:
```apache
# Physical files pass through directly (never hit home.php)
RewriteCond %{REQUEST_FILENAME} -f
RewriteRule ^ - [L]
# Only non-existent files route to home.php
RewriteCond %{REQUEST_URI} \.php$
RewriteRule ^(.*)$ home.php?_ROUTE=$1 [QSA,L]
When Front Controller is DISABLED:
- Existing PHP files (interface/login/login.php, etc.): Served directly by Apache via the -f check
- Virtual routes: Pass through home.php with transparent context preservation
- .inc.php files: Still blocked for security
When Front Controller is ENABLED:
- All routes: Go through front controller with full security validation
- Security checks: Router validates paths, blocks forbidden files
The Transparent Pass-Through
For the edge case where disabled mode receives routed requests, home.php now acts as a transparent proxy:
// Preserves correct execution context
$_SERVER['SCRIPT_FILENAME'] = $targetFile;
$_SERVER['SCRIPT_NAME'] = '/' . $route;
$_SERVER['PHP_SELF'] = '/' . $route;
chdir(dirname($targetFile));
// Security maintained even when disabled
- Directory traversal prevention
- .inc.php blocking
- File existence validation
require $targetFile; // __FILE__ and __DIR__ work correctly in target
Tested and verified: __FILE__, __DIR__, relative paths, and $_SERVER variables all work correctly for the target file.
The .htaccess + home.php combination now handles both enabled and disabled states correctly.
|
Caution Review failedThe pull request is closed. WalkthroughAdds a global front controller: new home.php, Router, RouteConfig(+interface), SecurityValidator, security-check bootstrap, .htaccess rules, tests, CI steps, docs, environment flags, composer PSR-4 namespace, and several test scripts and reports to validate routing, security, and performance. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant A as Apache (.htaccess)
participant H as home.php (Front Controller)
participant S as security-check.php
participant V as SecurityValidator
participant R as Router
participant E as Event Dispatcher
participant T as Target PHP
C->>A: HTTP Request
A->>A: Check static/allowed paths
alt Pass-through/static
A-->>C: Serve resource or existing front controller
else Routed to front controller
A->>H: Rewrite to home.php with _ROUTE
H->>S: Pre-exec guard (block .inc.php)
H->>V: Early validation/logging
H->>R: Init Router (baseDir + RouteConfig)
R->>R: extractRoute(), determineSiteId(), handleTrailingSlash()
R->>R: isForbidden()/requiresAdmin()
H->>E: Dispatch front_controller.early
R->>R: resolveTargetFile()
H->>T: include target file (try/catch)
H->>E: Dispatch front_controller.late (on shutdown/exception)
T-->>H: Output
H-->>C: Response
end
sequenceDiagram
autonumber
participant C as Client
participant H as home.php
Note over H: Feature flag: OPENEMR_ENABLE_FRONT_CONTROLLER
C->>H: Request rewritten to home.php
alt Feature flag disabled
H-->>C: Transparent passthrough to requested script (with safety checks)
else Feature flag enabled
H->>R: Full Router-driven front controller flow (see main diagram)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (9)
tests/scripts/test_performance.sh (2)
1-40: Setup is mostly sound, but error handling needs refinement.The script header, AI disclosure, and configuration setup are clear and appropriate. However:
Line 16 (
set -e): While this ensures the script exits on errors, it can cause unexpected exits with command substitutions. Several commands later in the script use|| trueto avoid this, but the pattern is inconsistent.Lines 19-20: No validation of
BASE_URLformat. If an invalid URL is provided, tests will fail silently or produce misleading results.Consider adding input validation:
# Configuration BASE_URL="${1:-http://localhost/openemr}" + +# Validate BASE_URL format +if ! [[ "$BASE_URL" =~ ^https?:// ]]; then + echo "${RED}ERROR: Invalid BASE_URL format. Must start with http:// or https://${NC}" >&2 + exit 1 +fi + ITERATIONS="${2:-100}"For
set -e, consider being more explicit about which commands should ignore errors:-set -e +set -euo pipefail +# Note: Individual commands that may fail use || true explicitly
95-182: Improve static asset test and add connectivity check
- Replace file-existence check with HTTP HEAD for static assets to ensure URL accessibility:
- if [[ -d "$(dirname "$0")/../../public/assets" ]]; then + if curl -sf --head "$BASE_URL/public/assets/css/style.css" > /dev/null 2>&1; then static_result=$(run_performance_test \ "Static CSS file" \ "$BASE_URL/public/assets/css/style.css" \ "Static assets should bypass front controller") static_rps=$(cut -d'|' -f1 <<< "$static_result") else echo " ℹ Skipping static asset tests (static asset URL not accessible)" | tee -a "$REPORT_FILE" fi
- Add an initial connectivity check after creating
REPORT_FILEto fail fast ifBASE_URLis unreachable:} | tee "$REPORT_FILE" +# Verify BASE_URL is reachable +if ! curl -sf --head "$BASE_URL/index.php" > /dev/null 2>&1; then + echo "ERROR: Cannot connect to BASE_URL: $BASE_URL" | tee -a "$REPORT_FILE" + exit 1 +fi # Check if Apache Bench (ab) is availabletests/reports/compatibility-test-report.json (1)
1-5: LGTM!The report structure is clear and the data is well-formed. The escaped forward slashes in the URL (
\/) are valid JSON but unnecessary in modern parsers—unescaped slashes work fine and improve readability.composer.json (1)
189-192: Consider simplifying the autoload configuration.The new
"OpenCoreEMR\\": "src/OpenCoreEMR"mapping is valid, but technically redundant since"OpenEMR\\": "src"already covers all namespaces undersrc/. This means classes insrc/OpenCoreEMRare already autoloadable via the existing mapping.Having both mappings works fine and may provide clarity or facilitate future namespace separation. If the intention is to keep both for organizational reasons, this is acceptable.
tests/scripts/test_security.sh (2)
43-68: Improve error handling in therun_testfunction.The function uses
curl -s -o /dev/null -w "%{http_code}" -Lwhich may hide connection errors. If curl fails to connect, it may return an empty string or "000", which won't match expected codes and will be reported as a test failure rather than a connection error.Apply this diff to add explicit error handling:
run_test() { local test_name="$1" local url="$2" local expected_code="$3" local description="$4" TOTAL_TESTS=$((TOTAL_TESTS + 1)) printf "Testing: %s... " "$test_name" | tee -a "$REPORT_FILE" # Make HTTP request and get status code - http_code=$(curl -s -o /dev/null -w "%{http_code}" -L "$url") + http_code=$(curl -s -o /dev/null -w "%{http_code}" -L "$url" 2>&1) + + # Check for curl errors (empty or 000 status code) + if [[ -z "$http_code" || "$http_code" == "000" ]]; then + printf "%sERROR%s (Connection failed)\n" "$RED" "$NC" | tee -a "$REPORT_FILE" + FAILED_TESTS=$((FAILED_TESTS + 1)) + { + echo " ✗ Unable to connect to server" + echo " URL: $url" + } | tee -a "$REPORT_FILE" + echo | tee -a "$REPORT_FILE" + return + fi if [[ "$http_code" = "$expected_code" ]]; then printf "%sPASS%s (HTTP %s)\n" "$GREEN" "$NC" "$http_code" | tee -a "$REPORT_FILE" PASSED_TESTS=$((PASSED_TESTS + 1)) echo " ✓ $description" | tee -a "$REPORT_FILE" else printf "%sFAIL%s (Expected %s, got %s)\n" "$RED" "$NC" "$expected_code" "$http_code" | tee -a "$REPORT_FILE" FAILED_TESTS=$((FAILED_TESTS + 1)) { echo " ✗ $description" echo " URL: $url" } | tee -a "$REPORT_FILE" fi echo | tee -a "$REPORT_FILE" }
194-218: Strengthen the security headers test.The current test uses
curl -s -Iand checks if headers contain the security header names, but doesn't validate the header values. This could pass even if the headers are present but set to insecure values.Apply this diff to validate header values:
TOTAL_TESTS=$((TOTAL_TESTS + 1)) printf "Testing: Security headers... " | tee -a "$REPORT_FILE" headers=$(curl -s -I "$BASE_URL/index.php") -if echo "$headers" | grep -q "X-Content-Type-Options" && \ - echo "$headers" | grep -q "X-XSS-Protection" && \ - echo "$headers" | grep -q "X-Frame-Options"; then +if echo "$headers" | grep -q "X-Content-Type-Options: nosniff" && \ + echo "$headers" | grep -q "X-XSS-Protection: 1" && \ + echo "$headers" | grep -q "X-Frame-Options: SAMEORIGIN"; then printf "%sPASS%s\n" "$GREEN" "$NC" | tee -a "$REPORT_FILE" PASSED_TESTS=$((PASSED_TESTS + 1)) echo " ✓ All required security headers present" | tee -a "$REPORT_FILE" else printf "%sFAIL%s\n" "$RED" "$NC" | tee -a "$REPORT_FILE" FAILED_TESTS=$((FAILED_TESTS + 1)) echo " ✗ Missing required security headers" | tee -a "$REPORT_FILE" fitests/scripts/test_compatibility.sh (1)
44-70: Apply the same error handling improvement as intest_security.sh.The
run_testfunction here has the same curl error handling issue. Connection failures should be explicitly detected and reported.Apply a similar diff as suggested for
test_security.sh:run_test() { local test_name="$1" local url="$2" local expected_codes="$3" local description="$4" TOTAL_TESTS=$((TOTAL_TESTS + 1)) printf "Testing: %s... " "$test_name" | tee -a "$REPORT_FILE" # Make HTTP request and get status code - http_code=$(curl -s -o /dev/null -w "%{http_code}" -L "$url") + http_code=$(curl -s -o /dev/null -w "%{http_code}" -L "$url" 2>&1) + + # Check for curl errors + if [[ -z "$http_code" || "$http_code" == "000" ]]; then + printf "%sERROR%s (Connection failed)\n" "$RED" "$NC" | tee -a "$REPORT_FILE" + FAILED_TESTS=$((FAILED_TESTS + 1)) + echo " ✗ Unable to connect to server" | tee -a "$REPORT_FILE" + echo " URL: $url" | tee -a "$REPORT_FILE" + echo | tee -a "$REPORT_FILE" + return + fi # Check if code matches any of the expected codes if [[ "$expected_codes" = *"$http_code"* ]]; then printf "%sPASS%s (HTTP %s)\n" "$GREEN" "$NC" "$http_code" | tee -a "$REPORT_FILE" PASSED_TESTS=$((PASSED_TESTS + 1)) echo " ✓ $description" | tee -a "$REPORT_FILE" else printf "%sFAIL%s (Expected [%s], got %s)\n" "$RED" "$NC" "$expected_codes" "$http_code" | tee -a "$REPORT_FILE" FAILED_TESTS=$((FAILED_TESTS + 1)) echo " ✗ $description" | tee -a "$REPORT_FILE" echo " URL: $url" | tee -a "$REPORT_FILE" fi echo | tee -a "$REPORT_FILE" }src/OpenCoreEMR/FrontController/RouteConfig.php (2)
51-70: Consider regex validation and pattern maintenance.The default patterns look reasonable, but consider these improvements:
Regex validation: The patterns are not validated at initialization. Invalid regex patterns will only fail at runtime during
preg_match()calls, potentially causing security bypasses.Admin pattern readability (lines 61-62): The concatenated regex with many alternatives is hard to maintain. Consider extracting to an array and joining with
|, or documenting each alternative.Document access blocking (line 56): The pattern
#^sites/[^/]+/documents/#blocks all direct document access. Verify this doesn't prevent legitimate document viewing functionality that may need controlled access.Consider adding regex validation in the add*Pattern methods:
public function addForbiddenPattern(string $pattern): void { + if (@preg_match($pattern, '') === false) { + throw new \InvalidArgumentException("Invalid regex pattern: {$pattern}"); + } $this->forbiddenPatterns[] = $pattern; }Apply similar validation to
addAdminPattern()andaddDeprecatedPath().
77-100: Add duplicate pattern prevention.The add methods allow duplicate patterns to be added, which could cause unnecessary performance overhead during pattern matching.
Consider checking for duplicates before adding:
public function addForbiddenPattern(string $pattern): void { + if (in_array($pattern, $this->forbiddenPatterns, true)) { + return; + } $this->forbiddenPatterns[] = $pattern; }Apply similar logic to
addAdminPattern()andaddDeprecatedPath().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.env.example(1 hunks).github/workflows/test.yml(1 hunks).htaccess(1 hunks)README-FRONT-CONTROLLER.md(1 hunks)composer.json(1 hunks)home.php(1 hunks)phpunit.xml(2 hunks)reports/.gitkeep(1 hunks)security-check.php(1 hunks)src/OpenCoreEMR/FrontController/RouteConfig.php(1 hunks)src/OpenCoreEMR/FrontController/Router.php(1 hunks)src/OpenCoreEMR/FrontController/SecurityValidator.php(1 hunks)tests/Tests/FrontController/CompatibilityTest.php(1 hunks)tests/Tests/FrontController/SecurityTest.php(1 hunks)tests/reports/compatibility-test-report.json(1 hunks)tests/reports/security-test-report.json(1 hunks)tests/scripts/test_compatibility.sh(1 hunks)tests/scripts/test_performance.sh(1 hunks)tests/scripts/test_security.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
security-check.php (1)
library/classes/thumbnail/ThumbnailGenerator.php (1)
error_log(212-216)
home.php (3)
src/OpenCoreEMR/FrontController/Router.php (7)
Router(20-141)determineSiteId(52-63)extractRoute(36-47)handleTrailingSlash(70-77)isForbiddenPath(82-91)requiresAdmin(96-99)resolveTargetFile(104-124)src/OpenCoreEMR/FrontController/SecurityValidator.php (4)
SecurityValidator(20-53)logSecurityEvent(41-44)isIncludeFile(33-36)getDebugMessage(49-52)src/OpenCoreEMR/FrontController/RouteConfig.php (1)
requiresAdmin(124-132)
tests/Tests/FrontController/CompatibilityTest.php (1)
tests/Tests/FrontController/SecurityTest.php (1)
setUpBeforeClass(30-39)
tests/scripts/test_security.sh (1)
tests/scripts/test_compatibility.sh (1)
run_test(45-70)
tests/scripts/test_compatibility.sh (1)
tests/scripts/test_security.sh (1)
run_test(43-68)
src/OpenCoreEMR/FrontController/Router.php (1)
src/OpenCoreEMR/FrontController/RouteConfig.php (5)
RouteConfig(20-221)__construct(43-46)isDeprecated(140-148)isForbidden(108-116)requiresAdmin(124-132)
src/OpenCoreEMR/FrontController/RouteConfig.php (1)
src/OpenCoreEMR/FrontController/Router.php (2)
__construct(27-31)requiresAdmin(96-99)
tests/Tests/FrontController/SecurityTest.php (2)
tests/Tests/FrontController/CompatibilityTest.php (1)
setUpBeforeClass(29-37)src/Common/Http/HttpRestRequest.php (2)
hasHeader(225-228)getHeaderLine(697-700)
🪛 markdownlint-cli2 (0.18.1)
README-FRONT-CONTROLLER.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (40)
.env.example (1)
5-11: LGTM!The environment variable additions are well-documented and follow existing conventions. The comments clearly explain the purpose, valid values, and default behavior (disabled by default).
security-check.php (2)
1-14: LGTM!The file header is well-documented with appropriate copyright, license, and usage information.
15-15: Request path detection is acceptable but could be more defensive.The current implementation using
SCRIPT_FILENAMEandPHP_SELFis functional, but consider these points:
SCRIPT_FILENAMEcan be affected by server configuration (e.g., CGI/FastCGI setups)PHP_SELFcan contain user input in some configurations- If both are unavailable, the empty string fallback will cause the regex check to pass (which is safe, but the intent isn't explicit)
The current logic is safe for the regex match use case, but for defense-in-depth, consider logging when both variables are unavailable to detect configuration issues.
tests/scripts/test_performance.sh (1)
42-50: LGTM!The Apache Bench validation is well-implemented with clear error messaging and platform-specific installation instructions.
.github/workflows/test.yml (1)
344-354: LGTM build_test() in ci/ciLibrary.source accepts the “frontcontroller” testsuite as expected.reports/.gitkeep (1)
1-1: No code issues to report.Empty placeholder file is fine for preserving the directory.
phpunit.xml (2)
30-30: LGTM!The FrontController test directory is correctly added to the main test suite for comprehensive coverage.
59-61: LGTM!The dedicated
frontcontrollertest suite enables isolated testing of front controller functionality via--testsuite frontcontroller, which is useful for focused test runs during development and CI.tests/scripts/test_security.sh (8)
1-14: LGTM!Clear script header with usage instructions and AI disclosure. The licensing and copyright information is properly documented.
15-42: LGTM!Configuration and initialization are clean. The script properly sets up directories, colors using
tputfor portability, and initializes test counters.
70-95: LGTM!The test cases for
.inc.phpfile blocking are well-structured and cover multiple file types (.inc.phpand.inc). The expected 403 responses are correct for security-sensitive include files.
96-127: LGTM!Path traversal prevention tests are comprehensive, covering basic traversal, URL-encoded, double-encoded, and backslash variants. The expected 404 responses are appropriate.
128-173: LGTM!Non-PHP file blocking and non-existent file handling tests are appropriate. The checks for
.htaccess,composer.json, and.envfiles returning 404 are correct security measures.
174-193: LGTM!Legitimate file access tests verify that the front controller doesn't break normal functionality. Testing
index.phpandlogin.phpfor 200 responses is appropriate.
220-242: LGTM!The feature flag test appropriately checks that the front controller responds when enabled. The informational message when disabled (404) is helpful.
243-268: LGTM!The final report summary is clear and provides actionable feedback. The exit codes (0 for success, 1 for failure) are correct for shell script conventions.
tests/scripts/test_compatibility.sh (7)
1-16: LGTM!Script header is clear with proper documentation and AI disclosure.
17-43: LGTM!Configuration and initialization are properly set up with portable color handling via
tput.
72-95: LGTM!Core entry point tests are well-defined with appropriate expected status codes (200 or 302 redirects).
96-161: LGTM!Tests for existing front controllers, multisite support, and core workflows are comprehensive. The expected status codes appropriately allow for both successful responses and authentication redirects.
162-184: LGTM!Static asset tests appropriately check that CSS and JS files can be served directly. The conditional skip when the public directory doesn't exist is a good fallback.
186-220: LGTM!Custom modules and POST request tests verify that the front controller doesn't break existing functionality. The explicit check for 403/404 blocking on POST requests is valuable.
222-273: LGTM!File upload and AJAX endpoint tests ensure backward compatibility. The final report summary is clear and actionable.
.htaccess (7)
1-6: LGTM!The Apache rewrite configuration is properly initialized with
RewriteEngine OnandRewriteBase /.
7-12: LGTM!Forbidden path rules correctly deny access to framework internals and patient documents with 403 Forbidden responses.
14-40: LGTM!The pass-through rules for existing front controllers preserve backward compatibility by allowing direct access to APIs, portals, OAuth2, and various interface modules. This ensures the new front controller doesn't break existing routing.
42-59: LGTM!Static asset pass-through rules are comprehensive, covering stylesheets, scripts, images, fonts, and document formats. This ensures optimal performance by serving static files directly without PHP processing.
61-63: LGTM!Blocking
.inc.phpfiles at the Apache level is a critical security measure. The rule is correctly placed before the physical file check to prevent direct access to include files.
65-70: LGTM!The pass-through rules for physical files and directories ensure that existing files are served directly when they exist on disk, maintaining normal behavior for standard resources.
81-86: LGTM!Security headers are properly configured to enhance protection against common web vulnerabilities:
X-Content-Type-Options: nosniffprevents MIME-sniffingX-XSS-Protection: 1; mode=blockenables XSS filteringX-Frame-Options: SAMEORIGINprevents clickjackingREADME-FRONT-CONTROLLER.md (8)
1-25: LGTM!The overview section clearly explains the problem (direct
.inc.phpaccess causing errors) and the solution (centralized front controller). The benefits are well-articulated with checkmarks for visual clarity.
80-116: LGTM!Security features are comprehensively documented with clear categorization of path restrictions, admin-only paths, and the multi-layer defense for
.inc.phpblocking. The path traversal prevention examples cover all common attack variants.
117-200: LGTM!Static asset pass-through and extension hooks are well-documented with practical examples. The rate limiting example demonstrates proper session-based throttling, and the performance monitoring example shows good practices for observability.
201-242: LGTM!Quick start instructions are clear and actionable with commands for Apache, Nginx, and verification steps. The security test examples are practical and demonstrate expected responses.
243-309: LGTM!Testing section is comprehensive with automated test suites, shell scripts, CLI support, and a manual testing checklist. Rollback procedures are clearly documented with both instant disable and full removal options.
310-349: LGTM!Files inventory, performance expectations, and compatibility notes are accurate. The <5% overhead estimate is reasonable for the routing logic, and compatibility details cover all major deployment scenarios.
350-443: LGTM!Configuration details and troubleshooting section are thorough. The
.htaccessfeature flag workaround is well-explained, addressing the limitation that Apache doesn't natively support.envfiles. Alternative approaches for Apache virtual hosts and PHP'sputenv()are documented.
444-482: LGTM!The FAQ section comprehensively addresses common questions about code changes, performance impact, deployment scenarios, and extension mechanisms. The Docker example with
docker-compose.ymlis particularly helpful.src/OpenCoreEMR/FrontController/RouteConfig.php (2)
20-46: LGTM! Clean class structure with typed properties.The class declaration and property initialization follow best practices with typed arrays and proper encapsulation.
197-220: LGTM! Simple and correct getter methods.The getter methods correctly return the pattern arrays. Since PHP arrays are passed by value, returning the arrays directly does not expose internal state for modification.
| public function isForbidden(string $route): bool | ||
| { | ||
| foreach ($this->forbiddenPatterns as $pattern) { | ||
| if (preg_match($pattern, $route)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Check if route matches any admin-only pattern. | ||
| * | ||
| * @param string $route The route to check | ||
| * @return bool True if admin-only | ||
| */ | ||
| public function requiresAdmin(string $route): bool | ||
| { | ||
| foreach ($this->adminPatterns as $pattern) { | ||
| if (preg_match($pattern, $route)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Check if route is a deprecated path. | ||
| * | ||
| * @param string $route The route to check | ||
| * @return bool True if deprecated | ||
| */ | ||
| public function isDeprecated(string $route): bool | ||
| { | ||
| foreach ($this->deprecatedPaths as $pattern) { | ||
| if (preg_match($pattern, $route)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Add error handling for preg_match failures.
The pattern matching methods use preg_match() without checking for errors. If a pattern is malformed or causes an error (e.g., catastrophic backtracking), preg_match() returns false, which is treated as "no match" – potentially allowing access to forbidden routes.
Add error checking after each preg_match() call:
public function isForbidden(string $route): bool
{
foreach ($this->forbiddenPatterns as $pattern) {
- if (preg_match($pattern, $route)) {
+ $result = preg_match($pattern, $route);
+ if ($result === false) {
+ error_log("RouteConfig: Invalid pattern or preg_match error: {$pattern}");
+ return true; // Fail-safe: treat as forbidden on error
+ }
+ if ($result === 1) {
return true;
}
}
return false;
}Apply similar error handling to requiresAdmin() and isDeprecated(). Note the fail-safe behavior: on pattern error, treat the route as forbidden/admin-required to prevent security bypasses.
Optional refactoring: Extract the common matching logic into a private method to reduce duplication:
private function matchesAnyPattern(string $route, array $patterns): bool
{
foreach ($patterns as $pattern) {
$result = preg_match($pattern, $route);
if ($result === false) {
error_log("RouteConfig: Invalid pattern: {$pattern}");
return true; // Fail-safe
}
if ($result === 1) {
return true;
}
}
return false;
}🤖 Prompt for AI Agents
In src/OpenCoreEMR/FrontController/RouteConfig.php around lines 108 to 148, the
three pattern-checking methods call preg_match() without handling a false return
which can occur for malformed patterns or runtime errors and would be treated as
"no match" (unsafe); update isForbidden(), requiresAdmin(), and isDeprecated()
to check preg_match() result === false, log the offending pattern (e.g.,
error_log with context), and treat false as a positive match (fail-safe: return
true) while keeping result === 1 behavior returning true for a match; to remove
duplication, extract the loop and error-handling into a private helper like
matchesAnyPattern(string $route, array $patterns): bool and have the three
public methods delegate to it.
| # Function to run performance test | ||
| run_performance_test() { | ||
| local test_name="$1" | ||
| local url="$2" | ||
| local description="$3" | ||
|
|
||
| { | ||
| echo "----------------------------------------" | ||
| printf "%sTest: %s%s\n" "$BLUE" "$test_name" "$NC" | ||
| echo "URL: $url" | ||
| echo "Description: $description" | ||
| echo | ||
| } | tee -a "$REPORT_FILE" | ||
|
|
||
| # Run Apache Bench test | ||
| ab_output=$(ab -n "$ITERATIONS" -c 10 -q "$url" 2>&1 || true) | ||
|
|
||
| # Extract key metrics using awk | ||
| requests_per_sec=$(awk '/Requests per second/ {print $4}' <<< "$ab_output") | ||
| time_per_request=$(awk '/Time per request.*mean\)/ {print $4; exit}' <<< "$ab_output") | ||
| failed_requests=$(awk '/Failed requests/ {print $3}' <<< "$ab_output") | ||
|
|
||
| # Extract percentiles using awk | ||
| p50=$(awk '/50%/ {print $2}' <<< "$ab_output") | ||
| p95=$(awk '/95%/ {print $2}' <<< "$ab_output") | ||
| p99=$(awk '/99%/ {print $2}' <<< "$ab_output") | ||
|
|
||
| # Display results | ||
| { | ||
| echo "Results:" | ||
| echo " Requests/sec: $requests_per_sec" | ||
| echo " Time/request: ${time_per_request}ms (mean)" | ||
| echo " Failed requests: $failed_requests" | ||
| echo " 50th percentile: ${p50}ms" | ||
| echo " 95th percentile: ${p95}ms" | ||
| echo " 99th percentile: ${p99}ms" | ||
| echo | ||
| } | tee -a "$REPORT_FILE" | ||
|
|
||
| # Return metrics as array (for comparison) | ||
| echo "$requests_per_sec|$time_per_request|$p95" | ||
| } |
There was a problem hiding this comment.
Parsing is fragile and error handling is insufficient.
The run_performance_test function has several issues:
-
Line 67: Using
|| truesuppresses all errors fromab, including genuine failures (e.g., connection refused, timeout). Failed tests will produce empty metrics, but the script continues without warning. -
Lines 70-77: The
awkparsing assumes a specific output format from Apache Bench. This is fragile and will break if:aboutput format changes- The test fails and produces different output
- Locale settings affect number formatting
-
Line 92: Returns unquoted variables in a pipe-delimited string. If any metric is empty (due to parsing failure), downstream code will receive malformed data like
||123. -
Line 67: Hardcoded
-c 10(concurrency) may not be appropriate for all test scenarios, especially for measuring single-request latency.
Apply these improvements:
# Function to run performance test
run_performance_test() {
local test_name="$1"
local url="$2"
local description="$3"
{
echo "----------------------------------------"
printf "%sTest: %s%s\n" "$BLUE" "$test_name" "$NC"
echo "URL: $url"
echo "Description: $description"
echo
} | tee -a "$REPORT_FILE"
# Run Apache Bench test
- ab_output=$(ab -n "$ITERATIONS" -c 10 -q "$url" 2>&1 || true)
+ if ! ab_output=$(ab -n "$ITERATIONS" -c 10 -q "$url" 2>&1); then
+ {
+ printf "%sERROR: Apache Bench failed for %s%s\n" "$RED" "$test_name" "$NC"
+ echo " URL: $url"
+ echo " Output: $ab_output"
+ } | tee -a "$REPORT_FILE"
+ echo "0|0|0"
+ return 1
+ fi
# Extract key metrics using awk
- requests_per_sec=$(awk '/Requests per second/ {print $4}' <<< "$ab_output")
- time_per_request=$(awk '/Time per request.*mean\)/ {print $4; exit}' <<< "$ab_output")
- failed_requests=$(awk '/Failed requests/ {print $3}' <<< "$ab_output")
+ requests_per_sec=$(awk '/Requests per second/ {print $4}' <<< "$ab_output" || echo "0")
+ time_per_request=$(awk '/Time per request.*mean\)/ {print $4; exit}' <<< "$ab_output" || echo "0")
+ failed_requests=$(awk '/Failed requests/ {print $3}' <<< "$ab_output" || echo "0")
# Extract percentiles using awk
- p50=$(awk '/50%/ {print $2}' <<< "$ab_output")
- p95=$(awk '/95%/ {print $2}' <<< "$ab_output")
- p99=$(awk '/99%/ {print $2}' <<< "$ab_output")
+ p50=$(awk '/50%/ {print $2}' <<< "$ab_output" || echo "0")
+ p95=$(awk '/95%/ {print $2}' <<< "$ab_output" || echo "0")
+ p99=$(awk '/99%/ {print $2}' <<< "$ab_output" || echo "0")
# Display results
{
echo "Results:"
- echo " Requests/sec: $requests_per_sec"
- echo " Time/request: ${time_per_request}ms (mean)"
- echo " Failed requests: $failed_requests"
- echo " 50th percentile: ${p50}ms"
- echo " 95th percentile: ${p95}ms"
- echo " 99th percentile: ${p99}ms"
+ echo " Requests/sec: ${requests_per_sec:-N/A}"
+ echo " Time/request: ${time_per_request:-N/A}ms (mean)"
+ echo " Failed requests: ${failed_requests:-N/A}"
+ echo " 50th percentile: ${p50:-N/A}ms"
+ echo " 95th percentile: ${p95:-N/A}ms"
+ echo " 99th percentile: ${p99:-N/A}ms"
echo
} | tee -a "$REPORT_FILE"
# Return metrics as array (for comparison)
- echo "$requests_per_sec|$time_per_request|$p95"
+ echo "${requests_per_sec:-0}|${time_per_request:-0}|${p95:-0}"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Function to run performance test | |
| run_performance_test() { | |
| local test_name="$1" | |
| local url="$2" | |
| local description="$3" | |
| { | |
| echo "----------------------------------------" | |
| printf "%sTest: %s%s\n" "$BLUE" "$test_name" "$NC" | |
| echo "URL: $url" | |
| echo "Description: $description" | |
| echo | |
| } | tee -a "$REPORT_FILE" | |
| # Run Apache Bench test | |
| ab_output=$(ab -n "$ITERATIONS" -c 10 -q "$url" 2>&1 || true) | |
| # Extract key metrics using awk | |
| requests_per_sec=$(awk '/Requests per second/ {print $4}' <<< "$ab_output") | |
| time_per_request=$(awk '/Time per request.*mean\)/ {print $4; exit}' <<< "$ab_output") | |
| failed_requests=$(awk '/Failed requests/ {print $3}' <<< "$ab_output") | |
| # Extract percentiles using awk | |
| p50=$(awk '/50%/ {print $2}' <<< "$ab_output") | |
| p95=$(awk '/95%/ {print $2}' <<< "$ab_output") | |
| p99=$(awk '/99%/ {print $2}' <<< "$ab_output") | |
| # Display results | |
| { | |
| echo "Results:" | |
| echo " Requests/sec: $requests_per_sec" | |
| echo " Time/request: ${time_per_request}ms (mean)" | |
| echo " Failed requests: $failed_requests" | |
| echo " 50th percentile: ${p50}ms" | |
| echo " 95th percentile: ${p95}ms" | |
| echo " 99th percentile: ${p99}ms" | |
| echo | |
| } | tee -a "$REPORT_FILE" | |
| # Return metrics as array (for comparison) | |
| echo "$requests_per_sec|$time_per_request|$p95" | |
| } | |
| # Function to run performance test | |
| run_performance_test() { | |
| local test_name="$1" | |
| local url="$2" | |
| local description="$3" | |
| { | |
| echo "----------------------------------------" | |
| printf "%sTest: %s%s\n" "$BLUE" "$test_name" "$NC" | |
| echo "URL: $url" | |
| echo "Description: $description" | |
| echo | |
| } | tee -a "$REPORT_FILE" | |
| # Run Apache Bench test | |
| if ! ab_output=$(ab -n "$ITERATIONS" -c 10 -q "$url" 2>&1); then | |
| { | |
| printf "%sERROR: Apache Bench failed for %s%s\n" "$RED" "$test_name" "$NC" | |
| echo " URL: $url" | |
| echo " Output: $ab_output" | |
| } | tee -a "$REPORT_FILE" | |
| echo "0|0|0" | |
| return 1 | |
| fi | |
| # Extract key metrics using awk | |
| requests_per_sec=$(awk '/Requests per second/ {print $4}' <<< "$ab_output" || echo "0") | |
| time_per_request=$(awk '/Time per request.*mean\)/ {print $4; exit}' <<< "$ab_output" || echo "0") | |
| failed_requests=$(awk '/Failed requests/ {print $3}' <<< "$ab_output" || echo "0") | |
| # Extract percentiles using awk | |
| p50=$(awk '/50%/ {print $2}' <<< "$ab_output" || echo "0") | |
| p95=$(awk '/95%/ {print $2}' <<< "$ab_output" || echo "0") | |
| p99=$(awk '/99%/ {print $2}' <<< "$ab_output" || echo "0") | |
| # Display results | |
| { | |
| echo "Results:" | |
| echo " Requests/sec: ${requests_per_sec:-N/A}" | |
| echo " Time/request: ${time_per_request:-N/A}ms (mean)" | |
| echo " Failed requests: ${failed_requests:-N/A}" | |
| echo " 50th percentile: ${p50:-N/A}ms" | |
| echo " 95th percentile: ${p95:-N/A}ms" | |
| echo " 99th percentile: ${p99:-N/A}ms" | |
| echo | |
| } | tee -a "$REPORT_FILE" | |
| # Return metrics as array (for comparison) | |
| echo "${requests_per_sec:-0}|${time_per_request:-0}|${p95:-0}" | |
| } |
🤖 Prompt for AI Agents
tests/scripts/test_performance.sh around lines 52 to 93: the performance runner
currently swallows ab failures, parses its output fragily, returns
unquoted/possibly empty fields, and hardcodes concurrency; update it to (1) make
concurrency configurable via a variable (e.g., CONCURRENCY defaulting to 10)
instead of hardcoding -c 10; (2) run ab capturing both stdout and stderr but do
not use "|| true" — check ab's exit status and on non-zero exit log the full
output to REPORT_FILE, mark the test as failed, and return a clearly
identifiable failure token (e.g., "FAILED") or exit non-zero; (3) make parsing
defensive: verify expected lines exist before extracting values, normalize
locale (LC_ALL=C) when calling ab/awk to prevent number-format issues, and
provide sensible defaults (e.g., "N/A" or 0) when a metric cannot be parsed; (4)
ensure the returned pipe-delimited string is quoted and fields are
escaped/normalized so empty values become explicit tokens (e.g., "N/A|N/A|N/A")
to avoid producing malformed downstream input; and (5) add concise logs to
REPORT_FILE when parsing fails so downstream consumers can detect and handle
test errors.
| { | ||
| echo "========================================" | ||
| echo "PERFORMANCE ANALYSIS" | ||
| echo "========================================" | ||
| echo | ||
| } | tee -a "$REPORT_FILE" | ||
|
|
||
| # Calculate overhead | ||
| if [[ -n "$baseline_time" && -n "$fc_time" ]]; then | ||
| overhead=$(awk "BEGIN {printf \"%.2f\", ($fc_time - $baseline_time) / $baseline_time * 100}") | ||
|
|
||
| { | ||
| echo "Front Controller Overhead:" | ||
| echo " Baseline (direct): ${baseline_time}ms" | ||
| echo " Front controller: ${fc_time}ms" | ||
| echo " Overhead: ${overhead}%" | ||
| echo | ||
| } | tee -a "$REPORT_FILE" | ||
|
|
||
| # Performance verdict | ||
| if (( $(echo "$overhead < 5" | bc -l) )); then | ||
| { | ||
| printf "%s✓ EXCELLENT: Overhead < 5%%%s\n" "$GREEN" "$NC" | ||
| echo " Front controller has negligible performance impact" | ||
| } | tee -a "$REPORT_FILE" | ||
| elif (( $(echo "$overhead < 10" | bc -l) )); then | ||
| { | ||
| printf "%s✓ GOOD: Overhead < 10%%%s\n" "$GREEN" "$NC" | ||
| echo " Front controller has acceptable performance impact" | ||
| } | tee -a "$REPORT_FILE" | ||
| elif (( $(echo "$overhead < 20" | bc -l) )); then | ||
| { | ||
| printf "%s⚠ ACCEPTABLE: Overhead < 20%%%s\n" "$YELLOW" "$NC" | ||
| echo " Front controller adds some overhead, consider optimization" | ||
| } | tee -a "$REPORT_FILE" | ||
| else | ||
| { | ||
| printf "%s✗ HIGH OVERHEAD: > 20%%%s\n" "$RED" "$NC" | ||
| echo " Front controller adds significant overhead, optimization needed" | ||
| } | tee -a "$REPORT_FILE" | ||
| fi | ||
| fi | ||
|
|
||
| echo | tee -a "$REPORT_FILE" | ||
|
|
||
| # Throughput comparison | ||
| { | ||
| echo "Throughput Comparison:" | ||
| echo " Baseline: ${baseline_rps} req/sec" | ||
| echo " Front controller: ${fc_rps} req/sec" | ||
| echo " Login page: ${login_rps} req/sec" | ||
| if [[ -n "$static_rps" ]]; then | ||
| echo " Static assets: ${static_rps} req/sec" | ||
| fi | ||
| echo " REST API: ${api_rps} req/sec" | ||
| echo | ||
| } | tee -a "$REPORT_FILE" | ||
|
|
||
| # Latency Analysis | ||
| { | ||
| echo "Latency Analysis (95th percentile):" | ||
| echo " Baseline: ${baseline_p95}ms" | ||
| echo " Front controller: ${fc_p95}ms" | ||
| echo | ||
| } | tee -a "$REPORT_FILE" | ||
|
|
||
| # Resource Usage (if available) | ||
| if command -v ps &> /dev/null; then | ||
| php_procs=$(ps aux | grep php | grep -v grep | wc -l) | ||
| { | ||
| echo "Current Resource Usage:" | ||
| echo " Active PHP processes: $php_procs" | ||
| } | tee -a "$REPORT_FILE" | ||
|
|
||
| if command -v top &> /dev/null; then | ||
| cpu_usage=$(grep "CPU usage" <<< "$(top -l 1)" | awk '{print $3}' || echo "N/A") | ||
| echo " CPU usage: $cpu_usage" | tee -a "$REPORT_FILE" | ||
| fi | ||
| echo | tee -a "$REPORT_FILE" | ||
| fi | ||
|
|
||
| # Recommendations | ||
| { | ||
| echo "========================================" | ||
| echo "RECOMMENDATIONS" | ||
| echo "========================================" | ||
| echo | ||
| } | tee -a "$REPORT_FILE" | ||
|
|
||
| if [[ -n "$overhead" ]]; then | ||
| if (( $(echo "$overhead < 10" | bc -l) )); then | ||
| { | ||
| echo "✓ Performance is acceptable for production use" | ||
| echo "✓ Front controller adds minimal overhead" | ||
| echo "✓ Security benefits outweigh performance impact" | ||
| } | tee -a "$REPORT_FILE" | ||
| else | ||
| { | ||
| echo "⚠ Consider performance optimizations:" | ||
| echo " - Enable OPcache for PHP" | ||
| echo " - Use FastCGI process manager (PHP-FPM)" | ||
| echo " - Enable static asset caching" | ||
| echo " - Consider CDN for static assets" | ||
| } | tee -a "$REPORT_FILE" | ||
| fi | ||
| fi | ||
|
|
||
| { | ||
| echo | ||
| echo "========================================" | ||
| echo "Report saved to: $REPORT_FILE" | ||
| echo "========================================" | ||
| } | tee -a "$REPORT_FILE" |
There was a problem hiding this comment.
Arithmetic operations lack error handling and portability concerns.
The analysis and reporting section has several reliability issues:
-
Lines 192-193: Arithmetic with
awkassumesbaseline_timeandfc_timeare valid numbers. If the previous tests failed or returned "0", the overhead calculation will be meaningless or produce division-by-zero errors. -
Lines 204-224: Multiple comparisons using
bc -lassumebcis available. While common, it's not always installed by default (e.g., minimal Docker images, some BSD systems). -
Line 259: The
topcommand parsing is macOS-specific (top -l 1) and thegreppattern is fragile. This will fail on Linux and other Unix systems. -
Lines 235-237: Variable expansions like
${static_rps}in the throughput comparison are not quoted. If the variable is empty (because the test was skipped), the output will show an empty line. -
Lines 273-289: Recommendations are based solely on overhead percentage without considering absolute time (e.g., 20% overhead on 5ms is very different from 20% on 500ms).
Apply these fixes:
# Calculate overhead
if [[ -n "$baseline_time" && -n "$fc_time" ]]; then
- overhead=$(awk "BEGIN {printf \"%.2f\", ($fc_time - $baseline_time) / $baseline_time * 100}")
+ # Validate numeric values and avoid division by zero
+ if [[ "$baseline_time" =~ ^[0-9]+\.?[0-9]*$ ]] && [[ "$fc_time" =~ ^[0-9]+\.?[0-9]*$ ]] && (( $(awk "BEGIN {print ($baseline_time > 0)}") )); then
+ overhead=$(awk "BEGIN {printf \"%.2f\", ($fc_time - $baseline_time) / $baseline_time * 100}")
+ else
+ overhead="N/A"
+ fi
{
echo "Front Controller Overhead:"
echo " Baseline (direct): ${baseline_time}ms"
echo " Front controller: ${fc_time}ms"
echo " Overhead: ${overhead}%"
echo
} | tee -a "$REPORT_FILE"
# Performance verdict
- if (( $(echo "$overhead < 5" | bc -l) )); then
+ if [[ "$overhead" != "N/A" ]] && awk "BEGIN {exit !($overhead < 5)}"; then
{
printf "%s✓ EXCELLENT: Overhead < 5%%%s\n" "$GREEN" "$NC"
echo " Front controller has negligible performance impact"
} | tee -a "$REPORT_FILE"
- elif (( $(echo "$overhead < 10" | bc -l) )); then
+ elif awk "BEGIN {exit !($overhead < 10)}"; then
{
printf "%s✓ GOOD: Overhead < 10%%%s\n" "$GREEN" "$NC"
echo " Front controller has acceptable performance impact"
} | tee -a "$REPORT_FILE"
- elif (( $(echo "$overhead < 20" | bc -l) )); then
+ elif awk "BEGIN {exit !($overhead < 20)}"; then
{
printf "%s⚠ ACCEPTABLE: Overhead < 20%%%s\n" "$YELLOW" "$NC"
echo " Front controller adds some overhead, consider optimization"
} | tee -a "$REPORT_FILE"
else
{
printf "%s✗ HIGH OVERHEAD: > 20%%%s\n" "$RED" "$NC"
echo " Front controller adds significant overhead, optimization needed"
} | tee -a "$REPORT_FILE"
fi
fi {
echo "Throughput Comparison:"
echo " Baseline: ${baseline_rps} req/sec"
echo " Front controller: ${fc_rps} req/sec"
echo " Login page: ${login_rps} req/sec"
- if [[ -n "$static_rps" ]]; then
- echo " Static assets: ${static_rps} req/sec"
- fi
+ [[ -n "${static_rps:-}" ]] && echo " Static assets: ${static_rps} req/sec"
echo " REST API: ${api_rps} req/sec"
echo
} | tee -a "$REPORT_FILE"-# Resource Usage (if available)
-if command -v ps &> /dev/null; then
- php_procs=$(ps aux | grep php | grep -v grep | wc -l)
- {
- echo "Current Resource Usage:"
- echo " Active PHP processes: $php_procs"
- } | tee -a "$REPORT_FILE"
-
- if command -v top &> /dev/null; then
- cpu_usage=$(grep "CPU usage" <<< "$(top -l 1)" | awk '{print $3}' || echo "N/A")
- echo " CPU usage: $cpu_usage" | tee -a "$REPORT_FILE"
- fi
- echo | tee -a "$REPORT_FILE"
-fi
+# Resource Usage (if available, cross-platform)
+if command -v pgrep &> /dev/null; then
+ php_procs=$(pgrep -c php || echo "0")
+ {
+ echo "Current Resource Usage:"
+ echo " Active PHP processes: $php_procs"
+ } | tee -a "$REPORT_FILE"
+
+ # CPU usage reporting is system-specific, so just note the count
+ echo " (CPU usage monitoring requires system-specific tools)"
+ echo | tee -a "$REPORT_FILE"
+fi if [[ -n "$overhead" ]]; then
- if (( $(echo "$overhead < 10" | bc -l) )); then
+ if [[ "$overhead" != "N/A" ]] && awk "BEGIN {exit !($overhead < 10)}"; then
{
echo "✓ Performance is acceptable for production use"
echo "✓ Front controller adds minimal overhead"
echo "✓ Security benefits outweigh performance impact"
} | tee -a "$REPORT_FILE"
else🤖 Prompt for AI Agents
In tests/scripts/test_performance.sh around lines 184-296, the reporting block
lacks input validation, portable tools, and robust parsing; ensure baseline_time
and fc_time are validated as numbers and baseline_time > 0 before computing
overhead (skip or set overhead="N/A" on invalid/zero), replace bc-dependent
numeric comparisons with awk (which is more likely present) or bash-safe integer
checks so colorized verdicts work without bc, detect and parse CPU usage
portably (on Linux read /proc/stat or use top -b -n1; on macOS keep top -l 1)
with branching based on uname, quote all variable expansions like
"${static_rps}" to avoid empty-word issues, and change recommendation logic to
consider both overhead percentage and absolute delta_ms (compute
delta_ms=fc_time-baseline_time and factor a threshold so recommendations use
both percentage and absolute latency before signaling optimizations).
| public static function setUpBeforeClass(): void | ||
| { | ||
| self::$baseUrl = getenv('OPENEMR_TEST_URL') ?: 'http://localhost/openemr'; | ||
| self::$client = new Client([ | ||
| 'base_uri' => self::$baseUrl, | ||
| 'http_errors' => false, | ||
| 'allow_redirects' => true, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Detect unreachable base URL before running compatibility checks
Like the security suite, this setup raises RequestException when no OpenEMR instance listens at OPENEMR_TEST_URL, causing the whole PHPUnit run to fail. Please wrap an initial request in a try/catch with a short timeout and skip (or fail fast with a helpful message) when the service is unavailable.
public static function setUpBeforeClass(): void
{
self::$baseUrl = getenv('OPENEMR_TEST_URL') ?: 'http://localhost/openemr';
- self::$client = new Client([
- 'base_uri' => self::$baseUrl,
- 'http_errors' => false,
- 'allow_redirects' => true,
- ]);
+ self::$client = new Client([
+ 'base_uri' => self::$baseUrl,
+ 'http_errors' => false,
+ 'allow_redirects' => true,
+ 'timeout' => 5,
+ ]);
+
+ try {
+ self::$client->get('/index.php', ['allow_redirects' => false]);
+ } catch (RequestException $e) {
+ self::markTestSkipped(
+ 'Front controller compatibility tests require a reachable OpenEMR instance at '
+ . self::$baseUrl . ' (' . $e->getMessage() . ')'
+ );
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function setUpBeforeClass(): void | |
| { | |
| self::$baseUrl = getenv('OPENEMR_TEST_URL') ?: 'http://localhost/openemr'; | |
| self::$client = new Client([ | |
| 'base_uri' => self::$baseUrl, | |
| 'http_errors' => false, | |
| 'allow_redirects' => true, | |
| ]); | |
| } | |
| public static function setUpBeforeClass(): void | |
| { | |
| self::$baseUrl = getenv('OPENEMR_TEST_URL') ?: 'http://localhost/openemr'; | |
| self::$client = new Client([ | |
| 'base_uri' => self::$baseUrl, | |
| 'http_errors' => false, | |
| 'allow_redirects' => true, | |
| 'timeout' => 5, | |
| ]); | |
| try { | |
| // Quick probe to ensure the OpenEMR instance is up | |
| self::$client->get('/index.php', ['allow_redirects' => false]); | |
| } catch (RequestException $e) { | |
| self::markTestSkipped( | |
| 'Front controller compatibility tests require a reachable OpenEMR instance at ' | |
| . self::$baseUrl . ' (' . $e->getMessage() . ')' | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/Tests/FrontController/CompatibilityTest.php around lines 29-37, the
test suite unconditionally creates an HTTP client which later causes a
RequestException when no OpenEMR instance is reachable; add an initial
short-timeout probe request to the base URL inside setUpBeforeClass wrapped in
try/catch, and on exception either throw a PHPUnit skip (or
SkippedTestSuiteError) with a clear message or call the appropriate skip helper
so the suite is skipped rather than failing the entire run; ensure the probe
uses a small timeout and preserves existing client configuration when
proceeding.
| public static function setUpBeforeClass(): void | ||
| { | ||
| self::$baseUrl = getenv('OPENEMR_TEST_URL') ?: 'http://localhost/openemr'; | ||
| self::$client = new Client([ | ||
| 'base_uri' => self::$baseUrl, | ||
| 'http_errors' => false, | ||
| 'allow_redirects' => true, | ||
| ]); | ||
| self::loadVulnerableIncFiles(); |
There was a problem hiding this comment.
Skip tests when the target instance is unreachable
Running the PHPUnit suite without a web server at OPENEMR_TEST_URL throws RequestException before any assertions run, breaking CI. Please probe the base URL with a short timeout and call self::markTestSkipped() (or fail with a clear message) when the endpoint is unreachable.
public static function setUpBeforeClass(): void
{
self::$baseUrl = getenv('OPENEMR_TEST_URL') ?: 'http://localhost/openemr';
- self::$client = new Client([
- 'base_uri' => self::$baseUrl,
- 'http_errors' => false,
- 'allow_redirects' => true,
- ]);
- self::loadVulnerableIncFiles();
+ self::$client = new Client([
+ 'base_uri' => self::$baseUrl,
+ 'http_errors' => false,
+ 'allow_redirects' => true,
+ 'timeout' => 5,
+ ]);
+
+ try {
+ self::$client->head('/index.php', ['allow_redirects' => false]);
+ } catch (RequestException $e) {
+ self::markTestSkipped(
+ 'Front controller security tests require a reachable OpenEMR instance at '
+ . self::$baseUrl . ' (' . $e->getMessage() . ')'
+ );
+ }
+
+ self::loadVulnerableIncFiles();
}| private static function loadVulnerableIncFiles(): void | ||
| { | ||
| $basePath = dirname(dirname(__DIR__)); | ||
| $iterator = new \RecursiveIteratorIterator( | ||
| new \RecursiveDirectoryIterator($basePath) | ||
| ); | ||
|
|
||
| foreach ($iterator as $file) { | ||
| if ($file->isFile() && preg_match('/\.inc\.php$/i', $file->getFilename())) { | ||
| $relativePath = str_replace($basePath . '/', '', $file->getPathname()); | ||
| self::$vulnerable_inc_files[] = $relativePath; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Scan the real application tree for .inc.php files
dirname(dirname(__DIR__)) resolves to the tests/ directory, so self::$vulnerable_inc_files stays empty and all .inc.php assertions silently do nothing. Please root the scan at the project base (and optionally skip when no files are found).
- $basePath = dirname(dirname(__DIR__));
+ $basePath = dirname(__DIR__, 3);After pointing to the application root, also consider failing or skipping when the collection stays empty so the test cannot silently pass without coverage.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static function loadVulnerableIncFiles(): void | |
| { | |
| $basePath = dirname(dirname(__DIR__)); | |
| $iterator = new \RecursiveIteratorIterator( | |
| new \RecursiveDirectoryIterator($basePath) | |
| ); | |
| foreach ($iterator as $file) { | |
| if ($file->isFile() && preg_match('/\.inc\.php$/i', $file->getFilename())) { | |
| $relativePath = str_replace($basePath . '/', '', $file->getPathname()); | |
| self::$vulnerable_inc_files[] = $relativePath; | |
| } | |
| } | |
| } | |
| private static function loadVulnerableIncFiles(): void | |
| { | |
| $basePath = dirname(__DIR__, 3); | |
| $iterator = new \RecursiveIteratorIterator( | |
| new \RecursiveDirectoryIterator($basePath) | |
| ); | |
| foreach ($iterator as $file) { | |
| if ($file->isFile() && preg_match('/\.inc\.php$/i', $file->getFilename())) { | |
| $relativePath = str_replace($basePath . '/', '', $file->getPathname()); | |
| self::$vulnerable_inc_files[] = $relativePath; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/Tests/FrontController/SecurityTest.php around lines 41-54 the loader
roots the scan at dirname(dirname(__DIR__)) (the tests/ dir) so no .inc.php
files in the application are found; change the basePath to the
project/application root (e.g. resolve from getcwd() or climb from __DIR__ to
the repo root) and ensure the resolved path exists; after scanning, if
self::$vulnerable_inc_files is empty either fail the test explicitly or mark it
as skipped so assertions cannot silently no-op.
…tation ARCHITECTURAL IMPROVEMENTS: - Extracted routing logic from home.php into dedicated Router class (src/OpenCoreEMR/FrontController/Router.php) - Created RouteConfig class for centralized route configuration (src/OpenCoreEMR/FrontController/RouteConfig.php) - Delegated security validation to SecurityValidator class - Implemented event system hooks for module extensibility - Removed global variables, improved testability and maintainability ROUTING SYSTEM: - Route patterns now centralized in RouteConfig with clear categorization - Admin paths, forbidden paths, bypass patterns all configurable - Event system allows modules to extend routing rules dynamically - Removed hardcoded path matching from home.php CI INTEGRATION: - Integrated FrontController tests with PHPUnit CI pipeline (phpunit.xml) - Created dedicated 'frontcontroller' test suite for targeted testing - Added tests to main 'openemr' suite for comprehensive coverage - Enabled CLI test execution via OPENEMR_TEST_URL environment variable TEST QUALITY IMPROVEMENTS: - Replaced curl with Guzzle HTTP client in CompatibilityTest.php - Fixed HTTP assertions to be specific per endpoint (200, 302, 401, 400) - Moved tests to PSR-4 compliant location (tests/Tests/FrontController/) - Added proper client setup with base_uri and error handling configuration SHELL SCRIPT QUALITY (POSIX compliance): - Replaced ANSI escape sequences with tput commands for portability - Changed echo -n/-e to printf for POSIX compliance - Replaced [ ] with [[ ]] for consistency and safety - Changed == to = for string comparisons (POSIX compliant) - Consolidated echo statements into compound commands - Used herestrings instead of echo | pipe for efficiency - Removed auto-install logic for security best practices DOCUMENTATION: - Updated architecture diagram showing Router/RouteConfig delegation - Added extensible configuration examples with event system - Updated file counts (6→8 core files, correct test paths) - Added troubleshooting section (.htaccess flags, performance, CLI) - Documented CI integration commands and test execution - Added FAQ about extending routing rules via Event System SECURITY: - Maintained all security validations (path traversal, .inc.php blocking) - Preserved feature flag controls and multisite selection - No changes to security-check.php auto-prepend mechanism - All security patterns now in RouteConfig for easier auditing BACKWARD COMPATIBILITY: - Zero functional changes to routing behavior - All existing front controllers preserved (/apis/, /portal/, /oauth2/) - Static assets still bypass routing - Extension hooks remain optional Addresses Client Review Comments #1-21:
…tation ARCHITECTURAL IMPROVEMENTS: - Extracted routing logic from home.php into dedicated Router class (src/OpenCoreEMR/FrontController/Router.php) - Created RouteConfig class for centralized route configuration (src/OpenCoreEMR/FrontController/RouteConfig.php) - Delegated security validation to SecurityValidator class - Implemented event system hooks for module extensibility - Removed global variables, improved testability and maintainability ROUTING SYSTEM: - Route patterns now centralized in RouteConfig with clear categorization - Admin paths, forbidden paths, bypass patterns all configurable - Event system allows modules to extend routing rules dynamically - Removed hardcoded path matching from home.php CI INTEGRATION: - Integrated FrontController tests with PHPUnit CI pipeline (phpunit.xml) - Created dedicated 'frontcontroller' test suite for targeted testing - Added tests to main 'openemr' suite for comprehensive coverage - Enabled CLI test execution via OPENEMR_TEST_URL environment variable TEST QUALITY IMPROVEMENTS: - Replaced curl with Guzzle HTTP client in CompatibilityTest.php - Fixed HTTP assertions to be specific per endpoint (200, 302, 401, 400) - Moved tests to PSR-4 compliant location (tests/Tests/FrontController/) - Added proper client setup with base_uri and error handling configuration SHELL SCRIPT QUALITY (POSIX compliance): - Replaced ANSI escape sequences with tput commands for portability - Changed echo -n/-e to printf for POSIX compliance - Replaced [ ] with [[ ]] for consistency and safety - Changed == to = for string comparisons (POSIX compliant) - Consolidated echo statements into compound commands - Used herestrings instead of echo | pipe for efficiency - Removed auto-install logic for security best practices DOCUMENTATION: - Updated architecture diagram showing Router/RouteConfig delegation - Added extensible configuration examples with event system - Updated file counts (6→8 core files, correct test paths) - Added troubleshooting section (.htaccess flags, performance, CLI) - Documented CI integration commands and test execution - Added FAQ about extending routing rules via Event System SECURITY: - Maintained all security validations (path traversal, .inc.php blocking) - Preserved feature flag controls and multisite selection - No changes to security-check.php auto-prepend mechanism - All security patterns now in RouteConfig for easier auditing BACKWARD COMPATIBILITY: - Zero functional changes to routing behavior - All existing front controllers preserved (/apis/, /portal/, /oauth2/) - Static assets still bypass routing - Extension hooks remain optional Addresses Client Review Comments #1-21:
1 participant
Fixes #
Short description of what this resolves:
Changes proposed in this pull request:
Does your code include anything generated by an AI Engine? Yes / No
If you answered yes: Verify that each file that has AI generated code has a description that describes what AI engine was used and that the file includes AI generated code. Sections of code that are entirely or mostly generated by AI should be marked with a comment header and footer that includes the AI engine used and stating the code was AI.
Summary by CodeRabbit
New Features
Security
Configuration
Documentation
Tests / CI