Add global front controller with security hardening #9026
+2,944
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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)
kojiromike
requested changes
Oct 3, 2025
There was a problem hiding this comment.
Good first pass. There are some things that need to be corrected, so I will request changes here.
One thing is that the routing is all hard-coded into home.php right now. But might there be a way to define the routing in a more extensible way?
I left specific comments where I thought it would be useful. It might also be useful to consider some of the ideas from https://symfony.com/doc/current/create_framework/front_controller.html because this project is heading in a Symfony-friendly direction.
One thing I found interesting from that doc was the idea of putting the front controller in a subdirectory and then setting the webroot to that subdirectory. So configured, all the content would only be reachable by the front controller via a controlled path traversal.
Another way to think of it might be, how could we build a front controller that would work if openemr were a phar?
home.php
Outdated
| } | ||
|
|
||
| // Late extension hook | ||
| if (file_exists(__DIR__ . '/custom/front_controller_late.php')) { |
There was a problem hiding this comment.
The late extension should come after the requested content.
|
I was moving towards moving portal to a module. |
@sjpadgett I'm confused – did you mean to make this comment on this PR? I don't see a lot of interaction with Phreeze here. |
…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.
kojiromike
requested changes
Oct 3, 2025
Comment on lines
110
to
115
| foreach ($this->forbiddenPatterns as $pattern) { | ||
| if (preg_match($pattern, $route)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Suggested change
| foreach ($this->forbiddenPatterns as $pattern) { | |
| if (preg_match($pattern, $route)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| return array_any( | |
| $this->forbiddenPatterns, | |
| fn(string: $pattern): bool => (bool) preg_match($pattern, $route) | |
| ); |
| */ | ||
| public function requiresAdmin(string $route): bool | ||
| { | ||
| foreach ($this->adminPatterns as $pattern) { |
| */ | ||
| public function isDeprecated(string $route): bool | ||
| { | ||
| foreach ($this->deprecatedPaths as $pattern) { |
There was a problem hiding this comment.
You can probably create a single private function that determines isDeprecated, requiresAdmin, and isForbidden using array_any and another parameter to determine which paths to reference.
| public function __construct(string $baseDir, ?RouteConfig $config = null) | ||
| { | ||
| $this->baseDir = realpath($baseDir); | ||
| $this->config = $config ?? new RouteConfig(); |
There was a problem hiding this comment.
To enable DI, the RouteConfig type should be an interface.
Comment on lines
41
to
43
| if (($pos = strpos($route, '?')) !== false) { | ||
| $route = substr($route, 0, $pos); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (($pos = strpos($route, '?')) !== false) { | |
| $route = substr($route, 0, $pos); | |
| } | |
| $route = strtok($route, '?'); |
home.php
Outdated
| $dotenv->safeLoad(); | ||
|
|
||
| // Check feature flag - when disabled, provide transparent pass-through | ||
| if (!getenv('OPENEMR_ENABLE_FRONT_CONTROLLER') || getenv('OPENEMR_ENABLE_FRONT_CONTROLLER') !== '1') { |
There was a problem hiding this comment.
Looks like a good spot for the null coalesce operator.
home.php
Outdated
| // See https://github.com/adunsulag/oe-module-custom-skeleton for module development | ||
| $GLOBALS['kernel']->getEventDispatcher()->dispatch( | ||
| new \Symfony\Component\EventDispatcher\GenericEvent(), | ||
| 'front_controller.early' |
There was a problem hiding this comment.
Please create a new event entry in src/Events/FrontController.
home.php
Outdated
Comment on lines
128
to
131
| $GLOBALS['kernel']->getEventDispatcher()->dispatch( | ||
| new \Symfony\Component\EventDispatcher\GenericEvent(), | ||
| 'front_controller.late' | ||
| ); |
There was a problem hiding this comment.
We should think about what to do if the $targetFile exits or crashes. We could use try/finally, but that wouldn't necessarily handle exit(). Maybe register_shutdown_function?
I think we may want to even provide that context to the event, so that the event handler can do things based on whether the included script completed normally, threw an exception, or exited early.
| ]; | ||
|
|
||
| $reportPath = __DIR__ . '/../../reports/compatibility-test-report.json'; | ||
| @mkdir(dirname($reportPath), 0755, true); |
There was a problem hiding this comment.
why do we want to hide warnings here?
custom/.gitkeep
Outdated
There was a problem hiding this comment.
is this directory still being used?
|
|
@coding-sunshine I didn't realize how recent array_any and array_all are. They're very new features to PHP (>=8.4). We'll need to go back to a foreach loop in those cases, as we still need to support php 8.2 and up here. I apologize for the error. |
08df75e to
90cd613
Compare
Fixed PHPStan errors in RouteConfig.php: - Renamed addForbiddenPattern() → addForbiddenPath() to match interface - Renamed addAdminPattern() → addAdminPath() to match interface - Replaced non-existent array_any() with standard foreach loop Fixed ShellCheck warnings in test scripts: - Added tput fallback for non-interactive environments in test_compatibility.sh - Added tput fallback for non-interactive environments in test_performance.sh
Add spaces in arithmetic expressions to comply with ShellCheck SC2250. Changed all arithmetic from $((VAR + 1)) to $(( VAR + 1 )) format. Changes: - test_compatibility.sh: Fixed arithmetic spacing on lines 56, 66, 70, 223, 232, 236 - test_security.sh: Fixed arithmetic spacing on lines 54, 62, 66, 207, 216, 220, 233, 240 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- test_performance.sh: Remove orphaned $YELLOW reference, add braces to all variables
- test_security.sh: Add braces to all variable references including $headers and $response
- test_compatibility.sh: Already compliant with SC2250
ShellCheck SC2250 requires braces on ALL variable references throughout shell scripts,
not just in specific contexts. All $VAR references changed to ${VAR}.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Address SC2312: "Consider invoking this command separately to avoid masking its return value" by refactoring command substitutions in test conditions. Changes: - test_performance.sh: Separate bc command substitutions from test conditions in evaluate_overhead() and overhead calculation to avoid masking exit codes - test_security.sh: Replace piped echo commands with here-strings for grep to prevent masking return values in security header checks All changes preserve existing functionality while ensuring proper error handling and exit code visibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Address multiple ShellCheck warnings to improve script reliability: SC2311 - set -e disabled in command substitutions: - Add 'shopt -s inherit_errexit' to all test scripts to ensure error handling works correctly in command substitutions - Prevents silent failures when functions are called via $() SC2034 - unused variables: - Prefix unused login_time variable with underscore (_login_time) - Indicates intentionally unused value from command output SC2009 - avoid grepping ps output: - Replace 'ps aux | grep php' with 'pgrep -c php' - More reliable and portable process counting All changes maintain backward compatibility and improve error handling. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Address ShellCheck SC2312 warnings by ensuring command substitutions are executed separately before being used in pipes, preventing masked return values and improving error detection. Changes: - Extract date command outside of pipe blocks in all test scripts - Move uname check outside command substitution in CPU detection - Refactor top command execution to separate steps - Build throughput output string before piping to tee - Replace inline grep pipes with here-strings for top output parsing All changes maintain the same functionality while ensuring proper error handling and exit code visibility as required by ShellCheck.
cfb7ea2 to
5b45567
Compare
Split grep|awk pipelines in CPU usage detection to avoid masking return values. Extract grep output first, then conditionally apply awk to prevent SC2312 warnings. Changes: - macOS: separate 'grep CPU usage' and 'awk' into distinct steps - Linux: separate 'grep Cpu(s)' and 'awk' into distinct steps - Add conditional checks for grep output before awk processing - Maintain same error handling with fallback to N/A
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8964
Short description of what this resolves:
Implements centralized routing architecture with multi-layer security:
Changes proposed in this pull request:
Implements centralized routing architecture with multi-layer security:
Security improvements:
Backward compatibility:
Files added:
Files modified: