feat: add xray, amneziawg, hysteria2 protocols and harden openvpn client - #210
Merged
Conversation
…022 inbounds multi-user
…amneziawg-hysteria2 # Conflicts: # go.mod # go.sum
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (34)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (26)
📝 WalkthroughWalkthroughThis PR adds AmneziaWG, Hysteria2, and Xray service support with new config models, templates, lifecycle management, and peer/statistics flows. It also updates OpenVPN and WireGuard runtime behavior, shared service typing and validation, proxy command helpers, and a few dependency and lint settings. ChangesAmneziaWG service
Hysteria2 service
Proxycmd and Xray
OpenVPN, WireGuard, and shared runtime
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
hysteria2/server.go-557-581 (1)
557-581: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
createdAtis the zero time, making the stats rollover logic dead code.
createdAtis set totime.Time{}(January 1, year 1), socreatedAt.After(v.Current.CreatedAt)is always false. The rolver block that accumulates current stats into previous stats never executes. If the Hysteria2 server restarts and resets its traffic counters, previous stats won't be preserved.Either set
createdAtto a meaningful value (e.g., server start time or the traffic API's session timestamp if available) or remove the dead code to avoid confusion.🤖 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 `@hysteria2/server.go` around lines 557 - 581, `createdAt` in the traffic stats rollover path is initialized to the zero time, so the `createdAt.After(v.Current.CreatedAt)` branch in `server.go` never runs and the previous-stats accumulation in `s.peers.Update` is dead code. Update the `createdAt` value in this stats collection flow to a meaningful timestamp (for example, the server start time or the session/traffic snapshot time if available), or remove the rollover block if it is no longer needed; make sure the fix keeps the logic around `traffic`, `now`, and `types.NewPeerStatistics` consistent.Source: Linters/SAST tools
xray/server.json.tmpl-21-73 (1)
21-73: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd JSON-safety validation to server-side template inputs.
server.json.tmplinjects operator-controlled strings directly into JSON.ServerConfig.Validate()only checks presence and enum validity, so a quote, backslash, or control character in fields likeReality.Dest,Reality.Fingerprint,Reality.ServerNames,TLSCertFile, orTLSKeyFilewill produce invalid config and prevent Xray from starting.🤖 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 `@xray/server.json.tmpl` around lines 21 - 73, Add JSON-safety validation to the values rendered by server.json.tmpl, since ServerConfig.Validate() currently only checks required/enumerated fields and does not prevent invalid JSON from operator-controlled strings. Update the validation logic on ServerConfig and the nested Reality/TLS-related inputs used in the template render path to reject quotes, backslashes, and control characters in fields like Reality.Dest, Reality.Fingerprint, Reality.ServerNames, TLSCertFile, and TLSKeyFile before template execution.amneziawg/server_config.go-117-121 (1)
117-121: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix copy-paste bug in IPv6 validation error message.
Line 119 uses
c.IPv4Addrin the error message for IPv6 address parsing failure. It should referencec.IPv6Addr— when an invalid IPv6 address is provided, the error currently reports the IPv4 address instead, making debugging misleading.🐛 Proposed fix
if c.IPv6Addr != "" { if _, err := netip.NewPrefix(c.IPv6Addr); err != nil { - return fmt.Errorf("parsing ipv6_addr %q: %w", c.IPv4Addr, err) + return fmt.Errorf("parsing ipv6_addr %q: %w", c.IPv6Addr, err) } }🤖 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 `@amneziawg/server_config.go` around lines 117 - 121, The IPv6 validation error message in the server config parsing logic is using the wrong field, so the failure reported by the `netip.NewPrefix` check is misleading. Update the `IPv6Addr` parsing branch in `server_config.go` to reference `c.IPv6Addr` in the `fmt.Errorf` message instead of `c.IPv4Addr`, keeping the rest of the `c.IPv6Addr` validation flow unchanged.xray/requests.go-20-23 (1)
20-23: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate()is a no-op — add UUID validation.
PeerRequest.Validate()always returnsnil. At minimum, it should rejectuuid.Nil(zero-value UUID), which would pass through unvalidated.🛡️ Proposed fix
func (r *PeerRequest) Validate() error { + if r.UUID == uuid.Nil { + return errors.New("uuid is nil") + } return nil }🤖 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 `@xray/requests.go` around lines 20 - 23, PeerRequest.Validate is currently a no-op, so zero-value requests can slip through unchecked. Update the Validate method on PeerRequest in requests.go to validate the UUID field and return an error when it is uuid.Nil, while keeping the existing validation entry point in place. Use the PeerRequest.Validate symbol to locate the fix and add the minimum UUID check needed for this struct.amneziawg/server.go-336-364 (1)
336-364: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
RemovePeerremoves from interface before checking local map — state inconsistency on error.The
awg set ... peer ... removecommand at Lines 338–342 executes first and removes the peer from the WireGuard interface. If the peer is then not found in the locals.peersmap (Line 350–353), the function returns an error. The caller believes removal failed, but the peer was already removed from the interface. This also means the peer's addresses cannot be released from the pool.Consider checking the local map first, or returning
nilif the interface removal succeeded.🔧 Suggested reorder: check local map before interface removal
func (s *Server) RemovePeer(ctx context.Context, id string) error { + // Get the peer from the map first. + peer, ok := s.peers.Get(id) + if !ok { + return fmt.Errorf("peer %q does not exist", id) + } + // Executes the 'awg set' command to remove the peer from the AmneziaWG interface. cmd := exec.CommandContext( ctx, s.execFile("awg"), "set", s.device, "peer", id, "remove", ) // Run the command and check for errors. if err := cmd.Run(); err != nil { return fmt.Errorf("running command: %w", err) } - // Get the peer from the map. - peer, ok := s.peers.Get(id) - if !ok { - return fmt.Errorf("peer %q does not exist", id) - } - // Release the addrs back to the pool. if err := s.pools.Release(peer.Addrs); err != nil { return fmt.Errorf("releasing peer %q addrs %v: %w", id, peer.Addrs, err)🤖 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 `@amneziawg/server.go` around lines 336 - 364, Remove the state inconsistency in RemovePeer by validating the local peer entry before issuing the awg set ... peer ... remove command. In Server.RemovePeer, look up s.peers.Get(id) first, and only if the peer exists proceed with exec.CommandContext and the pool release, then delete it from s.peers; alternatively, if the interface removal already succeeded, do not return an error just because the local map entry is missing. Use the RemovePeer, s.peers.Get, and s.pools.Release flow to keep interface state and local state aligned.hysteria2/client_config.go-132-192 (1)
132-192: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winValidate
ObfsPasswordandServerAddrfor unsafe TOML characters.These fields are injected directly into double-quoted TOML strings in
client_config.toml.tmpl. A value containing",\, or newline characters could corrupt the generated TOML or inject additional config keys. The v2ray client config already guards against this withutils.HasJSONUnsafeCharsfor JSON templates; the same check applies to TOML basic strings.🛡️ Proposed fix: add unsafe-character validation
func (c *ClientConfig) Validate() error { // Ensure ServerAddr is not empty. if c.ServerAddr == "" { return errors.New("server_addr is empty") } + // Reject values that could break out of a TOML string literal. + for _, v := range []string{c.ServerAddr, c.ObfsPassword} { + if utils.HasJSONUnsafeChars(v) { + return fmt.Errorf("field contains unsafe characters: %q", v) + } + } + // Ensure Auth is not empty.🤖 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 `@hysteria2/client_config.go` around lines 132 - 192, Add unsafe-character validation in ClientConfig.Validate for the fields that are rendered into TOML basic strings, especially ServerAddr and ObfsPassword. Reuse the same kind of check already used in the v2ray config path (for example, utils.HasJSONUnsafeChars) and return a validation error if either field contains quotes, backslashes, or newlines. Keep the new checks near the existing empty-field validation in Validate so the TOML template client_config.toml.tmpl cannot be corrupted by unsafe input.v2ray/server.go-547-549 (1)
547-549: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude
TxBytesin thesyncPeersno-op check.QueryStatsfillsRxBytesandTxBytesseparately, but the early return only comparesRxBytes. If uplink stays unchanged while downlink increases, the update is skipped andTxBytesremains stale. Update the guard to consider both counters (same pattern exists inxray/server.gotoo).🤖 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 `@v2ray/server.go` around lines 547 - 549, The no-op guard in syncPeers only checks RxBytes, so updates can be skipped when TxBytes changes; update the early-return condition to compare both RxBytes and TxBytes on the peer status object. Use the syncPeers logic around QueryStats and the existing RxBytes/TxBytes assignments as the place to adjust the guard, and apply the same fix to the matching implementation in xray/server.go.
🧹 Nitpick comments (8)
xray/server_windows.go (1)
8-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
filepath.Joinfor the full path.String concatenation with
".\\" +bypassesfilepath.Joinfor the leading.segment. Usingfilepath.Join(".", "Xray", name+".exe")is more idiomatic and handles edge cases consistently.♻️ Proposed refactor
func (s *Server) execFile(name string) string { - return ".\\" + filepath.Join("Xray", name+".exe") + return filepath.Join(".", "Xray", name+".exe") }🤖 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 `@xray/server_windows.go` around lines 8 - 10, The execFile helper in Server builds the Windows executable path with manual string concatenation, so update it to use filepath.Join for the entire path assembly instead of prefixing with ".\\". Keep the logic in execFile and replace the current leading segment handling with a filepath.Join call that includes ".", "Xray", and name+".exe" so path construction stays idiomatic and consistent.xray/server.go (1)
436-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace fragile string-based error matching with typed error handling.
strings.Contains(err.Error(), "not found")depends on the exact error message from the gRPC/proxycmd library. If the library changes the message wording, this check silently breaks and "not found" errors would be treated as hard failures, preventing peer removal. Use a typed error or status code check (e.g.,status.Code(err) == codes.NotFound) instead.🤖 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 `@xray/server.go` around lines 436 - 437, The peer-removal path in xray/server.go is using fragile string matching on err.Error() to detect a missing peer, which can break if the underlying gRPC/proxycmd message changes. Update the error handling in the peer inbound alteration logic to use typed gRPC status checking instead of strings.Contains, ideally by checking the status code for NotFound in the same block that currently wraps the error with fmt.Errorf("altering peer %q inbound: %w", id, err). Make sure the helper/imports used in this path clearly reference the existing error handling around the peer inbound removal flow.hysteria2/client_unix.go (1)
14-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared Unix client firewall/DNS logic to avoid duplication.
execFile,applyFirewall,removeFirewall,waitForInterface,applyDNS, andremoveDNSare byte-for-byte identical toopenvpn/client_unix.go. As more protocols are added (the PR already adds three), this duplication will compound maintenance risk — a fix in one file must be manually replicated in all others.Consider extracting these into a shared utility (e.g., a
unixfwpackage or a generic helper accepting an interface likePostUp(ctx) / PreDown(ctx) / DNSAddrs / TUNIface).🤖 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 `@hysteria2/client_unix.go` around lines 14 - 111, The Unix client firewall/DNS helpers are duplicated across client implementations, so the fix is to extract the shared logic into a common helper and make each client reuse it. Move the behavior behind a shared utility or small interface that covers execFile, applyFirewall, removeFirewall, waitForInterface, applyDNS, and removeDNS, then have Client in client_unix.go delegate to that shared code instead of keeping identical copies. Keep the existing method names and behavior intact where they are called so the refactor is internal and minimizes churn.hysteria2/auth.go (1)
36-42: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOptional: cap the request body size.
io.ReadAll(r.Body)is unbounded. Even though the handler is loopback-only, wrapping withhttp.MaxBytesReader(w, r.Body, N)bounds memory use from a misbehaving local process.🤖 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 `@hysteria2/auth.go` around lines 36 - 42, The request body read in the auth handler is unbounded, so cap it before calling io.ReadAll(r.Body) by wrapping the body with http.MaxBytesReader in the same request handling flow. Update the auth logic in the handler that reads the body to enforce a reasonable maximum size and keep the existing error handling path for oversized or invalid bodies.amneziawg/client_config_unix.go (1)
9-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating
PostUp/PreDown.The two functions are identical except for the
-Ivs-Dflag. Extracting a small helper (e.g.buildRules(flag string) []string) would remove the copy-paste and keep the IPv4/IPv6 and exclude-address logic in one place.♻️ Sketch
func (c *ClientConfig) buildRules(flag string) []string { addrs := c.GetExcludeAddrs() matchRule := fmt.Sprintf("! -o %s -m mark ! --mark $(awg show %s fwmark)", c.Name, c.Name) rules := make([]string, 0, 2+len(addrs)) rules = append(rules, fmt.Sprintf("iptables -%s OUTPUT %s -j DROP", flag, matchRule), fmt.Sprintf("ip6tables -%s OUTPUT %s -j DROP", flag, matchRule), ) for _, v := range addrs { execName := "iptables" if v.Addr().Is6() { execName = "ip6tables" } rules = append(rules, fmt.Sprintf("%s -%s OUTPUT %s -d %s -j ACCEPT", execName, flag, matchRule, v)) } return rules } func (c *ClientConfig) PostUp() []string { return c.buildRules("I") } func (c *ClientConfig) PreDown() []string { return c.buildRules("D") }🤖 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 `@amneziawg/client_config_unix.go` around lines 9 - 55, PostUp and PreDown duplicate the same rule-building logic and only differ by the iptables flag, so extract a shared helper from ClientConfig to keep the IPv4/IPv6 and exclude-address handling in one place. Add a small method such as buildRules(flag string) []string, move the common matchRule/addrs/rules construction into it, and have PostUp call it with the insert flag and PreDown call it with the delete flag. Keep the existing behavior for iptables vs ip6tables selection based on each address’s Addr().Is6().xray/client_config.go (1)
252-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix stale comment: "ServerConfig" → "ClientConfig".
The comment on line 252 says "Unmarshal the config data into the ServerConfig struct" but the method unmarshals into
ClientConfig.✏️ Proposed fix
- // Unmarshal the config data into the ServerConfig struct. + // Unmarshal the config data into the ClientConfig struct. if err := c.viper.Unmarshal(c); err != nil {🤖 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 `@xray/client_config.go` around lines 252 - 255, The comment in ClientConfig.Unmarshal is stale and refers to ServerConfig even though the method unmarshals into ClientConfig. Update the comment near c.viper.Unmarshal(c) to say ClientConfig instead of ServerConfig so it matches the actual target struct and stays consistent with the Unmarshal method.amneziawg/peer.go (1)
19-41: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNil-pointer dereference risk on
PreviousandCurrent.All five accessor methods dereference
p.Previousand/orp.Currentwithout nil guards. While the server always initializes both fields when creating peers, these methods will panic if aPeeris ever deserialized from JSON with missingcurrent/previousfields (the struct hasjsontags withomitempty).🛡️ Suggested nil guards
func (p *Peer) TotalDuration() time.Duration { + if p.Previous == nil || p.Current == nil { + return 0 + } return p.Previous.Duration() + p.Current.Duration() } func (p *Peer) TotalRxBytes() int64 { + if p.Previous == nil || p.Current == nil { + return 0 + } return p.Previous.RxBytes + p.Current.RxBytes } func (p *Peer) TotalTxBytes() int64 { + if p.Previous == nil || p.Current == nil { + return 0 + } return p.Previous.TxBytes + p.Current.TxBytes } func (p *Peer) CreatedAt() time.Time { + if p.Previous == nil { + return time.Time{} + } return p.Previous.CreatedAt } func (p *Peer) UpdatedAt() time.Time { + if p.Current == nil { + return time.Time{} + } return p.Current.UpdatedAt }🤖 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 `@amneziawg/peer.go` around lines 19 - 41, The accessor methods on Peer currently assume Previous and Current are always non-nil, which can panic when a Peer is deserialized with missing JSON fields. Update TotalDuration, TotalRxBytes, TotalTxBytes, CreatedAt, and UpdatedAt to guard against nil Previous/Current and return safe zero values when either side is absent, using the Peer type and its Previous/Current fields as the primary points to fix.v2ray/client.go (1)
263-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify Statistics by removing the unnecessary closure wrapper.
The
fnclosure capturesrawStatsanderrfrom the outer scope, adding cognitive complexity without benefit. The equivalent implementation inxray/client.go(lines 271–276) acquires the connection and queries stats directly — cleaner and more idiomatic.♻️ Proposed refactor
func (c *Client) Statistics(ctx context.Context) (int64, int64, error) { - var rawStats []proxycmd.Stat - - // Perform the gRPC call to fetch traffic stats - fn := func() (err error) { - conn, release := c.conn.Acquire() - if conn == nil { - return errors.New("acquiring connection: nil conn") - } - - defer release() - - // Send the request to get traffic stats - rawStats, err = proxycmd.QueryStats(ctx, conn, dialect, "", false) - if err != nil { - return fmt.Errorf("querying stats: %w", err) - } - - return nil - } - - // Execute stats query - if err := fn(); err != nil { - return 0, 0, err - } + conn, release := c.conn.Acquire() + if conn == nil { + return 0, 0, errors.New("acquiring connection: nil conn") + } + defer release() + + rawStats, err := proxycmd.QueryStats(ctx, conn, dialect, "", false) + if err != nil { + return 0, 0, fmt.Errorf("querying stats: %w", err) + } var download, upload int64🤖 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 `@v2ray/client.go` around lines 263 - 313, The Statistics method currently uses an unnecessary fn closure that only captures outer variables, making the flow harder to follow. Refactor Client.Statistics to acquire the connection, call proxycmd.QueryStats, and handle the error directly in the method body like the xray/client.go implementation, keeping the same error wrapping and rawStats processing without the extra closure.
🤖 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 `@amneziawg/server.go`:
- Around line 190-213: The peer sync loop in the periodic goroutine should not
exit on a transient error from s.IsRunning(); instead of returning from the func
passed to s.Go, handle the error locally and keep the loop alive. Update the
logic around IsRunning() in server.go to log the failure and continue (or add a
retry/backoff), while still preserving the existing early continue when the
service is simply not running.
- Around line 429-444: The peer stats rotation in server.go is never reached
because the `createdAt` zero value is compared against `v.Current.CreatedAt`, so
the `s.peers.Update` block never moves `Current` into `Previous`. Update the
`createdAt.After(...)` check in this peer statistics update path to use the
current timestamp (`now`) or another reset-detection condition, and ensure the
logic in `Peer.TotalRxBytes`, `Peer.TotalTxBytes`, and `types.NewPeerStatistics`
still correctly preserves pre-reset totals when rotation occurs.
In `@hysteria2/client_config.go`:
- Around line 103-129: `GetRouteExcludeIPv4Addrs` and `GetRouteExcludeIPv6Addrs`
currently call `c.serverIPs(context.Background())`, which can block forever
during DNS lookup. Update both methods to use a bounded context with a timeout
instead of `context.Background()`, and make sure any added timeout logic uses
the existing `serverIPs` path consistently. Add the needed `time` import and
keep the fix localized to these getter methods.
- Around line 36-67: Replace the panic paths in ipv4Prefixes and ipv6Prefixes
with non-fatal handling so invalid CIDR strings do not crash callers that use
the getters before Validate(). Update these helpers to either skip malformed
entries or return an error to the caller, and make sure the getter methods that
rely on them (such as GetIPv4Addr, GetRouteIPv4Addrs, and GetExcludeIPv4Addrs)
propagate or tolerate the result consistently. Apply the same behavior change to
both helpers so ipv4Prefixes and ipv6Prefixes remain symmetrical.
In `@hysteria2/client.go`:
- Around line 203-243: The Stop method in Client treats removeDNS and
removeFirewall as “best-effort” but currently returns early on those errors,
which prevents later cleanup and process termination. Update Client.Stop to
continue through all cleanup steps even if removeDNS or removeFirewall fails,
then still read the PID, resolve the process, and attempt proc.Terminate. Use
the existing symbols removeDNS, removeFirewall, readPID, procutils.NewProcess,
and Terminate, and aggregate or preserve cleanup errors so the final return
reports them without skipping termination.
In `@hysteria2/server.go`:
- Around line 227-231: If s.writePID fails after cmd.Start has already
succeeded, the started Hysteria2 process is left running without supervision
because the cmd.Wait goroutine is not yet in place. In the startup flow around
s.writePID, authListener.Close, and the cmd.Process handle, add cleanup that
explicitly kills the started process before returning the error, and keep the
existing listener close so the partially started server does not remain
orphaned.
In `@openvpn/client.go`:
- Around line 217-232: The c.Go goroutine wrapping c.cmd.Wait always turns any
non-nil result into a failure, so intentional termination during Stop still
bubbles up as an error; update that wait path in openvpn/client.go to check
ctx.Err() like the applyDNS goroutine and suppress errors caused by a normal
shutdown. Keep the existing timeout/exit handling in the c.cmd.Wait callback,
but only return a wrapped error from the wait goroutine when the context was not
canceled and the process exited unexpectedly.
- Around line 204-206: The Start path currently returns from c.writePID without
cleaning up the already-launched OpenVPN process, which can leave an orphaned
process if PID persistence fails. Update the error handling around c.writePID in
the Start logic to terminate c.cmd.Process before returning the wrapped error,
and make sure the cleanup uses the running process from
c.cmd.Process.Process/Pid so failures do not leave a live OpenVPN instance
behind.
In `@v2ray/client.go`:
- Around line 203-209: The Client.Stop method should not let a gRPC close
failure prevent process shutdown. Update Client.Stop so it retrieves the PID and
terminates the V2Ray process unconditionally before or independently of
c.conn.Close(), and treat the GRPCConn.Close call as best-effort cleanup rather
than a required step. Keep the logic in Client.Stop and the Manager.Stop
callback resilient so a failing or blocking conn.Close does not return early or
deadlock shutdown.
In `@xray/client_config.go`:
- Around line 199-208: `GetID()` panics on malformed UUIDs because `Validate()`
only checks that `ClientConfig.ID` is non-empty, so add UUID format validation
in `Validate()` alongside the existing empty-string check. Update the
`ClientConfig.Validate` method to parse `ID` with `uuid.Parse` and return a
validation error when it is not a valid UUID, and keep `GetID()` as the accessor
that assumes validated input. Ensure the fix is applied within the
`ClientConfig` validation path so non-empty but invalid IDs are rejected before
`GetID()` is called.
In `@xray/client.go`:
- Around line 208-243: Stop() currently closes the gRPC connection before PID
lookup and termination, so a close failure or blocking GRPCConn.Close() can
leave the Xray process running. Update Client.Stop to make process termination
unconditional: read the PID, resolve the process with procutils.NewProcess, and
terminate it before attempting c.conn.Close(). Keep the gRPC close best-effort
by handling its error without preventing the rest of the shutdown path from
completing.
In `@xray/server_config.go`:
- Around line 170-178: ServerConfig.Validate currently panics on port parse
failures instead of returning an error, which breaks the Validate contract.
Update the port parsing block that uses netip.NewPortFromString in
ServerConfig.Validate to return a wrapped error for both the parse failure and
the nil-port check, and keep the method’s error-returning behavior consistent
with inbound.Validate and the rest of Validate().
- Around line 181-196: The duplicate port checks in the server config loop can
hang when a range reaches 65535 because the loop index is a uint16 and wraps on
increment. Update the inbound and outbound loops in server_config.go to use a
wider loop variable in the logic around the inPortSet and outPortSet checks, and
cast only when using the value as a map key so the range comparison terminates
correctly.
In `@xray/server.go`:
- Around line 398-403: Add rollback handling in AddPeer so partial
proxycmd.AddUser failures don’t leave a half-provisioned peer behind. In the
loop over s.proxies, keep track of each successful AddUser call and, if a later
one fails, undo the already-added users and remove any peer state created for id
before returning the error. Use the existing AddPeer and RemovePeer flow in
xray/server.go as the place to centralize this cleanup so s.peers and the remote
inbounds stay consistent.
- Around line 287-309: The background sync loop in s.Go(ctx, func() error) stops
permanently because errors from s.IsRunning() and s.syncPeers(ctx) are returned
instead of being handled locally. Update the loop to treat these as transient
failures: log the wrapped error, then continue waiting for the next cycle
(optionally adding backoff), so the peer statistics worker keeps running. Use
the existing sync loop in xray/server.go and the s.IsRunning and s.syncPeers
call sites to make the change.
- Around line 333-346: Stop() currently terminates a PID from the file without
confirming it belongs to the Xray server, which can kill the wrong process if
the PID was reused. In the Stop method in server.go, add the same process-name
validation used by IsRunning() by checking the proc name before calling
proc.Terminate(), and return early or skip termination when the name does not
match xray. Use the existing procutils.NewProcess, proc.Name, and proc.Terminate
flow to keep the fix consistent with the current process handling.
- Around line 648-676: The stats rotation logic in the xray/server.go update
block is unreachable because createdAt is initialized to the zero time, so the
createdAt.After(v.Current.CreatedAt) check in the peer stats update path never
succeeds. Update the rotation timestamp in the surrounding stats loop (where
createdAt, now, and s.peers.Update are used) to a meaningful value such as now
or a real window boundary, so the Previous aggregation and
types.NewPeerStatistics(createdAt) path can execute; if rotation is not meant to
happen, remove the dead conditional and related reset code.
- Line 105: The process-name check in IsRunning is comparing proc.Name() against
the plain xray string, which fails on Windows because Name() returns the
executable basename like xray.exe. Update the comparison in xray/server.go to
use the Windows executable name (or normalize the process name before comparing)
so the xray process is detected correctly across platforms. Keep the fix
localized to the IsRunning logic and any xray name constant or helper it relies
on.
---
Minor comments:
In `@amneziawg/server_config.go`:
- Around line 117-121: The IPv6 validation error message in the server config
parsing logic is using the wrong field, so the failure reported by the
`netip.NewPrefix` check is misleading. Update the `IPv6Addr` parsing branch in
`server_config.go` to reference `c.IPv6Addr` in the `fmt.Errorf` message instead
of `c.IPv4Addr`, keeping the rest of the `c.IPv6Addr` validation flow unchanged.
In `@amneziawg/server.go`:
- Around line 336-364: Remove the state inconsistency in RemovePeer by
validating the local peer entry before issuing the awg set ... peer ... remove
command. In Server.RemovePeer, look up s.peers.Get(id) first, and only if the
peer exists proceed with exec.CommandContext and the pool release, then delete
it from s.peers; alternatively, if the interface removal already succeeded, do
not return an error just because the local map entry is missing. Use the
RemovePeer, s.peers.Get, and s.pools.Release flow to keep interface state and
local state aligned.
In `@hysteria2/client_config.go`:
- Around line 132-192: Add unsafe-character validation in ClientConfig.Validate
for the fields that are rendered into TOML basic strings, especially ServerAddr
and ObfsPassword. Reuse the same kind of check already used in the v2ray config
path (for example, utils.HasJSONUnsafeChars) and return a validation error if
either field contains quotes, backslashes, or newlines. Keep the new checks near
the existing empty-field validation in Validate so the TOML template
client_config.toml.tmpl cannot be corrupted by unsafe input.
In `@hysteria2/server.go`:
- Around line 557-581: `createdAt` in the traffic stats rollover path is
initialized to the zero time, so the `createdAt.After(v.Current.CreatedAt)`
branch in `server.go` never runs and the previous-stats accumulation in
`s.peers.Update` is dead code. Update the `createdAt` value in this stats
collection flow to a meaningful timestamp (for example, the server start time or
the session/traffic snapshot time if available), or remove the rollover block if
it is no longer needed; make sure the fix keeps the logic around `traffic`,
`now`, and `types.NewPeerStatistics` consistent.
In `@v2ray/server.go`:
- Around line 547-549: The no-op guard in syncPeers only checks RxBytes, so
updates can be skipped when TxBytes changes; update the early-return condition
to compare both RxBytes and TxBytes on the peer status object. Use the syncPeers
logic around QueryStats and the existing RxBytes/TxBytes assignments as the
place to adjust the guard, and apply the same fix to the matching implementation
in xray/server.go.
In `@xray/requests.go`:
- Around line 20-23: PeerRequest.Validate is currently a no-op, so zero-value
requests can slip through unchecked. Update the Validate method on PeerRequest
in requests.go to validate the UUID field and return an error when it is
uuid.Nil, while keeping the existing validation entry point in place. Use the
PeerRequest.Validate symbol to locate the fix and add the minimum UUID check
needed for this struct.
In `@xray/server.json.tmpl`:
- Around line 21-73: Add JSON-safety validation to the values rendered by
server.json.tmpl, since ServerConfig.Validate() currently only checks
required/enumerated fields and does not prevent invalid JSON from
operator-controlled strings. Update the validation logic on ServerConfig and the
nested Reality/TLS-related inputs used in the template render path to reject
quotes, backslashes, and control characters in fields like Reality.Dest,
Reality.Fingerprint, Reality.ServerNames, TLSCertFile, and TLSKeyFile before
template execution.
---
Nitpick comments:
In `@amneziawg/client_config_unix.go`:
- Around line 9-55: PostUp and PreDown duplicate the same rule-building logic
and only differ by the iptables flag, so extract a shared helper from
ClientConfig to keep the IPv4/IPv6 and exclude-address handling in one place.
Add a small method such as buildRules(flag string) []string, move the common
matchRule/addrs/rules construction into it, and have PostUp call it with the
insert flag and PreDown call it with the delete flag. Keep the existing behavior
for iptables vs ip6tables selection based on each address’s Addr().Is6().
In `@amneziawg/peer.go`:
- Around line 19-41: The accessor methods on Peer currently assume Previous and
Current are always non-nil, which can panic when a Peer is deserialized with
missing JSON fields. Update TotalDuration, TotalRxBytes, TotalTxBytes,
CreatedAt, and UpdatedAt to guard against nil Previous/Current and return safe
zero values when either side is absent, using the Peer type and its
Previous/Current fields as the primary points to fix.
In `@hysteria2/auth.go`:
- Around line 36-42: The request body read in the auth handler is unbounded, so
cap it before calling io.ReadAll(r.Body) by wrapping the body with
http.MaxBytesReader in the same request handling flow. Update the auth logic in
the handler that reads the body to enforce a reasonable maximum size and keep
the existing error handling path for oversized or invalid bodies.
In `@hysteria2/client_unix.go`:
- Around line 14-111: The Unix client firewall/DNS helpers are duplicated across
client implementations, so the fix is to extract the shared logic into a common
helper and make each client reuse it. Move the behavior behind a shared utility
or small interface that covers execFile, applyFirewall, removeFirewall,
waitForInterface, applyDNS, and removeDNS, then have Client in client_unix.go
delegate to that shared code instead of keeping identical copies. Keep the
existing method names and behavior intact where they are called so the refactor
is internal and minimizes churn.
In `@v2ray/client.go`:
- Around line 263-313: The Statistics method currently uses an unnecessary fn
closure that only captures outer variables, making the flow harder to follow.
Refactor Client.Statistics to acquire the connection, call proxycmd.QueryStats,
and handle the error directly in the method body like the xray/client.go
implementation, keeping the same error wrapping and rawStats processing without
the extra closure.
In `@xray/client_config.go`:
- Around line 252-255: The comment in ClientConfig.Unmarshal is stale and refers
to ServerConfig even though the method unmarshals into ClientConfig. Update the
comment near c.viper.Unmarshal(c) to say ClientConfig instead of ServerConfig so
it matches the actual target struct and stays consistent with the Unmarshal
method.
In `@xray/server_windows.go`:
- Around line 8-10: The execFile helper in Server builds the Windows executable
path with manual string concatenation, so update it to use filepath.Join for the
entire path assembly instead of prefixing with ".\\". Keep the logic in execFile
and replace the current leading segment handling with a filepath.Join call that
includes ".", "Xray", and name+".exe" so path construction stays idiomatic and
consistent.
In `@xray/server.go`:
- Around line 436-437: The peer-removal path in xray/server.go is using fragile
string matching on err.Error() to detect a missing peer, which can break if the
underlying gRPC/proxycmd message changes. Update the error handling in the peer
inbound alteration logic to use typed gRPC status checking instead of
strings.Contains, ideally by checking the status code for NotFound in the same
block that currently wraps the error with fmt.Errorf("altering peer %q inbound:
%w", id, err). Make sure the helper/imports used in this path clearly reference
the existing error handling around the peer inbound removal flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f85bf3cf-6d68-4d38-abb2-147180766485
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (104)
.golangci.yamlamneziawg/client.conf.tmplamneziawg/client.goamneziawg/client_config.goamneziawg/client_config.toml.tmplamneziawg/client_config_unix.goamneziawg/client_unix.goamneziawg/client_windows.goamneziawg/config.goamneziawg/crypto.goamneziawg/metadata.goamneziawg/obfs.goamneziawg/peer.goamneziawg/requests.goamneziawg/responses.goamneziawg/server.conf.tmplamneziawg/server.goamneziawg/server_config.goamneziawg/server_config.toml.tmplamneziawg/server_config_unix.goamneziawg/server_config_windows.goamneziawg/server_unix.goamneziawg/server_windows.gocore/client.gogo.modhysteria2/auth.gohysteria2/client.gohysteria2/client.yaml.tmplhysteria2/client_config.gohysteria2/client_config.toml.tmplhysteria2/client_config_unix.gohysteria2/client_unix.gohysteria2/client_windows.gohysteria2/common.gohysteria2/config.gohysteria2/metadata.gohysteria2/peer.gohysteria2/requests.gohysteria2/responses.gohysteria2/server.gohysteria2/server.yaml.tmplhysteria2/server_config.gohysteria2/server_config.toml.tmplhysteria2/server_unix.gohysteria2/server_windows.golibs/proxycmd/account.golibs/proxycmd/codec.golibs/proxycmd/command.golibs/proxycmd/wire.golibs/speedtest/speedtest.goopenvpn/client.conf.tmplopenvpn/client.goopenvpn/client_config.goopenvpn/client_config.toml.tmplopenvpn/client_config_unix.goopenvpn/client_unix.goopenvpn/client_windows.goopenvpn/requests.goopenvpn/server.conf.tmplopenvpn/server.goopenvpn/server_config.gotypes/service.goutils/validate.gov2ray/client.gov2ray/client.json.tmplv2ray/client_config.gov2ray/metadata.gov2ray/proxy.gov2ray/requests.gov2ray/server.gov2ray/server.json.tmplv2ray/server_config.gov2ray/transport.gov2ray/uuid.gowireguard/client.gowireguard/client_config.gowireguard/client_config_unix.gowireguard/client_unix.gowireguard/common.gowireguard/server.gowireguard/server_config.gowireguard/server_unix.gowireguard/server_windows.goxray/client.goxray/client.json.tmplxray/client_config.goxray/client_config.toml.tmplxray/client_unix.goxray/client_windows.goxray/common.goxray/config.goxray/metadata.goxray/peer.goxray/proxy.goxray/reality.goxray/requests.goxray/responses.goxray/server.goxray/server.json.tmplxray/server_config.goxray/server_config.toml.tmplxray/server_unix.goxray/server_windows.goxray/transport.go
💤 Files with no reviewable changes (1)
- v2ray/uuid.go
Comment on lines
+429
to
+444
| createdAt := time.Time{} | ||
| now := time.Now() | ||
|
|
||
| // Update peer statistics in thread-safe map. | ||
| s.peers.Update(columns[0], func(v Peer, found bool) (Peer, bool) { | ||
| if !found { | ||
| return v, false | ||
| } | ||
|
|
||
| if createdAt.After(v.Current.CreatedAt) { | ||
| v.Previous.RxBytes += v.Current.RxBytes | ||
| v.Previous.TxBytes += v.Current.TxBytes | ||
| v.Previous.UpdatedAt = now | ||
|
|
||
| v.Current = types.NewPeerStatistics(createdAt) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stats rotation logic is dead code — Previous statistics are never accumulated.
createdAt is initialized to time.Time{} (the zero value, January 1, year 1). The condition createdAt.After(v.Current.CreatedAt) at Line 438 will always be false because v.Current.CreatedAt is set to time.Now() at peer creation (Lines 320–321). As a result, the block at Lines 439–443 that moves Current stats into Previous and resets Current never executes.
This means Previous.RxBytes and Previous.TxBytes remain at their initial zero values forever. After a WireGuard counter reset (e.g., server restart), the pre-reset bandwidth totals are lost, and Peer.TotalRxBytes() / Peer.TotalTxBytes() will only reflect post-reset counters.
🐛 Proposed fix: use `now` instead of zero time
createdAt := time.Time{}
now := time.Now()
+ _ = createdAt // remove or restructureReplace createdAt with now in the condition, or restructure to detect counter resets (e.g., rxBytes < v.Current.RxBytes):
- createdAt := time.Time{}
now := time.Now()
s.peers.Update(columns[0], func(v Peer, found bool) (Peer, bool) {
if !found {
return v, false
}
- if createdAt.After(v.Current.CreatedAt) {
+ if now.After(v.Current.CreatedAt) {
v.Previous.RxBytes += v.Current.RxBytes
v.Previous.TxBytes += v.Current.TxBytes
v.Previous.UpdatedAt = now
- v.Current = types.NewPeerStatistics(createdAt)
+ v.Current = types.NewPeerStatistics(now)
}🤖 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 `@amneziawg/server.go` around lines 429 - 444, The peer stats rotation in
server.go is never reached because the `createdAt` zero value is compared
against `v.Current.CreatedAt`, so the `s.peers.Update` block never moves
`Current` into `Previous`. Update the `createdAt.After(...)` check in this peer
statistics update path to use the current timestamp (`now`) or another
reset-detection condition, and ensure the logic in `Peer.TotalRxBytes`,
`Peer.TotalTxBytes`, and `types.NewPeerStatistics` still correctly preserves
pre-reset totals when rotation occurs.
Comment on lines
+36
to
+67
| func ipv4Prefixes(addrs []string) []string { | ||
| out := make([]string, 0, len(addrs)) | ||
| for _, addr := range addrs { | ||
| prefix, err := netip.ParsePrefix(addr) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| if prefix.Addr().Is4() { | ||
| out = append(out, addr) | ||
| } | ||
| } | ||
|
|
||
| return out | ||
| } | ||
|
|
||
| // ipv6Prefixes returns the subset of addrs that are IPv6 prefixes. | ||
| func ipv6Prefixes(addrs []string) []string { | ||
| out := make([]string, 0, len(addrs)) | ||
| for _, addr := range addrs { | ||
| prefix, err := netip.ParsePrefix(addr) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| if prefix.Addr().Is6() { | ||
| out = append(out, addr) | ||
| } | ||
| } | ||
|
|
||
| return out | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace panic with error handling in ipv4Prefixes and ipv6Prefixes.
These functions panic on invalid CIDR input, but they're called by getter methods (GetIPv4Addr, GetRouteIPv4Addrs, GetExcludeIPv4Addrs, etc.) that may be invoked before Validate(). A library function should not crash the caller's process — it should return an error or skip invalid entries.
🛡️ Proposed fix: skip invalid prefixes instead of panicking
func ipv4Prefixes(addrs []string) []string {
out := make([]string, 0, len(addrs))
for _, addr := range addrs {
prefix, err := netip.ParsePrefix(addr)
if err != nil {
- panic(err)
+ continue
}
if prefix.Addr().Is4() {
out = append(out, addr)
}
}
return out
}Apply the same change to ipv6Prefixes.
🤖 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 `@hysteria2/client_config.go` around lines 36 - 67, Replace the panic paths in
ipv4Prefixes and ipv6Prefixes with non-fatal handling so invalid CIDR strings do
not crash callers that use the getters before Validate(). Update these helpers
to either skip malformed entries or return an error to the caller, and make sure
the getter methods that rely on them (such as GetIPv4Addr, GetRouteIPv4Addrs,
and GetExcludeIPv4Addrs) propagate or tolerate the result consistently. Apply
the same behavior change to both helpers so ipv4Prefixes and ipv6Prefixes remain
symmetrical.
Comment on lines
+103
to
+129
| func (c *ClientConfig) GetRouteExcludeIPv4Addrs() []string { | ||
| addrs := c.GetExcludeIPv4Addrs() | ||
|
|
||
| ips, _ := c.serverIPs(context.Background()) | ||
| for _, ip := range ips { | ||
| if ip.To4() != nil { | ||
| addrs = append(addrs, ip.String()+"/32") | ||
| } | ||
| } | ||
|
|
||
| return addrs | ||
| } | ||
|
|
||
| // GetRouteExcludeIPv6Addrs returns the IPv6 route excludes plus the server | ||
| // address, so the client's connection to the server bypasses the tunnel. | ||
| func (c *ClientConfig) GetRouteExcludeIPv6Addrs() []string { | ||
| addrs := c.GetExcludeIPv6Addrs() | ||
|
|
||
| ips, _ := c.serverIPs(context.Background()) | ||
| for _, ip := range ips { | ||
| if ip.To4() == nil && ip.To16() != nil { | ||
| addrs = append(addrs, ip.String()+"/128") | ||
| } | ||
| } | ||
|
|
||
| return addrs | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid unbounded context.Background() for DNS resolution in getter methods.
GetRouteExcludeIPv4Addrs and GetRouteExcludeIPv6Addrs call c.serverIPs(context.Background()) with no timeout. If DNS is slow or unresponsive, these getters will hang indefinitely during config generation. Use a bounded context instead.
🛡️ Proposed fix: add a timeout
func (c *ClientConfig) GetRouteExcludeIPv4Addrs() []string {
addrs := c.GetExcludeIPv4Addrs()
- ips, _ := c.serverIPs(context.Background())
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+
+ ips, _ := c.serverIPs(ctx)
for _, ip := range ips {
if ip.To4() != nil {
addrs = append(addrs, ip.String()+"/32")
}
}
return addrs
}Apply the same pattern to GetRouteExcludeIPv6Addrs. Add "time" to the import list.
🤖 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 `@hysteria2/client_config.go` around lines 103 - 129,
`GetRouteExcludeIPv4Addrs` and `GetRouteExcludeIPv6Addrs` currently call
`c.serverIPs(context.Background())`, which can block forever during DNS lookup.
Update both methods to use a bounded context with a timeout instead of
`context.Background()`, and make sure any added timeout logic uses the existing
`serverIPs` path consistently. Add the needed `time` import and keep the fix
localized to these getter methods.
Comment on lines
+203
to
+243
| // Stop stops the Hysteria2 client service. | ||
| func (c *Client) Stop() error { | ||
| return c.Manager.Stop(func() error { //nolint:wrapcheck | ||
| // Revert the system DNS (best-effort). | ||
| if err := c.removeDNS(context.Background()); err != nil { | ||
| return fmt.Errorf("removing dns: %w", err) | ||
| } | ||
|
|
||
| // Remove the kill-switch firewall rules (best-effort). | ||
| if err := c.removeFirewall(context.Background()); err != nil { | ||
| return fmt.Errorf("removing firewall rules: %w", err) | ||
| } | ||
|
|
||
| // Read PID from file. | ||
| pid, err := c.readPID() | ||
| if err != nil { | ||
| return fmt.Errorf("reading PID: %w", err) | ||
| } | ||
|
|
||
| if pid == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| // Retrieve process with the given PID. | ||
| proc, err := procutils.NewProcess(pid) | ||
| if err != nil { | ||
| if utils.ErrorIs(err, procutils.ErrorProcessNotRunning) { | ||
| return nil | ||
| } | ||
|
|
||
| return fmt.Errorf("getting process for PID %d: %w", pid, err) | ||
| } | ||
|
|
||
| // Terminate the process. | ||
| if err := proc.Terminate(); err != nil { | ||
| return fmt.Errorf("terminating process: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop() comments say "best-effort" but errors prevent process termination.
Lines 207 and 212 label DNS and firewall removal as "best-effort", yet the code returns immediately on error — skipping subsequent cleanup and process termination. If removeDNS fails, firewall rules are not removed; if removeFirewall fails, the Hysteria2 process is left running. This can leave the system in a broken state (e.g., kill-switch firewall active with no VPN tunnel).
Collect cleanup errors but always proceed to terminate the process:
🔧 Proposed fix
func (c *Client) Stop() error {
return c.Manager.Stop(func() error { //nolint:wrapcheck
+ var cleanupErrs []error
+
// Revert the system DNS (best-effort).
if err := c.removeDNS(context.Background()); err != nil {
- return fmt.Errorf("removing dns: %w", err)
+ cleanupErrs = append(cleanupErrs, fmt.Errorf("removing dns: %w", err))
}
// Remove the kill-switch firewall rules (best-effort).
if err := c.removeFirewall(context.Background()); err != nil {
- return fmt.Errorf("removing firewall rules: %w", err)
+ cleanupErrs = append(cleanupErrs, fmt.Errorf("removing firewall rules: %w", err))
}
// Read PID from file.
pid, err := c.readPID()
if err != nil {
return fmt.Errorf("reading PID: %w", err)
}
if pid == 0 {
+ return errors.Join(cleanupErrs...)
return nil
}
// Retrieve process with the given PID.
proc, err := procutils.NewProcess(pid)
if err != nil {
if utils.ErrorIs(err, procutils.ErrorProcessNotRunning) {
+ return errors.Join(cleanupErrs...)
return nil
}
return fmt.Errorf("getting process for PID %d: %w", pid, err)
}
// Terminate the process.
if err := proc.Terminate(); err != nil {
return fmt.Errorf("terminating process: %w", err)
}
+ return errors.Join(cleanupErrs...)
return nil
})
}📝 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
| // Stop stops the Hysteria2 client service. | |
| func (c *Client) Stop() error { | |
| return c.Manager.Stop(func() error { //nolint:wrapcheck | |
| // Revert the system DNS (best-effort). | |
| if err := c.removeDNS(context.Background()); err != nil { | |
| return fmt.Errorf("removing dns: %w", err) | |
| } | |
| // Remove the kill-switch firewall rules (best-effort). | |
| if err := c.removeFirewall(context.Background()); err != nil { | |
| return fmt.Errorf("removing firewall rules: %w", err) | |
| } | |
| // Read PID from file. | |
| pid, err := c.readPID() | |
| if err != nil { | |
| return fmt.Errorf("reading PID: %w", err) | |
| } | |
| if pid == 0 { | |
| return nil | |
| } | |
| // Retrieve process with the given PID. | |
| proc, err := procutils.NewProcess(pid) | |
| if err != nil { | |
| if utils.ErrorIs(err, procutils.ErrorProcessNotRunning) { | |
| return nil | |
| } | |
| return fmt.Errorf("getting process for PID %d: %w", pid, err) | |
| } | |
| // Terminate the process. | |
| if err := proc.Terminate(); err != nil { | |
| return fmt.Errorf("terminating process: %w", err) | |
| } | |
| return nil | |
| }) | |
| } | |
| // Stop stops the Hysteria2 client service. | |
| func (c *Client) Stop() error { | |
| return c.Manager.Stop(func() error { //nolint:wrapcheck | |
| var cleanupErrs []error | |
| // Revert the system DNS (best-effort). | |
| if err := c.removeDNS(context.Background()); err != nil { | |
| cleanupErrs = append(cleanupErrs, fmt.Errorf("removing dns: %w", err)) | |
| } | |
| // Remove the kill-switch firewall rules (best-effort). | |
| if err := c.removeFirewall(context.Background()); err != nil { | |
| cleanupErrs = append(cleanupErrs, fmt.Errorf("removing firewall rules: %w", err)) | |
| } | |
| // Read PID from file. | |
| pid, err := c.readPID() | |
| if err != nil { | |
| return fmt.Errorf("reading PID: %w", err) | |
| } | |
| if pid == 0 { | |
| return errors.Join(cleanupErrs...) | |
| return nil | |
| } | |
| // Retrieve process with the given PID. | |
| proc, err := procutils.NewProcess(pid) | |
| if err != nil { | |
| if utils.ErrorIs(err, procutils.ErrorProcessNotRunning) { | |
| return errors.Join(cleanupErrs...) | |
| return nil | |
| } | |
| return fmt.Errorf("getting process for PID %d: %w", pid, err) | |
| } | |
| // Terminate the process. | |
| if err := proc.Terminate(); err != nil { | |
| return fmt.Errorf("terminating process: %w", err) | |
| } | |
| return errors.Join(cleanupErrs...) | |
| return nil | |
| }) | |
| } |
🤖 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 `@hysteria2/client.go` around lines 203 - 243, The Stop method in Client treats
removeDNS and removeFirewall as “best-effort” but currently returns early on
those errors, which prevents later cleanup and process termination. Update
Client.Stop to continue through all cleanup steps even if removeDNS or
removeFirewall fails, then still read the PID, resolve the process, and attempt
proc.Terminate. Use the existing symbols removeDNS, removeFirewall, readPID,
procutils.NewProcess, and Terminate, and aggregate or preserve cleanup errors so
the final return reports them without skipping termination.
Comment on lines
+333
to
+346
| // Retrieve process with the given PID. | ||
| proc, err := procutils.NewProcess(pid) | ||
| if err != nil { | ||
| if utils.ErrorIs(err, procutils.ErrorProcessNotRunning) { | ||
| return nil | ||
| } | ||
|
|
||
| return fmt.Errorf("getting process for PID %d: %w", pid, err) | ||
| } | ||
|
|
||
| // Terminate the process. | ||
| if err := proc.Terminate(); err != nil { | ||
| return fmt.Errorf("terminating process: %w", err) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Stop() terminates process by PID without verifying process name.
Stop() reads a PID from file and calls proc.Terminate() without checking that the process is actually the Xray server (unlike IsRunning() which validates name != xray). If the PID file is stale and the PID was reused by an unrelated process, Stop() would terminate the wrong process. Add a process name check before terminating, mirroring the validation in IsRunning().
🔒 Proposed fix: verify process name before terminating
// Retrieve the process with the given PID.
proc, err := procutils.NewProcess(pid)
if err != nil {
if utils.ErrorIs(err, procutils.ErrorProcessNotRunning) {
return nil
}
return fmt.Errorf("getting process for PID %d: %w", pid, err)
}
+ // Verify the process is actually the Xray server before terminating.
+ name, err := proc.Name()
+ if err != nil {
+ return fmt.Errorf("getting process name: %w", err)
+ }
+ if name != xray {
+ return nil
+ }
+
// Terminate the process.
if err := proc.Terminate(); err != nil {
return fmt.Errorf("terminating process: %w", err)
}📝 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
| // Retrieve process with the given PID. | |
| proc, err := procutils.NewProcess(pid) | |
| if err != nil { | |
| if utils.ErrorIs(err, procutils.ErrorProcessNotRunning) { | |
| return nil | |
| } | |
| return fmt.Errorf("getting process for PID %d: %w", pid, err) | |
| } | |
| // Terminate the process. | |
| if err := proc.Terminate(); err != nil { | |
| return fmt.Errorf("terminating process: %w", err) | |
| } | |
| // Retrieve process with the given PID. | |
| proc, err := procutils.NewProcess(pid) | |
| if err != nil { | |
| if utils.ErrorIs(err, procutils.ErrorProcessNotRunning) { | |
| return nil | |
| } | |
| return fmt.Errorf("getting process for PID %d: %w", pid, err) | |
| } | |
| // Verify the process is actually the Xray server before terminating. | |
| name, err := proc.Name() | |
| if err != nil { | |
| return fmt.Errorf("getting process name: %w", err) | |
| } | |
| if name != xray { | |
| return nil | |
| } | |
| // Terminate the process. | |
| if err := proc.Terminate(); err != nil { | |
| return fmt.Errorf("terminating process: %w", err) | |
| } |
🤖 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 `@xray/server.go` around lines 333 - 346, Stop() currently terminates a PID
from the file without confirming it belongs to the Xray server, which can kill
the wrong process if the PID was reused. In the Stop method in server.go, add
the same process-name validation used by IsRunning() by checking the proc name
before calling proc.Terminate(), and return early or skip termination when the
name does not match xray. Use the existing procutils.NewProcess, proc.Name, and
proc.Terminate flow to keep the fix consistent with the current process
handling.
Comment on lines
+648
to
+676
| createdAt := time.Time{} | ||
| now := time.Now() | ||
|
|
||
| // Apply collected stats to the thread-safe map | ||
| for id, stat := range stats { | ||
| s.peers.Update(id, func(v Peer, ok bool) (Peer, bool) { | ||
| if !ok { | ||
| return v, false | ||
| } | ||
|
|
||
| if createdAt.After(v.Current.CreatedAt) { | ||
| v.Previous.RxBytes += v.Current.RxBytes | ||
| v.Previous.TxBytes += v.Current.TxBytes | ||
| v.Previous.UpdatedAt = now | ||
|
|
||
| v.Current = types.NewPeerStatistics(createdAt) | ||
| } | ||
|
|
||
| if v.Current.RxBytes > 0 && stat.RxBytes == v.Current.RxBytes { | ||
| return v, false | ||
| } | ||
|
|
||
| v.Current.RxBytes = stat.RxBytes | ||
| v.Current.TxBytes = stat.TxBytes | ||
| v.Current.UpdatedAt = now | ||
|
|
||
| return v, true | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stats rotation block is dead code due to zero-value createdAt.
createdAt := time.Time{} sets the variable to the zero time (January 1, year 1). The condition createdAt.After(v.Current.CreatedAt) is always false because no valid CreatedAt can precede the zero time. This means the Previous statistics are never populated from Current, and types.NewPeerStatistics(createdAt) on line 663 is never reached. If rotation is intended, createdAt should be set to a meaningful timestamp (e.g., now or a window boundary). If rotation is intentionally disabled, the dead code should be removed for clarity.
🔧 Proposed fix: use `now` for rotation timestamp
- createdAt := time.Time{}
now := time.Now()
// Apply collected stats to the thread-safe map
for id, stat := range stats {
s.peers.Update(id, func(v Peer, ok bool) (Peer, bool) {
if !ok {
return v, false
}
- if createdAt.After(v.Current.CreatedAt) {
+ if now.After(v.Current.CreatedAt) {
v.Previous.RxBytes += v.Current.RxBytes
v.Previous.TxBytes += v.Current.TxBytes
v.Previous.UpdatedAt = now
- v.Current = types.NewPeerStatistics(createdAt)
+ v.Current = types.NewPeerStatistics(now)
}📝 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
| createdAt := time.Time{} | |
| now := time.Now() | |
| // Apply collected stats to the thread-safe map | |
| for id, stat := range stats { | |
| s.peers.Update(id, func(v Peer, ok bool) (Peer, bool) { | |
| if !ok { | |
| return v, false | |
| } | |
| if createdAt.After(v.Current.CreatedAt) { | |
| v.Previous.RxBytes += v.Current.RxBytes | |
| v.Previous.TxBytes += v.Current.TxBytes | |
| v.Previous.UpdatedAt = now | |
| v.Current = types.NewPeerStatistics(createdAt) | |
| } | |
| if v.Current.RxBytes > 0 && stat.RxBytes == v.Current.RxBytes { | |
| return v, false | |
| } | |
| v.Current.RxBytes = stat.RxBytes | |
| v.Current.TxBytes = stat.TxBytes | |
| v.Current.UpdatedAt = now | |
| return v, true | |
| }) | |
| } | |
| now := time.Now() | |
| // Apply collected stats to the thread-safe map | |
| for id, stat := range stats { | |
| s.peers.Update(id, func(v Peer, ok bool) (Peer, bool) { | |
| if !ok { | |
| return v, false | |
| } | |
| if now.After(v.Current.CreatedAt) { | |
| v.Previous.RxBytes += v.Current.RxBytes | |
| v.Previous.TxBytes += v.Current.TxBytes | |
| v.Previous.UpdatedAt = now | |
| v.Current = types.NewPeerStatistics(now) | |
| } | |
| if v.Current.RxBytes > 0 && stat.RxBytes == v.Current.RxBytes { | |
| return v, false | |
| } | |
| v.Current.RxBytes = stat.RxBytes | |
| v.Current.TxBytes = stat.TxBytes | |
| v.Current.UpdatedAt = now | |
| return v, true | |
| }) | |
| } |
🤖 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 `@xray/server.go` around lines 648 - 676, The stats rotation logic in the
xray/server.go update block is unreachable because createdAt is initialized to
the zero time, so the createdAt.After(v.Current.CreatedAt) check in the peer
stats update path never succeeds. Update the rotation timestamp in the
surrounding stats loop (where createdAt, now, and s.peers.Update are used) to a
meaningful value such as now or a real window boundary, so the Previous
aggregation and types.NewPeerStatistics(createdAt) path can execute; if rotation
is not meant to happen, remove the dead conditional and related reset code.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openvpn/server.go (1)
577-591: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging when peer entries are skipped during sync.
The switch to
continuefor parse errors is a good resilience improvement — one malformed entry won't abort the entire sync cycle. However, persistent parse failures for a specific peer would be silently ignored, making it difficult to diagnose stale statistics. A debug-level log when a peer is skipped would help surface systemic issues without affecting the happy path.♻️ Optional: add debug logging for skipped peers
rxBytes, err := strconv.ParseInt(fields[5], 10, 64) if err != nil { + log.Debug().Str("id", id).Err(err).Msg("skipping peer: parsing rx bytes") continue } txBytes, err := strconv.ParseInt(fields[6], 10, 64) if err != nil { + log.Debug().Str("id", id).Err(err).Msg("skipping peer: parsing tx bytes") continue } sec, err := strconv.ParseInt(fields[8], 10, 64) if err != nil { + log.Debug().Str("id", id).Err(err).Msg("skipping peer: parsing connected-since timestamp") continue }🤖 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 `@openvpn/server.go` around lines 577 - 591, Peer entries with parse errors are now silently skipped in the sync path, which makes repeated malformed records hard to diagnose. In the peer sync logic around the parsing of txBytes and sec in the OpenVPN server code, add a debug-level log before each continue that identifies the skipped peer entry and includes the parse failure context, while keeping the resilient continue behavior unchanged.v2ray/client_config.go (1)
118-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTag() simplification risks duplicate tags for outbounds sharing the same addr+port.
The tag now only includes
AddrandPort, removing protocol/transport-security discriminators. If a client config has multiple outbounds to the same server with different protocols, the tags collide. V2Ray requires unique outbound tags for routing rules.Consider adding a duplicate-tag check in
ClientConfig.Validate():♻️ Suggested validation for duplicate tags
func (c *ClientConfig) Validate() error { // Validate the API client configuration. if err := c.API.Validate(); err != nil { return fmt.Errorf("validating API config: %w", err) } if _, err := uuid.Parse(c.ID); err != nil { return fmt.Errorf("parsing id %q: %w", c.ID, err) } if len(c.Outbounds) == 0 { return errors.New("outbounds are empty") } + // Check for duplicate outbound tags. + seen := make(map[string]struct{}, len(c.Outbounds)) // Validate each outbound client configuration. for _, outbound := range c.Outbounds { if err := outbound.Validate(); err != nil { return fmt.Errorf("validating outbound config: %w", err) } + tag := outbound.Tag() + if _, ok := seen[tag]; ok { + return fmt.Errorf("duplicate outbound tag %q", tag) + } + seen[tag] = struct{}{} }🤖 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 `@v2ray/client_config.go` around lines 118 - 123, OutboundClientConfig.Tag() now only uses Addr and Port, so multiple outbounds to the same endpoint can collide when they differ by protocol or transport settings. Fix this by adding a duplicate-tag validation in ClientConfig.Validate() that checks the Tag() values for all outbounds and rejects configs with repeated tags, using the existing OutboundClientConfig.Tag() and ClientConfig.Validate() symbols to locate the change.
🤖 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.
Nitpick comments:
In `@openvpn/server.go`:
- Around line 577-591: Peer entries with parse errors are now silently skipped
in the sync path, which makes repeated malformed records hard to diagnose. In
the peer sync logic around the parsing of txBytes and sec in the OpenVPN server
code, add a debug-level log before each continue that identifies the skipped
peer entry and includes the parse failure context, while keeping the resilient
continue behavior unchanged.
In `@v2ray/client_config.go`:
- Around line 118-123: OutboundClientConfig.Tag() now only uses Addr and Port,
so multiple outbounds to the same endpoint can collide when they differ by
protocol or transport settings. Fix this by adding a duplicate-tag validation in
ClientConfig.Validate() that checks the Tag() values for all outbounds and
rejects configs with repeated tags, using the existing
OutboundClientConfig.Tag() and ClientConfig.Validate() symbols to locate the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac99f11d-c0fe-4f81-8500-382ca8ee68f1
📒 Files selected for processing (35)
amneziawg/client.conf.tmplamneziawg/client_config.toml.tmplamneziawg/client_config_unix.goamneziawg/obfs.goamneziawg/server.gohysteria2/client.gohysteria2/client_config.gohysteria2/client_config_unix.gohysteria2/client_unix.gohysteria2/server.gohysteria2/server.yaml.tmpllibs/crypto/pki.goopenvpn/client.goopenvpn/client_config.toml.tmplopenvpn/client_unix.goopenvpn/server.goopenvpn/server_config_windows.gov2ray/client.gov2ray/client_config.gov2ray/client_config.toml.tmplv2ray/server.gov2ray/server_config.gowireguard/client_config.gowireguard/client_config.toml.tmplwireguard/client_config_unix.gowireguard/client_config_windows.gowireguard/server.gowireguard/server_config.goxray/client.goxray/client_config.goxray/client_config.toml.tmplxray/reality.goxray/server.goxray/server.json.tmplxray/server_config.go
💤 Files with no reviewable changes (3)
- amneziawg/client_config.toml.tmpl
- amneziawg/client.conf.tmpl
- xray/server.json.tmpl
✅ Files skipped from review due to trivial changes (3)
- wireguard/client_config_windows.go
- xray/client_config.toml.tmpl
- v2ray/client_config.toml.tmpl
🚧 Files skipped from review as they are similar to previous changes (17)
- hysteria2/server.yaml.tmpl
- wireguard/server_config.go
- amneziawg/obfs.go
- hysteria2/client_unix.go
- xray/reality.go
- amneziawg/client_config_unix.go
- xray/client.go
- hysteria2/client.go
- v2ray/client.go
- amneziawg/server.go
- v2ray/server.go
- openvpn/client_unix.go
- openvpn/client.go
- hysteria2/client_config.go
- hysteria2/server.go
- xray/client_config.go
- xray/server.go
ironman0x7b2
force-pushed
the
feat/add-xray-amneziawg-hysteria2
branch
from
July 9, 2026 10:41
387951a to
4a0a1b2
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.
Summary by CodeRabbit