fix: behind_proxy setting ignored across ~30 files, not just WingsAdminController#188
Open
MyMonsterVR wants to merge 3 commits into
Open
Conversation
Every new Wings(...) call in this controller omits the 6th $behindProxy constructor argument, so it always defaults to false regardless of the node's actual behind_proxy DB value. This makes Wings API calls always append the raw daemon port to the URL instead of respecting "Behind Proxy", causing connection failures (cURL error 7: Failed to connect ... port <daemonListen>) for any node running behind a reverse proxy — even when correctly configured. Pass WingsUrlHelper::isBehindProxy($node) through on all 10 call sites, matching what Wings::fromNode() already does correctly.
Contributor
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Walkthrough
ChangesWings proxy support
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Same issue as WingsAdminController - turns out it's not just that one file, every new Wings(...) call across the codebase (chatbot tools, server controllers, backup/power/logs controllers, node controllers, etc) has the same missing 6th argument and defaults behindProxy to false. Fixed all of them the same way, using WingsUrlHelper::isBehindProxy() with whatever the node variable is called in each spot ($node, $nodeInfo, $sourceNode, $destinationNode). This was blocking basically anything that talks to a proxied node - creating a server, backups, power actions, chatbot tools, etc all hit the same wrong port.
MyMonsterVR
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/app/Controllers/User/Server/ServerBackupController.php (1)
348-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the magic number with a constant or explicit variable.
Ah,
30. The universal magical incantation for "I hope the network doesn't drop before this finishes." Hardcoding timeouts directly into positional argument soup across half the codebase? Truly a bold strategy for when things inevitably hang on a Friday night. Extract this to a$timeoutvariable (like you did inServerLogsController) or a class constant before the network gods punish your hubris.
backend/app/Controllers/User/Server/ServerBackupController.php#L348-L349: extract30to a named variable or constant.backend/app/Services/Chatbot/Tools/CopyFilesTool.php#L140-L141: extract30to a named variable or constant.backend/app/Services/Chatbot/Tools/WriteFileTool.php#L132-L133: extract30to a named variable or constant.backend/app/Services/FeatherZeroTrust/SuspensionService.php#L99-L100: extract30to a named variable or constant.backend/app/Services/Server/LifecycleHookExecutorService.php#L260-L261: extract30to a named variable or constant.backend/app/Services/UserDataExport/UserDataExportService.php#L782-L783: extract30to a named variable or constant.🤖 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 `@backend/app/Controllers/User/Server/ServerBackupController.php` around lines 348 - 349, Replace each hardcoded 30-second timeout with a named local variable or class constant in ServerBackupController, CopyFilesTool, WriteFileTool, SuspensionService, LifecycleHookExecutorService, and UserDataExportService at the listed sites. Reuse the named timeout in the corresponding network or operation call while preserving the existing 30-second behavior.
🤖 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 `@backend/app/Controllers/Admin/NodeStatusController.php`:
- Around line 104-105: Fix the malformed argument lists by moving each separator
comma before its inline comment: update
backend/app/Controllers/Admin/NodeStatusController.php lines 104-105 to use 10,
before the comment; update backend/app/Controllers/Admin/NodesController.php
lines 1146-1147 to place the comma after ($timeoutSeconds ?? 60) + 10; and
update lines 1572-1573 to use 10, before the version-check comment.
In `@backend/app/Controllers/User/NodeStatusController.php`:
- Around line 153-154: Move the parameter-separating comma out of the inline
comment in NodeStatusController’s status-check call at
backend/app/Controllers/User/NodeStatusController.php:153-154, placing it after
the timeout argument. Apply the same correction to the status call at
backend/app/Services/Chatbot/Tools/GetServerStatusTool.php:109-110, removing the
comma from the “10 second timeout” comment and restoring valid PHP syntax.
In `@backend/app/Services/Chatbot/Tools/CompressFilesTool.php`:
- Around line 145-147: The timeout argument is being replaced by a duplicate
proxy boolean in both tool calls. In
backend/app/Services/Chatbot/Tools/CompressFilesTool.php lines 145-147 and
backend/app/Services/Chatbot/Tools/DecompressArchiveTool.php lines 125-127,
update the first duplicated proxy argument to the integer 30, while preserving
the subsequent WingsUrlHelper::isBehindProxy($node) argument.
---
Nitpick comments:
In `@backend/app/Controllers/User/Server/ServerBackupController.php`:
- Around line 348-349: Replace each hardcoded 30-second timeout with a named
local variable or class constant in ServerBackupController, CopyFilesTool,
WriteFileTool, SuspensionService, LifecycleHookExecutorService, and
UserDataExportService at the listed sites. Reuse the named timeout in the
corresponding network or operation call while preserving the existing 30-second
behavior.
🪄 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: d4c56969-66d1-43b2-9d62-ab82d41a918e
📒 Files selected for processing (30)
backend/app/Controllers/Admin/FeatherZeroTrustController.phpbackend/app/Controllers/Admin/NodeStatusController.phpbackend/app/Controllers/Admin/NodesController.phpbackend/app/Controllers/Admin/ServersController.phpbackend/app/Controllers/User/NodeStatusController.phpbackend/app/Controllers/User/Server/Logs/ServerLogsController.phpbackend/app/Controllers/User/Server/Power/ServerPowerController.phpbackend/app/Controllers/User/Server/ServerBackupController.phpbackend/app/Controllers/Wings/Transfer/WingsTransferStatusController.phpbackend/app/Services/Chatbot/Tools/AutoAllocateTool.phpbackend/app/Services/Chatbot/Tools/CompressFilesTool.phpbackend/app/Services/Chatbot/Tools/CopyFilesTool.phpbackend/app/Services/Chatbot/Tools/CreateBackupTool.phpbackend/app/Services/Chatbot/Tools/CreateDirectoryTool.phpbackend/app/Services/Chatbot/Tools/DecompressArchiveTool.phpbackend/app/Services/Chatbot/Tools/DeleteAllocationTool.phpbackend/app/Services/Chatbot/Tools/DeleteBackupTool.phpbackend/app/Services/Chatbot/Tools/DeleteFilesTool.phpbackend/app/Services/Chatbot/Tools/GetFileContentTool.phpbackend/app/Services/Chatbot/Tools/GetFilesTool.phpbackend/app/Services/Chatbot/Tools/GetServerFirewallRulesTool.phpbackend/app/Services/Chatbot/Tools/GetServerStatusTool.phpbackend/app/Services/Chatbot/Tools/PullFileTool.phpbackend/app/Services/Chatbot/Tools/RenameFileTool.phpbackend/app/Services/Chatbot/Tools/ServerPowerActionTool.phpbackend/app/Services/Chatbot/Tools/SetPrimaryAllocationTool.phpbackend/app/Services/Chatbot/Tools/WriteFileTool.phpbackend/app/Services/FeatherZeroTrust/SuspensionService.phpbackend/app/Services/Server/LifecycleHookExecutorService.phpbackend/app/Services/UserDataExport/UserDataExportService.php
A few of the call sites had a trailing inline comment on the timeout line (e.g. "10 // Short timeout for status checks"), and my earlier fix appended the comma after the comment instead of before it - so the comment swallowed the comma and PHP saw two args with nothing separating them. Fixed by moving the comma before the comment on those 4 lines. CompressFilesTool and DecompressArchiveTool had a different issue: their original calls only had 4 args (no timeout, no behindProxy), so the patch landed the new arg in the timeout slot instead of adding a 6th one, and somehow ended up duplicated on top of that. Fixed by putting an explicit 30 back in the timeout slot and keeping just one WingsUrlHelper::isBehindProxy($node) after it. Ran php -l on all 31 changed files, all clean now.
Member
|
Thank you for your contribution <3 |
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.
Turns out this bug is way bigger than I first thought. Opened this originally just for WingsAdminController, but it's actually every single new Wings(...) call in the codebase except Wings::fromNode() - 62 call sites across 30 files (server controllers, backup/power/logs controllers, node controllers, every chatbot tool, transfer status, etc).
All of them skip the 6th constructor arg and default behindProxy to false, so anything that talks to a proxied node hits the raw daemon port instead of going through the proxy. Was breaking server creation, backups, power actions, chatbot tools, basically everything node-related once behind_proxy is actually set to true.
Fixed the same way everywhere - added WingsUrlHelper::isBehindProxy($node) as the 6th arg, matching whatever the node variable is called in that spot ($node, $nodeInfo, $sourceNode, $destinationNode).
Tested on my own instance behind Caddy, server creation/backups/power actions all work now.
Summary by CodeRabbit