feat(interop): IVPN client interoperability#2135
Merged
stenya merged 13 commits intodevelopmentfrom Mar 10, 2026
Merged
Conversation
Allow Portmaster to cooperate with the IVPN client: - Accept IVPN VPN tunnel and service process connections - Delegate DNS control to Portmaster when custom DNS is configured - Auto-connect to IVPN daemon on startup and on ping - Hook into firewall verdict pipeline via new ExtVerdictHandler
Every connection verdict previously acquired an RWMutex to read IVPN state. Replace it with atomic.Pointer[clientStatus] using an immutable snapshot (copy-on-write) so reads are a single pointer load with no locking on the per-packet hot path. Apply the same pattern to the external verdict handler: replace a data-racy plain function variable and two auxiliary atomic.Bool flags with a single atomic.Pointer[ExtVerdictHandlerFunc]. Use CompareAndSwap for set-once semantics. Move the load into the default branch of filterHandler so pre-authenticated and DNS-redirect connections pay zero cost.
…e data in firewall verdicts
…th active remote endpoint
…ug configurations for portmaster-core
…ception is paused Add EventStartStopState event manager and IsStarted() method to the Interception module so other modules can react to start/stop state changes. Update IVPN interop to subscribe to interception start/stop events and skip applying custom DNS settings when interception is inactive, ensuring correct behavior when Portmaster is in the Paused state.
Bugs fixed: - Inbound UDP from VPN server incorrectly blocked - Firewall verdicts possible before client status initialized Also: move interop before interception in startup order, simplify DNS state tracking.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a new Interoperability module (with IVPN integration) that can register external firewall verdicts and manage DNS synchronization; wires the module into service startup. Also introduces notification suppression persistence and visibility flags, updates firewall packet handling to accept external verdicts, plus dependency and editor config changes. Changes
Sequence DiagramsequenceDiagram
participant ServiceInit as Service Init
participant Interop as Interoperability
participant IVPN as IVPN Interop
participant IVPNClient as IVPN Client
participant Firewall as Firewall
participant Network as Packet Handler
ServiceInit->>Interop: New(instance) / prep()
Interop->>Interop: register modules (IVPN), register API
Interop->>Firewall: EnsureVerdictHandlerRegistered()
Firewall->>Firewall: SetExternalVerdictHandler(verdict_handler)
Interop->>IVPN: Start()
IVPN->>IVPNClient: connect & hello
IVPNClient-->>IVPN: ConnectionStarting/Stopped events
IVPN->>Interop: update DNS state via Interoperability config events
Network->>Firewall: processPacket(conn)
Firewall->>Firewall: filterHandler invokes external verdict
Firewall->>Interop: external verdict handler(conn)
Interop->>IVPN: call module.VerdictHandler(conn)
IVPN-->>Interop: verdict (Accept/Block/Undecided, skipTunnel)
Interop-->>Firewall: verdict result
Firewall->>Network: apply verdict, maybe skip tunneling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
service/interop/api.go (2)
30-30: Inconsistent receiver naming.The receiver is named
chere butiis used in other methods in this package. Use consistent naming.♻️ Consistent receiver name
-func (c *Interoperability) handlePing(r *api.Request) (msg string, err error) { +func (i *Interoperability) handlePing(r *api.Request) (msg string, err error) { // Received ping, possibly from IVPN client // Try to connect to IVPN client if not already connected - for _, im := range c.interopModules { + for _, im := range i.interopModules { if err := im.PingHandler(); err != nil { - c.mgr.Warn("Failed to handle ping for interoperability module: " + err.Error()) + i.mgr.Warn("Failed to handle ping for interoperability module: " + err.Error())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/interop/api.go` at line 30, The method receiver for Interoperability is named `c` in `handlePing` but other methods use `i`; rename the receiver from `c` to `i` in the function signature of `handlePing` (function name: handlePing, type: Interoperability) so it matches the package-wide convention and avoids inconsistent receiver naming.
11-13: Unused struct field.The
pingParamsstruct and itsMessagefield are defined but never used inhandlePing. Either remove the unused struct or implement the intended functionality.🗑️ Remove unused struct
-type pingParams struct { - Message bool `json:"message"` -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/interop/api.go` around lines 11 - 13, The pingParams struct (type pingParams with field Message) is unused by handlePing; remove the dead declaration to avoid unused field warnings or, if intended, update handlePing to decode JSON into pingParams and use pingParams.Message in the response; locate the type pingParams and either delete it or add JSON decoding and usage in the handlePing function to consume Message.service/interop/verdict_handler.go (1)
7-15: Method name should use camelCase per Go conventions.The method
verdict_handleruses snake_case which is unconventional in Go. Standard Go naming uses camelCase for unexported identifiers.♻️ Rename to camelCase
-func (i *Interoperability) verdict_handler(conn *network.Connection) (verdict network.Verdict, reason string, skipTunnel bool) { +func (i *Interoperability) verdictHandler(conn *network.Connection) (verdict network.Verdict, reason string, skipTunnel bool) {Also update the reference in
service/interop/module.goline 127:- firewall.SetExternalVerdictHandler(i.verdict_handler) + firewall.SetExternalVerdictHandler(i.verdictHandler)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/interop/verdict_handler.go` around lines 7 - 15, Rename the method verdict_handler to camelCase (verdictHandler) on the Interoperability receiver and update all call sites (e.g., the call referenced in service/interop/module.go at the previous line 127) to use verdictHandler; ensure the method signature (func (i *Interoperability) verdictHandler(conn *network.Connection) (network.Verdict, string, bool)) and any variable shadowing inside the loop (verdict, reason, skipTunnel) remain correct and compile after renaming.service/interop/ivpn/ivpn.go (2)
279-281: Broad allowlist for IVPN service binary.All network traffic from the IVPN service binary is unconditionally accepted. While this is likely necessary for IVPN to function, document this trust assumption clearly.
📝 Add comment clarifying the trust model
+ // Trust all traffic from the IVPN service binary. + // This is required for IVPN to establish and maintain VPN connections, + // including control plane traffic that may not match the VPN tunnel endpoint. if status.serviceBinary != "" && conn.Process().Path == status.serviceBinary { return network.VerdictAccept, "IVPN Service connection", false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/interop/ivpn/ivpn.go` around lines 279 - 281, Add a clear code comment above the conditional that unconditionally accepts connections from the IVPN service binary (the block checking status.serviceBinary != "" && conn.Process().Path == status.serviceBinary and returning network.VerdictAccept) explaining the trust model: state that any process matching status.serviceBinary is fully trusted by this filter, why that trust is required for IVPN to function, and any limitations/assumptions (e.g., relies on path integrity, not performing further validation like signature checking or user ownership); reference the symbols status.serviceBinary, conn.Process().Path, and network.VerdictAccept so reviewers know exactly which check the comment applies to.
117-121: Consider debug logging for connection failures.Silently ignoring errors when the IVPN client isn't running is intentional, but having debug-level logging would help troubleshoot cases where the client is running but connections fail for other reasons.
💡 Add debug logging
client, err := ivpnclient.NewClientAsRoot(nil, time.Second*10, ci) if err != nil { + // Client not running or other connection issue - this is expected + // when IVPN is not installed or not active return nil }Also applies to: 133-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/interop/ivpn/ivpn.go` around lines 117 - 121, The ivpnclient.NewClientAsRoot call currently swallows errors (client, err := ivpnclient.NewClientAsRoot(...); if err != nil { return nil })—add a debug-level log that emits the error details before returning so connection failures are visible during diagnosis; specifically log err (e.g., logger.Debugf or similar) in the NewClientAsRoot error branch and apply the same change to the second identical block later (the other NewClientAsRoot error check around lines noted in the review) so both silent failures are recorded at debug level.service/firewall/packet_handler.go (1)
507-520: Log message format could be clearer.The log format at line 517 produces output like
"filter: special verdict Accept "IVPN VPN connection" <conn> for connection"which reads awkwardly due to the trailing "for connection" and the connection object placement.💡 Suggested log format improvement
- log.Tracer(pkt.Ctx()).Infof("filter: special verdict %s %q %s for connection", verdict, reason, conn) + log.Tracer(pkt.Ctx()).Infof("filter: external verdict %s for %s: %q", verdict, conn, reason)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/firewall/packet_handler.go` around lines 507 - 520, The log message is awkwardly formatted; update the Infof call (the one using log.Tracer(pkt.Ctx()).Infof inside the externalVerdictHandler handling) to produce a clearer, self-contained message by removing the trailing "for connection" and placing the connection representation in a sensible spot (or using conn.String()/%v explicitly) so the output reads like "filter: special verdict <verdict> <reason> for <conn>" or similar; keep references to externalVerdictHandler.Load(), conn.SetVerdict(...), and pkt.Ctx() so the change is localized to that Infof invocation.service/interop/module.go (2)
92-92: Minor typo in comment."Ot is ok" should be "It is ok".
📝 Fix typo
- // Ot is ok to check it once at startup, since it can only be changed by restarting Portmaster with a different config. + // It is ok to check it once at startup, since it can only be changed by restarting Portmaster with a different config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/interop/module.go` at line 92, Fix the typo in the comment that currently reads "Ot is ok to check it once at startup, since it can only be changed by restarting Portmaster with a different config." — change "Ot is ok" to "It is ok" in the comment containing that exact phrase so the comment reads "It is ok to check it once at startup, since it can only be changed by restarting Portmaster with a different config."
125-128: Error from SetExternalVerdictHandler is silently ignored.
firewall.SetExternalVerdictHandlerreturns an error if a handler is already set, but this error is discarded. While the atomic flag prevents double registration from this module, logging a warning would help diagnose issues if another component unexpectedly registers a handler first.💡 Log the error
func (i *Interoperability) EnsureVerdictHandlerRegistered() { if i.verdictHandlerRegistered.CompareAndSwap(false, true) { - firewall.SetExternalVerdictHandler(i.verdict_handler) + if err := firewall.SetExternalVerdictHandler(i.verdictHandler); err != nil { + i.mgr.Warn("failed to register external verdict handler: " + err.Error()) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/interop/module.go` around lines 125 - 128, EnsureVerdictHandlerRegistered currently ignores the error returned by firewall.SetExternalVerdictHandler; update the function to capture that error and log a warning if non-nil. Specifically, after calling firewall.SetExternalVerdictHandler(i.verdict_handler) inside EnsureVerdictHandlerRegistered, check the returned error and emit a warning that includes the error and context (e.g., another component may have already registered a handler). Use the module's logger (e.g., i.logger.Warnf or similar) or fallback to the package logger to record the warning while keeping the atomic verdictHandlerRegistered logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vscode/launch.json:
- Around line 19-25: The linux debug configuration currently uses "asRoot": true
with the Go debugger's default "mode": "auto", which causes the build to run as
root; change the configuration to set "mode": "exec" and add a "preLaunchTask"
that runs the build (e.g., go build --output=<bin> ...) as your normal user
before launch, keep "asRoot": true only for the launched executable, and keep
existing properties like "console" and "args" in the "linux" configuration so
the debug session executes the already-built binary with root privileges without
performing the build as root.
In `@service/interop/api.go`:
- Around line 15-28: The /interop/ping endpoint currently registers
APIEndpointPing with api.PermitAnyone for Read and Write, allowing
unauthenticated callers to trigger handlePing/PingHandler (which calls
connHandler.Go()); change the permissions in registerAPIEndpoints to use
api.PermitUser for both Read and Write so only authenticated users can invoke
this endpoint, and verify handlePing/PingHandler enforce/expect authenticated
context (adjust any auth checks or documentation accordingly).
In `@service/interop/module.go`:
- Around line 77-79: The stop() function currently does nothing; add a Stop()
error method to the interopModule interface and implement it on the IVPN module
(the IVPN type that currently provides Start()), then update stop() to iterate
the interopModules slice and call each module's Stop(), aggregating or logging
errors and returning a combined error if any; ensure the IVPN implementation
closes network connections and other resources gracefully and mirrors
Start()/Stop() semantics.
---
Nitpick comments:
In `@service/firewall/packet_handler.go`:
- Around line 507-520: The log message is awkwardly formatted; update the Infof
call (the one using log.Tracer(pkt.Ctx()).Infof inside the
externalVerdictHandler handling) to produce a clearer, self-contained message by
removing the trailing "for connection" and placing the connection representation
in a sensible spot (or using conn.String()/%v explicitly) so the output reads
like "filter: special verdict <verdict> <reason> for <conn>" or similar; keep
references to externalVerdictHandler.Load(), conn.SetVerdict(...), and pkt.Ctx()
so the change is localized to that Infof invocation.
In `@service/interop/api.go`:
- Line 30: The method receiver for Interoperability is named `c` in `handlePing`
but other methods use `i`; rename the receiver from `c` to `i` in the function
signature of `handlePing` (function name: handlePing, type: Interoperability) so
it matches the package-wide convention and avoids inconsistent receiver naming.
- Around line 11-13: The pingParams struct (type pingParams with field Message)
is unused by handlePing; remove the dead declaration to avoid unused field
warnings or, if intended, update handlePing to decode JSON into pingParams and
use pingParams.Message in the response; locate the type pingParams and either
delete it or add JSON decoding and usage in the handlePing function to consume
Message.
In `@service/interop/ivpn/ivpn.go`:
- Around line 279-281: Add a clear code comment above the conditional that
unconditionally accepts connections from the IVPN service binary (the block
checking status.serviceBinary != "" && conn.Process().Path ==
status.serviceBinary and returning network.VerdictAccept) explaining the trust
model: state that any process matching status.serviceBinary is fully trusted by
this filter, why that trust is required for IVPN to function, and any
limitations/assumptions (e.g., relies on path integrity, not performing further
validation like signature checking or user ownership); reference the symbols
status.serviceBinary, conn.Process().Path, and network.VerdictAccept so
reviewers know exactly which check the comment applies to.
- Around line 117-121: The ivpnclient.NewClientAsRoot call currently swallows
errors (client, err := ivpnclient.NewClientAsRoot(...); if err != nil { return
nil })—add a debug-level log that emits the error details before returning so
connection failures are visible during diagnosis; specifically log err (e.g.,
logger.Debugf or similar) in the NewClientAsRoot error branch and apply the same
change to the second identical block later (the other NewClientAsRoot error
check around lines noted in the review) so both silent failures are recorded at
debug level.
In `@service/interop/module.go`:
- Line 92: Fix the typo in the comment that currently reads "Ot is ok to check
it once at startup, since it can only be changed by restarting Portmaster with a
different config." — change "Ot is ok" to "It is ok" in the comment containing
that exact phrase so the comment reads "It is ok to check it once at startup,
since it can only be changed by restarting Portmaster with a different config."
- Around line 125-128: EnsureVerdictHandlerRegistered currently ignores the
error returned by firewall.SetExternalVerdictHandler; update the function to
capture that error and log a warning if non-nil. Specifically, after calling
firewall.SetExternalVerdictHandler(i.verdict_handler) inside
EnsureVerdictHandlerRegistered, check the returned error and emit a warning that
includes the error and context (e.g., another component may have already
registered a handler). Use the module's logger (e.g., i.logger.Warnf or similar)
or fallback to the package logger to record the warning while keeping the atomic
verdictHandlerRegistered logic intact.
In `@service/interop/verdict_handler.go`:
- Around line 7-15: Rename the method verdict_handler to camelCase
(verdictHandler) on the Interoperability receiver and update all call sites
(e.g., the call referenced in service/interop/module.go at the previous line
127) to use verdictHandler; ensure the method signature (func (i
*Interoperability) verdictHandler(conn *network.Connection) (network.Verdict,
string, bool)) and any variable shadowing inside the loop (verdict, reason,
skipTunnel) remain correct and compile after renaming.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 449a426d-6c14-4cf4-888d-3f2c9201716e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.vscode/launch.jsongo.modservice/firewall/interception/module.goservice/firewall/packet_handler.goservice/instance.goservice/interop/api.goservice/interop/ivpn/ivpn.goservice/interop/module.goservice/interop/verdict_handler.go
Comment on lines
19
to
25
| "linux": { | ||
| "console": "integratedTerminal", // required to debug as root | ||
| "asRoot": true, // required to debug as root | ||
|
|
||
| "args": ["--bin-dir=/usr/lib/portmaster", "--log-stdout", "--log", "trace"] | ||
|
|
||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the launch.json file to understand the context
cat -n .vscode/launch.jsonRepository: safing/portmaster
Length of output: 2028
🌐 Web query:
VS Code debug configuration asRoot property
💡 Result:
asRoot is not a built-in VS Code launch.json property. It’s a Go extension (golang.vscode-go) launch attribute used to debug Go programs/tests as root.
- When
"asRoot": true, the Go extension runs Delve (dlv) viasudo, so the debug session must run in a terminal (set"console"to"integratedTerminal"or"externalTerminal"). - Supported only on Linux and macOS (per the Go extension docs).
- If you use it with build modes (
auto/debug/test), the build runs as root too, which can cause env/cache permission issues; using"mode": "exec"+ apreLaunchTaskto build as your user is recommended. [1]
Example (Go extension):
{
"name": "Launch as Root",
"type": "go",
"request": "launch",
"mode": "exec",
"asRoot": true,
"program": "${workspaceFolder}/myprogram",
"console": "integratedTerminal"
}Sources: Go extension debugging docs (asRoot, console, sudo behavior, platform support, and caveats). [1]
Use "mode": "exec" with a preLaunchTask instead of "mode": "auto" when debugging as root.
The "asRoot": true property is valid and documented in the Go extension. However, using it with "mode": "auto" causes the build to run as root, which can lead to environment and cache permission issues. The recommended approach is to switch to "mode": "exec" and add a preLaunchTask that builds the binary as your regular user before launching the debug session with root privileges.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vscode/launch.json around lines 19 - 25, The linux debug configuration
currently uses "asRoot": true with the Go debugger's default "mode": "auto",
which causes the build to run as root; change the configuration to set "mode":
"exec" and add a "preLaunchTask" that runs the build (e.g., go build
--output=<bin> ...) as your normal user before launch, keep "asRoot": true only
for the launched executable, and keep existing properties like "console" and
"args" in the "linux" configuration so the debug session executes the
already-built binary with root privileges without performing the build as root.
Comment on lines
+15
to
+28
| func (i *Interoperability) registerAPIEndpoints() error { | ||
| if err := api.RegisterEndpoint(api.Endpoint{ | ||
| Path: APIEndpointPing, | ||
| Read: api.PermitAnyone, | ||
| Write: api.PermitAnyone, | ||
| ActionFunc: i.handlePing, | ||
| Name: "Interoperability: Ping Portmaster", | ||
| Description: "Ping the Portmaster Core Service for interoperability checks.", | ||
| }); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "api\.Permit" --type go -A1 -B1 | head -100Repository: safing/portmaster
Length of output: 5158
🏁 Script executed:
# Read the interop/api.go file to see handlePing and understand what it does
cat -n service/interop/api.goRepository: safing/portmaster
Length of output: 1350
🏁 Script executed:
# Search for PingHandler implementation
rg -n "PingHandler" --type go -A10 -B2 | head -80Repository: safing/portmaster
Length of output: 1882
🏁 Script executed:
# Find connHandler definition and Go method
rg -n "connHandler.*Go\(\)" --type go -B5 -A5Repository: safing/portmaster
Length of output: 1411
🏁 Script executed:
# Also check how the API service is set up and if it's local-only
rg -n "RegisterEndpoint\|api\.Endpoint" service/interop/api.go -B3 -A8Repository: safing/portmaster
Length of output: 43
🏁 Script executed:
# Check if there's API authentication setup or local-only binding
fd "api" service/ --type f -name "*.go" | head -5Repository: safing/portmaster
Length of output: 291
🏁 Script executed:
# Search for API server configuration and binding
rg -n "ListenAddr\|localhost\|127.0.0.1" service/ --type go -B2 -A2 | head -60Repository: safing/portmaster
Length of output: 43
🏁 Script executed:
# Check for rate limiting or auth middleware
rg -n "WrapInAuthHandler\|rate.*limit\|RateLimit" service/ --type go | head -30Repository: safing/portmaster
Length of output: 43
🏁 Script executed:
# Find base/api package files
find . -path "./base/api*" -type f -name "*.go" | head -20Repository: safing/portmaster
Length of output: 595
🏁 Script executed:
# Check what PermitAnyone actually means
rg -n "PermitAnyone\|PermitUser\|PermitAdmin" base/api --type go -B2 -A2 | head -80Repository: safing/portmaster
Length of output: 43
🏁 Script executed:
# Search for Permission type definition and constants
rg -n "type Permission\|const.*Permit" base/api/ --type go -B1 -A3 | head -100Repository: safing/portmaster
Length of output: 43
🏁 Script executed:
# Check authentication logic
cat -n base/api/authentication.go | head -80Repository: safing/portmaster
Length of output: 3024
🏁 Script executed:
# Search for API server initialization and listening setup
rg -n "Listen\|Server\|http\.Server" base/api/main.go --type go -B3 -A3 | head -100Repository: safing/portmaster
Length of output: 43
🏁 Script executed:
# Check if there's documentation about API security model
cat base/api/doc.goRepository: safing/portmaster
Length of output: 983
🏁 Script executed:
# Check if there are any other PermitAnyone endpoints that trigger operations
rg -n "PermitAnyone" service/ --type go -B3 -A3Repository: safing/portmaster
Length of output: 3608
Endpoint permission inconsistent with security pattern
The /interop/ping endpoint uses api.PermitAnyone on both Read and Write, allowing unauthenticated callers to trigger PingHandler(), which queues connection attempts via connHandler.Go(). This is inconsistent with other operational endpoints (e.g., netquery, process modules) which use PermitUser. Repeated unauthenticated calls could be abused to trigger resource-intensive connection attempts.
Align with the established pattern and restrict to authenticated users:
Suggested fix
if err := api.RegisterEndpoint(api.Endpoint{
Path: APIEndpointPing,
- Read: api.PermitAnyone,
- Write: api.PermitAnyone,
+ Read: api.PermitUser,
+ Write: api.PermitUser,
ActionFunc: i.handlePing,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/interop/api.go` around lines 15 - 28, The /interop/ping endpoint
currently registers APIEndpointPing with api.PermitAnyone for Read and Write,
allowing unauthenticated callers to trigger handlePing/PingHandler (which calls
connHandler.Go()); change the permissions in registerAPIEndpoints to use
api.PermitUser for both Read and Write so only authenticated users can invoke
this endpoint, and verify handlePing/PingHandler enforce/expect authenticated
context (adjust any auth checks or documentation accordingly).
Comment on lines
+77
to
+79
| func stop() error { | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
stop() does not clean up interop modules.
The start() function calls Start() on each interop module, but stop() is empty. This could lead to resource leaks or improper cleanup when the module is stopped (e.g., IVPN client connections not being closed gracefully).
💡 Suggested implementation
func stop() error {
+ // Stop all interop modules in reverse order
+ for j := len(module.interopModules) - 1; j >= 0; j-- {
+ // Consider adding Stop() to the interopModule interface
+ // and calling it here
+ }
return nil
}Consider adding a Stop() error method to the interopModule interface and implementing it in the IVPN module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/interop/module.go` around lines 77 - 79, The stop() function
currently does nothing; add a Stop() error method to the interopModule interface
and implement it on the IVPN module (the IVPN type that currently provides
Start()), then update stop() to iterate the interopModules slice and call each
module's Stop(), aggregating or logging errors and returning a combined error if
any; ensure the IVPN implementation closes network connections and other
resources gracefully and mirrors Start()/Stop() semantics.
stenya
changed the title
…isplay Add a `Visibility` field to the `Action` struct allowing actions to be hidden in the compact notification view and only shown when the user expands the full notification (value: "detailed"). - base/notifications: add `ActionVisibility` type and `ActionVisibilityDetailed` constant to `Action` struct - notifications.types.ts: expose `Visibility` field on the frontend `BaseAction` interface - notification-list.component.html: filter out `detailed` actions in the compact list view - generic-setting.ts: set default `Visibility: ''` on the inline UI action
|
Alexandr Stelnykovych seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
…uppress option When Portmaster connects to the IVPN Client, display an info notification informing the user that IVPN connections are allowed and that DNS will be handled by Portmaster's local resolver when configured. The notification includes a "Do not notify me anymore" action that permanently suppresses future notifications by writing a marker record to the core database. The check runs before showing the notification on each subsequent connection. The "Reset Notification States" API endpoint (and matching UI menu item) now also clears the IVPN suppression record alongside the broadcast states, so all suppressed notifications can be restored at once. - service/interop/ivpn: add notification.go with initAndShowNotification, isNotificationSuppressed, and suppressNotification - service/interop/ivpn/ivpn.go: show notification on connect if not suppressed - service/broadcasts/api.go: extend reset-state handler to also delete the IVPN suppression record; update endpoint name and description - desktop: rename "Reset Broadcast State" menu item and toast messages to "Reset Notifications State"
Actions with visibility "in-app-only" are shown in the UI but skipped when displaying system-level notifications (Tauri/OS). Updates Go, TypeScript/Angular and Rust projects accordingly.
…update action text
Add "Don't show again" action to the stale cache notification. Suppression state is stored in the database and checked on startup. System notification is shown only on first occurrence. Reset handler in broadcasts now also clears the suppression record. #2061
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
Introduces a new
service/interopmodule enabling Portmaster to interoperate with the IVPN desktop client.Changes
New:
service/interopmodule/interop/pingAPI endpoint so the IVPN client can trigger a connection attempt to Portmaster.IVPN client integration (
service/interop/ivpn)ivpnclientand subscribes toConnectionStarting/ConnectionStoppedevents.GetActiveRemoteEndpointso the active VPN endpoint is known immediately on connect.DNS coordination
SetTempPrioritizedDns, preventing DNS conflicts.Firewall: external verdict handler hook
SetExternalVerdictHandler/ExtVerdictHandlerFuncin the firewall packet handler, providing a lock-free (atomic.Pointer) hot-path hook for external modules to injectAccept/Blockverdicts without modifying core firewall logic.Interception: start/stop events
EventStartStopStateevent emitter andIsStarted()accessor toInterception, allowing other modules to react to interception state changes.Notes
Summary by CodeRabbit
New Features
Improvements
Bug Fixes