Enhancing Security, Error Handling, and Performance in Nginx for Moodle #75
Conversation
WalkthroughA new Nginx configuration file for the Moodle environment has been introduced. The file sets custom error pages for 404 and 403 responses, redirects forbidden errors to an error handler, and restricts access to specific paths and sensitive files (including fixtures, dot files not under well-known URIs, vendor and node modules). Additionally, an internal alias is defined to offload file serving from PHP for resources under the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Nginx
participant DataAlias as "Internal Alias (/dataroot)"
participant ErrorPage as "Error Handler (/error/index.php)"
Client->>Nginx: Sends HTTP request
alt Request for file in /dataroot/
Nginx->>DataAlias: Retrieve static file
DataAlias-->>Nginx: Returns file content
Nginx-->>Client: Delivers file content
else Request for restricted/sensitive resource
Nginx-->>Client: Returns 404 error (or 403 as 404)
else Other requests
Nginx-->>Client: Processes request normally
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
rootfs/etc/nginx/server-conf.d/moodle.conf (3)
5-9: Optimize the fixtures block by removing redundant directive.The
deny alldirective is redundant when followed byreturn 404. The return directive alone is sufficient.location ~ /fixtures/ { - deny all; return 404; }
1-60: Consider implementing pre and post location directives.As mentioned in the PR objectives, future updates plan to introduce
preandpostlocation directives. This would enhance configuration flexibility by allowing:
- Custom configurations before the main security rules
- Override capabilities after the main security rules
- Better organization of environment-specific rules
Consider this structure:
# Pre-security rules include /etc/nginx/server-conf.d/pre/*.conf; # Main security rules include /etc/nginx/server-conf.d/moodle.conf; # Post-security rules include /etc/nginx/server-conf.d/post/*.conf;
24-40:❓ Verification inconclusive
Optimize XML blocks and verify coverage.
- The
deny alldirectives are redundant when followed byreturn 404.- Verify if there are other XML files that should be protected.
location = /admin/environment.xml { - deny all; return 404; } location = /lib/db/install.xml { - deny all; return 404; } location = /mod/assign/db/install.xml { - deny all; return 404; }Let's search for other potential XML files that might need protection:
🏁 Script executed:
#!/bin/bash # Description: Find other XML files in the codebase fd -e xmlLength of output: 11
Optimize XML blocks and manually verify XML file coverage.
- The
deny alllines are redundant when immediately followed byreturn 404; they can be removed.- The automated search using
fd -e xmldid not return any additional XML files. However, this result may be incomplete, so please manually verify that no other XML files require similar protection.location = /admin/environment.xml { - deny all; return 404; } location = /lib/db/install.xml { - deny all; return 404; } location = /mod/assign/db/install.xml { - deny all; return 404; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rootfs/etc/nginx/server-conf.d/moodle.conf(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: buildx
🔇 Additional comments (4)
rootfs/etc/nginx/server-conf.d/moodle.conf (4)
1-3: LGTM! Well-implemented error handling.Good security practice to rewrite 403 (Forbidden) to 404 (Not Found) responses, preventing information disclosure about restricted resources.
42-45: LGTM! Well-implemented dotfiles protection.Excellent implementation that correctly follows RFC 5785 by allowing
.well-knownURIs while protecting all other dotfiles.
54-58:✅ Verification successful
LGTM! Well-implemented file serving optimization.
Excellent implementation that:
- Offloads static file serving from PHP to Nginx for better performance
- Uses
internaldirective to prevent direct access- Correctly aliases to the Moodle data directory
Let's verify the Moodle data directory path:
🏁 Script executed:
#!/bin/bash # Description: Check if the Moodle data directory path is configurable grep -r "moodledata" .Length of output: 340
LGTM! Verified and Consistent Configuration
The file serving configuration for
/dataroot/is correctly implemented:
- The
internaldirective properly restricts access.- The alias to
/var/www/moodledata/$1is consistent across the Nginx config, Docker entrypoint, and Compose settings, ensuring that static file serving is efficiently offloaded from PHP.No further changes are required.
47-52:✅ Verification successful
Optimize internal files block and verify Moodle issue.
- The
deny alldirective is redundant when followed byreturn 404.- Let's verify the referenced Moodle issue MDL-69333 for completeness.
location ~ (/vendor/|/node_modules/|composer\.json|/readme|/README|readme\.txt|README\.txt|upgrade\.txt|UPGRADING\.md|UPGRADING\-CURRENT\.md|db/install\.xml|/fixtures/|/behat/|phpunit\.xml|\.lock|environment\.xml|doesnotexist|CHANGELOG|CHANGELOG\.md|TRADEMARK\.txt|PULL_REQUEST_TEMPLATE\.txt|INSTALL\.txt|CONTRIBUTING\.txt|behat\.yml\.dist|phpunit\.xml\.dist|package\.json|npm-shrinkwrap\.json|security\.txt|index\.php\.inc) { - deny all; return 404; }Let's verify the Moodle issue:
🌐 Web query:
What is Moodle issue MDL-69333 about?💡 Result:
Moodle issue MDL-69333 is a security enhancement introduced in Moodle 3.11 aimed at reducing the ability for attackers to "fingerprint" a server by blocking access to sensitive directories and files that could reveal technical details about the system[1][3][10].
Key Details:
- Purpose: Prevents unauthorized users from identifying server software versions or configurations by restricting access to paths like
/vendor/,/node_modules/, and files such ascomposer.json,.lock, orenvironment.xml[1][7][10].- Implementation: Requires web server (Apache/Nginx) configuration updates to return 404 errors for restricted paths instead of allowing directory listings or forbidden (403) responses[1][7]. Example Apache rules include:
RewriteRule "(\/vendor\/)" - [F] RewriteRule "(composer\.json)" - [F]- Impact: Failing these security checks in Moodle’s admin interface indicates that sensitive paths are still accessible, requiring server adjustments[1][7].
Why It Matters:
This fix addresses vulnerabilities where exposed files/directories could help attackers exploit known weaknesses in dependencies or server setups[1][10]. For example, blocking
/vendor/prevents access to dependency manager files that might reveal outdated libraries[7].Troubleshooting Tips:
- Verify URLs like
https://yoursite.com/vendor/return 404[1].- Ensure the Moodle server can resolve its own domain (e.g., via
/etc/hosts) to avoid false negatives during self-checks[1].- Use tools like
curlor check Apache/Nginx logs for requests from "MoodleBot" during security scans[1].This issue highlights Moodle’s focus on hardening server configurations against reconnaissance attacks[3][10].
Citations:
- 1: https://moodle.org/mod/forum/discuss.php?d=422393
- 2: https://moodledev.io/general/development/tracker
- 3: https://docs.moodle.org/3x/fr/Notes_de_mise_%C3%A0_jour_de_Moodle_3.11
- 4: https://moodle.org/mod/forum/discuss.php?d=105891
- 5: https://moodledev.io/general/development/tracker/guide
- 6: https://moodledev.io/general/releases/2.2/2.2.5
- 7: Plugin not serving js files best practice way blindsidenetworks/moodle-tinymce_recordrtc#33
- 8: https://totara.community/mod/forum/discuss.php?d=18008
- 9: https://docs.moodle.org/4x/sv/Nginx
- 10: https://moodledev.io/general/releases/3.11
Optimize the internal files protection block per MDL-69333 requirements
- The Moodle security issue MDL-69333 mandates returning a 404 error for sensitive paths to prevent server fingerprinting. The web query confirms that using
return 404;aligns with the security enhancement introduced in Moodle 3.11.- The
deny all;directive becomes redundant when followed immediately by areturn 404;statement. Streamlining the configuration improves clarity while preserving the intended security behavior.location ~ (/vendor/|/node_modules/|composer\.json|/readme|/README|readme\.txt|README\.txt|upgrade\.txt|UPGRADING\.md|UPGRADING\-CURRENT\.md|db/install\.xml|/fixtures/|/behat/|phpunit\.xml|\.lock|environment\.xml|doesnotexist|CHANGELOG|CHANGELOG\.md|TRADEMARK\.txt|PULL_REQUEST_TEMPLATE\.txt|INSTALL\.txt|CONTRIBUTING\.txt|behat\.yml\.dist|phpunit\.xml\.dist|package\.json|npm-shrinkwrap\.json|security\.txt|index\.php\.inc) { - deny all; return 404; }This change both simplifies the configuration and meets the security recommendations.
| # TODO: Temporary block exact match for /privacy/tests/fixtures/logo.png (see issue #74) | ||
| location = /privacy/tests/fixtures/logo.png { | ||
| deny all; | ||
| return 404; | ||
| } | ||
|
|
||
| # TODO: Temporary block exact match for /enrol/lti/tests/fixtures/input.xml (see issue #74) | ||
| location = /enrol/lti/tests/fixtures/input.xml { | ||
| deny all; | ||
| return 404; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🛠️ Refactor suggestion
🧩 Analysis chain
Address TODO comments and optimize temporary blocks.
- These blocks are marked as temporary fixes for issue Public paths error #74. Consider implementing a more permanent solution.
- The
deny alldirectives are redundant when followed byreturn 404.
location = /privacy/tests/fixtures/logo.png {
- deny all;
return 404;
}
location = /enrol/lti/tests/fixtures/input.xml {
- deny all;
return 404;
}Let's verify the status of issue #74 to understand if a permanent solution is being tracked:
🏁 Script executed:
#!/bin/bash
# Description: Check the status of issue #74
gh issue view 74Length of output: 513
Update temporary fixture blocks per closed issue #74 and optimize directives.
- Issue Public paths error #74 ("Public paths error") is now closed, so please review whether these temporary location blocks are still needed. If a permanent fix has been implemented, consider removing these blocks entirely.
- If the blocks are retained as a fallback, note that using both
deny allandreturn 404is redundant. Removingdeny allensures clarity, as shown below.
location = /privacy/tests/fixtures/logo.png {
- deny all;
return 404;
}
location = /enrol/lti/tests/fixtures/input.xml {
- deny all;
return 404;
}📝 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.
| # TODO: Temporary block exact match for /privacy/tests/fixtures/logo.png (see issue #74) | |
| location = /privacy/tests/fixtures/logo.png { | |
| deny all; | |
| return 404; | |
| } | |
| # TODO: Temporary block exact match for /enrol/lti/tests/fixtures/input.xml (see issue #74) | |
| location = /enrol/lti/tests/fixtures/input.xml { | |
| deny all; | |
| return 404; | |
| } | |
| # TODO: Temporary block exact match for /privacy/tests/fixtures/logo.png (see issue #74) | |
| location = /privacy/tests/fixtures/logo.png { | |
| return 404; | |
| } | |
| # TODO: Temporary block exact match for /enrol/lti/tests/fixtures/input.xml (see issue #74) | |
| location = /enrol/lti/tests/fixtures/input.xml { | |
| return 404; | |
| } |
This pull request refines the
moodle.confNginx configuration by incorporating security enhancements, improved error handling, and optimized file serving. The changes are primarily based on this commit from @jimsihk’s fork, with additional refinements and a monkey patch to properly handle fixture files. These modifications ensure a more secure and efficient Moodle deployment.🔒 Security Enhancements & Error Handling
/error/index.php)./fixtures/,/privacy/tests/fixtures/logo.png,/enrol/lti/tests/fixtures/input.xml/admin/environment.xml,/lib/db/install.xml,/mod/assign/db/install.xml/.well-known/per [RFC 5785](https://datatracker.ietf.org/doc/html/rfc5785).🚀 Performance Optimization
/dataroot/files to Nginx, reducing PHP workload in line with Moodle’s recommended configuration.🔗 Reference to Issue #74
This PR also addresses concerns raised in Issue #74, where public paths were incorrectly accessible due to improper handling of
environment/publicpaths.php. Instead of modifying internal Moodle files, Nginx now enforces proper restrictions.🛠 Additional Improvements
.confdirectory support: As part of this update,.conffiles can now be placed insiderootfs/etc/nginx/server-conf.d/, reducing the need for direct.shpatching (see reference).erseco/alpine-php-webserverwill introducepreandpostlocation directives for more flexible configuration handling.🎉 Special Thanks
A huge thanks to @jimsihk for the insightful fork and suggestions! Your contributions significantly improved the security and maintainability of this setup. 🙌
Summary by CodeRabbit