Codex/fix candyconnect vpn configuration issues#10
Open
malliancea wants to merge 2 commits intoAmiRCandy:mainfrom
Open
Codex/fix candyconnect vpn configuration issues#10malliancea wants to merge 2 commits intoAmiRCandy:mainfrom
malliancea wants to merge 2 commits intoAmiRCandy:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request replaces the DNSTT protocol with Amnezia throughout the codebase, adds new backend endpoints for OpenVPN config downloads and WireGuard QR code generation, enhances WireGuard client provisioning with on-demand key generation, and updates frontend UI components accordingly. Changes
Sequence DiagramssequenceDiagram
actor Admin
participant ClientsUI as Clients UI
participant PanelAPI as Panel API
participant ProtocolMgr as Protocol Manager
participant DB as Database
Admin->>ClientsUI: Click "Generate WireGuard QR"
ClientsUI->>PanelAPI: POST /clients/{id}/wireguard-qr
PanelAPI->>ProtocolMgr: ensure_client_credentials(username)
ProtocolMgr->>DB: get_core_status("wireguard")
DB-->>ProtocolMgr: current status + config
ProtocolMgr->>ProtocolMgr: generate keys & address<br/>(if not present)
ProtocolMgr->>DB: set_core_status(updated config)
DB-->>ProtocolMgr: ack
ProtocolMgr-->>PanelAPI: credentials dict
PanelAPI->>ProtocolMgr: get_client_config(username, server_ip)
ProtocolMgr-->>PanelAPI: WireGuard config text
PanelAPI->>PanelAPI: generate QR code (PNG)<br/>encode to base64
PanelAPI-->>ClientsUI: { wg_config, qr_data_url }
ClientsUI->>ClientsUI: display QR modal
ClientsUI-->>Admin: QR code + config visible
sequenceDiagram
actor Admin
participant ClientsUI as Clients UI
participant PanelAPI as Panel API
participant ProtocolMgr as Protocol Manager
participant ConfigFile as OpenVPN Binary
Admin->>ClientsUI: Click "Download OpenVPN Config"
ClientsUI->>PanelAPI: GET /clients/{id}/openvpn-config
PanelAPI->>PanelAPI: resolve server IP
PanelAPI->>ProtocolMgr: get_client_config(username, server_ip)
ProtocolMgr-->>PanelAPI: OpenVPN config blob
PanelAPI-->>ClientsUI: PlainTextResponse (username.ovpn)
ClientsUI->>ClientsUI: trigger file download
ClientsUI-->>Admin: username.ovpn saved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/routes/client_api.py (1)
579-605:⚠️ Potential issue | 🟡 Minor
latencyis computed but never used — entire block is dead code.
base_latency,protocol_overhead, andlatencyare all calculated (lines 581–595) but the response unconditionally returns"latency": 0(line 601). The computation is wasted on every ping request.🐛 Proposed fix — remove dead computation
if is_running: - # Add a realistic mock network latency (server processing + simulated RTT) - import random - base_latency = elapsed_ms + random.uniform(15, 80) # base processing - # Different protocols have different typical latencies - protocol_overhead = { - "v2ray": random.uniform(10, 60), - "wireguard": random.uniform(5, 30), - "openvpn": random.uniform(20, 80), - "ikev2": random.uniform(15, 50), - "l2tp": random.uniform(25, 90), - "amnezia": random.uniform(50, 200), - "slipstream": random.uniform(15, 60), - "trusttunnel": random.uniform(20, 70), - } - overhead = protocol_overhead.get(resolved_proto, random.uniform(20, 60)) - latency = int(base_latency + overhead) - return { "success": True, "data": { "config_id": config_id, "latency": 0, # Client will measure real network RTT "reachable": True, "protocol_status": "running", } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/client_api.py` around lines 579 - 605, The computed latency variables (base_latency, protocol_overhead, overhead, latency) and the random import are dead because the response always returns "latency": 0; remove the unnecessary block starting from "import random" through the latency calculation (variables base_latency, protocol_overhead, overhead, latency) and ensure the return in the is_running branch keeps "latency": 0, "reachable": True, and "protocol_status": "running" so no unused variables remain; alternatively, if you intended to return the simulated value, replace the hardcoded 0 in the returned dict with the computed latency variable (latency) and keep the calculations.server/protocols/manager.py (1)
270-304:⚠️ Potential issue | 🟡 MinorUnreachable
return default_portat Line 304.The
tryblock returns on every code path (either an explicitreturnper protocol or through the v2ray fallback), and theexceptblock returns too. The finalreturn default_porton line 304 can never be reached.♻️ Proposed fix
except Exception: return default_port - return default_port🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/protocols/manager.py` around lines 270 - 304, The final "return default_port" in _get_protocol_port is unreachable because every branch inside the try returns and the except returns as well; remove the redundant trailing return to clean up the function. Locate the async def _get_protocol_port(self, pid: str) method and delete the last "return default_port" after the try/except block so the function ends immediately after the except (keeping the existing default_port declarations and all protocol-specific returns intact).
🧹 Nitpick comments (5)
server/protocols/wireguard.py (1)
200-205: Markusernameas intentionally unused.If the signature must stay for interface parity, consider prefixing with
_(or a lint suppression) to avoid ARG002 noise.♻️ Suggested tweak
- async def ensure_client_credentials(self, username: str, protocol_data: dict | None = None) -> dict: + async def ensure_client_credentials(self, _username: str, protocol_data: dict | None = None) -> dict:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/protocols/wireguard.py` around lines 200 - 205, The parameter username in ensure_client_credentials is intentionally unused and triggers ARG002; rename it to _username (or prefix it with an underscore) or add an inline lint suppression to indicate it's intentionally unused so linters stop flagging it; update the function signature for ensure_client_credentials accordingly and adjust any internal references (there are none) to avoid further warnings.install.sh (1)
88-101: Clarify the criticality of Amnezia support.The script lists
amneziawg-toolsas a package dependency (apt will install it) but only warns if the binary is missing afterward. This mirrors the Xray pattern (line 97), suggesting Amnezia is optional. If Amnezia is required for basic functionality, change the runtime check to useerr()instead ofwarn()to fail fast; if truly optional, the current approach is consistent with the script's design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 88 - 101, The script installs amneziawg-tools but only warns if the amneziawg binary is missing (the `command -v amneziawg >/dev/null 2>&1 || warn "amneziawg binary not found..."` line), so decide whether Amnezia is required and enforce it: if Amnezia is mandatory, replace the warn branch with a hard failure call (use `err` or the script's exit-on-error helper) to stop execution when `command -v amneziawg` fails; if Amnezia is optional, leave the warning but add a short comment clarifying it's optional to avoid confusion with the Xray install pattern. Ensure you update the `amneziawg-tools` install expectation and the runtime check (`command -v amneziawg`) consistently.server/routes/panel_api.py (2)
171-190: Missing null check forwg— same unguarded dereference pattern as the OpenVPN endpoint.
protocol_manager.get_protocol("wireguard")can theoretically returnNone, andensure_client_credentialsis not declared inBaseProtocol, so there's no type safety either.🛡️ Proposed fix
wg = protocol_manager.get_protocol("wireguard") +if not wg: + raise HTTPException(status_code=503, detail="WireGuard protocol manager not available") pdata = await wg.ensure_client_credentials(client["username"], pdata)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/panel_api.py` around lines 171 - 190, The handler generate_wireguard_qr currently calls protocol_manager.get_protocol("wireguard") without checking the result and then calls wg.ensure_client_credentials and wg.get_client_config; add a null-check after obtaining wg and raise an HTTPException(500, "WireGuard protocol not available") (or similar) if wg is None, so you never call methods on None, and ensure the implementation of the wireguard protocol exposes the expected methods (ensure_client_credentials and get_client_config) consistent with BaseProtocol or adjust the call-sites to use the concrete interface provided by the wireguard protocol.
150-168: Missing null check forp_mgr— unguardedAttributeErroron line 160 if protocol is unavailable.
protocol_manager.get_protocol("openvpn")returnsNoneif the protocol is not registered. Line 160 calls a method on the result without checking, yielding an opaqueAttributeErrortraceback instead of a meaningful HTTP response.🛡️ Proposed fix
p_mgr = protocol_manager.get_protocol("openvpn") +if not p_mgr: + raise HTTPException(status_code=503, detail="OpenVPN protocol manager not available") pdata_ovpn = (client.get("protocol_data", {}) or {}).get("openvpn", {}) cfg = await p_mgr.get_client_config(client["username"], server_ip, pdata_ovpn)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/panel_api.py` around lines 150 - 168, download_openvpn_config currently calls protocol_manager.get_protocol("openvpn") and immediately uses p_mgr.get_client_config, which will raise an AttributeError if get_protocol returns None; add a null check after assigning p_mgr and before calling p_mgr.get_client_config: if p_mgr is None, raise an HTTPException (e.g. status_code=503 or 404) with a clear message like "OpenVPN protocol not available" so the endpoint returns a meaningful HTTP error instead of an AttributeError; reference the symbols protocol_manager.get_protocol, p_mgr, and p_mgr.get_client_config in the download_openvpn_config function.web-panel/pages/ClientsPage.tsx (1)
161-175:handleGenerateWireGuardQrmakes two sequentialgetClients()calls redundantly.
fetchClients()(line 166) already callsgetClients()internally and updates theclientsstate. The immediate follow-upawait getClients()on line 167 is a second identical network round-trip. Since React state updates fromfetchClientsare not yet reflected synchronously, the intent is to get fresh data — but this can be satisfied by returning the data fromfetchClientsor restructuring slightly.♻️ Proposed refactor
const fetchClients = async () => { try { const data = await getClients(); setClients(data); + return data; } catch (e: any) { notify(e.message || 'Failed to load clients', 'error'); } finally { setLoading(false); } + return []; };const handleGenerateWireGuardQr = async (client: Client) => { try { setWgQrLoading(true); const data = await generateWireGuardQr(client.id); setWgQrData({ qr_data_url: data.qr_data_url, wg_config: data.wg_config }); - await fetchClients(); - const fresh = await getClients(); + const fresh = await fetchClients(); const updated = fresh.find(c => c.id === client.id); if (updated) setDetailClient(updated);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-panel/pages/ClientsPage.tsx` around lines 161 - 175, The handler handleGenerateWireGuardQr currently calls fetchClients() then immediately calls getClients() again causing a redundant network request; update fetchClients() (or its caller) to return the refreshed clients array (or have fetchClients resolve to the new list) and then in handleGenerateWireGuardQr use the returned array from fetchClients to find the updated client (e.g., const fresh = await fetchClients(); const updated = fresh.find(c => c.id === client.id); if (updated) setDetailClient(updated)). Remove the extra await getClients() call and ensure fetchClients still updates the clients state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/database.py`:
- Around line 214-219: The domain value f"amnezia.{PANEL_DOMAIN}" can produce
"amnezia." when PANEL_DOMAIN is empty; change the assignment for the "amnezia"
service to produce a valid hostname by checking PANEL_DOMAIN and using either
"amnezia" when PANEL_DOMAIN is falsy/empty or f"amnezia.{PANEL_DOMAIN}" when it
is set (i.e., replace the direct f-string with a conditional expression that
uses PANEL_DOMAIN only when non-empty), ensuring the sanitized value is what
gets persisted to Redis.
In `@server/protocols/amnezia.py`:
- Around line 12-19: install() currently discards the boolean result of
self._apt_install("amneziawg-tools") and always returns True; update install()
to capture the return value from self._apt_install, and if it is False await
add_log("ERROR", self.PROTOCOL_NAME, "Installation failed for amneziawg-tools")
(or include returned/failure context) and return False; only return True when
self._apt_install returns True, keeping the existing exception handling that
logs and returns False on exceptions.
- Around line 47-48: The add_client method currently returns only username and
status, causing frontend code (ClientsPage.tsx reading
protocol_data["amnezia"].domain and .obfuscation) to get undefined; update async
def add_client(self, username: str, client_data: dict) -> dict to include
Amnezia-specific fields (at minimum "domain" and "obfuscation") by extracting
them from the provided client_data or from the Amnezia DB/config defaults, and
return them (e.g., include "domain": client_data.get("domain", <default>) and
"obfuscation": client_data.get("obfuscation", <default>)) alongside username and
status so the UI receives the expected keys.
- Around line 53-58: get_client_config currently always returns
self.DEFAULT_PORT (DEFAULT_PORT) instead of using the configured port; update
get_client_config in amnezia.py to use the port from the configuration (prefer
protocol_data["port"] or protocol_data.get("amnezia.port") /
protocol_data.get("port")) and fall back to self.DEFAULT_PORT if not present, or
alternatively call the central reader _get_protocol_port from manager.py if
available; ensure you reference get_client_config and DEFAULT_PORT in the change
so the returned dict uses the resolved port value rather than the hardcoded
51830.
In `@server/protocols/wireguard.py`:
- Around line 220-223: The current allocation uses random.randint(2,254) to
build client_address (10.66.66.{last_octet}/32) which can collide; replace this
with deterministic allocation that enumerates used addresses and picks the next
free /32. Query existing peers (via `wg show` output or your persistent store)
to build a set of used last_octet values, then iterate from 2..254 to select the
first unused last_octet and assign client_address accordingly (fall back to an
error when none free). Update the allocation logic where last_octet and
client_address are created to reference this check-and-select routine so
duplicates are never returned.
- Around line 212-218: The code currently ignores failures from the "wg"
subprocesses and still returns status "ready"; update the logic in the method
containing the "wg pubkey" and the subsequent "wg set" call to check each
subprocess' return code and stderr, e.g., inspect proc.returncode (and the
second proc for "wg set") after await proc.communicate, and if non‑zero log the
error and short‑circuit by returning a non‑ready status (or raising) so
credentials/configs are not marked ready; use the existing local names (proc,
pub_out/client_pubkey for the pubkey step and the corresponding proc/outputs for
the wg set step) to locate and gate the readiness return.
In `@server/requirements.txt`:
- Line 13: The requirements list is missing Pillow, which qrcode.make() relies
on because the resulting object uses PIL's Image.save; update dependencies to
ensure Pillow is installed by either changing the qrcode line to the extra that
includes PIL (e.g., qrcode[pil]==7.4.2) or by adding an explicit Pillow
requirement (e.g., pillow>=9.1.0) in requirements.txt so qrcode.make() and
img.save(...) in panel_api.py will work at runtime.
---
Outside diff comments:
In `@server/protocols/manager.py`:
- Around line 270-304: The final "return default_port" in _get_protocol_port is
unreachable because every branch inside the try returns and the except returns
as well; remove the redundant trailing return to clean up the function. Locate
the async def _get_protocol_port(self, pid: str) method and delete the last
"return default_port" after the try/except block so the function ends
immediately after the except (keeping the existing default_port declarations and
all protocol-specific returns intact).
In `@server/routes/client_api.py`:
- Around line 579-605: The computed latency variables (base_latency,
protocol_overhead, overhead, latency) and the random import are dead because the
response always returns "latency": 0; remove the unnecessary block starting from
"import random" through the latency calculation (variables base_latency,
protocol_overhead, overhead, latency) and ensure the return in the is_running
branch keeps "latency": 0, "reachable": True, and "protocol_status": "running"
so no unused variables remain; alternatively, if you intended to return the
simulated value, replace the hardcoded 0 in the returned dict with the computed
latency variable (latency) and keep the calculations.
---
Nitpick comments:
In `@install.sh`:
- Around line 88-101: The script installs amneziawg-tools but only warns if the
amneziawg binary is missing (the `command -v amneziawg >/dev/null 2>&1 || warn
"amneziawg binary not found..."` line), so decide whether Amnezia is required
and enforce it: if Amnezia is mandatory, replace the warn branch with a hard
failure call (use `err` or the script's exit-on-error helper) to stop execution
when `command -v amneziawg` fails; if Amnezia is optional, leave the warning but
add a short comment clarifying it's optional to avoid confusion with the Xray
install pattern. Ensure you update the `amneziawg-tools` install expectation and
the runtime check (`command -v amneziawg`) consistently.
In `@server/protocols/wireguard.py`:
- Around line 200-205: The parameter username in ensure_client_credentials is
intentionally unused and triggers ARG002; rename it to _username (or prefix it
with an underscore) or add an inline lint suppression to indicate it's
intentionally unused so linters stop flagging it; update the function signature
for ensure_client_credentials accordingly and adjust any internal references
(there are none) to avoid further warnings.
In `@server/routes/panel_api.py`:
- Around line 171-190: The handler generate_wireguard_qr currently calls
protocol_manager.get_protocol("wireguard") without checking the result and then
calls wg.ensure_client_credentials and wg.get_client_config; add a null-check
after obtaining wg and raise an HTTPException(500, "WireGuard protocol not
available") (or similar) if wg is None, so you never call methods on None, and
ensure the implementation of the wireguard protocol exposes the expected methods
(ensure_client_credentials and get_client_config) consistent with BaseProtocol
or adjust the call-sites to use the concrete interface provided by the wireguard
protocol.
- Around line 150-168: download_openvpn_config currently calls
protocol_manager.get_protocol("openvpn") and immediately uses
p_mgr.get_client_config, which will raise an AttributeError if get_protocol
returns None; add a null check after assigning p_mgr and before calling
p_mgr.get_client_config: if p_mgr is None, raise an HTTPException (e.g.
status_code=503 or 404) with a clear message like "OpenVPN protocol not
available" so the endpoint returns a meaningful HTTP error instead of an
AttributeError; reference the symbols protocol_manager.get_protocol, p_mgr, and
p_mgr.get_client_config in the download_openvpn_config function.
In `@web-panel/pages/ClientsPage.tsx`:
- Around line 161-175: The handler handleGenerateWireGuardQr currently calls
fetchClients() then immediately calls getClients() again causing a redundant
network request; update fetchClients() (or its caller) to return the refreshed
clients array (or have fetchClients resolve to the new list) and then in
handleGenerateWireGuardQr use the returned array from fetchClients to find the
updated client (e.g., const fresh = await fetchClients(); const updated =
fresh.find(c => c.id === client.id); if (updated) setDetailClient(updated)).
Remove the extra await getClients() call and ensure fetchClients still updates
the clients state.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/ТЗ_CandyConnect_VPN.mdinstall.shserver/config.pyserver/database.pyserver/protocols/amnezia.pyserver/protocols/manager.pyserver/protocols/wireguard.pyserver/requirements.txtserver/routes/client_api.pyserver/routes/panel_api.pyweb-panel/pages/ClientsPage.tsxweb-panel/pages/CoreConfigsPage.tsxweb-panel/pages/DashboardPage.tsxweb-panel/services/api.tsweb-panel/utils/format.tsx
Comment on lines
+214
to
219
| "amnezia": { | ||
| "port": 51830, | ||
| "transport": "udp", | ||
| "obfuscation": "on", | ||
| "domain": f"amnezia.{PANEL_DOMAIN}", | ||
| "public_key": "", |
There was a problem hiding this comment.
f"amnezia.{PANEL_DOMAIN}" produces an invalid domain when PANEL_DOMAIN is empty.
The default value of PANEL_DOMAIN is often an empty string (set via env-var). When that happens, the seed value becomes "amnezia." — a syntactically invalid hostname that will be persisted to Redis on first boot until the admin overrides it.
🐛 Proposed fix
- "domain": f"amnezia.{PANEL_DOMAIN}",
+ "domain": f"amnezia.{PANEL_DOMAIN}" if PANEL_DOMAIN else "",📝 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.
Suggested change
| "amnezia": { | |
| "port": 51830, | |
| "transport": "udp", | |
| "obfuscation": "on", | |
| "domain": f"amnezia.{PANEL_DOMAIN}", | |
| "public_key": "", | |
| "amnezia": { | |
| "port": 51830, | |
| "transport": "udp", | |
| "obfuscation": "on", | |
| "domain": f"amnezia.{PANEL_DOMAIN}" if PANEL_DOMAIN else "", | |
| "public_key": "", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/database.py` around lines 214 - 219, The domain value
f"amnezia.{PANEL_DOMAIN}" can produce "amnezia." when PANEL_DOMAIN is empty;
change the assignment for the "amnezia" service to produce a valid hostname by
checking PANEL_DOMAIN and using either "amnezia" when PANEL_DOMAIN is
falsy/empty or f"amnezia.{PANEL_DOMAIN}" when it is set (i.e., replace the
direct f-string with a conditional expression that uses PANEL_DOMAIN only when
non-empty), ensuring the sanitized value is what gets persisted to Redis.
Comment on lines
+12
to
+19
| async def install(self) -> bool: | ||
| try: | ||
| await add_log("INFO", self.PROTOCOL_NAME, "Installing Amnezia dependencies...") | ||
| await self._apt_install("amneziawg-tools") | ||
| return True | ||
| except Exception as e: | ||
| await add_log("ERROR", self.PROTOCOL_NAME, f"Installation error: {e}") | ||
| return False |
There was a problem hiding this comment.
install() ignores _apt_install() return value — always reports success on failure.
_apt_install() returns False (without raising) when apt fails after all retries. Because the return value is discarded, install() always returns True even when the package wasn't installed.
🐛 Proposed fix
async def install(self) -> bool:
try:
await add_log("INFO", self.PROTOCOL_NAME, "Installing Amnezia dependencies...")
- await self._apt_install("amneziawg-tools")
- return True
+ ok = await self._apt_install("amneziawg-tools")
+ if not ok:
+ await add_log("ERROR", self.PROTOCOL_NAME, "Failed to install amneziawg-tools")
+ return ok
except Exception as e:
await add_log("ERROR", self.PROTOCOL_NAME, f"Installation error: {e}")
return False🧰 Tools
🪛 Ruff (0.15.2)
[warning] 16-16: Consider moving this statement to an else block
(TRY300)
[warning] 17-17: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/protocols/amnezia.py` around lines 12 - 19, install() currently
discards the boolean result of self._apt_install("amneziawg-tools") and always
returns True; update install() to capture the return value from
self._apt_install, and if it is False await add_log("ERROR", self.PROTOCOL_NAME,
"Installation failed for amneziawg-tools") (or include returned/failure context)
and return False; only return True when self._apt_install returns True, keeping
the existing exception handling that logs and returns False on exceptions.
Comment on lines
+53
to
+58
| async def get_client_config(self, username: str, server_ip: str, protocol_data: dict, config_id: str = None) -> dict: | ||
| return { | ||
| "type": "amnezia", | ||
| "server": server_ip, | ||
| "port": self.DEFAULT_PORT, | ||
| "username": username, |
There was a problem hiding this comment.
get_client_config() always returns the hardcoded DEFAULT_PORT instead of the configured port.
_default_core_configs() stores an amnezia.port in Redis, and the admin can change it. _get_protocol_port in manager.py (line 287) already reads from DB, but this method ignores DB config entirely and always returns 51830.
🐛 Proposed fix
+from database import get_core_config
+
async def get_client_config(self, username: str, server_ip: str, protocol_data: dict, config_id: str | None = None) -> dict:
+ cfg = await get_core_config(self.PROTOCOL_ID) or {}
+ port = int(cfg.get("port", self.DEFAULT_PORT))
return {
"type": "amnezia",
"server": server_ip,
- "port": self.DEFAULT_PORT,
+ "port": port,
"username": username,
}🧰 Tools
🪛 Ruff (0.15.2)
[warning] 53-53: Unused method argument: protocol_data
(ARG002)
[warning] 53-53: Unused method argument: config_id
(ARG002)
[warning] 53-53: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/protocols/amnezia.py` around lines 53 - 58, get_client_config
currently always returns self.DEFAULT_PORT (DEFAULT_PORT) instead of using the
configured port; update get_client_config in amnezia.py to use the port from the
configuration (prefer protocol_data["port"] or protocol_data.get("amnezia.port")
/ protocol_data.get("port")) and fall back to self.DEFAULT_PORT if not present,
or alternatively call the central reader _get_protocol_port from manager.py if
available; ensure you reference get_client_config and DEFAULT_PORT in the change
so the returned dict uses the resolved port value rather than the hardcoded
51830.
Comment on lines
+212
to
+218
| proc = await asyncio.create_subprocess_shell( | ||
| "wg pubkey", | ||
| stdin=asyncio.subprocess.PIPE, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| ) | ||
| pub_out, _ = await proc.communicate(client_privkey.encode()) | ||
| client_pubkey = pub_out.decode().strip() |
There was a problem hiding this comment.
Don’t mark credentials ready when wg commands fail.
If wg pubkey or wg set fails, the method still returns status: "ready" and hands out configs that won’t connect. Check return codes and short-circuit before returning readiness.
🛡️ Suggested fix
pub_out, _ = await proc.communicate(client_privkey.encode())
- client_pubkey = pub_out.decode().strip()
+ if proc.returncode != 0:
+ return {**protocol_data, "status": "pending_qr_generation"}
+ client_pubkey = pub_out.decode().strip()
+ if not client_pubkey:
+ return {**protocol_data, "status": "pending_qr_generation"}
@@
- await self._run_cmd(
+ rc, _, _ = await self._run_cmd(
f"sudo wg set wg0 peer {client_pubkey} allowed-ips {client_address}",
check=False,
)
+ if rc != 0:
+ return {**protocol_data, "status": "pending_qr_generation"}Also applies to: 224-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/protocols/wireguard.py` around lines 212 - 218, The code currently
ignores failures from the "wg" subprocesses and still returns status "ready";
update the logic in the method containing the "wg pubkey" and the subsequent "wg
set" call to check each subprocess' return code and stderr, e.g., inspect
proc.returncode (and the second proc for "wg set") after await proc.communicate,
and if non‑zero log the error and short‑circuit by returning a non‑ready status
(or raising) so credentials/configs are not marked ready; use the existing local
names (proc, pub_out/client_pubkey for the pubkey step and the corresponding
proc/outputs for the wg set step) to locate and gate the readiness return.
Comment on lines
220
to
223
| import random | ||
| last_octet = random.randint(2, 254) | ||
| client_address = f"10.66.66.{last_octet}/32" | ||
|
|
There was a problem hiding this comment.
Avoid address collisions when allocating /32s.
Random selection inside a /24 without checking existing peers can assign duplicate addresses, breaking routing for multiple clients. Consider tracking used addresses (DB or wg show) and picking the next free slot.
🧰 Tools
🪛 Ruff (0.15.2)
[error] 221-221: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/protocols/wireguard.py` around lines 220 - 223, The current allocation
uses random.randint(2,254) to build client_address (10.66.66.{last_octet}/32)
which can collide; replace this with deterministic allocation that enumerates
used addresses and picks the next free /32. Query existing peers (via `wg show`
output or your persistent store) to build a set of used last_octet values, then
iterate from 2..254 to select the first unused last_octet and assign
client_address accordingly (fall back to an error when none free). Update the
allocation logic where last_octet and client_address are created to reference
this check-and-select routine so duplicates are never returned.
| python-multipart==0.0.9 | ||
| httpx==0.27.2 | ||
| paramiko==3.5.0 | ||
| qrcode==7.4.2 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
qrcode 7.4.2 pillow PIL dependency requirements python
💡 Result:
For qrcode==7.4.2:
- Python requirement:
>=3.7.[1] - Default (no Pillow) install dependencies:
typing-extensions,pypng, andcoloramaonly on Windows (platform_system=="Windows").[1] - Pillow / PIL support is an extra, not required by default:
- Extra
pilpulls inpillow>=9.1.0.[1] - Extra
allalso includespillow>=9.1.0(plus dev/test/maintainer tooling).[1]
- Extra
Install examples:
- With Pillow:
pip install "qrcode[pil]==7.4.2".[1][2] - Without Pillow:
pip install "qrcode==7.4.2".[1][2]
Sources: [1] PyPI JSON metadata for qrcode 7.4.2, [2] PyPI project page (install/extras guidance.
🏁 Script executed:
# Check the full requirements.txt content
cat server/requirements.txtRepository: AmiRCandy/CandyConnect
Length of output: 315
🏁 Script executed:
# Search for qrcode usage in the codebase
rg "qrcode" --type py -B2 -A2Repository: AmiRCandy/CandyConnect
Length of output: 434
Add Pillow as an explicit dependency.
The code in server/routes/panel_api.py uses qrcode.make() followed by img.save(buf, format="PNG"). The .save() method is a PIL Image method, which only exists when Pillow is installed. qrcode==7.4.2 does not include Pillow as a default dependency—it is an optional extra. Either use qrcode[pil]==7.4.2 or add pillow>=9.1.0 as a separate requirement to avoid runtime failures on the QR endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/requirements.txt` at line 13, The requirements list is missing Pillow,
which qrcode.make() relies on because the resulting object uses PIL's
Image.save; update dependencies to ensure Pillow is installed by either changing
the qrcode line to the extra that includes PIL (e.g., qrcode[pil]==7.4.2) or by
adding an explicit Pillow requirement (e.g., pillow>=9.1.0) in requirements.txt
so qrcode.make() and img.save(...) in panel_api.py will work at runtime.
Author
|
123 |
Owner
|
Hi , first of all thanks for your PR |
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.
Summary by CodeRabbit
Release Notes
New Features
Documentation