Security check report for housekeeping#476
Open
deseven wants to merge 8 commits into
Open
Conversation
Guard the /-/housekeeping/security-check endpoint with a
has_permission("ADMIN") check and hide the corresponding panel in
the housekeeping template from non-admin users to avoid leaking
configuration details and active plugin names.
Replace the inline-styled severity badges in the housekeeping security check with halfmoon's badge classes. Add badge-warning and badge-notice variants in otterwiki.css since halfmoon ships only primary/success/secondary/danger, so dark-mode and theme colors are picked up consistently.
Replace the regex-based comment stripper with a BeautifulSoup parse so custom HTML detection correctly ignores comments and whitespace while still treating tag-only content (e.g. a bare script or meta tag) as real custom HTML. Yeah, this is nit picking.
Extend SecurityCheckResult with a `passed` flag and have the
pass/fail backend and frontend checks report both states. The
endpoint now returns {"issues": [...], "passed": [...]}, and the
housekeeping panel renders a second table behind a "Show passed
checks (N)" toggle that is collapsed by default. Admin gating is
also applied to the script block so non-admins do not receive the
(useless) JS.
REAL_IP_FROM is only relevant for the docker image bundled with nginx, so flagging it on a generic install is misleading. Keep the two generic signals (non-private remote with no proxy headers, and X-Forwarded-For without X-Forwarded-Proto) and the documentation links.
When the wiki is accessed via a loopback host (localhost or 127.0.0.0/8, ::1), the anonymous write/upload and HTTPS checks drop from CRITICAL to MEDIUM and append a note explaining the downgrade and that the wiki may still be reachable via tunnel or port forward.
Owner
|
Thanks for the PR, this is a really nice addition. I went ahead and pushed a few follow-up commits with the changes I would otherwise have asked for in review:
Happy to iterate on any of these if you'd prefer a different |
Contributor
Author
|
Makes sense to me and sorry about not checking for admin, I keep forgetting that this page is available for regular users too because of the drafts functionality. And no idea why I haven't added the admin check for the endpoint. Security check with fatal security flaws 👌 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added a security check section for Housekeeping.
Implemented checks:
Most of the checks are performed on the backend side with a couple running on the frontend instead since we need to see the situation from the outside. Backend checks are solidly organized and easily extensible I think, while the frontend ones are a bit hacky, so if someone has any better ideas please share (not a blocker, though - could be improved later).