FE: Notifications sorting fix #1713#1714
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe notification timestamp renderer now preserves raw values for sorting. The Telegram publisher sends bounded JSON requests through an argument-based curl subprocess. Notification cleanup and Omada normalization behavior were also updated. ChangesNotification timestamp rendering
Telegram message publishing
Notification history cleanup
Omada normalization
Sequence Diagram(s)sequenceDiagram
participant Publisher as Telegram publisher
participant Curl as curl subprocess
participant Telegram as Telegram API
Publisher->>Publisher: Truncate and serialize message
Publisher->>Curl: Execute curl with JSON argv request
Curl->>Telegram: Send JSON request
Telegram-->>Curl: Return response
Curl-->>Publisher: Return stdout or process error
Publisher->>Publisher: Raise error for ok false response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/userNotifications.php`:
- Around line 91-93: Update the conditional in the DataTables rendering callback
around the sort/type handling to return the raw value whenever type is not
"display". Replace the limited sort/type check with a type !== "display" check
so filter uses the unlocalized value as well.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 319f419c-5a35-4fb3-b8a9-6da928d0ff44
📒 Files selected for processing (1)
front/userNotifications.php
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/plugins/_publisher_telegram/tg.py`:
- Line 109: Update the debug logging around the Telegram command in the
publisher flow to avoid including the bot token embedded in the API URL. Log
only the operation name or a URL with the token redacted, while preserving the
existing command execution behavior.
- Around line 88-91: Update the truncation logic in the payload construction
block to ensure the final payload, including " (text was truncated)", never
exceeds TELEGRAM_SIZE. Reduce the text slice by the marker length before
appending it, while preserving the existing unmodified text path when no
truncation is needed.
- Around line 99-118: Add curl connection and total request timeouts to the
command assembled in the Telegram publisher flow before subprocess.run, using
values both below RUN_TIMEOUT. Update the cmd construction associated with the
sendMessage request while preserving the existing subprocess behavior and
logging.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5cb31b82-0f48-4585-80fd-a7ad5baeb671
📒 Files selected for processing (1)
front/plugins/_publisher_telegram/tg.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/plugins/_publisher_telegram/tg.py`:
- Around line 116-118: Remove the standalone stray period immediately before the
mylog("debug", ["Executing: Telegram sendMessage", "--data <json>"]) call,
leaving the surrounding list or statement structure valid so tg.py parses and
runs successfully.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 552e935e-1710-42ec-b6ea-cda26f2375f8
📒 Files selected for processing (1)
front/plugins/_publisher_telegram/tg.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
front/plugins/db_cleanup/script.py (3)
132-138: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winParameterize the SQL query to prevent potential injection or syntax errors.
Using string formatting for query construction (flagged by Ruff S608) can lead to SQL syntax errors if
histCountevaluates to an empty string, or pose an injection risk if the setting is tampered with. Parameterizing theLIMITclause and casting tointensures type safety. Additionally, using"index"(lowercase) perfectly matches the table schema.🛡️ Proposed fix
- delete_query = f"""DELETE FROM Notifications - WHERE "Index" NOT IN ( - SELECT "Index" FROM Notifications - ORDER BY dateTimeCreated DESC - LIMIT {histCount} - );""" - cursor.execute(delete_query) + delete_query = """DELETE FROM Notifications + WHERE "index" NOT IN ( + SELECT "index" FROM Notifications + ORDER BY dateTimeCreated DESC + LIMIT ? + );""" + # Provide a safe integer fallback to avoid runtime errors + cursor.execute(delete_query, (int(histCount) if str(histCount).isdigit() else 100,))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/db_cleanup/script.py` around lines 132 - 138, Update the delete query construction and cursor.execute call in the cleanup flow to remove f-string interpolation, cast histCount to int, and bind it as a SQL parameter for the LIMIT clause. Use the schema-matching lowercase "index" identifier in both column references while preserving the existing notification-retention behavior.
132-138: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winParameterize the SQL query to prevent injection and syntax errors.
Using string formatting for query construction (flagged by Ruff
S608) can lead to SQL syntax errors ifhistCountevaluates to an empty string (the default when the setting is missing), or pose an injection risk if the setting is tampered with. Parameterizing theLIMITclause and safely casting tointensures type safety and stability.Additionally, using
"index"(lowercase) perfectly matches the table schema.🛡️ Proposed fix
- delete_query = f"""DELETE FROM Notifications - WHERE "Index" NOT IN ( - SELECT "Index" FROM Notifications - ORDER BY dateTimeCreated DESC - LIMIT {histCount} - );""" - cursor.execute(delete_query) + delete_query = """DELETE FROM Notifications + WHERE "index" NOT IN ( + SELECT "index" FROM Notifications + ORDER BY dateTimeCreated DESC + LIMIT ? + );""" + # Provide a safe integer fallback in case `histCount` is empty/None + cursor.execute(delete_query, (int(histCount or 100),))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/db_cleanup/script.py` around lines 132 - 138, Update the delete_query construction and cursor.execute call to parameterize the LIMIT value instead of interpolating histCount. Safely convert the configured histCount to an integer, including handling its missing or empty default, and pass it as the query parameter; also use the schema-matching lowercase "index" identifier in both column references.Source: Linters/SAST tools
132-138: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winParameterize the SQL query to prevent injection and syntax errors.
Static analysis tools (Ruff S608) flag this string-based query construction as a potential SQL injection vector. Since
histCountis fetched from database settings, parameterizing the limit clause avoids potential injection or syntax errors (e.g., ifhistCountevaluates to an empty string). Additionally, it's safer to use"index"(lowercase) to strictly match the table schema.🛡️ Proposed fix to parameterize the query
- delete_query = f"""DELETE FROM Notifications - WHERE "Index" NOT IN ( - SELECT "Index" FROM Notifications - ORDER BY dateTimeCreated DESC - LIMIT {histCount} - );""" - cursor.execute(delete_query) + delete_query = """DELETE FROM Notifications + WHERE "index" NOT IN ( + SELECT "index" FROM Notifications + ORDER BY dateTimeCreated DESC + LIMIT ? + );""" + # Cast to int to ensure type safety; provide a safe fallback if empty + cursor.execute(delete_query, (int(histCount) if str(histCount).isdigit() else 1000,))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/db_cleanup/script.py` around lines 132 - 138, Update the delete_query construction before cursor.execute to parameterize histCount in the LIMIT clause instead of interpolating it into the SQL string, ensuring the value is validated or converted to the expected numeric type and handling an empty setting safely. Use the schema’s lowercase "index" identifier consistently in both the DELETE and subquery references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/plugins/omada_sdn_openapi/script.py`:
- Around line 172-175: Update the missing-IP handling in the entry-processing
logic to assign Python None rather than the string "null" when
entry["ip_address"] is absent or equals "None". Preserve this value through
serialization so the plugin contract emits JSON null, without introducing a
sentinel string.
---
Nitpick comments:
In `@front/plugins/db_cleanup/script.py`:
- Around line 132-138: Update the delete query construction and cursor.execute
call in the cleanup flow to remove f-string interpolation, cast histCount to
int, and bind it as a SQL parameter for the LIMIT clause. Use the
schema-matching lowercase "index" identifier in both column references while
preserving the existing notification-retention behavior.
- Around line 132-138: Update the delete_query construction and cursor.execute
call to parameterize the LIMIT value instead of interpolating histCount. Safely
convert the configured histCount to an integer, including handling its missing
or empty default, and pass it as the query parameter; also use the
schema-matching lowercase "index" identifier in both column references.
- Around line 132-138: Update the delete_query construction before
cursor.execute to parameterize histCount in the LIMIT clause instead of
interpolating it into the SQL string, ensuring the value is validated or
converted to the expected numeric type and handling an empty setting safely. Use
the schema’s lowercase "index" identifier consistently in both the DELETE and
subquery references.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72963693-2d2b-46ad-beea-e14b4e91f997
📒 Files selected for processing (3)
front/plugins/_publisher_telegram/tg.pyfront/plugins/db_cleanup/script.pyfront/plugins/omada_sdn_openapi/script.py
🚧 Files skipped from review as they are similar to previous changes (1)
- front/plugins/_publisher_telegram/tg.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
front/plugins/_publisher_telegram/tg.py (2)
102-115: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep the bot token and notification body out of
curlarguments.The argv list removes shell-injection risk, but it still exposes the bot token and full notification payload through process listings such as
/procandps. Pass the JSON body through stdin and use a protected mechanism for the token instead of embedding either value incmd.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/_publisher_telegram/tg.py` around lines 102 - 115, Update the curl invocation in the command-building flow so neither the Telegram bot token nor the notification payload appears in cmd: pass the JSON payload through subprocess stdin and supply the token via a protected non-argv mechanism supported by curl, while preserving the existing endpoint, headers, and timeout behavior.
120-135: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate failed Telegram sends instead of returning them as normal results.
check=Falseignores curl failures, and the code never checksproc.returncodeor Telegram’sokresponse field.OSErroris also converted into an ordinary string. Sincemain()unconditionally writes the returned value towatched2, failed deliveries can become indistinguishable from successful publisher results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/_publisher_telegram/tg.py` around lines 120 - 135, Update the Telegram send subprocess flow around subprocess.run so nonzero curl return codes and Telegram responses with ok=false are treated as failures and propagated to main() rather than returned as successful output. Ensure OSError exceptions are likewise propagated instead of converted to strings, while preserving normal return behavior for successful deliveries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@front/plugins/_publisher_telegram/tg.py`:
- Around line 102-115: Update the curl invocation in the command-building flow
so neither the Telegram bot token nor the notification payload appears in cmd:
pass the JSON payload through subprocess stdin and supply the token via a
protected non-argv mechanism supported by curl, while preserving the existing
endpoint, headers, and timeout behavior.
- Around line 120-135: Update the Telegram send subprocess flow around
subprocess.run so nonzero curl return codes and Telegram responses with ok=false
are treated as failures and propagated to main() rather than returned as
successful output. Ensure OSError exceptions are likewise propagated instead of
converted to strings, while preserving normal return behavior for successful
deliveries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23329338-72fd-43e2-beeb-72cc89637f55
📒 Files selected for processing (1)
front/plugins/_publisher_telegram/tg.py
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements
null.