Conversation
… message Signed-off-by: jokob-sk <jokob.sk@gmail.com>
Contributor
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR centralizes safe trigger/object handling and refactors Action.execute() to a no-argument contract with a new Action.get_object() normalizer; updates all action subclasses and ActionGroup orchestration; extracts and async-ifies device page title resolution with cache retry and REST fallback and hides navigation when missing; replaces emoji with Font Awesome icons in the upgrade notification; updates README badge repo id. ChangesWorkflow Action Execution Refactoring
Notification UI Icon Update
Device Page Title & Navigation
README Badge Update
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server/workflows/actions.py`:
- Around line 105-107: When the code logs "[WF] Unsupported object format:
{obj}" it should follow the same early-return pattern used in UpdateFieldAction
and return None instead of returning the original obj; update the
function/method that contains the mylog call (the block that currently does
mylog("none", f"[WF] Unsupported object format: {obj}") followed by return obj)
to return None so callers get a clear failure signal for unsupported formats.
- Around line 70-72: The function logs unsupported object formats with mylog but
then returns the original obj, which is inconsistent with the earlier
None-return pattern; modify the branch that detects missing objectGuid and
devGUID (the unsupported-format case) to return None instead of obj so callers
receive a clear failure signal; update any docstring/comments for the enclosing
function (in server/workflows/actions.py) if present to reflect that unsupported
formats return None and run tests that call this function to ensure callers
handle None appropriately.
🪄 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: 8e7d1402-381e-4f54-a7d8-294fa1901f80
📒 Files selected for processing (3)
server/initialise.pyserver/workflows/actions.pyserver/workflows/triggers.py
- Updated Trendshift repository link in README.md. - Improved error handling and user feedback in deviceDetails.php: - Added warnings for device not found scenarios. - Refactored device name and owner retrieval logic with caching and REST API fallback.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front/deviceDetails.php (1)
507-530:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winEscape device metadata before writing it into the DOM.
Device names and owners are stored in the database and user-editable via the device edit form. However, when writing them to the page title and details page, this function uses
.html()without escaping, creating a stored XSS sink. An attacker with device management permissions could inject markup intodevNameordevOwnerthat executes when the details page loads.🔒 Suggested fix
function applyDevicePageTitle(mac, name, owner) { + const $documentTitle = $('title'); + const baseTitle = $documentTitle.data('baseTitle') || document.title; + $documentTitle.data('baseTitle', baseTitle); let pageTitleText; if (mac === "new") { pageTitleText = getString("Gen_create_new_device"); - $('#pageTitle').html( - `<i title="${pageTitleText}" class="fa fa-square-plus"></i> ` + pageTitleText - ); + $('#pageTitle') + .empty() + .append($('<i>', { title: pageTitleText, class: 'fa fa-square-plus' })) + .append(document.createTextNode(` ${pageTitleText}`)); $('#devicePageInfoPlc .inner').html( `<i class="fa fa-circle-info"></i> ` + getString("Gen_create_new_device_info") ); $('#devicePageInfoPlc').show(); } else if (!owner || (name && name.toString().includes(owner))) { pageTitleText = name ?? getString("DevDetail_EveandAl_NewDevice"); - $('#pageTitle').html(pageTitleText); + $('#pageTitle').text(pageTitleText); $('#devicePageInfoPlc').hide(); } else { pageTitleText = `${name ?? getString("DevDetail_EveandAl_NewDevice")} (${owner})`; - $('#pageTitle').html(pageTitleText); + $('#pageTitle').text(pageTitleText); $('#devicePageInfoPlc').hide(); } - $('title').html(pageTitleText + ' - ' + $('title').html()); + document.title = `${pageTitleText} - ${baseTitle}`; }🤖 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/deviceDetails.php` around lines 507 - 530, The function applyDevicePageTitle currently injects unescaped user-controlled values (name/owner) via .html() into `#pageTitle`, `#devicePageInfoPlc` .inner and the <title>, creating an XSS sink; fix by treating name and owner as plain text: build the title text with safe string concatenation, use .text(pageTitleText) when inserting pure text, and when you need icons keep them as separate DOM nodes (e.g., create $('<i>').addClass('fa ...') and prepend/append it) instead of interpolating into HTML; also set the browser title via document.title = pageTitleText + ' - ' + document.title (or use $('title').text(...)) so no raw HTML is injected. Ensure changes touch applyDevicePageTitle and the three DOM targets: '#pageTitle', '#devicePageInfoPlc .inner', and 'title'.
🤖 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/deviceDetails.php`:
- Around line 543-578: The current checks treat a null devOwner as "missing" and
always trigger a re-cache and REST fetch; change the conditional guards so they
only trigger when devName is null (i.e., replace occurrences of (name === null
|| owner === null) with (name === null)) so ownerless devices don't go through
cacheDevices(), showSpinner()/hideSpinner(), or the REST fallback; update both
the re-cache block (which calls cacheDevices() and getDevDataByMac()) and the
REST fallback block (which calls getAuthContext() and fetch()) while leaving
applyDevicePageTitle(mac, name, owner) behavior unchanged.
---
Outside diff comments:
In `@front/deviceDetails.php`:
- Around line 507-530: The function applyDevicePageTitle currently injects
unescaped user-controlled values (name/owner) via .html() into `#pageTitle`,
`#devicePageInfoPlc` .inner and the <title>, creating an XSS sink; fix by treating
name and owner as plain text: build the title text with safe string
concatenation, use .text(pageTitleText) when inserting pure text, and when you
need icons keep them as separate DOM nodes (e.g., create $('<i>').addClass('fa
...') and prepend/append it) instead of interpolating into HTML; also set the
browser title via document.title = pageTitleText + ' - ' + document.title (or
use $('title').text(...)) so no raw HTML is injected. Ensure changes touch
applyDevicePageTitle and the three DOM targets: '#pageTitle',
'#devicePageInfoPlc .inner', and 'title'.
🪄 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: daca9f14-dec4-4bb3-aad6-43595cb403ff
📒 Files selected for processing (3)
README.mdfront/deviceDetails.phpserver/workflows/actions.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- server/workflows/actions.py
…fine device management logic in deviceDetails.php
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
1 participant
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.
Summary by CodeRabbit
Improvements
Documentation