[fix] Unhandled exception when parsing invalid captive portal action url in status.js#1066
Open
Ontiomacer wants to merge 1 commit into
Open
[fix] Unhandled exception when parsing invalid captive portal action url in status.js#1066Ontiomacer wants to merge 1 commit into
Ontiomacer wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2026-03-06T23:26:11.526ZApplied to files:
📚 Learning: 2026-03-06T23:26:11.526ZApplied to files:
📚 Learning: 2026-03-06T23:26:11.526ZApplied to files:
📚 Learning: 2026-03-06T23:26:11.526ZApplied to files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughThe change hardens the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Ontiomacer
changed the title
Test Failures and Code Style IssuesHello @Ontiomacer,
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
client/components/status/status.js (1)
753-775:⚠️ Potential issue | 🔴 CriticalCritical bug: Undefined variable
trustedOriginreferenced on line 768.The code declares
isTrustedOriginon line 763 but referencestrustedOriginon line 768. SincetrustedOriginis undefined, the condition!trustedOriginwill always evaluate totrue, causing the function to return early on every postMessage event. This completely breaks the handler's functionality.Additionally, the indentation of lines 753-765 is inconsistent with the rest of the method.
🐛 Proposed fix
-let actionOrigin = null; - -try { - if (captivePortalLoginForm.action?.trim()) { - actionOrigin = new URL(captivePortalLoginForm.action).origin; - } -} catch { - // invalid URL, ignore -} - -const isTrustedOrigin = - event.origin === actionOrigin || - event.origin === window.location.origin; + let actionOrigin = null; + + try { + if (captivePortalLoginForm.action?.trim()) { + actionOrigin = new URL(captivePortalLoginForm.action).origin; + } + } catch { + // invalid URL, ignore + } + + const isTrustedOrigin = + event.origin === actionOrigin || + event.origin === window.location.origin; if ( - !trustedOrigin || + !isTrustedOrigin || !type || // internet-mode will not contain message, but message // is required for authError and authMessage type (!message && type !== "internet-mode") ) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/status/status.js` around lines 753 - 775, The handler mistakenly references an undefined variable `trustedOrigin` instead of the computed `isTrustedOrigin`, causing early returns; change the condition to use `isTrustedOrigin` (i.e., replace `!trustedOrigin` with `!isTrustedOrigin`) and keep the rest of the condition as-is (checking `type` and `message`), and also fix the indentation of the block that computes `actionOrigin`/`isTrustedOrigin` (the code around `captivePortalLoginForm`, `actionOrigin`, and `isTrustedOrigin`) to match the surrounding method style so it's consistently indented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@client/components/status/status.js`:
- Around line 753-775: The handler mistakenly references an undefined variable
`trustedOrigin` instead of the computed `isTrustedOrigin`, causing early
returns; change the condition to use `isTrustedOrigin` (i.e., replace
`!trustedOrigin` with `!isTrustedOrigin`) and keep the rest of the condition
as-is (checking `type` and `message`), and also fix the indentation of the block
that computes `actionOrigin`/`isTrustedOrigin` (the code around
`captivePortalLoginForm`, `actionOrigin`, and `isTrustedOrigin`) to match the
surrounding method style so it's consistently indented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82dea135-bec7-4e18-ae1b-281a9e818c1e
📒 Files selected for processing (1)
client/components/status/status.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests and Coverage
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), catch blocks deliberately do NOT check `isComponentMounted` — console warnings from catch paths are considered harmless. Only real memory leaks (timers, event listeners) and "setState on unmounted" in success paths are fixed. The maintainer (nemesifier) explicitly wants to avoid over-engineering with defensive checks in every catch block.
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), `finalOperations()` has a single early return before setting up intervals/listeners to prevent the scenario where the component unmounts during async awaits. `safeSetState` is applied to success paths in `getUserRadiusSessions()`, `getUserRadiusUsage()`, `getPlansSuccessCallback()`, and `handlePostMessage()`.
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), real memory leaks are fixed by calling `clearTimeout(this.usageRetryTimeoutId)` and `window.removeEventListener("message", ...)` in `componentWillUnmount()`. These were actual leaks — the retry timeout could fire and the message handler could run after unmount.
Learnt from: prathmeshkulkarni-coder
Repo: openwisp/openwisp-wifi-login-pages PR: 1063
File: client/components/status/status.test.js:2890-2916
Timestamp: 2026-03-18T17:34:17.347Z
Learning: In openwisp/openwisp-wifi-login-pages, JSDOM marks `window.location` as non-configurable (configurable: false) in the Jest/Enzyme test environment used by this project. Object.defineProperty on window.location throws a TypeError. Hacks like `delete window.location` cause state leaks across sequential tests. The maintainer (prathmeshkulkarni-coder) considers the window.location mocking approach infeasible and plans to migrate detection tests to RTL instead.
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), the Status component uses `this.isComponentMounted` (set to `true` in the constructor, `false` in `componentWillUnmount`) together with a `safeSetState()` helper method that checks `isComponentMounted` before calling `setState()`. This is the preferred pattern to guard against "setState on unmounted component" warnings.
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), `finalOperations()` has a single early return before setting up intervals/listeners to prevent the scenario where the component unmounts during async awaits. `safeSetState` is applied to success paths in `getUserRadiusSessions()`, `getUserRadiusUsage()`, `getPlansSuccessCallback()`, and `handlePostMessage()`.
Applied to files:
client/components/status/status.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), catch blocks deliberately do NOT check `isComponentMounted` — console warnings from catch paths are considered harmless. Only real memory leaks (timers, event listeners) and "setState on unmounted" in success paths are fixed. The maintainer (nemesifier) explicitly wants to avoid over-engineering with defensive checks in every catch block.
Applied to files:
client/components/status/status.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), the Status component uses `this.isComponentMounted` (set to `true` in the constructor, `false` in `componentWillUnmount`) together with a `safeSetState()` helper method that checks `isComponentMounted` before calling `setState()`. This is the preferred pattern to guard against "setState on unmounted component" warnings.
Applied to files:
client/components/status/status.js
🔇 Additional comments (1)
client/components/status/status.js (1)
748-748: LGTM!Good defensive coding - defaulting
event.datato{}prevents runtime errors when the message payload is undefined.
Handle invalid or empty captivePortalLoginForm.action values safely by wrapping URL parsing in a try/catch block. Also ensure safe destructuring of event.data to avoid runtime errors when the message payload is undefined. This improves robustness of the postMessage handler without changing existing behavior.
Ontiomacer
force-pushed
the
status-url-parse-fix
branch
from
27b92b2 to
f40dbbe
Compare
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.
Handle invalid or empty captivePortalLoginForm.action values safely by wrapping URL parsing in a try/catch block.
Also ensure safe destructuring of event.data to avoid runtime errors when the message payload is undefined.
This improves robustness of the postMessage handler without changing existing behavior.
Checklist
Reference to Existing Issue
Closes #1065.
Description of Changes
The handlePostMessage function parses captivePortalLoginForm.action using the URL constructor.
If the action value is empty or malformed, URL parsing may throw an exception. This change wraps the parsing logic in a try/catch block and validates the action before parsing.
Additionally, event.data is safely destructured to prevent runtime errors when the message payload is undefined.
These changes improve robustness of the message handler without altering existing behavior.
No functional changes are introduced.
Screenshot
N/A