Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a global PHP front controller ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Agent
participant A as Web Server (.htaccess)
participant H as home.php (Front Controller)
participant R as Router
participant S as SecurityValidator
participant T as Target PHP
participant E as Event Dispatcher
U->>A: HTTP request (URI)
alt Static or existing file
A-->>U: Serve static asset or existing controller
else Rewritten to front controller
A->>H: Rewrite to /home.php?_ROUTE=...
H->>E: Dispatch front_controller.early (if available)
H->>R: extractRoute() / determineSiteId()
H->>S: validate include / forbidden / admin rules
alt Blocked (.inc.php or forbidden)
H-->>U: 403 or 404 response
H->>E: Dispatch front_controller.late (error context)
else Resolve target
R-->>H: target path or null
alt Not found
H-->>U: 404 response
H->>E: Dispatch front_controller.late (error context)
else Include/execute target (adjust SCRIPT_* vars)
H->>T: include target
T-->>H: output/response
H->>E: Dispatch front_controller.late (success context)
H-->>U: Response
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Files/areas meriting extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 17
♻️ Duplicate comments (1)
tests/reports/compatibility-test-report.json (1)
1-5: Duplicate: Test artifacts should not be committed.Same issue as
security-test-report.json- this generated test artifact should be excluded from version control.See comment on
tests/reports/security-test-report.jsonfor the recommended approach.
🧹 Nitpick comments (13)
tests/scripts/test_performance.sh (2)
94-101: Add timeout to curl/ab commands to prevent hanging.The Apache Bench command lacks a timeout, which could cause the script to hang indefinitely if the server is unresponsive. This is particularly problematic in CI/CD pipelines.
Apply this diff to add a timeout:
- if ! ab_output=$(ab -n "$ITERATIONS" -c 10 -q "$url" 2>&1); then + if ! ab_output=$(timeout 60 ab -n "$ITERATIONS" -c 10 -q "$url" 2>&1); then { printf "%sERROR: Apache Bench failed for %s%s\n" "$RED" "$url" "$NC" echo "Output: $ab_output"
272-291: Resource usage detection may fail silently on some systems.The script attempts to detect PHP processes and CPU usage but the methods used may not be portable across all environments:
- Line 273:
grep -c '[p]hp'may count unrelated processes or miss PHP-FPM processes- Lines 281-286: Different
topsyntax between macOS/Linux could fail silently with|| echo "N/A"Consider more robust process detection:
# More specific PHP process detection php_procs=$(ps aux | grep -E '[p]hp(-fpm|-cgi)?(\s|$)' | grep -v grep | wc -l)Alternatively, document these as "best effort" metrics in the report.
tests/reports/security-test-report.json (1)
1-6: Consider excluding test report artifacts from version control.Test reports like this JSON file are typically generated during test execution and should not be committed to the repository. They can become stale, create merge conflicts, and increase repository size over time.
Add test reports to
.gitignore:+# Test reports +tests/reports/*.json +tests/reports/*.txtIf you need example reports for documentation, consider:
- Keeping them in a separate
examples/directory- Documenting the expected format in README files
- Generating them during CI/CD only
tests/scripts/test_compatibility.sh (1)
56-56: Add timeout and connection limits to curl commands.The curl commands lack timeouts, which could cause the script to hang indefinitely if a server is unresponsive. This is problematic in CI/CD environments.
Apply these diffs:
# 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 --max-time 30 --connect-timeout 10 "${url}")http_code=$(curl -s -o /dev/null -w "%{http_code}" -X POST \ + --max-time 30 --connect-timeout 10 \ -d "authUser=test&authPass=test" \ "${BASE_URL}/interface/login/login.php")Also applies to: 221-223
README-FRONT-CONTROLLER.md (1)
28-490: Add fenced-code languages to satisfy markdownlint.CI lint (MD040) flags the many
blocks without a language. Tag them (`bash, ```php, ```apache`, etc.) to keep docs lint-clean.src/OpenCoreEMR/FrontController/Router.php (1)
38-44: Validate the_ROUTEparameter to prevent potential issues.The route is extracted from
$_GET['_ROUTE']without validation. Whilestrtok()removes query strings, the route itself could contain malicious or malformed input that might cause issues downstream.Consider adding basic input validation:
public function extractRoute(): string { $route = $_GET['_ROUTE'] ?? ''; + + // Basic validation: ensure route is a string and not empty after trimming + $route = trim((string) $route); // Remove query string $route = strtok($route, '?'); $this->route = $route; return $route; }src/OpenCoreEMR/FrontController/RouteConfig.php (3)
169-173: File extension validation is too restrictive.The validation only allows
.phpfiles, but the docblock (lines 153-158) and the logic suggest this is for loading configuration, not executing arbitrary PHP. Consider if JSON or other safer formats should be supported.If the intention is to only load PHP config arrays, the current implementation is acceptable. However, consider supporting JSON for better security:
// Validate file extension $ext = strtolower(pathinfo($configPath, PATHINFO_EXTENSION)); if ($ext === 'php') { $config = require $realPath; } elseif ($ext === 'json') { $json = file_get_contents($realPath); $config = json_decode($json, true); if (json_last_error() !== JSON_ERROR_NONE) { return false; } } else { return false; }
176-179: Redundantrealpath()check afterfile_exists().Line 165 already checks
file_exists(), and if true, the path is valid. Therealpath()check on line 176 will always succeed iffile_exists()returned true (on most systems), making this check redundant.The check is defensive programming and doesn't hurt, but could be simplified:
- // Validate realpath to prevent path traversal - $realPath = realpath($configPath); - if ($realPath === false) { - return false; - } + // Use realpath for consistent path resolution + $realPath = realpath($configPath);
183-199: No validation of pattern array contents.The code iterates through the config arrays and adds patterns without validating that they are valid regex patterns. Invalid patterns could cause
preg_match()failures inmatchesAnyPattern().Add pattern validation:
if (isset($config['forbidden']) && is_array($config['forbidden'])) { foreach ($config['forbidden'] as $pattern) { + // Validate regex pattern + if (@preg_match($pattern, '') === false) { + continue; // Skip invalid patterns + } $this->addForbiddenPattern($pattern); } } if (isset($config['admin']) && is_array($config['admin'])) { foreach ($config['admin'] as $pattern) { + if (@preg_match($pattern, '') === false) { + continue; + } $this->addAdminPattern($pattern); } } if (isset($config['deprecated']) && is_array($config['deprecated'])) { foreach ($config['deprecated'] as $pattern) { + if (@preg_match($pattern, '') === false) { + continue; + } $this->addDeprecatedPath($pattern); } }tests/Tests/FrontController/CompatibilityTest.php (4)
31-36: HTTP client allows redirects by default, which may hide routing issues.Setting
allow_redirectstotruein the default client configuration means tests may not catch redirect loops or incorrect redirect behavior. Individual tests override this, but the default could mask issues.Consider setting redirects to false by default and explicitly enabling them in tests that need to follow redirects:
self::$baseUrl = getenv('OPENEMR_TEST_URL') ?: 'http://localhost/openemr'; self::$client = new Client([ 'base_uri' => self::$baseUrl, 'http_errors' => false, - 'allow_redirects' => true, + 'allow_redirects' => false, ]);
169-180: Test assumes redirect behavior without verifying redirect target.The test expects a 302 redirect but doesn't verify that the redirect is to the login page. A redirect to any location would pass this test.
Enhance the test to verify the redirect location:
$response = self::$client->get('/interface/patient_file/summary/demographics.php', [ 'allow_redirects' => false ]); $httpCode = $response->getStatusCode(); + $location = $response->getHeader('Location')[0] ?? ''; // Should redirect to login (302) since not authenticated $this->assertEquals( 302, $httpCode, 'Patient file should redirect to login when not authenticated' ); + $this->assertStringContainsString( + 'login', + strtolower($location), + 'Redirect should be to login page' + );
244-259: Test uses hardcoded credentials without cleanup.The test posts hardcoded test credentials (
authUserandauthPassboth set to 'test') without any cleanup. If these credentials exist in the test environment, this could create a session that affects subsequent tests.Add a comment noting the limitation and consider using unique test credentials:
public function testPostRequestsWork(): void { + // Note: Using invalid credentials to test POST handling without creating sessions $response = self::$client->post('/interface/login/login.php', [ 'form_params' => [ - 'authUser' => 'test', - 'authPass' => 'test' + 'authUser' => 'nonexistent_test_user_' . time(), + 'authPass' => 'invalid_password_' . time() ], 'allow_redirects' => false ]);
291-293: Report file path uses relative path that may fail in different contexts.Using
__DIR__ . '/../../reports/'assumes a specific directory structure relative to the test file. This could break if tests are run from different working directories or if the directory structure changes.Use a more robust path resolution:
- $reportPath = __DIR__ . '/../../reports/compatibility-test-report.json'; + // Use a path relative to the project root or a configurable location + $projectRoot = dirname(__DIR__, 3); // Assumes tests are in tests/Tests/FrontController/ + $reportPath = $projectRoot . '/tests/reports/compatibility-test-report.json'; mkdir(dirname($reportPath), 0755, true); file_put_contents($reportPath, json_encode($report, JSON_PRETTY_PRINT) . "\n");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.env.example(1 hunks).github/workflows/test.yml(1 hunks).gitignore(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/Events/FrontController/FrontControllerEvent.php(1 hunks)src/OpenCoreEMR/FrontController/RouteConfig.php(1 hunks)src/OpenCoreEMR/FrontController/RouteConfigInterface.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 (11)
src/OpenCoreEMR/FrontController/SecurityValidator.php (1)
library/classes/thumbnail/ThumbnailGenerator.php (1)
error_log(212-216)
tests/scripts/test_security.sh (1)
tests/scripts/test_compatibility.sh (1)
run_test(45-70)
home.php (3)
src/OpenCoreEMR/FrontController/Router.php (8)
Router(20-129)determineSiteId(50-57)extractRoute(36-45)handleTrailingSlash(64-71)isForbiddenPath(76-79)requiresAdmin(84-87)resolveTargetFile(92-112)getSiteId(125-128)src/OpenCoreEMR/FrontController/SecurityValidator.php (4)
SecurityValidator(20-53)logSecurityEvent(41-44)isIncludeFile(33-36)getDebugMessage(49-52)src/Events/FrontController/FrontControllerEvent.php (2)
FrontControllerEvent(23-136)getSiteId(86-89)
src/OpenCoreEMR/FrontController/Router.php (3)
src/Events/FrontController/FrontControllerEvent.php (3)
__construct(64-69)getRoute(76-79)getSiteId(86-89)src/OpenCoreEMR/FrontController/RouteConfig.php (5)
__construct(43-46)RouteConfig(20-233)isDeprecated(145-148)isForbidden(123-126)requiresAdmin(134-137)src/OpenCoreEMR/FrontController/RouteConfigInterface.php (3)
isDeprecated(65-65)isForbidden(49-49)requiresAdmin(57-57)
src/Events/FrontController/FrontControllerEvent.php (2)
src/OpenCoreEMR/FrontController/RouteConfig.php (1)
__construct(43-46)src/OpenCoreEMR/FrontController/Router.php (3)
__construct(27-31)getRoute(117-120)getSiteId(125-128)
src/OpenCoreEMR/FrontController/RouteConfig.php (3)
src/Events/FrontController/FrontControllerEvent.php (1)
__construct(64-69)src/OpenCoreEMR/FrontController/Router.php (2)
__construct(27-31)requiresAdmin(84-87)src/OpenCoreEMR/FrontController/RouteConfigInterface.php (5)
addDeprecatedPath(41-41)isForbidden(49-49)requiresAdmin(57-57)isDeprecated(65-65)loadFromFile(73-73)
tests/Tests/FrontController/CompatibilityTest.php (1)
tests/Tests/FrontController/SecurityTest.php (1)
setUpBeforeClass(30-39)
security-check.php (1)
library/classes/thumbnail/ThumbnailGenerator.php (1)
error_log(212-216)
src/OpenCoreEMR/FrontController/RouteConfigInterface.php (2)
src/OpenCoreEMR/FrontController/RouteConfig.php (5)
addDeprecatedPath(97-100)isForbidden(123-126)requiresAdmin(134-137)isDeprecated(145-148)loadFromFile(163-202)src/OpenCoreEMR/FrontController/Router.php (1)
requiresAdmin(84-87)
tests/scripts/test_compatibility.sh (1)
tests/scripts/test_security.sh (1)
run_test(43-68)
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)
🔇 Additional comments (11)
.github/workflows/test.yml (1)
344-355: FrontController test integration looks consistent.The new
build_test frontcontrollerstep and Codecov upload mirror the existing suite pattern and slot cleanly into the workflow. No issues from my side..gitignore (1)
31-31: Ignore generated test reportsIgnoring
tests/reports/keeps transient artifacts out of version control. Change looks good..env.example (1)
7-12: LGTM! Clear documentation for optional front controller feature.The new environment variables are well-documented with clear explanations of their purpose and expected values. The optional nature and default behavior (disabled) are appropriately communicated.
tests/scripts/test_performance.sh (2)
1-50: LGTM! Well-structured script setup with proper error handling.The script initialization is solid:
- Proper shebang and
set -efor error handling- Clear configuration with sensible defaults
- Good user feedback with color-coded output
- Proactive check for Apache Bench availability with installation instructions
226-246: LGTM! Robust numeric validation before calculations.Good defensive programming with:
- Validation of numeric values using regex
- Clear warning message when validation fails
- Proper use of bc for portable floating-point arithmetic
phpunit.xml (1)
30-30: LGTM! Consistent test suite additions.The new
frontcontrollertest suite is properly integrated:
- Added to the main
openemrsuite for comprehensive test runs- Available as a standalone suite for targeted testing
- Follows the existing naming and structure conventions
Also applies to: 59-61
composer.json (1)
190-191: LGTM! Proper PSR-4 autoload configuration.The new
OpenCoreEMRnamespace is correctly mapped tosrc/OpenCoreEMR, enabling autoloading for the new front controller components. The mapping follows PSR-4 standards and coexists properly with the existingOpenEMRnamespace.tests/scripts/test_compatibility.sh (3)
1-70: LGTM! Clean script structure with good organization.The script setup is well-designed:
- Proper error handling with
set -e- Clear configuration and reporting
- Reusable
run_testfunction that handles multiple expected codes- Color-coded output for better readability
178-194: LGTM! Graceful handling of optional test dependencies.Good defensive programming:
- Checks for the existence of the public directory before running static asset tests
- Provides informative skip message
- Prevents false failures when directory structure varies
277-293: LGTM! Clear success/failure reporting with actionable guidance.The final summary is excellent:
- Color-coded pass/fail status
- Contextual guidance based on results
- Proper exit codes for CI/CD integration
- Encourages review of failures
tests/Tests/FrontController/CompatibilityTest.php (1)
86-97: LGTM: REST API test correctly validates routing without authentication.The test appropriately checks that the REST API endpoint returns 401 (unauthorized), which confirms that routing is working and authentication is required. This is a good way to test routing without needing valid credentials.
| @@ -1,4 +1,12 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
Remove the shebang line from .env.example.
A .env file is a data file loaded by libraries like phpdotenv, not an executable script. The shebang #!/bin/sh serves no purpose here and could confuse developers about the file's role. Environment variable files should contain only variable definitions and comments.
Apply this diff:
-#!/bin/sh
-
# Possible options are prod or dev. No spaces, no quotes.📝 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.
| #!/bin/sh | |
| # Possible options are prod or dev. No spaces, no quotes. |
🤖 Prompt for AI Agents
In .env.example around lines 1 to 1, remove the shebang line ("#!/bin/sh")
because .env files are not executable scripts; edit the file to delete that
first line so it contains only environment variable declarations and comments,
saving the file without adding any new executable directives.
| 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 with safe defaults | ||
| 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 | ||
| } | 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.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Verify that metric extraction handles edge cases correctly.
The awk commands use || echo "0" as fallback values when parsing fails. While this prevents script failures, it could mask legitimate parsing errors or unexpected ab output formats, leading to misleading "0" values in the report.
Consider adding validation to distinguish between:
- Legitimate zero values (e.g., no failed requests)
- Parsing failures due to unexpected output format
- Network/server errors that prevent ab from running
For example, check if the ab_output contains expected patterns before extracting metrics:
🏁 Script executed:
# Verify ab output contains expected sections
if ! grep -q "Requests per second" <<< "$ab_output"; then
echo "WARNING: Unexpected ab output format" | tee -a "$REPORT_FILE"
fiLength of output: 39
Validate ab exit code and output format before parsing metrics
Add a POSIX-compatible pre-check of the ab command and ensure expected sections exist. For example:
# Run ab and capture its exit code and output
ab_output=$(ab "$AB_OPTS" 2>&1) || ab_failed=1
ab_exit_code=${ab_failed:-0}
# Verify ab succeeded and output contains "Requests per second"
if [ "$ab_exit_code" -ne 0 ] || ! printf '%s\n' "$ab_output" | grep -q "Requests per second"; then
echo "WARNING: ab failed (exit code $ab_exit_code) or unexpected output format" | tee -a "$REPORT_FILE"
return 1
fi
# Safe parsing with pipe (no here-string)
requests_per_sec=$(printf '%s\n' "$ab_output" | awk '/Requests per second/ {print $4}')
time_per_request=$(printf '%s\n' "$ab_output" | awk '/Time per request.*mean\)/ {print $4; exit}')
failed_requests=$(printf '%s\n' "$ab_output" | awk '/Failed requests/ {print $3}')
# Percentiles
p50=$(printf '%s\n' "$ab_output" | awk '/50%/ {print $2}')
p95=$(printf '%s\n' "$ab_output" | awk '/95%/ {print $2}')
p99=$(printf '%s\n' "$ab_output" | awk '/99%/ {print $2}')This ensures you distinguish between:
abexecution failures- Missing or malformed sections in its output
- Legitimate zero values when parsing succeeds
🤖 Prompt for AI Agents
tests/scripts/test_performance.sh lines 104-127: the script currently parses ab
output unconditionally using here-strings and fallbacks, which can mask ab
failures or malformed output; update the code to capture ab's exit code and
stdout/stderr (e.g., ab_output=$(ab "$AB_OPTS" 2>&1; ab_exit=$?; export
ab_exit)), then check that ab_exit is zero and that ab_output contains the
expected marker like "Requests per second" (log a warning to $REPORT_FILE and
return 1 if not), and after that parse metrics using printf '%s\n' "$ab_output"
| awk ... (replace here-strings and the || echo "0" fallbacks) so parsing only
happens on validated output and legitimate zero values are preserved.
| private static function loadVulnerableIncFiles(): void | ||
| { | ||
| $basePath = dirname(__DIR__, 2); | ||
| $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.
Test never inspects production .inc.php files.
dirname(__DIR__, 2) resolves to the tests/ folder, so $iterator never touches the real application tree. self::$vulnerable_inc_files stays empty, the loop skips, and the test can’t catch regressions. Walk up one more level (repo root) before iterating.
- $basePath = dirname(__DIR__, 2);
+ $basePath = dirname(__DIR__, 3);📝 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(__DIR__, 2); | |
| $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 test
sets $basePath = dirname(__DIR__, 2) which resolves to the tests/ directory so
the RecursiveDirectoryIterator never scans the application tree; change the base
path to walk up one more level (e.g. dirname(__DIR__, 3) or realpath(__DIR__ .
'/../../..')) so $iterator starts at the repository root and the loop can
collect production .inc.php files into self::$vulnerable_inc_files.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/Tests/FrontController/SecurityTest.php (1)
41-55: Scan the real codebase, not justtests/
dirname(__DIR__, 2)lands inside the tests directory, so the iterator never reaches production.inc.phpfiles—self::$vulnerable_inc_filesstays empty and the test vacuously passes. Walk up one more level to the repo root.- $basePath = dirname(__DIR__, 2); + $basePath = dirname(__DIR__, 3);src/OpenCoreEMR/FrontController/RouteConfig.php (1)
110-115: Replace PHP 8.4-onlyarray_any()
array_any()doesn’t exist before PHP 8.4. With our composer constraint at 8.2, hittingisForbidden()/requiresAdmin()/isDeprecated()will fatal. Use a simple loop instead.Apply this diff:
- return array_any( - $patterns, - fn(string $pattern): bool => (bool) preg_match($pattern, $route) - ); + foreach ($patterns as $pattern) { + if (preg_match($pattern, $route)) { + return true; + } + } + + return false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.htaccess(1 hunks)home.php(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/SecurityTest.php(1 hunks)tests/scripts/test_security.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/OpenCoreEMR/FrontController/SecurityValidator.php
- src/OpenCoreEMR/FrontController/Router.php
- .htaccess
🧰 Additional context used
🧬 Code graph analysis (5)
src/OpenCoreEMR/FrontController/RouteConfig.php (3)
src/OpenCoreEMR/FrontController/Router.php (2)
__construct(27-35)requiresAdmin(91-94)src/Events/FrontController/FrontControllerEvent.php (1)
__construct(64-69)src/OpenCoreEMR/FrontController/RouteConfigInterface.php (5)
addDeprecatedPath(41-41)isForbidden(49-49)requiresAdmin(57-57)isDeprecated(65-65)loadFromFile(73-73)
home.php (3)
src/OpenCoreEMR/FrontController/Router.php (8)
Router(20-136)determineSiteId(54-61)extractRoute(40-49)handleTrailingSlash(68-78)isForbiddenPath(83-86)requiresAdmin(91-94)resolveTargetFile(99-119)getSiteId(132-135)src/OpenCoreEMR/FrontController/SecurityValidator.php (4)
SecurityValidator(20-53)logSecurityEvent(41-44)isIncludeFile(33-36)getDebugMessage(49-52)src/Events/FrontController/FrontControllerEvent.php (2)
FrontControllerEvent(23-136)getSiteId(86-89)
tests/scripts/test_security.sh (1)
tests/scripts/test_compatibility.sh (1)
run_test(45-70)
security-check.php (1)
library/classes/thumbnail/ThumbnailGenerator.php (1)
error_log(212-216)
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)
🪛 GitHub Actions: ShellCheck
tests/scripts/test_security.sh
[info] 20-20: Prefer putting braces around variable references even when not strictly required.
[info] 40-40: Consider invoking this command separately to avoid masking its return value (or use || true to ignore).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: PHP 8.4 - apache - mysql 5.7
- GitHub Check: PHP 8.4 - nginx - mariadb 11.8
- GitHub Check: PHP 8.5 - nginx - mariadb 11.8
- GitHub Check: PHP 8.3 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 11.4
- GitHub Check: PHP 8.4 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 9.3
- GitHub Check: PHP 8.4 - apache - mysql 8.4
- GitHub Check: PHP 8.2 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 8.0
- GitHub Check: PHP 8.4 - apache - mariadb 10.11
- GitHub Check: PHP 8.2 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 10.6
- GitHub Check: PHP 8.3 - apache - mariadb 11.8
- GitHub Check: Rector PHP Analysis (8.4)
- GitHub Check: phpstan (8.4)
- GitHub Check: PHP 8.4 - Isolated Tests
- GitHub Check: PHP 8.3 - Isolated Tests
- GitHub Check: PHP 8.2 - Isolated Tests
- GitHub Check: PHP 8.5 - Isolated Tests
| // Block .inc.php and .config.php files, and any PHP under /includes/ or /inc/ | ||
| if (preg_match('/\.(?:inc|config)\.php$/i', $requestedFile) || preg_match('#/(?:includes?|inc)/#i', $requestedFile)) { | ||
| http_response_code(403); | ||
| // Sanitize: remove control chars, limit length, preserve only filename | ||
| $sanitizedFile = basename(preg_replace('/[\x00-\x1F\x7F]/', '', $requestedFile)); | ||
| $sanitizedFile = substr($sanitizedFile, 0, 255); // Limit length | ||
| error_log("OpenEMR Security: Blocked .inc.php access attempt: " . $sanitizedFile); | ||
| exit('Access Denied'); |
There was a problem hiding this comment.
Block raw .inc requests too.
The guard only matches *.inc.php/.config.php. A direct hit on *.inc still leaks the include file (served as plain text). Align with the front controller check and the new tests by rejecting both .inc and .inc.php.
Apply this diff:
-if (preg_match('/\.(?:inc|config)\.php$/i', $requestedFile) || preg_match('#/(?:includes?|inc)/#i', $requestedFile)) {
+if (
+ preg_match('/\.inc(?:\.php)?$/i', $requestedFile) ||
+ preg_match('/\.config\.php$/i', $requestedFile) ||
+ preg_match('#/(?:includes?|inc)/#i', $requestedFile)
+) {📝 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.
| // Block .inc.php and .config.php files, and any PHP under /includes/ or /inc/ | |
| if (preg_match('/\.(?:inc|config)\.php$/i', $requestedFile) || preg_match('#/(?:includes?|inc)/#i', $requestedFile)) { | |
| http_response_code(403); | |
| // Sanitize: remove control chars, limit length, preserve only filename | |
| $sanitizedFile = basename(preg_replace('/[\x00-\x1F\x7F]/', '', $requestedFile)); | |
| $sanitizedFile = substr($sanitizedFile, 0, 255); // Limit length | |
| error_log("OpenEMR Security: Blocked .inc.php access attempt: " . $sanitizedFile); | |
| exit('Access Denied'); | |
| // Block .inc, .inc.php, .config.php files, and any PHP under /includes/ or /inc/ | |
| if ( | |
| preg_match('/\.inc(?:\.php)?$/i', $requestedFile) || | |
| preg_match('/\.config\.php$/i', $requestedFile) || | |
| preg_match('#/(?:includes?|inc)/#i', $requestedFile) | |
| ) { | |
| http_response_code(403); | |
| // Sanitize: remove control chars, limit length, preserve only filename | |
| $sanitizedFile = basename(preg_replace('/[\x00-\x1F\x7F]/', '', $requestedFile)); | |
| $sanitizedFile = substr($sanitizedFile, 0, 255); // Limit length | |
| error_log("OpenEMR Security: Blocked .inc.php access attempt: " . $sanitizedFile); | |
| exit('Access Denied'); | |
| } |
🤖 Prompt for AI Agents
In security-check.php around lines 17 to 24, the current guard only blocks
*.inc.php and .config.php but not raw *.inc files; update the first preg_match
to reject both .inc and the existing .inc.php/.config.php variants by replacing
the pattern with a single combined regex that matches either a trailing ".inc"
or the ".inc.php"/".config.php" forms (keep the i flag), leaving the rest of the
block (sanitization, logging, exit) intact.
| set -e | ||
|
|
||
| # Configuration | ||
| BASE_URL="${1:-http://localhost/openemr}" | ||
| REPORT_DIR="$(dirname "$0")/../../reports" | ||
| REPORT_FILE="$REPORT_DIR/security-test-report-$(date +%Y%m%d-%H%M%S).txt" | ||
|
|
||
| # Colors for output using tput (with fallback for non-interactive environments) | ||
| if command -v tput >/dev/null 2>&1 && [ -n "$TERM" ]; then | ||
| RED=$(tput setaf 1) | ||
| GREEN=$(tput setaf 2) | ||
| YELLOW=$(tput setaf 3) | ||
| NC=$(tput sgr0) # No Color | ||
| else | ||
| RED="" | ||
| GREEN="" | ||
| YELLOW="" | ||
| NC="" | ||
| fi | ||
|
|
||
| # Create report directory | ||
| mkdir -p "$REPORT_DIR" | ||
|
|
||
| { | ||
| echo "Security Test Report" | ||
| echo "Date: $(date)" | ||
| echo "URL: $BASE_URL" | ||
| echo | ||
| } | tee "$REPORT_FILE" | ||
|
|
There was a problem hiding this comment.
Honor pipeline failures (fix ShellCheck).
ShellCheck is failing because set -e + | tee masks upstream errors. Add set -o pipefail so any command in the pipeline aborts the script, and keep CI green.
-set -e
+set -e
+set -o pipefail(While you’re here, wrap bare variables like "$BASE_URL" in braces to silence the remaining warning.) Based on pipeline failures
📝 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.
| set -e | |
| # Configuration | |
| BASE_URL="${1:-http://localhost/openemr}" | |
| REPORT_DIR="$(dirname "$0")/../../reports" | |
| REPORT_FILE="$REPORT_DIR/security-test-report-$(date +%Y%m%d-%H%M%S).txt" | |
| # Colors for output using tput (with fallback for non-interactive environments) | |
| if command -v tput >/dev/null 2>&1 && [ -n "$TERM" ]; then | |
| RED=$(tput setaf 1) | |
| GREEN=$(tput setaf 2) | |
| YELLOW=$(tput setaf 3) | |
| NC=$(tput sgr0) # No Color | |
| else | |
| RED="" | |
| GREEN="" | |
| YELLOW="" | |
| NC="" | |
| fi | |
| # Create report directory | |
| mkdir -p "$REPORT_DIR" | |
| { | |
| echo "Security Test Report" | |
| echo "Date: $(date)" | |
| echo "URL: $BASE_URL" | |
| echo | |
| } | tee "$REPORT_FILE" | |
| set -e | |
| set -o pipefail | |
| # Configuration | |
| BASE_URL="${1:-http://localhost/openemr}" | |
| REPORT_DIR="$(dirname "$0")/../../reports" | |
| REPORT_FILE="$REPORT_DIR/security-test-report-$(date +%Y%m%d-%H%M%S).txt" | |
| # Colors for output using tput (with fallback for non-interactive environments) | |
| if command -v tput >/dev/null 2>&1 && [ -n "$TERM" ]; then | |
| RED=$(tput setaf 1) | |
| GREEN=$(tput setaf 2) | |
| YELLOW=$(tput setaf 3) | |
| NC=$(tput sgr0) # No Color | |
| else | |
| RED="" | |
| GREEN="" | |
| YELLOW="" | |
| NC="" | |
| fi | |
| # Create report directory | |
| mkdir -p "$REPORT_DIR" | |
| { | |
| echo "Security Test Report" | |
| echo "Date: $(date)" | |
| echo "URL: $BASE_URL" | |
| echo | |
| } | tee "$REPORT_FILE" |
🧰 Tools
🪛 GitHub Actions: ShellCheck
[info] 20-20: Prefer putting braces around variable references even when not strictly required.
[info] 40-40: Consider invoking this command separately to avoid masking its return value (or use || true to ignore).
🤖 Prompt for AI Agents
In tests/scripts/test_security.sh around lines 15 to 44, the script uses set -e
but not pipefail which allows failures in earlier pipeline stages to be masked
by commands like | tee; add set -o pipefail immediately after set -e so any
failing command in a pipeline causes the script to exit, and update any bare
variable expansions to use braced forms (e.g. "${BASE_URL}", "${REPORT_DIR}",
"${REPORT_FILE}") throughout this block to satisfy ShellCheck and avoid
word-splitting issues.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Summary
This PR implements a comprehensive Global Front Controller security feature for OpenEMR. The change introduces a centralized request routing system that intercepts all PHP requests through a new `home.php` front controller, providing enhanced security controls while maintaining backward compatibility.Core Architecture Changes:
- Front Controller Pattern: All PHP requests are now routed through
home.phpwhen the feature is enabled viaOPENEMR_ENABLE_FRONT_CONTROLLER=1 - Security Layer: Blocks direct access to
.inc.phpfiles (which were causing fatal errors when accessed directly) and prevents path traversal attacks - Event System: Introduces
FrontControllerEventfor module extensibility with early/late request lifecycle hooks - Route Configuration: Implements
RouteConfigInterfacewith forbidden/admin/deprecated path management using regex patterns - Multisite Support: Preserves existing multisite functionality while adding centralized routing
Security Enhancements:
- Prevents direct access to include files that weren't designed as web entry points
- Implements path traversal protection and directory restriction controls
- Adds comprehensive security headers and validation
- Includes multiple defense layers: Apache
.htaccessrules, PHP-level blocking viasecurity-check.php, and front controller validation
Backward Compatibility:
- Feature flag controlled (
OPENEMR_ENABLE_FRONT_CONTROLLER) with transparent pass-through when disabled - Preserves all existing entry points and workflows
- Maintains context manipulation so target files see correct
__FILE__and__DIR__values - Extensive pass-through rules for static assets and existing front controllers
Testing Infrastructure:
- Comprehensive test suites covering compatibility, security, and performance
- Automated report generation for CI/CD integration
- Shell script validation alongside PHPUnit tests
New Namespace Structure:
- Adds
OpenCoreEMR\namespace undersrc/OpenCoreEMRfor core front controller infrastructure - Separates architectural concerns from existing OpenEMR classes
This implementation follows modern PHP framework patterns (similar to Symfony) and addresses real security vulnerabilities where direct file access was causing system errors and potential security exposure.
PR Description Notes:
- Missing issue number reference ("Fixes #" is empty)
- Missing description of what the PR resolves
- Missing list of proposed changes
- Missing AI engine disclosure confirmation
Important Files Changed
Changed Files
| Filename | Score | Overview |
|---|---|---|
| home.php | 2/5 | New global front controller with feature flag, security validation, event system integration, and routing logic - has critical bugs in feature flag check and interface mismatches |
| src/OpenCoreEMR/FrontController/RouteConfig.php | 1/5 | Route configuration implementation using PHP 8.4 array_any() function incompatible with required PHP 8.2, plus method naming mismatch with interface |
| src/OpenCoreEMR/FrontController/Router.php | 2/5 | Core routing class with path security validation and multisite support - contains security vulnerabilities in redirect handling and inconsistent path validation |
| src/OpenCoreEMR/FrontController/SecurityValidator.php | 2/5 | Security validation utilities for CLI detection and include file validation - has documentation mismatch and environment variable naming inconsistency |
| .htaccess | 4/5 | Comprehensive Apache configuration implementing front controller pattern with security blocking and pass-through rules |
| security-check.php | 4/5 | PHP security layer that blocks direct .inc.php file access with proper sanitization and logging |
| README-FRONT-CONTROLLER.md | 4/5 | Extensive documentation covering architecture, configuration, security features, testing, and troubleshooting |
| src/OpenCoreEMR/FrontController/RouteConfigInterface.php | 4/5 | Interface defining contract for routing configuration with security-critical path management methods |
| src/Events/FrontController/FrontControllerEvent.php | 4/5 | Event system for front controller extensibility following Symfony patterns with lifecycle hooks |
| tests/Tests/FrontController/CompatibilityTest.php | 4/5 | Comprehensive compatibility testing ensuring existing functionality remains intact during migration |
| tests/Tests/FrontController/SecurityTest.php | 4/5 | Security test suite validating .inc.php blocking, path traversal prevention, and security headers |
| tests/reports/security-test-report.json | 4/5 | Generated security test report documenting successful .inc.php file blocking validation |
| tests/reports/compatibility-test-report.json | 4/5 | Generated compatibility test report confirming preservation of core functionality |
| tests/scripts/test_performance.sh | 4/5 | Performance testing script comparing direct access vs front controller routing with Apache Bench |
| tests/scripts/test_compatibility.sh | 4/5 | Bash script for comprehensive backward compatibility testing of various entry points and workflows |
| tests/scripts/test_security.sh | 4/5 | Security testing script validating .inc.php blocking, path traversal prevention, and security headers |
| composer.json | 5/5 | Adds new PSR-4 autoloader namespace mapping for OpenCoreEMR\ to src/OpenCoreEMR directory |
| .github/workflows/test.yml | 5/5 | Adds FrontController testing step to CI workflow with Codecov integration |
| phpunit.xml | 5/5 | Integrates FrontController tests into both main test suite and dedicated isolated test suite |
| .env.example | 3/5 | Adds front controller configuration variables but includes incorrect shebang line for a config file |
| reports/.gitkeep | 5/5 | Standard .gitkeep file to preserve empty reports directory structure in version control |
| .gitignore | 5/5 | Excludes tests/reports/ directory from version control to prevent committing generated test artifacts |
Confidence score: 2/5
- This PR introduces significant architectural changes with several critical bugs and compatibility issues that could cause production failures
- Score reflects serious implementation flaws including PHP version incompatibility, logic errors in core routing, security vulnerabilities, and interface mismatches
- Pay close attention to RouteConfig.php (PHP 8.4 incompatibility), home.php (feature flag logic bug), Router.php (security vulnerabilities), and SecurityValidator.php (environment variable inconsistency)
Sequence Diagram
sequenceDiagram
participant User
participant WebServer as Web Server (.htaccess)
participant HomeController as home.php
participant Dotenv as Dotenv Library
participant EventDispatcher as Event Dispatcher
participant Router as Router
participant RouteConfig as Route Config
participant SecurityValidator as Security Validator
participant TargetFile as Target PHP File
User->>WebServer: "HTTP Request to /some/file.php"
Note over WebServer: Check routing rules
alt Static assets (CSS, JS, images)
WebServer-->>User: "Serve directly (no routing)"
else Existing front controllers (/apis/, /portal/, /oauth2/)
WebServer-->>User: "Pass through to existing controllers"
else .inc.php files
WebServer-->>User: "403 Forbidden"
else PHP files
WebServer->>HomeController: "Route to home.php?_ROUTE=some/file.php"
end
HomeController->>Dotenv: "Load .env configuration"
Dotenv-->>HomeController: "Environment variables loaded"
alt Feature flag disabled
Note over HomeController: OPENEMR_ENABLE_FRONT_CONTROLLER != 1
HomeController->>SecurityValidator: "Check for .inc.php files"
SecurityValidator-->>HomeController: "Security check result"
alt .inc.php file detected
HomeController-->>User: "403 Access Denied"
else Valid PHP file
HomeController->>TargetFile: "Include target file directly"
TargetFile-->>User: "Response from target file"
end
else Feature flag enabled
HomeController->>EventDispatcher: "Dispatch front_controller.early event"
EventDispatcher-->>HomeController: "Early event processed"
HomeController->>Router: "new Router(__DIR__)"
Router-->>HomeController: "Router instance created"
HomeController->>Router: "determineSiteId()"
Router-->>HomeController: "Site ID determined"
HomeController->>Router: "extractRoute()"
Router-->>HomeController: "Route extracted from _ROUTE parameter"
HomeController->>Router: "handleTrailingSlash()"
alt Trailing slash redirect needed
Router-->>User: "301 Redirect"
else No redirect needed
Router-->>HomeController: "Continue processing"
end
HomeController->>Router: "isForbiddenPath()"
Router->>RouteConfig: "Check forbidden patterns"
RouteConfig-->>Router: "Pattern match result"
Router-->>HomeController: "Forbidden check result"
alt Forbidden path
HomeController->>SecurityValidator: "logSecurityEvent()"
HomeController-->>User: "404 Not Found"
else Path allowed
HomeController->>Router: "requiresAdmin()"
Router->>RouteConfig: "Check admin patterns"
RouteConfig-->>Router: "Admin check result"
Router-->>HomeController: "Admin requirement result"
alt Admin required
HomeController->>HomeController: "Set $_SERVER['REQUIRE_ADMIN'] = true"
end
HomeController->>SecurityValidator: "isIncludeFile(route)"
SecurityValidator-->>HomeController: "Include file check result"
alt .inc.php file
HomeController->>SecurityValidator: "logSecurityEvent()"
HomeController-->>User: "403 Access Denied"
else Valid file
HomeController->>Router: "resolveTargetFile()"
Router-->>HomeController: "Target file path"
alt Invalid or non-existent file
HomeController->>SecurityValidator: "logSecurityEvent()"
HomeController-->>User: "404 Not Found"
else Valid target file
HomeController->>HomeController: "register_shutdown_function()"
HomeController->>TargetFile: "require targetFile"
TargetFile-->>User: "Response from target file"
HomeController->>EventDispatcher: "Dispatch front_controller.late event"
end
end
end
end
22 files reviewed, 16 comments
| evaluate_overhead() { | ||
| local overhead="$1" | ||
|
|
||
| if (( $(bc -l <<< "$overhead < 5") )); then |
There was a problem hiding this comment.
logic: Missing dependency check for bc command used in arithmetic comparisons
| if (( $(bc -l <<< "$overhead < 5") )); then | |
| # Check if bc (basic calculator) is available for floating point arithmetic | |
| if ! command -v bc &> /dev/null; then | |
| { | |
| printf "%sERROR: bc (basic calculator) not available. Please install it.%s\n" "$RED" "$NC" | |
| echo " macOS: brew install bc" | |
| echo " Linux: sudo apt-get install bc" | |
| } | tee -a "$REPORT_FILE" | |
| exit 1 | |
| fi | |
| # Check if Apache Bench (ab) is available | |
| if ! command -v ab &> /dev/null; then |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/scripts/test_performance.sh
Line: 57:57
Comment:
**logic:** Missing dependency check for `bc` command used in arithmetic comparisons
```suggestion
# Check if bc (basic calculator) is available for floating point arithmetic
if ! command -v bc &> /dev/null; then
{
printf "%sERROR: bc (basic calculator) not available. Please install it.%s\n" "$RED" "$NC"
echo " macOS: brew install bc"
echo " Linux: sudo apt-get install bc"
} | tee -a "$REPORT_FILE"
exit 1
fi
# Check if Apache Bench (ab) is available
if ! command -v ab &> /dev/null; then
```
How can I resolve this? If you propose a fix, please make it concise.| # Calculate overhead | ||
| if [[ -n "$baseline_time" && -n "$fc_time" && "$baseline_time" != "0" ]]; then | ||
| # Validate numeric values | ||
| if ! [[ "$baseline_time" =~ ^[0-9]+\.?[0-9]*$ ]] || ! [[ "$fc_time" =~ ^[0-9]+\.?[0-9]*$ ]]; then |
There was a problem hiding this comment.
logic: Regex pattern doesn't handle negative numbers which could occur in performance measurements
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/scripts/test_performance.sh
Line: 228:228
Comment:
**logic:** Regex pattern doesn't handle negative numbers which could occur in performance measurements
How can I resolve this? If you propose a fix, please make it concise.| printf "Testing: Front controller responds when enabled... " | tee -a "$REPORT_FILE" | ||
|
|
||
| response=$(curl -s -o /dev/null -w "%{http_code}" "$BASE_URL/home.php?_ROUTE=index.php") | ||
|
|
There was a problem hiding this comment.
syntax: Environment variable name has typo - should be 'OPENEMR_ENABLE_FRONT_CONTROLLER' (single underscore)
| echo " ℹ Front controller may be disabled or error occurred (OPENEMR_ENABLE_FRONT_CONTROLLER not set)" | tee -a "$REPORT_FILE" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/scripts/test_security.sh
Line: 239:239
Comment:
**syntax:** Environment variable name has typo - should be 'OPENEMR_ENABLE_FRONT_CONTROLLER' (single underscore)
```suggestion
echo " ℹ Front controller may be disabled or error occurred (OPENEMR_ENABLE_FRONT_CONTROLLER not set)" | tee -a "$REPORT_FILE"
```
How can I resolve this? If you propose a fix, please make it concise.| { | ||
| return array_any( | ||
| $patterns, | ||
| fn(string $pattern): bool => (bool) preg_match($pattern, $route) |
There was a problem hiding this comment.
logic: array_any() is not a native PHP function. Use array_filter() or a manual loop instead.
| { | |
| return array_any( | |
| $patterns, | |
| fn(string $pattern): bool => (bool) preg_match($pattern, $route) | |
| foreach ($patterns as $pattern) { | |
| if (preg_match($pattern, $route)) { | |
| return true; | |
| } | |
| } | |
| return false; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/OpenCoreEMR/FrontController/RouteConfig.php
Line: 111:114
Comment:
**logic:** `array_any()` is not a native PHP function. Use `array_filter()` or a manual loop instead.
```suggestion
foreach ($patterns as $pattern) {
if (preg_match($pattern, $route)) {
return true;
}
}
return false;
```
How can I resolve this? If you propose a fix, please make it concise.| <?php | ||
|
|
||
| /** | ||
| * Security check for include/config files and sensitive directories. |
There was a problem hiding this comment.
logic: Comment doesn't match implementation - only blocks .inc.php files, not 'include/config files and sensitive directories' as originally intended
| * Security check for include/config files and sensitive directories. | |
| * Security check for .inc.php files. | |
| * | |
| * Auto-loaded before any PHP execution via .user.ini. | |
| * | |
| * @package OpenCoreEMR | |
| * @link http://www.open-emr.org | |
| * @author OpenCoreEMR, Inc. | |
| * @copyright Copyright (c) 2025 OpenCoreEMR, Inc. | |
| * @license GPLv3 |
Prompt To Fix With AI
This is a comment left during a code review.
Path: security-check.php
Line: 4:4
Comment:
**logic:** Comment doesn't match implementation - only blocks .inc.php files, not 'include/config files and sensitive directories' as originally intended
```suggestion
* Security check for .inc.php files.
*
* Auto-loaded before any PHP execution via .user.ini.
*
* @package OpenCoreEMR
* @link http://www.open-emr.org
* @author OpenCoreEMR, Inc.
* @copyright Copyright (c) 2025 OpenCoreEMR, Inc.
* @license GPLv3
```
How can I resolve this? If you propose a fix, please make it concise.| $targetFile = __DIR__ . '/' . ltrim($route, '/'); | ||
|
|
||
| // Block .inc and .inc.php files even when front controller is disabled | ||
| if (preg_match('/\.inc(?:\.php)?$/i', $route)) { |
There was a problem hiding this comment.
logic: Regex pattern only matches .inc.php but original change description mentions blocking .inc files too. Should use /\.inc(?:\.php)?$/i to match both patterns consistently
| if (preg_match('/\.inc(?:\.php)?$/i', $route)) { | |
| if (preg_match('/\.inc(?:\.php)?$/i', $route)) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: home.php
Line: 44:44
Comment:
**logic:** Regex pattern only matches `.inc.php` but original change description mentions blocking `.inc` files too. Should use `/\.inc(?:\.php)?$/i` to match both patterns consistently
```suggestion
if (preg_match('/\.inc(?:\.php)?$/i', $route)) {
```
How can I resolve this? If you propose a fix, please make it concise.| */ | ||
| public static function getDebugMessage(string $defaultMessage, string $debugMessage): string | ||
| { | ||
| return getenv('OPENEMR_FC_LOG_LEVEL') === 'debug' ? $debugMessage : $defaultMessage; |
There was a problem hiding this comment.
logic: Environment variable name has extra underscore - should be OPENEMR_FC_LOG_LEVEL not OPENEMR__FC_LOG_LEVEL to match the pattern used elsewhere
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/OpenCoreEMR/FrontController/SecurityValidator.php
Line: 51:51
Comment:
**logic:** Environment variable name has extra underscore - should be `OPENEMR_FC_LOG_LEVEL` not `OPENEMR__FC_LOG_LEVEL` to match the pattern used elsewhere
How can I resolve this? If you propose a fix, please make it concise.| $this->baseDir = realpath($baseDir); | ||
| // Ensure baseDir is valid | ||
| if ($this->baseDir === false || $this->baseDir === '') { |
There was a problem hiding this comment.
logic: Missing validation after realpath() call. If baseDir is false, the constructor should throw an exception immediately.
| $this->baseDir = realpath($baseDir); | |
| // Ensure baseDir is valid | |
| if ($this->baseDir === false || $this->baseDir === '') { | |
| $this->baseDir = realpath($baseDir); | |
| // Ensure baseDir is valid | |
| if ($this->baseDir === false || $this->baseDir === '') { | |
| throw new \RuntimeException('Invalid base directory provided to Router'); | |
| } | |
| $this->config = $config ?? new RouteConfig(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/OpenCoreEMR/FrontController/Router.php
Line: 29:31
Comment:
**logic:** Missing validation after realpath() call. If baseDir is false, the constructor should throw an exception immediately.
```suggestion
$this->baseDir = realpath($baseDir);
// Ensure baseDir is valid
if ($this->baseDir === false || $this->baseDir === '') {
throw new \RuntimeException('Invalid base directory provided to Router');
}
$this->config = $config ?? new RouteConfig();
```
How can I resolve this? If you propose a fix, please make it concise.| */ | ||
| public function handleTrailingSlash(): void | ||
| { | ||
| // Skip trailing slash redirect for .php files |
There was a problem hiding this comment.
logic: The diff shows query string preservation logic that's missing from the current implementation. This will break redirects with query parameters.
| */ | |
| public function handleTrailingSlash(): void | |
| { | |
| // Skip trailing slash redirect for .php files | |
| if ($this->route !== '' && !str_ends_with($this->route, '/') && pathinfo($this->route, PATHINFO_EXTENSION) !== 'php') { | |
| // Ensure proper relative redirect with query string preservation | |
| $queryString = $_SERVER['QUERY_STRING'] ?? ''; | |
| $redirectUrl = $this->route . '/' . ($queryString ? '?' . $queryString : ''); | |
| header('Location: ' . $redirectUrl, true, 301); | |
| exit; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/OpenCoreEMR/FrontController/Router.php
Line: 67:70
Comment:
**logic:** The diff shows query string preservation logic that's missing from the current implementation. This will break redirects with query parameters.
```suggestion
if ($this->route !== '' && !str_ends_with($this->route, '/') && pathinfo($this->route, PATHINFO_EXTENSION) !== 'php') {
// Ensure proper relative redirect with query string preservation
$queryString = $_SERVER['QUERY_STRING'] ?? '';
$redirectUrl = $this->route . '/' . ($queryString ? '?' . $queryString : '');
header('Location: ' . $redirectUrl, true, 301);
exit;
}
```
How can I resolve this? If you propose a fix, please make it concise.| * Resolve target file path | ||
| */ |
There was a problem hiding this comment.
logic: Path traversal prevention uses strpos() instead of str_starts_with() as shown in the diff. The str_starts_with() approach with DIRECTORY_SEPARATOR is more secure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/OpenCoreEMR/FrontController/Router.php
Line: 97:98
Comment:
**logic:** Path traversal prevention uses strpos() instead of str_starts_with() as shown in the diff. The str_starts_with() approach with DIRECTORY_SEPARATOR is more secure.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/scripts/test_performance.sh (2)
50-58: Guard against missingbc.The script exits with
command not foundwhenbcisn’t installed (common in minimal CI images). Add the same availability check you already do forab.Apply this diff:
# Check if Apache Bench (ab) is available if ! command -v ab &> /dev/null; then @@ fi +# Check if bc (basic calculator) is available +if ! command -v bc &> /dev/null; then + { + printf "%sERROR: bc (basic calculator) not available. Please install it.%s\n" "$RED" "$NC" + echo " macOS: brew install bc" + echo " Linux: sudo apt-get install bc" + } | tee -a "$REPORT_FILE" + exit 1 +fi +
102-135: Validateaboutput before parsing metrics.Blindly defaulting to
"0"masks realabfailures or format changes, producing misleading numbers in the report. Capture the exit code, ensure the expected sections exist, and only then parse.Apply this diff:
- # Run Apache Bench test - if ! ab_output=$(ab -n "$ITERATIONS" -c 10 -q "$url" 2>&1); then + # Run Apache Bench test + ab_output=$(ab -n "$ITERATIONS" -c 10 -q "$url" 2>&1) + ab_status=$? + + if [[ $ab_status -ne 0 ]]; then { printf "%sERROR: Apache Bench failed for %s%s\n" "$RED" "$url" "$NC" echo "Output: $ab_output" } | tee -a "$REPORT_FILE" return 1 fi - # Extract key metrics using awk with safe defaults - 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") + if ! printf '%s\n' "$ab_output" | grep -q "Requests per second"; then + { + printf "%sERROR: Apache Bench output missing expected metrics for %s%s\n" "$RED" "$url" "$NC" + echo "Output: $ab_output" + } | tee -a "$REPORT_FILE" + return 1 + fi + + # Extract key metrics using awk now that output is validated + requests_per_sec=$(printf '%s\n' "$ab_output" | awk '/Requests per second/ {print $4}') + time_per_request=$(printf '%s\n' "$ab_output" | awk '/Time per request.*mean\)/ {print $4; exit}') + failed_requests=$(printf '%s\n' "$ab_output" | awk '/Failed requests/ {print $3}') # Extract percentiles using awk with safe defaults - 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") + p50=$(printf '%s\n' "$ab_output" | awk '/50%/ {print $2}') + p95=$(printf '%s\n' "$ab_output" | awk '/95%/ {print $2}') + p99=$(printf '%s\n' "$ab_output" | awk '/99%/ {print $2}')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/OpenCoreEMR/FrontController/RouteConfig.php(1 hunks)tests/scripts/test_compatibility.sh(1 hunks)tests/scripts/test_performance.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/OpenCoreEMR/FrontController/RouteConfig.php (2)
src/OpenCoreEMR/FrontController/Router.php (2)
__construct(27-35)requiresAdmin(91-94)src/OpenCoreEMR/FrontController/RouteConfigInterface.php (7)
addForbiddenPath(27-27)addAdminPath(34-34)addDeprecatedPath(41-41)isForbidden(49-49)requiresAdmin(57-57)isDeprecated(65-65)loadFromFile(73-73)
tests/scripts/test_compatibility.sh (1)
tests/scripts/test_security.sh (1)
run_test(50-75)
🪛 GitHub Actions: ShellCheck
tests/scripts/test_performance.sh
[info] 22-22: ShellCheck SC2250: Prefer braces around variable references.
[info] 25-25: ShellCheck SC2292: Prefer [[ ]] over [ ] for tests in Bash/Ksh.
[info] 25-25: ShellCheck SC2250: Prefer braces around variable references.
tests/scripts/test_compatibility.sh
[info] 24-24: ShellCheck SC2292: Prefer [[ ]] over [ ] for tests in Bash/Ksh.
[info] 24-24: ShellCheck SC2250: Prefer braces around variable references.
🪛 GitHub Check: ShellCheck
tests/scripts/test_performance.sh
[warning] 45-45:
Prefer putting braces around variable references even when not strictly required.
[warning] 44-44:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 40-40:
Prefer putting braces around variable references even when not strictly required.
[warning] 25-25:
Prefer putting braces around variable references even when not strictly required.
[warning] 25-25:
Prefer [[ ]] over [ ] for tests in Bash/Ksh.
[warning] 22-22:
Prefer putting braces around variable references even when not strictly required.
tests/scripts/test_compatibility.sh
[warning] 41-41:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 32-32:
YELLOW appears unused. Verify use (or export if used externally).
[warning] 24-24:
Prefer putting braces around variable references even when not strictly required.
[warning] 24-24:
Prefer [[ ]] over [ ] for tests in Bash/Ksh.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: PHP 8.4 - apache - mysql 5.7
- GitHub Check: PHP 8.5 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 9.3
- GitHub Check: PHP 8.6 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 8.4
- GitHub Check: PHP 8.4 - apache - mysql 8.0
- GitHub Check: PHP 8.4 - nginx - mariadb 11.8
- GitHub Check: PHP 8.3 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 10.11
- GitHub Check: PHP 8.4 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 10.6
- GitHub Check: PHP 8.4 - apache - mariadb 11.4
- GitHub Check: PHP 8.3 - apache - mariadb 11.8
- GitHub Check: PHP 8.2 - apache - mariadb 11.8
- GitHub Check: PHP 8.3 - Isolated Tests
- GitHub Check: PHP 8.5 - Isolated Tests
- GitHub Check: PHP 8.2 - Isolated Tests
- GitHub Check: PHP 8.4 - Isolated Tests
- GitHub Check: phpstan (8.4)
| $config = require $realPath; | ||
|
|
||
| if (isset($config['forbidden']) && is_array($config['forbidden'])) { | ||
| foreach ($config['forbidden'] as $pattern) { | ||
| $this->addForbiddenPath($pattern); | ||
| } | ||
| } | ||
|
|
||
| if (isset($config['admin']) && is_array($config['admin'])) { | ||
| foreach ($config['admin'] as $pattern) { | ||
| $this->addAdminPath($pattern); | ||
| } | ||
| } | ||
|
|
||
| if (isset($config['deprecated']) && is_array($config['deprecated'])) { | ||
| foreach ($config['deprecated'] as $pattern) { | ||
| $this->addDeprecatedPath($pattern); | ||
| } | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Guard against non-array configs before accessing offsets
If the required config file returns null, bool, or any non-array value (syntax error, empty include, etc.), the subsequent isset($config['…']) calls will raise “Trying to access array offset on… ” warnings/fatal errors. Add an is_array guard and fail fast to keep loadFromFile reliable.
- $config = require $realPath;
+ $config = require $realPath;
+
+ if (!is_array($config)) {
+ return false;
+ }📝 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.
| $config = require $realPath; | |
| if (isset($config['forbidden']) && is_array($config['forbidden'])) { | |
| foreach ($config['forbidden'] as $pattern) { | |
| $this->addForbiddenPath($pattern); | |
| } | |
| } | |
| if (isset($config['admin']) && is_array($config['admin'])) { | |
| foreach ($config['admin'] as $pattern) { | |
| $this->addAdminPath($pattern); | |
| } | |
| } | |
| if (isset($config['deprecated']) && is_array($config['deprecated'])) { | |
| foreach ($config['deprecated'] as $pattern) { | |
| $this->addDeprecatedPath($pattern); | |
| } | |
| } | |
| return true; | |
| $config = require $realPath; | |
| if (!is_array($config)) { | |
| return false; | |
| } | |
| if (isset($config['forbidden']) && is_array($config['forbidden'])) { | |
| foreach ($config['forbidden'] as $pattern) { | |
| $this->addForbiddenPath($pattern); | |
| } | |
| } | |
| if (isset($config['admin']) && is_array($config['admin'])) { | |
| foreach ($config['admin'] as $pattern) { | |
| $this->addAdminPath($pattern); | |
| } | |
| } | |
| if (isset($config['deprecated']) && is_array($config['deprecated'])) { | |
| foreach ($config['deprecated'] as $pattern) { | |
| $this->addDeprecatedPath($pattern); | |
| } | |
| } | |
| return true; |
🤖 Prompt for AI Agents
In src/OpenCoreEMR/FrontController/RouteConfig.php around lines 184 to 204, the
code assumes the required file returns an array and directly accesses offsets
which can trigger "Trying to access array offset on..." warnings if the require
returns null/false/non-array; add an immediate is_array($config) guard after
$config = require $realPath and fail fast (return false or throw a descriptive
exception) when it's not an array so the subsequent isset/access code only runs
on valid array configs.
| if [[ "${http_code}" = "404" || "${http_code}" = "403" ]]; then | ||
| printf "%sFAIL%s (HTTP %s)\n" "${RED}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" | ||
| FAILED_TESTS=$((FAILED_TESTS + 1)) | ||
| echo " ✗ POST requests are being blocked" | tee -a "${REPORT_FILE}" | ||
| else | ||
| printf "%sPASS%s (HTTP %s)\n" "${GREEN}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" | ||
| PASSED_TESTS=$((PASSED_TESTS + 1)) | ||
| echo " ✓ POST requests are processed correctly" | tee -a "${REPORT_FILE}" | ||
| fi |
There was a problem hiding this comment.
Fail POST compatibility tests on 5xx/connection errors.
Any 5xx (or 000 curl error) currently lands in the “PASS” branch, so a broken login POST can still count as success. Tighten the check so unexpected 3‑digit codes fail the test.
Apply this diff:
-if [[ "${http_code}" = "404" || "${http_code}" = "403" ]]; then
+if [[ "${http_code}" = "404" || "${http_code}" = "403" || "${http_code}" = "000" || "${http_code}" =~ ^5 ]]; then
printf "%sFAIL%s (HTTP %s)\n" "${RED}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}"
FAILED_TESTS=$((FAILED_TESTS + 1))
echo " ✗ POST requests are being blocked" | tee -a "${REPORT_FILE}"
else📝 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.
| if [[ "${http_code}" = "404" || "${http_code}" = "403" ]]; then | |
| printf "%sFAIL%s (HTTP %s)\n" "${RED}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" | |
| FAILED_TESTS=$((FAILED_TESTS + 1)) | |
| echo " ✗ POST requests are being blocked" | tee -a "${REPORT_FILE}" | |
| else | |
| printf "%sPASS%s (HTTP %s)\n" "${GREEN}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" | |
| PASSED_TESTS=$((PASSED_TESTS + 1)) | |
| echo " ✓ POST requests are processed correctly" | tee -a "${REPORT_FILE}" | |
| fi | |
| if [[ "${http_code}" = "404" || "${http_code}" = "403" || "${http_code}" = "000" || "${http_code}" =~ ^5 ]]; then | |
| printf "%sFAIL%s (HTTP %s)\n" "${RED}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" | |
| FAILED_TESTS=$((FAILED_TESTS + 1)) | |
| echo " ✗ POST requests are being blocked" | tee -a "${REPORT_FILE}" | |
| else | |
| printf "%sPASS%s (HTTP %s)\n" "${GREEN}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" | |
| PASSED_TESTS=$((PASSED_TESTS + 1)) | |
| echo " ✓ POST requests are processed correctly" | tee -a "${REPORT_FILE}" | |
| fi |
🤖 Prompt for AI Agents
In tests/scripts/test_compatibility.sh around lines 232 to 240, the current
logic treats any HTTP response other than 403/404 as a PASS (so 5xx or curl
"000" succeed); change the branching so only 2xx responses are considered PASS
and everything else (including 5xx and "000") is treated as FAIL. Concretely,
replace the condition with a check that http_code starts with "2" (e.g. using
shell pattern or regex) to mark PASS and otherwise increment FAILED_TESTS and
print the FAIL messages; ensure the PASS branch increments PASSED_TESTS and
prints the success message as before.
coding-sunshine
force-pushed
the
8964-global-front-controller
branch
from
08df75e to
90cd613
Compare
There was a problem hiding this comment.
Greptile Overview
Summary
**Note: This review covers only the changes made since the last review, not the entire PR.**This incremental update addresses specific reliability and robustness issues in the OpenEMR front controller testing infrastructure. The changes focus on improving the compatibility and performance test scripts to work correctly across different execution environments, particularly non-interactive ones like CI/CD pipelines.
The key improvements include:
-
Enhanced Environment Compatibility: Both
test_compatibility.shandtest_performance.shnow include fallback logic for color output handling. The scripts check fortputavailability and theTERMenvironment variable before attempting to set color codes, preventing script failures in headless or automated environments. -
Critical Bug Fix in Route Configuration: The
RouteConfig.phpfile addresses a fatal error where the non-existentarray_any()function was replaced with a proper manualforeachloop implementation. Additionally, method names were improved from "Pattern" to "Path" for better semantic clarity (addForbiddenPattern→addForbiddenPath, etc.).
These changes ensure the front controller testing suite can run reliably in automated testing environments while maintaining colored output when available in interactive terminals. The route configuration fix prevents runtime fatal errors and provides clearer method naming that better reflects the functionality of adding path-based routing rules.
Important Files Changed
Changed Files
| Filename | Score | Overview |
|---|---|---|
| tests/scripts/test_compatibility.sh | 4/5 | Added fallback color handling for non-interactive environments to prevent script failures in CI/CD |
| src/OpenCoreEMR/FrontController/RouteConfig.php | 4/5 | Fixed critical bug replacing non-existent array_any() function and renamed methods for clarity |
| tests/scripts/test_performance.sh | 4/5 | Added color output fallbacks for headless environments to improve script portability |
Confidence score: 4/5
- This PR addresses important reliability issues in the testing infrastructure with minimal risk of breaking existing functionality
- Score reflects well-targeted fixes for specific environmental compatibility issues and a critical bug fix
- All changes are focused on testing scripts and internal routing logic that don't affect end-user functionality
3 files reviewed, no comments
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
tests/scripts/test_performance.sh (3)
232-252: Regex doesn't handle negative numbers in performance measurements.Line 234's regex
^[0-9]+\.?[0-9]*$rejects negative values, which could occur in performance metrics (e.g., negative overhead if front controller is faster than baseline).Update the regex to handle negative numbers:
- if ! [[ "$baseline_time" =~ ^[0-9]+\.?[0-9]*$ ]] || ! [[ "$fc_time" =~ ^[0-9]+\.?[0-9]*$ ]]; then + if ! [[ "$baseline_time" =~ ^-?[0-9]+\.?[0-9]*$ ]] || ! [[ "$fc_time" =~ ^-?[0-9]+\.?[0-9]*$ ]]; thenAlso update line 307 for consistency:
-if [[ -n "$overhead" ]] && [[ "$overhead" =~ ^[0-9]+\.?[0-9]*$ ]]; then +if [[ -n "$overhead" ]] && [[ "$overhead" =~ ^-?[0-9]+\.?[0-9]*$ ]]; thenBased on past review comments.
48-56: Add dependency check forbccommand.The script uses
bcfor floating-point arithmetic comparisons (lines 63, 68, 73, 239, 250, 308) but doesn't verify its availability. Missingbcwill cause runtime failures.Apply this diff to add the check before the
abcheck:+# Check if bc (basic calculator) is available for floating point arithmetic +if ! command -v bc &> /dev/null; then + { + printf "%sERROR: bc (basic calculator) not available. Please install it.%s\n" "$RED" "$NC" + echo " macOS: brew install bc" + echo " Linux: sudo apt-get install bc" + } | tee -a "$REPORT_FILE" + exit 1 +fi + # Check if Apache Bench (ab) is availableBased on past review comments.
100-133: Validateabexit code and output format before parsing metrics.Current implementation uses
|| echo "0"fallbacks that maskabfailures and parsing errors, potentially reporting misleading zero values for legitimate errors (network failures, ab crashes, malformed output).Replace the metric extraction logic with validated parsing:
- # Run Apache Bench test - if ! ab_output=$(ab -n "$ITERATIONS" -c 10 -q "$url" 2>&1); then + # Run ab and capture output + exit code + set +e + ab_output=$(ab -n "$ITERATIONS" -c 10 -q "$url" 2>&1) + ab_exit_code=$? + set -e + + # Verify ab succeeded and output contains expected markers + if [ "$ab_exit_code" -ne 0 ] || ! printf '%s\n' "$ab_output" | grep -q "Requests per second"; then { - printf "%sERROR: Apache Bench failed for %s%s\n" "$RED" "$url" "$NC" - echo "Output: $ab_output" + printf "%sERROR: ab failed (exit code %s) or unexpected output format for %s%s\n" "$RED" "$ab_exit_code" "$url" "$NC" } | tee -a "$REPORT_FILE" return 1 fi - # Extract key metrics using awk with safe defaults - 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 key metrics using printf + pipe (POSIX-compatible, no here-strings) + requests_per_sec=$(printf '%s\n' "$ab_output" | awk '/Requests per second/ {print $4}') + time_per_request=$(printf '%s\n' "$ab_output" | awk '/Time per request.*mean\)/ {print $4; exit}') + failed_requests=$(printf '%s\n' "$ab_output" | awk '/Failed requests/ {print $3}') - # Extract percentiles using awk with safe defaults - 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") + # Extract percentiles + p50=$(printf '%s\n' "$ab_output" | awk '/50%/ {print $2}') + p95=$(printf '%s\n' "$ab_output" | awk '/95%/ {print $2}') + p99=$(printf '%s\n' "$ab_output" | awk '/99%/ {print $2}')This ensures:
abexecution failures are detected- Missing sections in output trigger warnings
- Legitimate zero values (e.g., no failed requests) are preserved
- POSIX-compatible (no bash-specific here-strings)
Based on past review comments.
tests/Tests/FrontController/CompatibilityTest.php (4)
20-22: Remove unused import.
RequestExceptionis imported but never used in the test suite.use GuzzleHttp\Client; -use GuzzleHttp\Exception\RequestException; use PHPUnit\Framework\TestCase;Based on past review comments.
193-197: UseassertContainscorrectly for arrays.In PHPUnit 9+,
assertContainsis for arrays. For string searches, useassertStringContainsString. Since you're checking if a value is in an array of status codes,assertContainsis correct here, but ensure you're using a compatible PHPUnit version.Verify your PHPUnit version supports this assertion:
#!/bin/bash # Check PHPUnit version in composer.json and composer.lock echo "=== composer.json ===" if [ -f composer.json ]; then jq -r '.require["phpunit/phpunit"] // .["require-dev"]["phpunit/phpunit"] // "not found"' composer.json fi echo -e "\n=== composer.lock (installed version) ===" if [ -f composer.lock ]; then jq -r '.packages[] | select(.name == "phpunit/phpunit") | .version' composer.lock fi echo -e "\n=== Recommendation ===" echo "PHPUnit 9+ is recommended (PHPUnit 2.x is obsolete)." echo "If using PHPUnit 9+, assertContains for arrays is correct." echo "If seeing deprecation warnings, update PHPUnit or use assertContains() explicitly."Based on past review comments and learnings about PHPUnit versions.
254-258: Same issue: verify assertContains usage.See comment on lines 193-197. Apply the same verification.
Based on past review comments.
273-277: Same issue: verify assertContains usage.See comment on lines 193-197. Apply the same verification.
Based on past review comments.
tests/scripts/test_compatibility.sh (1)
223-239: POST compatibility test has two issues.
Inconsistent test counting: Manually incrementing
TOTAL_TESTSinstead of using therun_testfunction breaks the established pattern (lines 85-90, 91-95, etc.).Weak error detection: The test passes for 5xx server errors and curl connection failures (000), masking broken POST handling.
Refactor to use
run_testor tighten the logic:Option 1 (preferred): Use run_test function
-TOTAL_TESTS=$((TOTAL_TESTS + 1)) -printf "Testing: POST request handling... " | tee -a "${REPORT_FILE}" - -http_code=$(curl -s -o /dev/null -w "%{http_code}" -X POST \ - -d "authUser=test&authPass=test" \ - "${BASE_URL}/interface/login/login.php") - -if [[ "${http_code}" = "404" || "${http_code}" = "403" ]]; then - printf "%sFAIL%s (HTTP %s)\n" "${RED}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" - FAILED_TESTS=$((FAILED_TESTS + 1)) - echo " ✗ POST requests are being blocked" | tee -a "${REPORT_FILE}" -else - printf "%sPASS%s (HTTP %s)\n" "${GREEN}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" - PASSED_TESTS=$((PASSED_TESTS + 1)) - echo " ✓ POST requests are processed correctly" | tee -a "${REPORT_FILE}" -fi -echo | tee -a "${REPORT_FILE}" +# Note: run_test doesn't support POST, so we keep manual test but fix the logic +TOTAL_TESTS=$((TOTAL_TESTS + 1)) +printf "Testing: POST request handling... " | tee -a "${REPORT_FILE}" + +http_code=$(curl -s -o /dev/null -w "%{http_code}" -X POST \ + -d "authUser=test&authPass=test" \ + "${BASE_URL}/interface/login/login.php") + +# Only 2xx responses are considered success +if [[ "${http_code}" =~ ^2 ]]; then + printf "%sPASS%s (HTTP %s)\n" "${GREEN}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" + PASSED_TESTS=$((PASSED_TESTS + 1)) + echo " ✓ POST requests are processed correctly" | tee -a "${REPORT_FILE}" +else + printf "%sFAIL%s (HTTP %s)\n" "${RED}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" + FAILED_TESTS=$((FAILED_TESTS + 1)) + echo " ✗ POST requests failed or blocked (expected 2xx)" | tee -a "${REPORT_FILE}" +fi +echo | tee -a "${REPORT_FILE}"Based on past review comments.
🧹 Nitpick comments (4)
index.php (1)
8-11: LGTM! Security headers improve defense-in-depth.The addition of
X-Content-Type-Options,X-Frame-Options, andX-XSS-Protectionheaders early in the request lifecycle strengthens security posture against MIME-sniffing, clickjacking, and XSS attacks.Consider adding a
Content-Security-Policyheader for additional protection:header('X-Content-Type-Options: nosniff'); header('X-Frame-Options: SAMEORIGIN'); header('X-XSS-Protection: 1; mode=block'); +header("Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'");Note: Adjust CSP directives based on your actual resource requirements. Start with a restrictive policy and relax as needed.
tests/scripts/test_performance.sh (1)
16-56: Consider adding braces around variable references.ShellCheck flags multiple instances where variable references could have braces for consistency and safety (lines 22, 38, 42-44, 46, 51, 54).
While not strictly required for these cases, adding braces improves consistency:
-REPORT_FILE="$REPORT_DIR/performance-test-report-$(date +%Y%m%d-%H%M%S).txt" +REPORT_FILE="${REPORT_DIR}/performance-test-report-$(date +%Y%m%d-%H%M%S).txt"Apply similar changes to other flagged lines if you prefer consistent style throughout the script.
tests/scripts/test_compatibility.sh (1)
37-42: Consider separating command invocation to avoid masking return value.ShellCheck SC2312 warns that the
datecommand in the pipeline could fail silently. While unlikely, separating the command improves error detection.Apply this diff:
+date_output=$(date) { echo "Compatibility Test Report" - echo "Date: $(date)" + echo "Date: ${date_output}" echo "URL: ${BASE_URL}" echo } | tee "${REPORT_FILE}"Or add
|| trueif you want to explicitly ignore date failures:{ echo "Compatibility Test Report" - echo "Date: $(date)" + echo "Date: $(date || true)" echo "URL: ${BASE_URL}" echo } | tee "${REPORT_FILE}"home.php (1)
66-82: Consider extracting pass-through logic into a function.The pass-through block (lines 35-82) is a substantial chunk of code that could benefit from extraction into a named function or separate file for improved maintainability and testability.
Example refactor:
+/** + * Execute pass-through mode (feature flag disabled) + * @return never + */ +function executePassThrough(): void { + // Extract target route from query parameter + $route = $_GET['_ROUTE'] ?? 'index.php'; + + // Security: Prevent directory traversal and null byte injection + if (strpos($route, '..') !== false || strpos($route, "\0") !== false) { + http_response_code(404); + exit('Not Found'); + } + + // ... rest of pass-through logic ... + + // Include target file - it now executes with correct context + require $targetFile; + exit; +} + // Check feature flag - when disabled, provide transparent pass-through if ((getenv('OPENEMR_ENABLE_FRONT_CONTROLLER') ?: '0') !== '1') { - // Extract target route from query parameter - $route = $_GET['_ROUTE'] ?? 'index.php'; - // ... rest of pass-through logic ... - require $targetFile; - exit; + executePassThrough(); }This would:
- Improve code organization
- Make the pass-through logic easier to test
- Reduce cognitive load in the main flow
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
home.php(1 hunks)index.php(1 hunks)tests/Tests/FrontController/CompatibilityTest.php(1 hunks)tests/Tests/FrontController/SecurityTest.php(1 hunks)tests/scripts/test_compatibility.sh(1 hunks)tests/scripts/test_performance.sh(1 hunks)tests/scripts/test_security.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Tests/FrontController/SecurityTest.php
- tests/scripts/test_security.sh
🧰 Additional context used
🧬 Code graph analysis (3)
tests/scripts/test_compatibility.sh (1)
tests/scripts/test_security.sh (1)
run_test(48-73)
tests/Tests/FrontController/CompatibilityTest.php (1)
tests/Tests/FrontController/SecurityTest.php (1)
setUpBeforeClass(30-39)
home.php (3)
src/OpenCoreEMR/FrontController/Router.php (8)
Router(20-136)determineSiteId(54-61)extractRoute(40-49)handleTrailingSlash(68-78)isForbiddenPath(83-86)requiresAdmin(91-94)resolveTargetFile(99-119)getSiteId(132-135)src/OpenCoreEMR/FrontController/SecurityValidator.php (4)
SecurityValidator(20-53)logSecurityEvent(41-44)isIncludeFile(33-36)getDebugMessage(49-52)src/Events/FrontController/FrontControllerEvent.php (2)
FrontControllerEvent(23-136)getSiteId(86-89)
🪛 GitHub Actions: ShellCheck
tests/scripts/test_compatibility.sh
[info] 39-39: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
tests/scripts/test_performance.sh
[info] 22-22: ShellCheck SC2250: Prefer putting braces around variable references even when not strictly required.
[info] 38-38: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
🪛 GitHub Check: ShellCheck
tests/scripts/test_compatibility.sh
[warning] 39-39:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
tests/scripts/test_performance.sh
[warning] 54-54:
Prefer putting braces around variable references even when not strictly required.
[warning] 51-51:
Prefer putting braces around variable references even when not strictly required.
[warning] 51-51:
Prefer putting braces around variable references even when not strictly required.
[warning] 46-46:
Prefer putting braces around variable references even when not strictly required.
[warning] 44-44:
Prefer putting braces around variable references even when not strictly required.
[warning] 43-43:
Prefer putting braces around variable references even when not strictly required.
[warning] 42-42:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 38-38:
Prefer putting braces around variable references even when not strictly required.
[warning] 22-22:
Prefer putting braces around variable references even when not strictly required.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: PHP 8.4 - nginx - mariadb 11.8
- GitHub Check: PHP 8.5 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 5.7
- GitHub Check: PHP 8.4 - apache - mariadb 11.8
- GitHub Check: PHP 8.6 - nginx - mariadb 11.8
- GitHub Check: PHP 8.3 - nginx - mariadb 11.8
- GitHub Check: PHP 8.2 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 8.4
- GitHub Check: PHP 8.4 - apache - mysql 9.3
- GitHub Check: PHP 8.4 - apache - mysql 8.0
- GitHub Check: PHP 8.4 - apache - mariadb 11.4
- GitHub Check: PHP 8.4 - apache - mariadb 10.6
- GitHub Check: PHP 8.3 - apache - mariadb 11.8
- GitHub Check: PHP 8.2 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 10.11
- GitHub Check: Rector PHP Analysis (8.4)
- GitHub Check: PHP 8.3 - Isolated Tests
- GitHub Check: PHP 8.5 - Isolated Tests
- GitHub Check: PHP 8.2 - Isolated Tests
- GitHub Check: PHP 8.4 - Isolated Tests
🔇 Additional comments (8)
home.php (6)
25-28: LGTM! Security headers are set consistently.The security headers match those added to index.php, providing consistent protection across entry points.
35-82: Pass-through mode provides good fallback behavior.The transparent pass-through when the feature flag is disabled ensures backward compatibility while the front controller is being adopted. The security checks (path traversal, .inc blocking, file validation) provide essential protection even in pass-through mode.
84-90: Event dispatching enables extensibility.The early event hook allows modules to intercept requests before routing, which is useful for authentication, rate limiting, or custom logging. Good integration with the existing kernel event system.
92-133: Router integration is clean and secure.The router properly:
- Determines site ID for multisite support
- Extracts and validates routes
- Handles trailing slashes
- Checks forbidden/admin paths
- Blocks .inc files
- Resolves target files with path traversal protection
135-166: Error handling and late events provide robust lifecycle management.The shutdown function ensures late events fire even if the target exits early, and the try/catch block properly logs and re-throws exceptions while still dispatching events. This provides good observability for monitoring and debugging.
30-33: Dotenv configuration verified: .env.example includes OPENEMR_ENABLE_FRONT_CONTROLLER and front-controller docs reference the flag; safeLoad()’s silent fallback is acceptable.tests/Tests/FrontController/CompatibilityTest.php (2)
29-37: Test setup looks good.The
setUpBeforeClassmethod properly initializes the Guzzle client with sensible defaults (disable errors, allow redirects). The base URL is configurable via environment variable with a sensible default.
39-300: Comprehensive test coverage validates compatibility.The test suite covers all critical paths:
- Core entry points (index, login, setup)
- Existing front controllers (REST API, portal, OAuth2)
- Multisite support
- Core workflows (patient file, calendar, billing)
- Static assets
- Custom modules
- POST requests
- File uploads
- Query string preservation
- Report generation
The assertions appropriately handle multiple valid response codes (200, 302, 401, etc.) and validate content types for static assets.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/scripts/test_performance.sh (3)
58-83: Check forbcbefore using it.
bcis required inevaluate_overhead, but on many minimal images it isn’t installed, so the script dies the first time it hits those comparisons. Please add a dependency check (just like you do forab) so the script fails fast with a friendly message.-# Check if Apache Bench (ab) is available -if ! command -v ab &> /dev/null; then +# Check if bc (basic calculator) is available +if ! command -v bc &> /dev/null; then + { + printf "%sERROR: bc (basic calculator) not available. Please install it.%s\n" "${RED}" "${NC}" + echo " macOS: brew install bc" + echo " Linux: sudo apt-get install bc" + } | tee -a "${REPORT_FILE}" + exit 1 +fi + +# Check if Apache Bench (ab) is available +if ! command -v ab &> /dev/null; then
109-118: Validateaboutput before parsing metrics.This still pipes raw output through
awk <<< … || echo "0", so any parsing failure silently becomes zero and hides real issues (exactly the scenario flagged in the earlier review). Please gate the parsing onabexit code and expected markers, then parse viaprintf '%s\n' "$ab_output" | awk …so legitimate zeros stay intact and malformed output trips an error.
307-323: Allow negative overhead values.When the front controller beats the baseline,
overheadbecomes negative (e.g.,-3.25), but the regex^[0-9]+\.?[0-9]*$rejects it, so you skip recommendations entirely. Let the pattern accept an optional leading minus (and perhaps more flexible decimal formats) before testing thresholds.-if [[ -n "${overhead}" ]] && [[ "${overhead}" =~ ^[0-9]+\.?[0-9]*$ ]]; then +if [[ -n "${overhead}" ]] && [[ "${overhead}" =~ ^-?[0-9]+(\.[0-9]+)?$ ]]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/scripts/test_performance.sh(1 hunks)tests/scripts/test_security.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/scripts/test_security.sh (1)
tests/scripts/test_compatibility.sh (1)
run_test(50-75)
🪛 GitHub Actions: ShellCheck
tests/scripts/test_performance.sh
[info] 42-42: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 63-63: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 68-68: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 73-73: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 143-143: ShellCheck SC2311: Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e before it or enable inherit_errexit.
[info] 160-160: ShellCheck SC2311: Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e before it or enable inherit_errexit.
[info] 177-177: ShellCheck SC2311: Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e before it or enable inherit_errexit.
[warning] 183-183: ShellCheck SC2034: login_time appears unused. Verify use (or export if used externally).
[info] 195-195: ShellCheck SC2311: Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e before it or enable inherit_errexit.
[info] 216-216: ShellCheck SC2311: Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e before it or enable inherit_errexit.
[info] 262-262: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 279-279: ShellCheck SC2009: Consider using pgrep instead of grepping ps output.
[info] 279-279: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 287-287: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 289-289: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 289-289: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 292-292: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 292-292: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 308-308: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
🪛 GitHub Check: ShellCheck
tests/scripts/test_performance.sh
[warning] 195-195:
Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e; before it or enable inherit_errexit.
[warning] 183-183:
login_time appears unused. Verify use (or export if used externally).
[warning] 177-177:
Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e; before it or enable inherit_errexit.
[warning] 160-160:
Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e; before it or enable inherit_errexit.
[warning] 143-143:
Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e; before it or enable inherit_errexit.
[warning] 73-73:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 68-68:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 63-63:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 42-42:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: PHP 8.4 - nginx - mariadb 11.8
- GitHub Check: PHP 8.2 - nginx - mariadb 11.8
- GitHub Check: PHP 8.6 - nginx - mariadb 11.8
- GitHub Check: PHP 8.3 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 9.3
- GitHub Check: PHP 8.4 - apache - mariadb 11.8
- GitHub Check: PHP 8.5 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 8.0
- GitHub Check: PHP 8.4 - apache - mariadb 11.4
- GitHub Check: PHP 8.4 - apache - mysql 5.7
- GitHub Check: PHP 8.4 - apache - mariadb 10.6
- GitHub Check: PHP 8.2 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 8.4
- GitHub Check: PHP 8.3 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 10.11
- GitHub Check: PHP 8.2 - Isolated Tests
- GitHub Check: PHP 8.5 - Isolated Tests
- GitHub Check: PHP 8.4 - Isolated Tests
- GitHub Check: PHP 8.3 - Isolated Tests
| baseline=$(run_performance_test \ | ||
| "Direct index.php access" \ | ||
| "${BASE_URL}/index.php" \ | ||
| "Direct file access without front controller") | ||
|
|
||
| baseline_rps=$(cut -d'|' -f1 <<< "${baseline}") | ||
| baseline_time=$(cut -d'|' -f2 <<< "${baseline}") | ||
| baseline_p95=$(cut -d'|' -f3 <<< "${baseline}") | ||
|
|
||
| # Test 2: Front Controller Routing | ||
| { | ||
| echo "========================================" | ||
| echo "FRONT CONTROLLER: Routed Access" | ||
| echo "========================================" | ||
| echo | ||
| } | tee -a "${REPORT_FILE}" | ||
|
|
||
| fc_result=$(run_performance_test \ | ||
| "Front controller routing" \ | ||
| "${BASE_URL}/home.php?_ROUTE=index.php" \ | ||
| "Access through front controller") | ||
|
|
||
| fc_rps=$(cut -d'|' -f1 <<< "${fc_result}") | ||
| fc_time=$(cut -d'|' -f2 <<< "${fc_result}") | ||
| fc_p95=$(cut -d'|' -f3 <<< "${fc_result}") | ||
|
|
There was a problem hiding this comment.
Handle failed performance runs before using their metrics.
run_performance_test returns 1 when ab blows up, yet these command substitutions keep the script running with unset metrics (because set -e is disabled inside $(...)). The analysis phase then divides empty strings, so the report shows garbage instead of failing fast. Guard each assignment with an if ! …; then exit 1; fi (or similar) so a broken test aborts before producing bogus numbers.
-if ! ab_output=$(ab -n "${ITERATIONS}" -c 10 -q "${url}" 2>&1); then
+if ! ab_output=$(ab -n "${ITERATIONS}" -c 10 -q "${url}" 2>&1); then
@@
-baseline=$(run_performance_test \
- "Direct index.php access" \
- "${BASE_URL}/index.php" \
- "Direct file access without front controller")
+if ! baseline=$(run_performance_test \
+ "Direct index.php access" \
+ "${BASE_URL}/index.php" \
+ "Direct file access without front controller"); then
+ exit 1
+fi
@@
-fc_result=$(run_performance_test \
- "Front controller routing" \
- "${BASE_URL}/home.php?_ROUTE=index.php" \
- "Access through front controller")
+if ! fc_result=$(run_performance_test \
+ "Front controller routing" \
+ "${BASE_URL}/home.php?_ROUTE=index.php" \
+ "Access through front controller"); then
+ exit 1
+fi(Apply the same pattern to the other run_performance_test assignments.)
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: ShellCheck
[info] 143-143: ShellCheck SC2311: Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e before it or enable inherit_errexit.
[info] 160-160: ShellCheck SC2311: Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e before it or enable inherit_errexit.
🪛 GitHub Check: ShellCheck
[warning] 160-160:
Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e; before it or enable inherit_errexit.
[warning] 143-143:
Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e; before it or enable inherit_errexit.
🤖 Prompt for AI Agents
In tests/scripts/test_performance.sh around lines 143 to 168, the script assigns
results from run_performance_test into variables but doesn't check for failure,
so a failed run returns non-zero/empty output and later metric parsing produces
garbage; update each assignment to first capture the command exit status (e.g.,
run the command and test its exit code or use if ! result=$(run_performance_test
...); then echo "Performance test failed" >&2; exit 1; fi) and only parse
metrics when the run succeeded, and apply the same guard pattern to all other
run_performance_test calls in the file.
| if command -v tput >/dev/null 2>&1 && [[ -n "${TERM}" ]]; then | ||
| RED=$(tput setaf 1) | ||
| GREEN=$(tput setaf 2) | ||
| NC=$(tput sgr0) # No Color | ||
| else | ||
| RED="" | ||
| GREEN="" | ||
| NC="" | ||
| fi |
There was a problem hiding this comment.
Prevent tput from aborting under set -e.
In CI we often get TERM=dumb (or a terminfo without color support). In that case each tput setaf … returns exit code 1, and because the script runs with set -e, the entire test suite exits before executing any checks. Please swallow those failures and fall back to plain text.
-if command -v tput >/dev/null 2>&1 && [[ -n "${TERM}" ]]; then
- RED=$(tput setaf 1)
- GREEN=$(tput setaf 2)
- NC=$(tput sgr0) # No Color
-else
- RED=""
- GREEN=""
- NC=""
-fi
+if command -v tput >/dev/null 2>&1 && [[ -n "${TERM:-}" ]]; then
+ RED=$(tput setaf 1 2>/dev/null || true)
+ GREEN=$(tput setaf 2 2>/dev/null || true)
+ NC=$(tput sgr0 2>/dev/null || true)
+ if [[ -z "${RED}" || -z "${GREEN}" || -z "${NC}" ]]; then
+ RED=""; GREEN=""; NC=""
+ fi
+else
+ RED=""; GREEN=""; NC=""
+fi📝 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.
| if command -v tput >/dev/null 2>&1 && [[ -n "${TERM}" ]]; then | |
| RED=$(tput setaf 1) | |
| GREEN=$(tput setaf 2) | |
| NC=$(tput sgr0) # No Color | |
| else | |
| RED="" | |
| GREEN="" | |
| NC="" | |
| fi | |
| if command -v tput >/dev/null 2>&1 && [[ -n "${TERM:-}" ]]; then | |
| RED=$(tput setaf 1 2>/dev/null || true) | |
| GREEN=$(tput setaf 2 2>/dev/null || true) | |
| NC=$(tput sgr0 2>/dev/null || true) | |
| if [[ -z "${RED}" || -z "${GREEN}" || -z "${NC}" ]]; then | |
| RED=""; GREEN=""; NC="" | |
| fi | |
| else | |
| RED=""; GREEN=""; NC="" | |
| fi |
🤖 Prompt for AI Agents
In tests/scripts/test_security.sh around lines 23 to 31, tput calls can return
non-zero under CI (TERM=dumb) and cause the script to abort with set -e; modify
the assignments to tolerate failures by redirecting tput stderr and ensuring
non-zero exits are ignored (e.g., run each tput with stderr redirected and use
"|| true" or capture output into variables only if tput succeeds), then fall
back to empty strings when tput fails so colors are optional and the script
never exits due to tput errors.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
tests/scripts/test_performance.sh (4)
65-92: Add a guard for thebcdependency before calling it
bcis invoked here without first confirming it exists, so the script will die with “command not found” on environments lackingbc(common on fresh macOS/Linux installs). Please fail fast with a dependency check the same way you already do forab.+# Check if bc (basic calculator) is available +if ! command -v bc >/dev/null 2>&1; then + { + printf "%sERROR: bc (basic calculator) not available. Please install it.%s\n" "${RED}" "${NC}" + echo " macOS: brew install bc" + echo " Linux: sudo apt-get install bc" + } | tee -a "${REPORT_FILE}" + exit 1 +fi +
118-126: Validateaboutput before parsing metricsThese here-strings plus
|| echo "0"still mask malformedaboutput. Ifabprints an error while exiting 0, every metric silently becomes0, so your report claims success instead of failing fast. Capture the exit code and assert the expected markers exist before you parse.- 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 with safe defaults - 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") + ab_exit_code=0 + requests_marker=$(printf '%s\n' "${ab_output}" | grep -c "Requests per second" || ab_exit_code=$?) + if [[ "${ab_exit_code}" -ne 0 || "${requests_marker}" -eq 0 ]]; then + { + printf "%sWARNING: ab output missing expected markers for %s%s\n" "${RED}" "${url}" "${NC}" + } | tee -a "${REPORT_FILE}" + return 1 + fi + + requests_per_sec=$(printf '%s\n' "${ab_output}" | awk '/Requests per second/ {print $4}') + time_per_request=$(printf '%s\n' "${ab_output}" | awk '/Time per request.*mean\)/ {print $4; exit}') + failed_requests=$(printf '%s\n' "${ab_output}" | awk '/Failed requests/ {print $3}') + + p50=$(printf '%s\n' "${ab_output}" | awk '/50%/ {print $2}') + p95=$(printf '%s\n' "${ab_output}" | awk '/95%/ {print $2}') + p99=$(printf '%s\n' "${ab_output}" | awk '/99%/ {print $2}')
152-231: Stop on failedrun_performance_testexecutions
run_performance_testreturns1whenabfails, but because these calls sit inside command substitutions,set -eis suppressed and the script keeps going with empty metrics. Later arithmetic then manipulates blank strings. Wrap each call in anif ! …; then exit 1; figuard so a busted run aborts instead of producing nonsense.-if ! ab_output=$(ab -n "${ITERATIONS}" -c 10 -q "${url}" 2>&1); then +if ! ab_output=$(ab -n "${ITERATIONS}" -c 10 -q "${url}" 2>&1); then @@ -baseline=$(run_performance_test \ - "Direct index.php access" \ - "${BASE_URL}/index.php" \ - "Direct file access without front controller") +if ! baseline=$(run_performance_test \ + "Direct index.php access" \ + "${BASE_URL}/index.php" \ + "Direct file access without front controller"); then + exit 1 +fi @@ -fc_result=$(run_performance_test \ - "Front controller routing" \ - "${BASE_URL}/home.php?_ROUTE=index.php" \ - "Access through front controller") +if ! fc_result=$(run_performance_test \ + "Front controller routing" \ + "${BASE_URL}/home.php?_ROUTE=index.php" \ + "Access through front controller"); then + exit 1 +fi @@ -login_result=$(run_performance_test \ - "Login page access" \ - "${BASE_URL}/interface/login/login.php" \ - "Anonymous page with ignoreAuth pattern") +if ! login_result=$(run_performance_test \ + "Login page access" \ + "${BASE_URL}/interface/login/login.php" \ + "Anonymous page with ignoreAuth pattern"); then + exit 1 +fi @@ - static_result=$(run_performance_test \ - "Static CSS file" \ - "${BASE_URL}/public/assets/css/style.css" \ - "Static assets should bypass front controller") + if ! static_result=$(run_performance_test \ + "Static CSS file" \ + "${BASE_URL}/public/assets/css/style.css" \ + "Static assets should bypass front controller"); then + exit 1 + fi @@ -api_result=$(run_performance_test \ - "REST API endpoint" \ - "${BASE_URL}/apis/default/api/patient" \ - "Existing API front controller (should not change)") +if ! api_result=$(run_performance_test \ + "REST API endpoint" \ + "${BASE_URL}/apis/default/api/patient" \ + "Existing API front controller (should not change)"); then + exit 1 +fi
317-333: Allow negative overhead values when front controller is fasterA faster front controller yields a negative overhead, but the current
[[ "${overhead}" =~ ^[0-9]+\.?[0-9]*$ ]]check rejects the minus sign and drops into “Unable to calculate overhead.” Accept an optional leading-(and matching decimal forms) so improvements are reported instead of discarded.-if [[ -n "${overhead}" ]] && [[ "${overhead}" =~ ^[0-9]+\.?[0-9]*$ ]]; then +if [[ -n "${overhead}" ]] && [[ "${overhead}" =~ ^-?[0-9]+([.][0-9]+)?$ ]]; thentests/scripts/test_compatibility.sh (1)
225-240: POST requests must fail on 5xx/connection errors.Reiterating prior feedback: anything other than a 2xx (including 5xx or curl “000”) currently reports PASS. Tighten the check so only 2xx pass and everything else fails.
-if [[ "${http_code}" = "404" || "${http_code}" = "403" ]]; then - printf "%sFAIL%s (HTTP %s)\n" "${RED}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" - FAILED_TESTS=$(( FAILED_TESTS + 1 )) - echo " ✗ POST requests are being blocked" | tee -a "${REPORT_FILE}" -else - printf "%sPASS%s (HTTP %s)\n" "${GREEN}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" - PASSED_TESTS=$(( PASSED_TESTS + 1 )) - echo " ✓ POST requests are processed correctly" | tee -a "${REPORT_FILE}" -fi +if [[ "${http_code}" =~ ^2[0-9][0-9]$ ]]; then + printf "%sPASS%s (HTTP %s)\n" "${GREEN}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" + PASSED_TESTS=$(( PASSED_TESTS + 1 )) + echo " ✓ POST requests are processed correctly" | tee -a "${REPORT_FILE}" +else + printf "%sFAIL%s (HTTP %s)\n" "${RED}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" + FAILED_TESTS=$(( FAILED_TESTS + 1 )) + if [[ "${http_code}" = "000" ]]; then + echo " ✗ POST request failed to reach the server" | tee -a "${REPORT_FILE}" + else + echo " ✗ POST requests are being blocked or failing" | tee -a "${REPORT_FILE}" + fi +fitests/scripts/test_security.sh (2)
15-18: Propagate pipeline failures withset -o pipefail.Previous feedback still applies: without
set -o pipefail, errors before| teeare swallowed. Keep CI honest by enabling pipefail right afterset -e.set -e +# Ensure pipeline failures bubble up (addresses ShellCheck SC2312) +set -o pipefail # Ensure errexit is inherited in command substitutions (addresses SC2311) shopt -s inherit_errexit 2>/dev/null || true
25-33: Stoptputfrom killing the script.Reiterating: under
set -e, a failingtputexits the suite (TERM=dumb, missing terminfo, etc.). Guard the calls and fall back to plain output.-if command -v tput >/dev/null 2>&1 && [[ -n "${TERM}" ]]; then - RED=$(tput setaf 1) - GREEN=$(tput setaf 2) - NC=$(tput sgr0) # No Color -else - RED="" - GREEN="" - NC="" -fi +if command -v tput >/dev/null 2>&1 && [[ -n "${TERM:-}" ]]; then + RED=$(tput setaf 1 2>/dev/null || printf '') + GREEN=$(tput setaf 2 2>/dev/null || printf '') + NC=$(tput sgr0 2>/dev/null || printf '') + if [[ -z "${RED}" || -z "${GREEN}" || -z "${NC}" ]]; then + RED=""; GREEN=""; NC="" + fi +else + RED=""; GREEN=""; NC="" +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
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 (2)
tests/scripts/test_security.sh (1)
tests/scripts/test_compatibility.sh (1)
run_test(52-77)
tests/scripts/test_compatibility.sh (1)
tests/scripts/test_security.sh (1)
run_test(50-75)
🪛 GitHub Actions: ShellCheck
tests/scripts/test_security.sh
[info] 40-40: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
tests/scripts/test_compatibility.sh
[info] 41-41: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
tests/scripts/test_performance.sh
[info] 44-44: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 271-271: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 297-297: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 299-299: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[info] 302-302: ShellCheck SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
🪛 GitHub Check: ShellCheck
tests/scripts/test_security.sh
[warning] 40-40:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
tests/scripts/test_compatibility.sh
[warning] 41-41:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
tests/scripts/test_performance.sh
[warning] 302-302:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 302-302:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 299-299:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 299-299:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 297-297:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 271-271:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
[warning] 44-44:
Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: PHP 8.6 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 8.0
- GitHub Check: PHP 8.4 - apache - mysql 9.3
- GitHub Check: PHP 8.4 - apache - mariadb 10.11
- GitHub Check: PHP 8.2 - nginx - mariadb 11.8
- GitHub Check: PHP 8.3 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 8.4
- GitHub Check: PHP 8.4 - apache - mysql 5.7
- GitHub Check: PHP 8.5 - nginx - mariadb 11.8
- GitHub Check: PHP 8.3 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 10.6
- GitHub Check: PHP 8.4 - apache - mariadb 11.4
- GitHub Check: PHP 8.2 - apache - mariadb 11.8
- GitHub Check: PHP 8.2 - Isolated Tests
- GitHub Check: PHP 8.5 - Isolated Tests
- GitHub Check: PHP 8.4 - Isolated Tests
- GitHub Check: PHP 8.3 - Isolated Tests
| # Colors for output using tput (with fallback for non-interactive environments) | ||
| if command -v tput >/dev/null 2>&1 && [[ -n "${TERM}" ]]; then | ||
| RED=$(tput setaf 1) | ||
| GREEN=$(tput setaf 2) | ||
| NC=$(tput sgr0) # No Color | ||
| else | ||
| RED="" | ||
| GREEN="" | ||
| NC="" |
There was a problem hiding this comment.
Let tput failures fall back instead of aborting.
With set -e, any non-zero tput (common when TERM lacks color support) terminates the suite before tests run. Swallow those failures and fall back to plain text.
-if command -v tput >/dev/null 2>&1 && [[ -n "${TERM}" ]]; then
- RED=$(tput setaf 1)
- GREEN=$(tput setaf 2)
- NC=$(tput sgr0) # No Color
-else
- RED=""
- GREEN=""
- NC=""
-fi
+if command -v tput >/dev/null 2>&1 && [[ -n "${TERM:-}" ]]; then
+ RED=$(tput setaf 1 2>/dev/null || printf '')
+ GREEN=$(tput setaf 2 2>/dev/null || printf '')
+ NC=$(tput sgr0 2>/dev/null || printf '')
+ if [[ -z "${RED}" || -z "${GREEN}" || -z "${NC}" ]]; then
+ RED=""; GREEN=""; NC=""
+ fi
+else
+ RED=""; GREEN=""; NC=""
+fi🤖 Prompt for AI Agents
In tests/scripts/test_compatibility.sh around lines 25 to 33, tput failures
currently can produce non-zero exits under set -e and abort the test suite;
change the assignments to swallow tput errors by redirecting stderr and making
the command non-fatal (e.g., capture tput output with 2>/dev/null and fall back
on an empty string on failure using || true or || :), so RED, GREEN, and NC are
set to color values when available but quietly fall back to empty strings
without causing the script to exit.
cfb7ea2 to
5b45567
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/scripts/test_compatibility.sh (2)
25-34: Keep going whentputis unavailable.With
set -e, a non-zerotput(common in TERM-less CI) still aborts the script, so the suite never runs. Swallow the failure and fall back to empty strings as previously suggested.Apply this diff:
-if command -v tput >/dev/null 2>&1 && [[ -n "${TERM}" ]]; then - RED=$(tput setaf 1) - GREEN=$(tput setaf 2) - NC=$(tput sgr0) # No Color -else - RED="" - GREEN="" - NC="" -fi +if command -v tput >/dev/null 2>&1 && [[ -n "${TERM:-}" ]]; then + RED=$(tput setaf 1 2>/dev/null || printf '') + GREEN=$(tput setaf 2 2>/dev/null || printf '') + NC=$(tput sgr0 2>/dev/null || printf '') + if [[ -z "${RED}" || -z "${GREEN}" || -z "${NC}" ]]; then + RED=''; GREEN=''; NC='' + fi +else + RED=''; GREEN=''; NC='' +fi
234-241: Fail POST test on 5xx/000 responses.This branch still marks 5xx or curl
000as PASS, so a broken login slips through. Only 2xx should succeed; everything else must fail.Apply this diff:
-if [[ "${http_code}" = "404" || "${http_code}" = "403" ]]; then +if [[ "${http_code}" = "404" || "${http_code}" = "403" || "${http_code}" = "000" || "${http_code}" =~ ^5 ]]; then printf "%sFAIL%s (HTTP %s)\n" "${RED}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" FAILED_TESTS=$(( FAILED_TESTS + 1 )) echo " ✗ POST requests are being blocked" | tee -a "${REPORT_FILE}" else
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/scripts/test_compatibility.sh(1 hunks)tests/scripts/test_performance.sh(1 hunks)tests/scripts/test_security.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/scripts/test_security.sh
- tests/scripts/test_performance.sh
🧰 Additional context used
🧬 Code graph analysis (1)
tests/scripts/test_compatibility.sh (1)
tests/scripts/test_security.sh (1)
run_test(52-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: PHP 8.5 - Isolated Tests
- GitHub Check: PHP 8.4 - Isolated Tests
- GitHub Check: PHP 8.3 - Isolated Tests
- GitHub Check: PHP (8.4)
| # 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}" |
There was a problem hiding this comment.
Fix status-code matching to avoid false positives.
[[ "${expected_codes}" = *"${http_code}"* ]] treats 000 and other garbage codes as success because they are substrings of 200, so we miss real outages.
Apply this diff:
- if [[ "${expected_codes}" = *"${http_code}"* ]]; then
+ if [[ " ${expected_codes} " == *" ${http_code} "* ]]; then📝 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.
| # 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}" | |
| # 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}" |
🤖 Prompt for AI Agents
In tests/scripts/test_compatibility.sh around lines 67-76, the check [[
"${expected_codes}" = *"${http_code}"* ]] falsely matches substrings (e.g. "000"
inside "200"); change the comparison to match whole codes only by treating
expected_codes as a space- or comma-delimited list and checking for a full token
match (for example, pad expected_codes with surrounding separators and test for
" ${http_code} " within it or loop over tokens and compare equality). Ensure
http_code is trimmed and you update the conditional and its branches only (keep
logging/increment behavior unchanged).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
.env.example (2)
1-1: Remove the shebang line from .env.example.A
.envfile is a data file loaded by libraries like phpdotenv, not an executable script. The shebang#!/bin/shserves no purpose here and could confuse developers about the file's role.Apply this diff:
-#!/bin/sh - # Possible options are prod or dev. No spaces, no quotes.
9-9: Fix environment variable naming inconsistency.The new environment variables use a single underscore prefix (
OPENEMR_) while existing variables consistently use double underscores (OPENEMR__). This inconsistency was flagged in previous review comments and will cause the front controller code (home.php) to fail when checking forOPENEMR__ENABLE_FRONT_CONTROLLER(double underscore).Apply this diff to maintain naming consistency:
# Front controller routing (optional, disabled by default) # Set to 1 to enable centralized routing and block .inc.php files -export OPENEMR_ENABLE_FRONT_CONTROLLER= +OPENEMR__ENABLE_FRONT_CONTROLLER= # Front controller logging: debug or empty -export OPENEMR_FC_LOG_LEVEL= +OPENEMR__FC_LOG_LEVEL=Also applies to: 12-12
tests/Tests/FrontController/SecurityTest.php (2)
44-44: Fix: Test scans wrong directory.
dirname(__DIR__, 2)resolves totests/instead of the repository root, so the iterator never scans production.inc.phpfiles. This matches the issue flagged in the previous review.- $basePath = dirname(__DIR__, 2); + $basePath = dirname(__DIR__, 3);
51-51: Fix: Path separator breaks on Windows.Hardcoded
'/'separator will fail on Windows systems. UseDIRECTORY_SEPARATORfor cross-platform compatibility. This matches the issue flagged in the previous review.- $relativePath = str_replace($basePath . '/', '', $file->getPathname()); + $relativePath = str_replace($basePath . DIRECTORY_SEPARATOR, '', $file->getPathname());
🧹 Nitpick comments (3)
tests/Tests/FrontController/SecurityTest.php (3)
21-21: Remove unused import.
GuzzleHttp\Exception\RequestExceptionis imported but never used in this test class.-use GuzzleHttp\Exception\RequestException;
219-219: Consider using GET instead of HEAD for header verification.While security headers should be present on HEAD responses, some server configurations or middleware may only send complete headers on GET requests. Using GET would make this test more reliable across different environments.
- $response = $client->head('/index.php'); + $response = $client->get('/index.php');
260-260: Consider verifying file write success.
file_put_contentscan return false on failure. While the subsequent assertion checks file existence, verifying the write operation itself would catch write permission issues more explicitly.- file_put_contents($reportPath, json_encode($report, JSON_PRETTY_PRINT) . "\n"); + $bytesWritten = file_put_contents($reportPath, json_encode($report, JSON_PRETTY_PRINT) . "\n"); + $this->assertNotFalse($bytesWritten, 'Failed to write security report');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.env.example(1 hunks)interface/login/login.php(1 hunks)tests/Tests/FrontController/CompatibilityTest.php(1 hunks)tests/Tests/FrontController/SecurityTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Tests/FrontController/CompatibilityTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: PHP 8.4 - apache - mysql 8.4
- GitHub Check: PHP 8.5 - nginx - mariadb 11.8
- GitHub Check: PHP 8.6 - nginx - mariadb 11.8
- GitHub Check: PHP 8.3 - nginx - mariadb 11.8
- GitHub Check: PHP 8.2 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 11.4
- GitHub Check: PHP 8.4 - apache - mysql 5.7
- GitHub Check: PHP 8.2 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 9.3
- GitHub Check: PHP 8.4 - apache - mysql 8.0
- GitHub Check: PHP 8.4 - apache - mariadb 10.6
- GitHub Check: PHP 8.4 - apache - mariadb 10.11
- GitHub Check: PHP 8.3 - apache - mariadb 11.8
- GitHub Check: PHP 8.2 - Isolated Tests
- GitHub Check: phpstan (8.4)
- GitHub Check: PHP 8.4 - Isolated Tests
- GitHub Check: PHP 8.5 - Isolated Tests
- GitHub Check: PHP 8.3 - Isolated Tests
| #!/bin/sh | ||
| # Possible options are prod or dev. No spaces, no quotes. | ||
| OPENEMR__ENVIRONMENT= | ||
| export OPENEMR__ENVIRONMENT= |
There was a problem hiding this comment.
Remove export keywords; use standard .env syntax.
The export keyword is shell-specific and incompatible with standard PHP .env parsers like vlucas/phpdotenv. These libraries expect simple KEY=VALUE format without the export prefix. Using export will cause parsing errors or require shell-based loading, which is non-standard for PHP applications.
Apply this diff to use standard .env syntax:
# Possible options are prod or dev. No spaces, no quotes.
-export OPENEMR__ENVIRONMENT=
+OPENEMR__ENVIRONMENT=
# Possible options are true to prevent dated reminders and background apps.Else empty or false. No spaces, no quotes.
-export OPENEMR__NO_BACKGROUND_TASKS=
+OPENEMR__NO_BACKGROUND_TASKS=
# Front controller routing (optional, disabled by default)
# Set to 1 to enable centralized routing and block .inc.php files
-export OPENEMR_ENABLE_FRONT_CONTROLLER=
+OPENEMR__ENABLE_FRONT_CONTROLLER=
# Front controller logging: debug or empty
-export OPENEMR_FC_LOG_LEVEL=
+OPENEMR__FC_LOG_LEVEL=Also applies to: 5-5, 9-9, 12-12
🤖 Prompt for AI Agents
.in.env.example lines 3,5,9,12: remove the shell-specific "export" prefix from
the listed entries and convert them to standard dotenv KEY=VALUE syntax (e.g.
OPENEMR__ENVIRONMENT=) so PHP .env parsers like vlucas/phpdotenv can read them;
update each affected line by deleting the word "export" and any leading
whitespace so each variable appears as KEY=VALUE on its own line.
| Header("X-Content-Type-Options: nosniff"); | ||
| Header("X-XSS-Protection: 1; mode=block"); |
There was a problem hiding this comment.
X-XSS-Protection header is deprecated; consider removing it.
The X-XSS-Protection header was deprecated and removed from modern browsers (Chrome in 2019, Edge follows, Firefox never implemented it). Modern browsers rely on Content Security Policy (CSP) instead. While X-Content-Type-Options: nosniff is appropriate and recommended, the XSS Protection header serves no purpose and could create a false sense of security.
Consider removing line 32 or documenting that it's only for legacy browser support.
Additionally, since the AI summary indicates that multiple entry points (index.php, login.php) are adding similar security headers, consider centralizing these headers in the front controller or a shared security middleware to avoid duplication and ensure consistency.
Apply this diff to remove the deprecated header:
// prevent UI redressing and set security headers
Header("X-Frame-Options: DENY");
Header("Content-Security-Policy: frame-ancestors 'none'");
Header("X-Content-Type-Options: nosniff");
-Header("X-XSS-Protection: 1; mode=block");🤖 Prompt for AI Agents
interface/login/login.php around lines 31-32: remove the deprecated
Header("X-XSS-Protection: 1; mode=block") line (keep the X-Content-Type-Options
header), and instead centralize all common security headers into the front
controller or a shared security middleware (e.g., a single include loaded by
index.php/login.php) so headers aren’t duplicated across entry points; delete
the deprecated header line here and add the remaining necessary headers in the
centralized file, then remove duplicates from other entry files.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/Tests/FrontController/CompatibilityTest.php (3)
20-22: Remove unused import.Cleanup unused RequestException import.
use GuzzleHttp\Client; -use GuzzleHttp\Exception\RequestException; use PHPUnit\Framework\TestCase;
208-214: Use version‑agnostic assertion for status set membership.assertTrue + in_array avoids PHPUnit version deprecations.
- // Setup should be accessible (200) or redirect (302) - $this->assertContains( - $httpCode, - [200, 302], - 'Setup workflow should be accessible' - ); + // Setup should be accessible (200) or redirect (302) + $this->assertTrue( + in_array($httpCode, [200, 302], true), + 'Setup workflow should be accessible' + );
269-275: Use version‑agnostic assertion for status set membership.Same rationale as above.
- // POST should return 200 or 302 (redirect) - $this->assertContains( - $httpCode, - [200, 302], - 'POST requests should be processed' - ); + // POST should return 200 or 302 (redirect) + $this->assertTrue( + in_array($httpCode, [200, 302], true), + 'POST requests should be processed' + );tests/Tests/FrontController/SecurityTest.php (1)
45-55: Fix repo-root path and make relative path computation cross‑platform.Iterator starts at tests/ instead of repo root and Windows paths break due to hardcoded '/'. Apply below to walk from repo root, skip dots, and normalize separators. This also addresses prior feedback.
- // Walk up to repository root to scan production .inc.php files - $basePath = dirname(__DIR__, 2); - $iterator = new \RecursiveIteratorIterator( - new \RecursiveDirectoryIterator($basePath) - ); + // Walk up to repository root to scan production .inc.php files + $basePath = dirname(__DIR__, 3); + $iterator = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($basePath, \FilesystemIterator::SKIP_DOTS) + ); @@ - 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; - } - } + foreach ($iterator as $file) { + if ($file->isFile() && preg_match('/\.inc\.php$/i', $file->getFilename())) { + // Cross‑platform relative path (normalize separators) + $baseNorm = rtrim(str_replace('\\', '/', realpath($basePath)), '/'); + $pathNorm = str_replace('\\', '/', $file->getPathname()); + $relativePath = strpos($pathNorm, $baseNorm . '/') === 0 + ? substr($pathNorm, strlen($baseNorm) + 1) + : $pathNorm; + self::$vulnerable_inc_files[] = $relativePath; + } + }
🧹 Nitpick comments (4)
tests/Tests/FrontController/SecurityTest.php (4)
60-62: Skip test if no .inc.php files found (avoid false green).// Test a sample of vulnerable .inc.php files $sampleFiles = array_slice(self::$vulnerable_inc_files, 0, 10); + if (empty($sampleFiles)) { + $this->markTestSkipped('No .inc.php files found to test'); + }
21-22: Remove unused import.use GuzzleHttp\Client; -use GuzzleHttp\Exception\RequestException; use PHPUnit\Framework\TestCase;
212-243: Modernize header assertions (X‑XSS‑Protection is deprecated).Prefer asserting CSP (and optionally Referrer‑Policy/Permissions‑Policy). Accept X‑XSS‑Protection or CSP to avoid brittle failures across environments.
- $this->assertTrue( - $response->hasHeader('X-XSS-Protection'), - 'Security header X-XSS-Protection should be present' - ); + // X-XSS-Protection is deprecated in modern browsers; accept CSP as an alternative + $this->assertTrue( + $response->hasHeader('X-XSS-Protection') || $response->hasHeader('Content-Security-Policy'), + 'Expect X-XSS-Protection or Content-Security-Policy to be present' + );
60-73: HEAD may behave inconsistently; consider HEAD→GET fallback.Some endpoints return 405/404 for HEAD though GET would be 200/403. Add a helper to try HEAD then fallback to GET to reduce test flakiness.
Also applies to: 80-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Tests/FrontController/CompatibilityTest.php(1 hunks)tests/Tests/FrontController/SecurityTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Tests/FrontController/CompatibilityTest.php (1)
tests/Tests/FrontController/SecurityTest.php (1)
setUpBeforeClass(30-41)
tests/Tests/FrontController/SecurityTest.php (1)
tests/Tests/FrontController/CompatibilityTest.php (1)
setUpBeforeClass(29-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: PHP 8.3 - nginx - mariadb 11.8
- GitHub Check: PHP 8.3 - apache - mariadb 11.8
- GitHub Check: PHP 8.5 - nginx - mariadb 11.8
- GitHub Check: PHP 8.2 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - nginx - mariadb 11.8
- GitHub Check: PHP 8.6 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 9.3
- GitHub Check: PHP 8.4 - apache - mysql 8.4
- GitHub Check: PHP 8.4 - apache - mariadb 10.6
- GitHub Check: PHP 8.4 - apache - mysql 8.0
- GitHub Check: PHP 8.4 - apache - mariadb 10.11
- GitHub Check: PHP 8.4 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 5.7
- GitHub Check: PHP 8.4 - apache - mariadb 11.4
- GitHub Check: PHP 8.2 - apache - mariadb 11.8
- GitHub Check: PHP 8.2 - Isolated Tests
- GitHub Check: PHP 8.5 - Isolated Tests
- GitHub Check: PHP 8.4 - Isolated Tests
- GitHub Check: PHP 8.3 - Isolated Tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/OpenCoreEMR/FrontController/Router.php (3)
37-49: Consider validating and sanitizing the route parameter.The route from
$_GET['_ROUTE']is used without validation. WhileresolveTargetFile()provides path traversal protection viarealpath(), validating input early can prevent issues and improve security posture. Consider rejecting routes with null bytes, excessive path segments, or suspicious patterns.Example validation to add after line 45:
// Remove query string $route = strtok($route, '?'); // Basic validation if (str_contains($route, "\0") || preg_match('/[<>"|*?]/', $route)) { $route = ''; } $this->route = $route;
51-61: Validate the site parameter to prevent injection.The
siteparameter from$_GETis stored without validation. Consider validating against a whitelist or checking for malicious patterns (null bytes, path traversal sequences, etc.) to prevent potential injection attacks.Example validation:
public function determineSiteId(): string { // explicit site parameter or session-based detection. - $this->siteId = $_GET['site'] ?? 'default'; + $site = $_GET['site'] ?? 'default'; + + // Validate site parameter (alphanumeric, hyphens, underscores only) + if (!preg_match('/^[a-zA-Z0-9_-]+$/', $site)) { + $site = 'default'; + } + + $this->siteId = $site; $_GET['site'] = $this->siteId; return $this->siteId; }
56-56: Update or remove incomplete comment.The comment mentions "session-based detection" but the implementation only uses
$_GET['site']. Either implement session-based fallback or update the comment to reflect the actual behavior.- // explicit site parameter or session-based detection. + // Use explicit site parameter from query string, defaulting to 'default' $this->siteId = $_GET['site'] ?? 'default';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Events/FrontController/FrontControllerEvent.php(1 hunks)src/OpenCoreEMR/FrontController/Router.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Events/FrontController/FrontControllerEvent.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/OpenCoreEMR/FrontController/Router.php (2)
src/Events/FrontController/FrontControllerEvent.php (3)
__construct(46-60)getRoute(67-70)getSiteId(77-80)src/OpenCoreEMR/FrontController/RouteConfig.php (5)
__construct(43-46)RouteConfig(20-236)isDeprecated(148-151)isForbidden(126-129)requiresAdmin(137-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: PHP 8.5 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - nginx - mariadb 11.8
- GitHub Check: PHP 8.6 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 8.4
- GitHub Check: PHP 8.2 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 9.3
- GitHub Check: PHP 8.3 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 10.11
- GitHub Check: PHP 8.4 - apache - mysql 8.0
- GitHub Check: PHP 8.4 - apache - mysql 5.7
- GitHub Check: PHP 8.4 - apache - mariadb 11.4
- GitHub Check: PHP 8.3 - apache - mariadb 11.8
- GitHub Check: PHP 8.2 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 10.6
- GitHub Check: PHP 8.3 - Isolated Tests
- GitHub Check: PHP 8.2 - Isolated Tests
- GitHub Check: PHP 8.5 - Isolated Tests
- GitHub Check: PHP 8.4 - Isolated Tests
🔇 Additional comments (4)
src/OpenCoreEMR/FrontController/Router.php (4)
27-35: LGTM! Constructor validation is solid.The realpath() validation and exception handling address the previous review concerns. The error message is clear, and the fallback to a default RouteConfig is appropriate.
80-94: LGTM! Configuration delegation is clean.Both
isForbiddenPath()andrequiresAdmin()properly delegate to the configuration interface, maintaining good separation of concerns.
96-119: LGTM! Path traversal protection is robust.The
resolveTargetFile()method properly addresses previous security concerns:
- Uses
str_starts_with()withDIRECTORY_SEPARATORfor path traversal prevention- Validates file existence before returning
- Restricts routing to
.phpfiles onlyThis provides strong security guarantees for file resolution.
121-135: LGTM! Getters are correctly implemented.The getter methods are straightforward and correctly return the internal state. The uninitialized property concern was addressed in the earlier comment on the property declarations.
| private readonly string $baseDir; | ||
| private string $route; | ||
| private string $siteId; |
There was a problem hiding this comment.
Initialize $route and $siteId properties to prevent uninitialized access.
Properties $route and $siteId are declared but not initialized in the constructor. If getRoute() or getSiteId() are called before extractRoute() or determineSiteId(), this will throw an error (uninitialized typed property in PHP 8.1+).
Apply this diff to initialize the properties:
private readonly string $baseDir;
- private string $route;
- private string $siteId;
+ private string $route = '';
+ private string $siteId = 'default';
private readonly RouteConfigInterface $config;📝 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 readonly string $baseDir; | |
| private string $route; | |
| private string $siteId; | |
| private readonly string $baseDir; | |
| private string $route = ''; | |
| private string $siteId = 'default'; | |
| private readonly RouteConfigInterface $config; |
🤖 Prompt for AI Agents
In src/OpenCoreEMR/FrontController/Router.php around lines 22 to 24, the typed
properties $route and $siteId are declared but not initialized which can cause
an "uninitialized typed property" error if getters are called before
extractRoute()/determineSiteId(); initialize them to safe defaults (e.g., empty
strings) either by assigning default values at declaration or by assigning them
in the constructor so getRoute() and getSiteId() always return initialized
values.
| /** | ||
| * Handle trailing slash redirect | ||
| * | ||
| * We always want there to be a trailing slash for consistency | ||
| */ | ||
| public function handleTrailingSlash(): void | ||
| { | ||
| // Skip trailing slash redirect for .php files | ||
| if ($this->route !== '' && !str_ends_with($this->route, '/') && pathinfo($this->route, PATHINFO_EXTENSION) !== 'php') { | ||
| // Ensure proper relative redirect with query string preservation | ||
| $queryString = $_SERVER['QUERY_STRING'] ?? ''; | ||
| $redirectUrl = $this->route . '/' . ($queryString ? '?' . $queryString : ''); | ||
| header('Location: ' . $redirectUrl, true, 301); | ||
| exit; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Query string preservation is correct, but validate the redirect URL.
The query string preservation logic addresses the previous review concern. However, ensure $this->route doesn't contain absolute URLs or protocol-relative URLs before constructing the redirect. If an attacker can control the route to be //evil.com, the redirect could send users to an external site.
Add validation before the redirect:
public function handleTrailingSlash(): void
{
// Skip trailing slash redirect for .php files
if ($this->route !== '' && !str_ends_with($this->route, '/') && pathinfo($this->route, PATHINFO_EXTENSION) !== 'php') {
+ // Prevent open redirect: ensure route is relative
+ if (str_starts_with($this->route, '//') || str_contains($this->route, '://')) {
+ return;
+ }
+
// Ensure proper relative redirect with query string preservation
$queryString = $_SERVER['QUERY_STRING'] ?? '';
$redirectUrl = $this->route . '/' . ($queryString ? '?' . $queryString : '');
header('Location: ' . $redirectUrl, true, 301);
exit;
}
}Run the following script to check if there are existing guards against absolute URLs in the route extraction:
🏁 Script executed:
#!/bin/bash
# Search for route validation or sanitization in related files
rg -nP --type=php -C3 '(https?://|protocol.*relative|absolute.*url|open.*redirect)' src/OpenCoreEMR/FrontController/Length of output: 2502
Add open-redirect protection in handleTrailingSlash
Ensure $this->route is a relative path before redirecting:
public function handleTrailingSlash(): void
{
// Skip trailing slash redirect for .php files
if ($this->route !== '' && !str_ends_with($this->route, '/') && pathinfo($this->route, PATHINFO_EXTENSION) !== 'php') {
+ // Prevent open redirect: route must not start with "//" or contain a scheme
+ if (str_starts_with($this->route, '//') || str_contains($this->route, '://')) {
+ return;
+ }
// Ensure proper relative redirect with query string preservation
$queryString = $_SERVER['QUERY_STRING'] ?? '';
$redirectUrl = $this->route . '/' . ($queryString ? '?' . $queryString : '');
header('Location: ' . $redirectUrl, true, 301);
exit;
}
}📝 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.
| /** | |
| * Handle trailing slash redirect | |
| * | |
| * We always want there to be a trailing slash for consistency | |
| */ | |
| public function handleTrailingSlash(): void | |
| { | |
| // Skip trailing slash redirect for .php files | |
| if ($this->route !== '' && !str_ends_with($this->route, '/') && pathinfo($this->route, PATHINFO_EXTENSION) !== 'php') { | |
| // Ensure proper relative redirect with query string preservation | |
| $queryString = $_SERVER['QUERY_STRING'] ?? ''; | |
| $redirectUrl = $this->route . '/' . ($queryString ? '?' . $queryString : ''); | |
| header('Location: ' . $redirectUrl, true, 301); | |
| exit; | |
| } | |
| } | |
| /** | |
| * Handle trailing slash redirect | |
| * | |
| * We always want there to be a trailing slash for consistency | |
| */ | |
| public function handleTrailingSlash(): void | |
| { | |
| // Skip trailing slash redirect for .php files | |
| if ($this->route !== '' && !str_ends_with($this->route, '/') && pathinfo($this->route, PATHINFO_EXTENSION) !== 'php') { | |
| // Prevent open redirect: route must not start with "//" or contain a scheme | |
| if (str_starts_with($this->route, '//') || str_contains($this->route, '://')) { | |
| return; | |
| } | |
| // Ensure proper relative redirect with query string preservation | |
| $queryString = $_SERVER['QUERY_STRING'] ?? ''; | |
| $redirectUrl = $this->route . '/' . ($queryString ? '?' . $queryString : ''); | |
| header('Location: ' . $redirectUrl, true, 301); | |
| exit; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/OpenCoreEMR/FrontController/Router.php around lines 63 to 78, the
trailing-slash redirect currently uses $this->route directly which can enable an
open-redirect; before issuing the header Location, verify the route is a safe
internal path by ensuring it does not contain a URL scheme
(parse_url($this->route, PHP_URL_SCHEME) is null), is not protocol-relative
(does not start with '//'), and is an internal path (e.g. starts with '/'); only
perform the redirect when those checks pass, otherwise skip the redirect (or
return) to prevent open-redirects.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/Tests/FrontController/SecurityTest.php (1)
47-47: Scan the real application tree.
dirname(__DIR__, 2)still resolves to thetests/directory, so the iterator never touches production.inc.phpfiles and the test misses regressions. Walk up one more level (e.g.,dirname(__DIR__, 3)or anchoring on project root) before scanning.
🧹 Nitpick comments (1)
tests/Tests/FrontController/SecurityTest.php (1)
54-55: Normalize separators when deriving relative paths.
str_replace($basePath . '/', …)only works on POSIX paths; Windows backslashes leave prefixes intact and the test requests/tests\foo.inc.php. UseDIRECTORY_SEPARATOR(orsubstr/ltrim) to produce portable URLs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Tests/FrontController/CompatibilityTest.php(1 hunks)tests/Tests/FrontController/SecurityTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Tests/FrontController/SecurityTest.php (1)
tests/Tests/FrontController/CompatibilityTest.php (1)
setUpBeforeClass(29-40)
tests/Tests/FrontController/CompatibilityTest.php (1)
tests/Tests/FrontController/SecurityTest.php (1)
setUpBeforeClass(30-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: PHP 8.5 - nginx - mariadb 11.8
- GitHub Check: PHP 8.3 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 5.7
- GitHub Check: PHP 8.3 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 11.4
- GitHub Check: PHP 8.4 - apache - mysql 8.0
- GitHub Check: PHP 8.6 - nginx - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 9.3
- GitHub Check: PHP 8.2 - nginx - mariadb 11.8
- GitHub Check: PHP 8.2 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mysql 8.4
- GitHub Check: PHP 8.4 - apache - mariadb 10.11
- GitHub Check: PHP 8.4 - apache - mariadb 11.8
- GitHub Check: PHP 8.4 - apache - mariadb 10.6
- GitHub Check: PHP 8.3 - Isolated Tests
- GitHub Check: PHP 8.2 - Isolated Tests
- GitHub Check: PHP 8.5 - Isolated Tests
- GitHub Check: PHP 8.4 - Isolated Tests
| $attackVectors = [ | ||
| '../../../etc/passwd', | ||
| '....//....//....//etc/passwd', | ||
| '..%2F..%2F..%2Fetc%2Fpasswd', | ||
| '..%5c..%5c..%5cetc%5cpasswd', | ||
| '%2e%2e%2f%2e%2e%2f%2e%2e%2fetc%2fpasswd', | ||
| ]; | ||
|
|
||
| foreach ($attackVectors as $vector) { | ||
| $response = self::$client->head('/home.php?_ROUTE=' . urlencode($vector), [ | ||
| 'allow_redirects' => false | ||
| ]); | ||
| $httpCode = $response->getStatusCode(); | ||
|
|
||
| $this->assertEquals( | ||
| 404, | ||
| $httpCode, | ||
| "Path traversal attack '{$vector}' should return 404, got {$httpCode}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Stop double-encoding traversal payloads.
Already escaped vectors (those containing %) get urlencode()d again, turning % into %25 and weakening coverage. Keep raw vectors (encode them once) and pass pre-encoded ones through untouched to exercise the real attack strings.
🤖 Prompt for AI Agents
In tests/Tests/FrontController/SecurityTest.php around lines 97 to 116, the test
double-encodes already percent-encoded attack vectors by always calling
urlencode(), which turns '%' into '%25' and defeats coverage; change the request
assembly so that vectors containing '%' (pre-encoded) are used as-is while raw
vectors are encoded once (e.g., if strpos($vector, '%') !== false then use
$vector else urlencode($vector)), then build the URL with the chosen value so
pre-encoded payloads are not altered and raw payloads remain properly encoded.
4326c3b to
94b8a1a
Compare
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.
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.
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
Removed redundant security headers from home.php, index.php, and login.php, centralizing header management. Refactored event dispatching in home.php to use a helper function for late event dispatch. Updated license comments to reference the full GPLv3 license URL. Improved type hints and return types in FrontControllerEvent. Standardized path concatenation using DIRECTORY_SEPARATOR and simplified baseDir validation in Router.
- Remove testSecurityHeadersPresent() test as headers will be in separate PR - Update license format to full URL in test files for consistency
coding-sunshine
force-pushed
the
8964-global-front-controller
branch
from
3a8f9e4 to
64cc024
Compare
- Remove security header test from test_security.sh script - Update license format to full URL in all remaining files: - security-check.php - test_security.sh - test_performance.sh - test_compatibility.sh
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (12)
security-check.php (1)
17-18: Block raw.incfiles too.The regex only matches
.inc.phpand.config.php, but raw.incfiles (e.g.,foo.inc) are not blocked. This is inconsistent withSecurityValidator::isIncludeFile()at src/OpenCoreEMR/FrontController/SecurityValidator.php:35 which blocks both.incand.inc.php.Apply this diff:
-if (preg_match('/\.(?:inc|config)\.php$/i', $requestedFile) || preg_match('#/(?:includes?|inc)/#i', $requestedFile)) { +if ( + preg_match('/\.inc(?:\.php)?$/i', $requestedFile) || + preg_match('/\.config\.php$/i', $requestedFile) || + preg_match('#/(?:includes?|inc)/#i', $requestedFile) +) {src/OpenCoreEMR/FrontController/Router.php (1)
68-78: Prevent open redirect vulnerability.The redirect uses
$this->routewithout validating that it's a relative internal path. An attacker could set_ROUTE=//evil.comto redirect users to an external site.Apply this diff to add validation:
public function handleTrailingSlash(): void { // Skip trailing slash redirect for .php files if ($this->route !== '' && !str_ends_with($this->route, '/') && pathinfo($this->route, PATHINFO_EXTENSION) !== 'php') { + // Prevent open redirect: ensure route is relative + if (str_starts_with($this->route, '//') || str_contains($this->route, '://')) { + return; + } // Ensure proper relative redirect with query string preservation $queryString = $_SERVER['QUERY_STRING'] ?? ''; $redirectUrl = $this->route . '/' . ($queryString ? '?' . $queryString : ''); header('Location: ' . $redirectUrl, true, 301); exit; } }tests/scripts/test_compatibility.sh (3)
25-34: Handletputfailures gracefully.With
set -e, anytputfailure (common when TERM lacks color support) will terminate the script before tests run. Redirect stderr and provide fallbacks.Apply this diff:
-if command -v tput >/dev/null 2>&1 && [[ -n "${TERM}" ]]; then - RED=$(tput setaf 1) - GREEN=$(tput setaf 2) - NC=$(tput sgr0) # No Color +if command -v tput >/dev/null 2>&1 && [[ -n "${TERM:-}" ]]; then + RED=$(tput setaf 1 2>/dev/null || printf '') + GREEN=$(tput setaf 2 2>/dev/null || printf '') + NC=$(tput sgr0 2>/dev/null || printf '') + if [[ -z "${RED}" || -z "${GREEN}" || -z "${NC}" ]]; then + RED=""; GREEN=""; NC="" + fi else - RED="" - GREEN="" - NC="" + RED=""; GREEN=""; NC="" fi
67-68: Fix status code matching to prevent false positives.The substring match
[[ "${expected_codes}" = *"${http_code}"* ]]treats"000"as matching"200", causing false positives when curl fails.Apply this diff to match whole codes only:
- if [[ "${expected_codes}" = *"${http_code}"* ]]; then + if [[ " ${expected_codes} " == *" ${http_code} "* ]]; then
234-242: Tighten POST test to fail on server errors.The current logic treats any response except 403/404 as success, so 5xx errors and curl failures (000) incorrectly pass.
Apply this diff to only pass on 2xx:
-if [[ "${http_code}" = "404" || "${http_code}" = "403" ]]; then +if [[ "${http_code}" =~ ^2 ]]; then + printf "%sPASS%s (HTTP %s)\n" "${GREEN}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" + PASSED_TESTS=$(( PASSED_TESTS + 1 )) + echo " ✓ POST requests are processed correctly" | tee -a "${REPORT_FILE}" +else printf "%sFAIL%s (HTTP %s)\n" "${RED}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" FAILED_TESTS=$(( FAILED_TESTS + 1 )) echo " ✗ POST requests are being blocked" | tee -a "${REPORT_FILE}" -else - printf "%sPASS%s (HTTP %s)\n" "${GREEN}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" - PASSED_TESTS=$(( PASSED_TESTS + 1 )) - echo " ✓ POST requests are processed correctly" | tee -a "${REPORT_FILE}" fitests/Tests/FrontController/SecurityTest.php (4)
18-22: Remove unused import.
GuzzleHttp\Exception\RequestExceptionis imported but never referenced in this test class.Apply this diff:
namespace OpenCoreEMR\Tests\FrontController; use GuzzleHttp\Client; -use GuzzleHttp\Exception\RequestException; use PHPUnit\Framework\TestCase;
53-54: UseDIRECTORY_SEPARATORfor cross-platform compatibility.Hardcoding
/in path replacement fails on Windows systems where the directory separator is\.Apply this diff:
- $relativePath = str_replace($basePath . '/', '', $file->getPathname()); + $relativePath = str_replace($basePath . DIRECTORY_SEPARATOR, '', $file->getPathname());
44-58: Fix base path to scan repository root, nottests/folder.
dirname(__DIR__, 2)fromtests/Tests/FrontController/SecurityTest.phpwalks up only totests/, so the iterator never scans production.inc.phpfiles. Theself::$vulnerable_inc_filesarray stays empty, andtestIncPhpFilesBlockedcannot detect regressions.Apply this diff:
- $basePath = dirname(__DIR__, 2); + $basePath = dirname(__DIR__, 3);
97-119: Separate raw and pre-encoded traversal vectors to avoid double-encoding.Calling
urlencode()on already percent-encoded vectors (containing%2F,%5c,%2e) transforms%into%25, which weakens test coverage by not exercising the actual attack strings.Apply this diff to split the vectors:
+ // Raw vectors that need encoding + $rawVectors = [ + '../../../etc/passwd', + '....//....//....//etc/passwd', + ]; + + // Pre-encoded vectors to test as-is + $encodedVectors = [ + '..%2F..%2F..%2Fetc%2Fpasswd', + '..%5c..%5c..%5cetc%5cpasswd', + '%2e%2e%2f%2e%2e%2f%2e%2e%2fetc%2fpasswd', + ]; + - $attackVectors = [ - '../../../etc/passwd', - '....//....//....//etc/passwd', - '..%2F..%2F..%2Fetc%2Fpasswd', - '..%5c..%5c..%5cetc%5cpasswd', - '%2e%2e%2f%2e%2e%2f%2e%2e%2fetc%2fpasswd', - ]; - - foreach ($attackVectors as $vector) { - $response = self::$client->head('/home.php?_ROUTE=' . urlencode($vector), [ + foreach ($rawVectors as $vector) { + $response = self::$client->head('/home.php?_ROUTE=' . urlencode($vector), [ + 'allow_redirects' => false + ]); + $httpCode = $response->getStatusCode(); + $this->assertEquals( + 404, + $httpCode, + "Path traversal attack '{$vector}' should return 404, got {$httpCode}" + ); + } + + foreach ($encodedVectors as $vector) { + $response = self::$client->head('/home.php?_ROUTE=' . $vector, [ 'allow_redirects' => false ]); $httpCode = $response->getStatusCode(); - $this->assertEquals( 404, $httpCode,tests/Tests/FrontController/CompatibilityTest.php (3)
18-22: Remove unused import.
GuzzleHttp\Exception\RequestExceptionis imported but never used in this test class.Apply this diff:
namespace OpenCoreEMR\Tests\FrontController; use GuzzleHttp\Client; -use GuzzleHttp\Exception\RequestException; use PHPUnit\Framework\TestCase;
202-215: Use PHPUnit 9+ compatible assertion.
assertContainsis deprecated in PHPUnit 9+ for checking array membership. UseassertContainsEqualsor restructure the assertion.Apply this diff:
- $this->assertContains( - $httpCode, - [200, 302], - 'Setup workflow should be accessible' - ); + $this->assertThat( + $httpCode, + $this->logicalOr( + $this->equalTo(200), + $this->equalTo(302) + ), + 'Setup workflow should be accessible' + );Or alternatively:
- $this->assertContains( - $httpCode, - [200, 302], - 'Setup workflow should be accessible' - ); + $this->assertTrue( + in_array($httpCode, [200, 302]), + "Setup workflow should be accessible (expected 200 or 302, got {$httpCode})" + );
259-276: Use PHPUnit 9+ compatible assertion.
assertContainsis deprecated in PHPUnit 9+ for checking array membership.Apply this diff:
- $this->assertContains( - $httpCode, - [200, 302], - 'POST requests should be processed' - ); + $this->assertTrue( + in_array($httpCode, [200, 302]), + "POST requests should be processed (expected 200 or 302, got {$httpCode})" + );
🧹 Nitpick comments (2)
.github/workflows/test.yml (1)
829-833: Consider adding coverage collection for FrontController tests.Unlike other test suites (unit, api, fixtures, etc.), the FrontController test step doesn't include coverage collection and upload steps. If coverage is intended, add steps to copy/convert coverage files and upload to Codecov, following the pattern used in lines 462-502 for API tests.
tests/scripts/test_compatibility.sh (1)
227-243: Consider refactoring POST test to userun_test.The POST test manually manages counters and logic instead of using the
run_testhelper. Consider refactoring to use the helper for consistency, or document why manual handling is needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.env.example(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks).htaccess(1 hunks)README-FRONT-CONTROLLER.md(1 hunks)composer.json(1 hunks)home.php(1 hunks)index.php(1 hunks)interface/login/login.php(1 hunks)phpunit.xml(2 hunks)reports/.gitkeep(1 hunks)security-check.php(1 hunks)src/Events/FrontController/FrontControllerEvent.php(1 hunks)src/OpenCoreEMR/FrontController/RouteConfig.php(1 hunks)src/OpenCoreEMR/FrontController/RouteConfigInterface.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)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/reports/security-test-report.json
- src/Events/FrontController/FrontControllerEvent.php
- tests/reports/compatibility-test-report.json
- interface/login/login.php
- tests/scripts/test_performance.sh
- reports/.gitkeep
- src/OpenCoreEMR/FrontController/RouteConfigInterface.php
- tests/scripts/test_security.sh
- composer.json
- .env.example
- .htaccess
- src/OpenCoreEMR/FrontController/RouteConfig.php
- index.php
🧰 Additional context used
🧬 Code graph analysis (6)
tests/scripts/test_compatibility.sh (1)
tests/scripts/test_security.sh (1)
run_test(52-77)
home.php (3)
src/OpenCoreEMR/FrontController/Router.php (8)
Router(20-136)determineSiteId(54-61)extractRoute(40-49)handleTrailingSlash(68-78)isForbiddenPath(83-86)requiresAdmin(91-94)resolveTargetFile(99-119)getSiteId(132-135)src/OpenCoreEMR/FrontController/SecurityValidator.php (4)
SecurityValidator(20-53)logSecurityEvent(41-44)isIncludeFile(33-36)getDebugMessage(49-52)src/Events/FrontController/FrontControllerEvent.php (2)
FrontControllerEvent(23-111)getSiteId(61-64)
tests/Tests/FrontController/CompatibilityTest.php (1)
tests/Tests/FrontController/SecurityTest.php (1)
setUpBeforeClass(30-42)
src/OpenCoreEMR/FrontController/Router.php (3)
src/Events/FrontController/FrontControllerEvent.php (3)
__construct(39-44)getRoute(51-54)getSiteId(61-64)src/OpenCoreEMR/FrontController/RouteConfig.php (5)
__construct(43-46)RouteConfig(20-236)isDeprecated(148-151)isForbidden(126-129)requiresAdmin(137-140)src/OpenCoreEMR/FrontController/RouteConfigInterface.php (3)
isDeprecated(65-65)isForbidden(49-49)requiresAdmin(57-57)
security-check.php (1)
library/classes/thumbnail/ThumbnailGenerator.php (1)
error_log(212-216)
tests/Tests/FrontController/SecurityTest.php (1)
tests/Tests/FrontController/CompatibilityTest.php (1)
setUpBeforeClass(29-40)
🪛 LanguageTool
README-FRONT-CONTROLLER.md
[uncategorized] ~139-~139: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...lemetry - Custom authentication layer - Rate limiting - Request preprocessing **Example - Re...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
README-FRONT-CONTROLLER.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (28)
security-check.php (1)
20-24: LGTM!The sanitization properly prevents log injection by:
- Using
basename()to avoid path disclosure- Removing control characters
- Limiting length to 255 characters
The generic exit message avoids information leakage.
src/OpenCoreEMR/FrontController/SecurityValidator.php (4)
25-28: LGTM!Strict CLI detection using
php_sapi_name() === 'cli'is the correct approach.
33-36: LGTM!The regex correctly matches both
.incand.inc.phpfiles (case-insensitive), addressing the security requirement.
41-44: LGTM!Centralized security logging with a consistent prefix is good practice.
49-52: LGTM!The environment variable name
OPENEMR_FC_LOG_LEVELuses the correct single underscore convention, matching the documented pattern.src/OpenCoreEMR/FrontController/Router.php (6)
27-35: LGTM!The constructor properly validates that
realpath($baseDir)succeeds before proceeding, throwing a clear exception if the directory is invalid. This addresses the previous review concern.
40-49: LGTM!Route extraction using
strtok()correctly strips the query string. Validation happens in later methods.
54-61: LGTM!Multisite detection with a sensible default and explicit parameter setting for downstream compatibility.
83-86: LGTM!Clean delegation to configuration with appropriate OR logic for deprecated and forbidden checks.
91-94: LGTM!Clean delegation to configuration for admin path detection.
99-119: LGTM!Robust target file resolution with:
realpath()for path normalization- Path traversal prevention using
str_starts_with()withDIRECTORY_SEPARATOR- File existence and extension validation
This addresses previous review concerns about path traversal security.
phpunit.xml (1)
30-30: LGTM!The FrontController test suite is properly configured, following the same pattern as existing test suites. It's included in both the main
openemrsuite and as a standalone suite for selective execution.Also applies to: 61-63
home.php (7)
23-27: LGTM!Standard dependency loading pattern. Using
safeLoad()is appropriate to avoid overriding existing environment variables.
30-30: LGTM!The environment variable name
OPENEMR_ENABLE_FRONT_CONTROLLERuses the correct single underscore, matching the documented convention.
30-77: LGTM with minor observation.Pass-through mode correctly:
- Blocks
.incand.inc.phpfiles- Checks for path traversal (
..and null bytes)- Only routes
.phpfiles- Updates server variables for correct execution context
Note: The path traversal check at line 35 uses string matching. While effective for basic cases,
realpath()validation (used in the main flow) is more robust. Consider adding it here if not relying on Apache/Nginx to filter these already.
79-85: LGTM!Good extension point for modules with defensive checks for kernel existence.
88-108: LGTM!Clean routing setup with proper separation of concerns:
- Multisite detection
- Trailing slash handling
- Forbidden path checks
- Admin flag propagation via
$_SERVER
110-119: LGTM!Consistent
.inc/.inc.phpblocking with:
- Centralized validation via
SecurityValidator- Appropriate 403 response
- Security event logging
- Conditional debug message detail
122-163: LGTM!Robust error handling with:
- Null safety checks
- Shutdown function to ensure late events fire even on early exit
- Try-catch with logging and re-throw to preserve normal error handling
- Context propagation for observability
The error handling architecture is well-designed for production use.
tests/Tests/FrontController/SecurityTest.php (5)
59-74: Test logic is sound but depends onloadVulnerableIncFilesfix.Once the base path issue in
loadVulnerableIncFilesis resolved, this test will correctly validate that.inc.phpfiles return 403.
79-92: Test logic correctly handles both Apache and nginx responses.The dual acceptance of 403 (blocked) or 500 (execution failure) appropriately handles different web server configurations.
124-189: File validation tests are well-structured.The tests for non-PHP files, non-existent files, and legitimate PHP file access correctly validate the front controller's security boundaries.
198-210: Appropriate skip with clear rationale.Skipping this test is correct since environment variable control requires infrastructure not available in the test environment. The comment provides clear guidance for manual testing.
215-232: Report generation logic is correct.The security report generation properly handles directory creation and JSON formatting. This provides valuable audit artifacts.
tests/Tests/FrontController/CompatibilityTest.php (4)
29-40: Setup is consistent with SecurityTest.The base URL resolution and HTTP client configuration follow the same pattern as
SecurityTest.php, ensuring consistent test behavior across the suite.
42-197: Core compatibility tests are well-designed.The test methods for index access, login, multisite, API routing, portal access, OAuth2, static assets, and patient workflows provide comprehensive coverage of backward compatibility requirements.
240-254: Custom module accessibility check is appropriate.Testing that custom modules in
sites/default/custom/are not blocked (expecting 404 or 200, but not 403) correctly validates the routing rules.
281-318: File upload and report generation tests are sound.The test for file upload paths correctly expects authentication checks rather than outright blocking. The compatibility report generation follows the same good pattern as
SecurityTest.php.
| - name: Upload FrontController test results to Codecov | ||
| if: ${{ !cancelled() }} | ||
| uses: codecov/test-results-action@v1 | ||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
|
|
There was a problem hiding this comment.
Add missing parameters to Codecov upload.
The FrontController test results upload is missing critical parameters that all other test uploads include:
filesparameter should specify the junit file (e.g.,junit-frontcontroller.xml)flagsparameter should categorize results (e.g.,frontcontroller,php${{ steps.parse.outputs.php }},...)
Apply this diff to align with other test uploads:
- name: Upload FrontController test results to Codecov
if: ${{ !cancelled() }}
uses: codecov/test-results-action@v1
with:
token: ${{ secrets.CODECOV_TOKEN }}
+ files: junit-frontcontroller.xml
+ flags: frontcontroller,php${{ steps.parse.outputs.php }},${{ steps.parse.outputs.webserver }},${{ steps.parse.outputs.database }}${{ steps.parse.outputs.db }}📝 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.
| - name: Upload FrontController test results to Codecov | |
| if: ${{ !cancelled() }} | |
| uses: codecov/test-results-action@v1 | |
| with: | |
| token: ${{ secrets.CODECOV_TOKEN }} | |
| - name: Upload FrontController test results to Codecov | |
| if: ${{ !cancelled() }} | |
| uses: codecov/test-results-action@v1 | |
| with: | |
| token: ${{ secrets.CODECOV_TOKEN }} | |
| files: junit-frontcontroller.xml | |
| flags: frontcontroller,php${{ steps.parse.outputs.php }},${{ steps.parse.outputs.webserver }},${{ steps.parse.outputs.database }}${{ steps.parse.outputs.db }} |
🤖 Prompt for AI Agents
.github/workflows/test.yml around lines 835 to 840: the Codecov step is missing
the files and flags parameters; update this step to include files: the junit
file for FrontController (e.g., junit-frontcontroller.xml) and flags: a
comma-separated set that includes at least frontcontroller and a PHP-version tag
using the parsed output (e.g., frontcontroller,php${{ steps.parse.outputs.php
}}); ensure the flags format matches other test uploads in the workflow so
results are categorized consistently.
| ``` | ||
| interface/patient_file/history/history.inc.php | ||
| Fatal error: Call to undefined function xl() | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to code block.
The fenced code block at line 10 should specify a language (e.g., text or log) for proper syntax highlighting.
Apply this diff:
-```
+```text
interface/patient_file/history/history.inc.php
Fatal error: Call to undefined function xl()
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In README-FRONT-CONTROLLER.md around lines 10 to 13, the fenced code block is
missing a language identifier; update the opening fence from totext so
the block becomes a text/log block (i.e., replace the existing opening triple
backticks with ```text and leave the inner lines unchanged), ensuring proper
syntax highlighting.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
tests/scripts/test_compatibility.sh (4)
227-228: Inconsistent test counting—delegate to run_test helper.The manual
TOTAL_TESTSincrement duplicates logic already inrun_test(). Consider refactoring this POST test block to use the helper function for consistency.
25-34: Lettputfailures fall back instead of aborting.With
set -e, any non-zerotput(common when TERM lacks color support) terminates the suite before tests run. Swallow those failures and fall back to plain text.# Colors for output using tput (with fallback for non-interactive environments) -if command -v tput >/dev/null 2>&1 && [[ -n "${TERM}" ]]; then - RED=$(tput setaf 1) - GREEN=$(tput setaf 2) - NC=$(tput sgr0) # No Color +if command -v tput >/dev/null 2>&1 && [[ -n "${TERM:-}" ]]; then + RED=$(tput setaf 1 2>/dev/null || printf '') + GREEN=$(tput setaf 2 2>/dev/null || printf '') + NC=$(tput sgr0 2>/dev/null || printf '') + if [[ -z "${RED}" || -z "${GREEN}" || -z "${NC}" ]]; then + RED=""; GREEN=""; NC="" + fi else RED="" GREEN="" NC="" fi
54-79: Fix status-code matching to avoid false positives.
[[ "${expected_codes}" = *"${http_code}"* ]]treats000and other garbage codes as success because they are substrings of200, so we miss real outages.- # Check if code matches any of the expected codes - if [[ "${expected_codes}" = *"${http_code}"* ]]; then + # Check if code matches any of the expected codes (as whole token, not substring) + if [[ " ${expected_codes} " == *" ${http_code} "* ]]; then
234-242: Fail POST compatibility tests on 5xx/connection errors.Any 5xx (or 000 curl error) currently lands in the "PASS" branch, so a broken login POST can still count as success. Tighten the check so unexpected 3-digit codes and connection errors fail the test.
-if [[ "${http_code}" = "404" || "${http_code}" = "403" ]]; then +if [[ "${http_code}" = "404" || "${http_code}" = "403" || "${http_code}" = "000" || "${http_code}" =~ ^5 ]]; then printf "%sFAIL%s (HTTP %s)\n" "${RED}" "${NC}" "${http_code}" | tee -a "${REPORT_FILE}" FAILED_TESTS=$((FAILED_TESTS + 1)) echo " ✗ POST requests are being blocked" | tee -a "${REPORT_FILE}" elsetests/scripts/test_security.sh (1)
15-17: Addset -o pipefailto honor pipeline failures.Line 15 has
set -e, but the script uses pipes extensively (e.g.,| tee). Withoutset -o pipefail, errors in the piped command are masked. ShellCheck flags this.Apply this diff:
set -e +set -o pipefail # Ensure errexit is inherited in command substitutions (addresses SC2311) shopt -s inherit_errexit 2>/dev/null || trueBased on pipeline failures
tests/scripts/test_performance.sh (2)
52-60: Add missingbcdependency check (critical blocker).The script uses
bcfor floating-point arithmetic starting at line 67 (e.g.,bc -l <<< "${overhead} < 5"), but there is no check to ensurebcis available. Ifbcis not installed, the script will fail with a cryptic error at runtime. Add a dependency check alongside theabcheck using the same pattern.# Check if Apache Bench (ab) is available if ! command -v ab &> /dev/null; then { printf "%sERROR: Apache Bench (ab) not available. Please install it manually.%s\n" "${RED}" "${NC}" echo " macOS: brew install httpd" echo " Linux: sudo apt-get install apache2-utils" } | tee -a "${REPORT_FILE}" exit 1 fi + +# Check if bc (basic calculator) is available for floating-point arithmetic +if ! command -v bc &> /dev/null; then + { + printf "%sERROR: bc (basic calculator) not available. Please install it.%s\n" "${RED}" "${NC}" + echo " macOS: brew install bc" + echo " Linux: sudo apt-get install bc" + } | tee -a "${REPORT_FILE}" + exit 1 +fi
120-128: Validateaboutput format before parsing metrics.The metric extraction uses
|| echo "0"fallback whenawkfails, which masks parsing errors and distinguishes poorly between legitimate zero values and extraction failures. Previous reviews flagged this; the validation on lines 243–248 occurs too late (after metrics are already captured). Validate theaboutput contains expected sections immediately after theabcommand succeeds, before extracting metrics.# 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}" "${url}" "${NC}" echo "Output: ${ab_output}" } | tee -a "${REPORT_FILE}" return 1 fi + + # Validate ab output contains expected sections + if ! printf '%s\n' "$ab_output" | grep -q "Requests per second"; then + { + printf "%sWARNING: Unexpected ab output format (missing 'Requests per second' section)%s\n" "${RED}" "${NC}" + } | tee -a "${REPORT_FILE}" + return 1 + fi # Extract key metrics using awk with safe defaults - 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") + requests_per_sec=$(printf '%s\n' "$ab_output" | awk '/Requests per second/ {print $4}') + time_per_request=$(printf '%s\n' "$ab_output" | awk '/Time per request.*mean\)/ {print $4; exit}') + failed_requests=$(printf '%s\n' "$ab_output" | awk '/Failed requests/ {print $3}') # Extract percentiles using awk with safe defaults - 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") + p50=$(printf '%s\n' "$ab_output" | awk '/50%/ {print $2}') + p95=$(printf '%s\n' "$ab_output" | awk '/95%/ {print $2}') + p99=$(printf '%s\n' "$ab_output" | awk '/99%/ {print $2}')
🧹 Nitpick comments (3)
tests/scripts/test_security.sh (1)
24-33: Suppress tput errors to prevent abort underset -ein CI environments.The current check for
tputandTERMis reasonable, but in CI (e.g.,TERM=dumb) tput can still fail with exit code 1, causing the script to abort despite the fallback. Suppress individual tput errors so the script gracefully degrades to plain output.Apply this diff:
if command -v tput >/dev/null 2>&1 && [[ -n "${TERM}" ]]; then - RED=$(tput setaf 1) - GREEN=$(tput setaf 2) - NC=$(tput sgr0) # No Color + RED=$(tput setaf 1 2>/dev/null || true) + GREEN=$(tput setaf 2 2>/dev/null || true) + NC=$(tput sgr0 2>/dev/null || true) + # Fall back to plain text if any tput call failed (returned empty) + if [[ -z "${RED}" ]] || [[ -z "${GREEN}" ]] || [[ -z "${NC}" ]]; then + RED=""; GREEN=""; NC="" + fi else RED="" GREEN="" NC="" fitests/scripts/test_performance.sh (2)
16-18: Use portable error handling instead of bash-specificinherit_errexit.Line 18 uses
shopt -s inherit_errexit, which is bash-specific and not available in POSIXshordash. While the fallback|| trueprevents script failure, it silently disables the fix on non-bash systems, reintroducing the SC2311 issues (command substitutions not inheritingset -e). A more portable approach is to explicitly check return codes on critical command assignments (e.g., usingif ! var=$(cmd); then exit 1; fi), similar to the existing pattern inrun_performance_testat lines 112–118.For critical calls like lines 154–157, 171–174, 188–191, 206–209, and 227–230, explicitly guard them:
-baseline=$(run_performance_test \ - "Direct index.php access" \ - "${BASE_URL}/index.php" \ - "Direct file access without front controller") +if ! baseline=$(run_performance_test \ + "Direct index.php access" \ + "${BASE_URL}/index.php" \ + "Direct file access without front controller"); then + { + printf "%sERROR: Baseline performance test failed%s\n" "${RED}" "${NC}" + } | tee -a "${REPORT_FILE}" + exit 1 +fi(Apply the same pattern to all other
run_performance_testassignments.)
243-248: Improve regex to handle all numeric formats (optional refinement).The numeric validation regex on line 245 (
^[0-9]+\.?[0-9]*$) does not match negative numbers (e.g.,-5.2). Although performance metrics are typically non-negative, consider using a more inclusive pattern for robustness:^-?[0-9]+\.?[0-9]*$.- if ! [[ "${baseline_time}" =~ ^[0-9]+\.?[0-9]*$ ]] || ! [[ "${fc_time}" =~ ^[0-9]+\.?[0-9]*$ ]]; then + if ! [[ "${baseline_time}" =~ ^-?[0-9]+\.?[0-9]*$ ]] || ! [[ "${fc_time}" =~ ^-?[0-9]+\.?[0-9]*$ ]]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
security-check.php(1 hunks)tests/scripts/test_compatibility.sh(1 hunks)tests/scripts/test_performance.sh(1 hunks)tests/scripts/test_security.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- security-check.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/scripts/test_compatibility.sh (1)
tests/scripts/test_security.sh (1)
run_test(52-77)
tests/scripts/test_security.sh (1)
tests/scripts/test_compatibility.sh (1)
run_test(54-79)
🔇 Additional comments (2)
tests/scripts/test_security.sh (2)
52-77: run_test function implementation looks good.The helper properly captures HTTP status codes, compares against expected values, and logs results to both terminal and report file. Variable bracing and error handling are correct.
79-251: Test categories and report structure are well-organized.The six test categories are clearly delineated with headers, each test case has a descriptive message, and the final report properly summarizes results. The feature flag test (lines 211–224) correctly accepts both 200 and 302 responses. Exit codes align with test outcomes.
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
Documentation
Tests
Chores