Matrix Notification Plugin#106
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Warning Review limit reached
More reviews will be available in 41 minutes and 56 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughA new Matrix notification plugin is introduced for BunkerWeb, enabling the system to send attack notifications to a configured Matrix room. The implementation includes core Lua logic, plugin configuration, documentation, and UI integration for health monitoring. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@matrix/matrix.lua`:
- Around line 106-136: The send path currently continues after http_new() or
httpc:request_uri() failures, which causes httpc:close() and res.status
dereferences on nil; update the logic in the Matrix send function(s) to
immediately return an error when http_new() fails (http_new -> httpc nil) and
likewise after request_uri() fails (res nil), ensuring you do not call
httpc:close() or access res.status in those failure branches; specifically, in
the code using http_new(), httpc, httpc:request_uri, httpc:close and res.status
(also apply the same fix to the second send path around the block referenced at
185-203), log the error and return early before touching httpc or res.
- Around line 75-85: When MATRIX_INCLUDE_HEADERS is enabled in matrix.lua, avoid
forwarding raw headers in the loop that appends to data["formatted_body"] and
data["body"]; instead implement an allow-list (e.g., only include common safe
headers like "User-Agent", "Accept", "Host") or explicitly redact sensitive
names (e.g., "Authorization", "Cookie", "Set-Cookie", "X-CSRF-Token",
"Proxy-Authorization", "Authentication") before serialising, and in the for
header,value in pairs(headers) block replace sensitive values with a placeholder
like "[REDACTED]" (or skip adding those headers entirely) so no bearer tokens,
cookies, CSRF tokens, or secrets are sent to Matrix.
- Around line 48-50: The country lookup fallback has a typo and uses an
undefined variable: replace "elf.logger" with "self.logger", reference the IP
with "self.ctx.bw.remote_addr" in the log message (instead of "remote_addr"),
and ensure you set a safe fallback value (e.g. assign country = "Country
unknown") after logging so callers of get_country handling the nil case don't
raise; look for get_country, self.ctx.bw.remote_addr, self.logger and the local
variable country in the surrounding code to apply these changes.
- Around line 124-131: The Matrix HTTP calls use httpc:request_uri() without
timeouts; before each request_uri call (e.g., the PUT call that sends post_data
to url using access_token and the other request_uri call later) call
httpc:set_timeout(1000) or httpc:set_timeouts(...) to enforce a 1s (or
configured) timeout so the worker thread won't block; update both places that
call httpc:request_uri() to set the timeout on the httpc instance immediately
before invoking request_uri().
- Around line 111-115: The txn_id currently set via tostring(os.time()) is not
collision-resistant; replace it with a per-send unique id and update both places
where txn_id is generated/used (the variable named txn_id in matrix.lua). For
example, build txn_id by concatenating os.time() with a higher-resolution or
unique per-request value: use ngx.var.msec if available (tostring(os.time()) ..
"-" .. ngx.var.msec), or ngx.var.request_id when present, or combine
ngx.now()*1000 with a short random suffix to guarantee uniqueness across rapid
successive sends; ensure both locations that set txn_id use the same new
generation logic so Matrix idempotency keys are unique per send.
- Around line 70-85: Sanitise and flatten request-derived inputs before
concatenating into data["formatted_body"] and data["body"]: run request_method,
remote_addr, request_host, self.ctx.bw.uri, reason, reason_data and every header
name/value through an HTML-escape helper (e.g., escape_html) for formatted_body
and tostring for plain body, and when reading headers from ngx_req.get_headers()
handle table values by flattening with table.concat(v, ", ") (or tostring on
non-tables) so duplicate headers do not cause concatenation errors; apply this
transformation where data["formatted_body"] and data["body"] are built and when
iterating headers returned by ngx_req.get_headers().
In `@matrix/README.md`:
- Around line 5-15: The Table of contents contains a stale entry linking to
"#todo" that does not exist; open the README.md section containing the bulleted
list (the lines under "Table of contents") and remove the "- [TODO](`#todo`)" list
item so the TOC matches the actual headings and keeps the documentation concise
and accurate.
- Around line 34-50: Update the Docker examples in matrix/README.md to reference
the 1.6 image series: replace all occurrences of the image tag
"bunkerity/bunkerweb:1.5.10" (seen in the bunkerweb service blocks) with the
appropriate 1.6 tag (e.g., "bunkerity/bunkerweb:1.6" or the exact 1.6.x release
you intend to document) so examples match this PR's 1.6 compatibility; ensure
both instances mentioned in the diff are updated.
- Around line 86-93: The settings table in README.md has drifted from
matrix/plugin.json; rerun the JSON-to-Markdown generator
(.tests/misc/json2md.py) against matrix/plugin.json to regenerate the table (so
defaults like MATRIX_ACCESS_TOKEN show as an empty string), replace the manually
edited table in README.md with the generated output, and commit the regenerated
README instead of hand-editing the table.
In `@matrix/ui/actions.py`:
- Around line 18-21: The code assigns str(e) to ret["error"], exposing
potentially sensitive details; change this so the full exception remains only in
server logs (keep logger.debug(format_exc()) and logger.error(f"Failed to get
matrix ping: {e}") for diagnostics) but replace ret["error"] with a generic,
non-sensitive marker such as "Unable to retrieve matrix ping" or
"internal_error" (update the assignment to ret["error"] accordingly) — locate
the try/except handling around the ping path where ret["error"] is set and
remove any raw exception string from the payload.
- Around line 29-30: The function matrix(**kwargs) currently returns None which
violates the plugin main-page contract; update the matrix function to return an
empty dict ({}). Locate the matrix function definition and replace the
pass/implicit None with a return {} so it matches other plugin entry points
(e.g., clamav, coraza, crowdsec, discord) and ensures callers receive a
dictionary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 753a0cbf-db03-4998-b08e-b65f9b264c6e
📒 Files selected for processing (6)
README.mdmatrix/README.mdmatrix/matrix.luamatrix/plugin.jsonmatrix/ui/actions.pymatrix/utils.lua
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Documentation should be concise, accurate, and written in British English:
- Keep the structure clear with a sensible heading hierarchy.
- Each plugin README contains a settings table generated from
plugin.jsonvia.tests/misc/json2md.py; regenerate it whenever settings change rather than hand-editing the table.- The repository README shows a compatibility badge tied to the plugins-collection version in
COMPATIBILITY.json— keep it in sync with any version bump.- Prefer concrete instructions, accurate examples, and explicit prerequisites (Docker,
sudo, required env vars likeVIRUSTOTAL_API_KEY).- When a PR changes behaviour, defaults, packaging, or security posture, ask for the corresponding documentation update.
Files:
README.mdmatrix/README.md
**/*.lua
⚙️ CodeRabbit configuration file
**/*.lua: Lua code runs on OpenResty inside the BunkerWeb nginx container and sits on the request hot path:
- Every plugin subclasses
bunkerweb.pluginviamiddleclass:local <name> = class("<name>", plugin)andplugin.initialize(self, "<id>", ctx)ininitialize. Hook methods (init_worker,access,log,preread, ...) must returnself:ret(ok_bool, msg, [http_status]). To deny a request, returnself:ret(true, "reason", utils.get_deny_status()).- Gate expensive work at
init_workerwithutils.has_variable("USE_<PLUGIN>", "yes")and skip whenself.is_loadingis true, matching the pattern used acrossclamav.lua,coraza.lua, andvirustotal.lua.- Use local variables and local module tables; avoid globals. Cache
ngx.var.*andngx.req.*values in locals instead of re-reading them repeatedly.- Precompile regular expressions in module-level locals; never compile inside request loops. For
ngx.re.match/find/gmatch/sub, pass the option string"jo"(jenables PCRE JIT,ocompiles the pattern once and caches it), anchor patterns with^...$when a full match is intended, and cap input length before matching to prevent ReDoS.- Validate and sanitise all request-derived input. Never evaluate request-derived code via
load,loadstring, or similar mechanisms.- Use
ngx.socketfor raw TCP (see the ClamAV INSTREAM pattern inclamav.lua) andresty.httpfor HTTP upstreams (seevirustotal.luaandcoraza.lua). Preferresty.uploadfor streaming request bodies —clamav.luais the reference.- Cache upstream scan results by a strong digest of the body (SHA-512 in
virustotal.lua) rather than by filename or weaker hashes; mind cache key length.- Shared-dictionary read-modify-write sequences are race-prone; prefer atomic operations such as
incror explicit locking where correctness matters.- Never log request bodies, cookies, bearer tokens, webhook secrets, or API keys.
- Use
pcallor explicit error...
Files:
matrix/utils.luamatrix/matrix.lua
**/plugin.json
⚙️ CodeRabbit configuration file
**/plugin.json:plugin.jsonfiles define the settings schema that BunkerWeb reads to register settings and render the UI:
- The top-level fields
id,name,version, andstream(yes/no/partial) must stay consistent with the plugin directory name and its Lua module.- Ensure setting IDs remain stable unless there is an intentional breaking change documented in the PR.
- Each setting must declare
context(globalormultisite),default,help,id,label,regex, andtype. Regex validators must be anchored where appropriate, compile cleanly, and avoid catastrophic backtracking. Default values must satisfy their own validators.USE_<PLUGIN>toggles are the standard gate forinit_workershort-circuiting; keep them in sync with the Lua implementation.- Bump the per-plugin
versionfield through./misc/update_version.sh <new_version>so everyplugin.jsonand the README badge move together. Do not edit the version in a singleplugin.jsonby hand.- If a PR changes a setting ID, type, context, accepted value shape, or compatibility behaviour, require migration notes and confirm that the README settings table is regenerated via
.tests/misc/json2md.py.
Files:
matrix/plugin.json
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Follow BunkerWeb's Python standards and security posture:
- Use snake_case for functions and variables, PascalCase for classes, and provide concise, accurate docstrings for public classes, functions, and methods.
- Respect Black formatting with a 160-character line limit (
pyproject.toml) and flake8 with--max-line-length=250 --ignore=E266,E402,E722,W503from.pre-commit-config.yaml.- Catch specific exceptions; never use bare
except:. CatchingBaseException/Exceptionis acceptable only at explicit process boundaries (for example UIpre_renderhooks that must degrade gracefully) when the code logs enough context and either re-raises, returns an error status, or terminates safely.ui/actions.pyusesBaseExceptiondeliberately — do not regress that to a bareexcept:.- Never invoke the shell from Python via
os.system(...),subprocesswithshell=True,eval, orexec. Pass subprocess arguments as a list and prefer explicit binary paths for privileged operations.- Do not use unsafe binary deserialisation libraries for untrusted data. Use
yaml.safe_load()rather than unsafe YAML loading.- Open files with an explicit encoding (normally
utf-8) and usewithstatements for files, sockets, and temporary resources.- For HTTP clients (
requests,httpx): always set an explicit timeout, validate destination URLs to block RFC1918/loopback/link-local ranges (SSRF), disable automatic redirects to internal hosts, and be careful with proxy settings.- Scrub secrets, tokens, cookies, database URIs, webhook URLs, and
Authorizationheaders from logs. Use the logging framework rather thanprint().- Secrets, API keys, and credentials must never be hard-coded; read them from plugin settings or environment variables.
Files:
matrix/ui/actions.py
*/ui/actions.py
⚙️ CodeRabbit configuration file
*/ui/actions.py: Per-pluginui/actions.pymodules hook into the BunkerWeb web UI:
pre_render(**kwargs)must return a dict of card data and must never raise out to the caller. The existing pattern wraps the body intry/except BaseExceptionand logs viagetLogger("UI")+format_exc(); preserve that posture.- The top-level function named after the plugin (e.g.
discord(**kwargs)) is the main-page entry point; keep it side-effect free unless the plugin genuinely needs server-side state.kwargs["bw_instances_utils"]exposes BW helpers such asget_ping(service); do not assume other keys exist without checking.- Never expose secrets, webhook URLs, API keys, or tokens in the returned card payload — they are rendered into the UI.
- Keep the module importable with only the BunkerWeb UI runtime; do not add heavy third-party dependencies without justification.
Files:
matrix/ui/actions.py
🪛 LanguageTool
matrix/README.md
[grammar] ~23-~23: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive.
Context: ...s token for the Matrix user you want to sent notifications from. - A room ID where n...
(TO_NON_BASE)
🪛 Luacheck (1.2.0)
matrix/matrix.lua
[warning] 55-55: variable 'err' was previously defined on line 48
(W411)
[warning] 62-62: variable 'err' was previously defined on line 55
(W411)
[warning] 76-76: shadowing definition of variable 'err' on line 62
(W421)
[warning] 97-97: variable 'err' was previously defined on line 62
(W411)
[warning] 194-194: line contains trailing whitespace
(W612)
🪛 markdownlint-cli2 (0.22.0)
matrix/README.md
[warning] 15-15: Link fragments should be valid
(MD051, link-fragments)
| if self.variables["MATRIX_INCLUDE_HEADERS"] == "yes" then | ||
| local headers, err = ngx_req.get_headers() | ||
| if not headers then | ||
| data["formatted_body"] = data["formatted_body"] .. "error while getting headers: " .. err | ||
| data["body"] = data["body"] .. "\n error while getting headers: " .. err | ||
| else | ||
| data["formatted_body"] = data["formatted_body"] .. "<table><tr><th>Header</th><th>Value</th></tr>" | ||
| data["body"] = data["body"] .. "\n\n" | ||
| for header, value in pairs(headers) do | ||
| data["formatted_body"] = data["formatted_body"] .. "<tr><td>" .. header .. "</td><td>" .. value .. "</td></tr>" | ||
| data["body"] = data["body"] .. header .. ": " .. value .. "\n" |
There was a problem hiding this comment.
Do not forward raw request headers to Matrix.
With MATRIX_INCLUDE_HEADERS=yes, this sends Authorization, Cookie, CSRF tokens, and similar secrets into a third-party room. Switch to an allow-list or redact sensitive header names before serialising them.
As per coding guidelines, Never log request bodies, cookies, bearer tokens, webhook secrets, or API keys.
🧰 Tools
🪛 Luacheck (1.2.0)
[warning] 76-76: shadowing definition of variable 'err' on line 62
(W421)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@matrix/matrix.lua` around lines 75 - 85, When MATRIX_INCLUDE_HEADERS is
enabled in matrix.lua, avoid forwarding raw headers in the loop that appends to
data["formatted_body"] and data["body"]; instead implement an allow-list (e.g.,
only include common safe headers like "User-Agent", "Accept", "Host") or
explicitly redact sensitive names (e.g., "Authorization", "Cookie",
"Set-Cookie", "X-CSRF-Token", "Proxy-Authorization", "Authentication") before
serialising, and in the for header,value in pairs(headers) block replace
sensitive values with a placeholder like "[REDACTED]" (or skip adding those
headers entirely) so no bearer tokens, cookies, CSRF tokens, or secrets are sent
to Matrix.
| ## Docker | ||
|
|
||
| ```yaml | ||
| version: '3' | ||
|
|
||
| services: | ||
|
|
||
| bunkerweb: | ||
| image: bunkerity/bunkerweb:1.5.10 | ||
| ... | ||
| environment: | ||
| - USE_MATRIX=yes | ||
| - MATRIX_BASE_URL=https://matrix.org | ||
| - MATRIX_ROOM_ID=!yourRoomID:matrix.org | ||
| - MATRIX_ACCESS_TOKEN=your-access-token | ||
| ... | ||
| ``` |
There was a problem hiding this comment.
Update the examples to the 1.6 image series.
Lines 42 and 60 still pin bunkerity/bunkerweb:1.5.10, but this PR is explicitly about 1.6 compatibility. The examples should not steer users onto the wrong runtime version.
As per coding guidelines, Prefer concrete instructions, accurate examples, and explicit prerequisites.
Also applies to: 52-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@matrix/README.md` around lines 34 - 50, Update the Docker examples in
matrix/README.md to reference the 1.6 image series: replace all occurrences of
the image tag "bunkerity/bunkerweb:1.5.10" (seen in the bunkerweb service
blocks) with the appropriate 1.6 tag (e.g., "bunkerity/bunkerweb:1.6" or the
exact 1.6.x release you intend to document) so examples match this PR's 1.6
compatibility; ensure both instances mentioned in the diff are updated.
| except BaseException as e: | ||
| logger.debug(format_exc()) | ||
| logger.error(f"Failed to get matrix ping: {e}") | ||
| ret["error"] = str(e) |
There was a problem hiding this comment.
Do not expose raw exceptions in the card payload.
Line 21 returns str(e) to the UI. Errors from the ping path can include internal URLs or other sensitive details, and this payload is rendered. Keep the full exception in server-side logs and return a generic UI error marker instead.
As per coding guidelines, Never expose secrets, webhook URLs, API keys, or tokens in the returned card payload — they are rendered into the UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@matrix/ui/actions.py` around lines 18 - 21, The code assigns str(e) to
ret["error"], exposing potentially sensitive details; change this so the full
exception remains only in server logs (keep logger.debug(format_exc()) and
logger.error(f"Failed to get matrix ping: {e}") for diagnostics) but replace
ret["error"] with a generic, non-sensitive marker such as "Unable to retrieve
matrix ping" or "internal_error" (update the assignment to ret["error"]
accordingly) — locate the try/except handling around the ping path where
ret["error"] is set and remove any raw exception string from the payload.
| def matrix(**kwargs): | ||
| pass No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the matrix plugin directory and examine ui/actions.py
find . -path "*matrix/ui/actions.py" -type fRepository: bunkerity/bunkerweb-plugins
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Read the matrix ui/actions.py file
if [ -f "./matrix/ui/actions.py" ]; then
wc -l ./matrix/ui/actions.py
fiRepository: bunkerity/bunkerweb-plugins
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Display the file contents around lines 29-30
if [ -f "./matrix/ui/actions.py" ]; then
cat -n ./matrix/ui/actions.py | sed -n '20,40p'
fiRepository: bunkerity/bunkerweb-plugins
Length of output: 335
🏁 Script executed:
#!/bin/bash
# Search for calls to the matrix() function to see what callers expect
rg "matrix\(" --type py -B 2 -A 2Repository: bunkerity/bunkerweb-plugins
Length of output: 190
🏁 Script executed:
#!/bin/bash
# Look at other plugin ui/actions.py files to understand the pattern
find . -path "*/ui/actions.py" -type f | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -40 {}'Repository: bunkerity/bunkerweb-plugins
Length of output: 3687
Return an empty dict from matrix().
Line 29-30: The matrix() function currently returns None implicitly, breaking the expected contract that all plugin main-page entry points return a dictionary. Other plugins (clamav, coraza, crowdsec, discord) consistently implement this pattern. Return {} at minimum to match the standard.
Suggested fix
def matrix(**kwargs):
- pass
+ return {}📝 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.
| def matrix(**kwargs): | |
| pass | |
| def matrix(**kwargs): | |
| return {} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@matrix/ui/actions.py` around lines 29 - 30, The function matrix(**kwargs)
currently returns None which violates the plugin main-page contract; update the
matrix function to return an empty dict ({}). Locate the matrix function
definition and replace the pass/implicit None with a return {} so it matches
other plugin entry points (e.g., clamav, coraza, crowdsec, discord) and ensures
callers receive a dictionary.
|
Sorry for the delay and thank you for the work! |
2 participants
Initially made this for myself, maybe you want to add it to your collection.
Code is heavily inspired by your Webhook plugin.
This is how it looks in Element.
