Repository navigation
fix(helper)!: check ports 80/443 as root and drop kernel-table inspection - #409
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (57)
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe platform replaces listener-port enumeration with privileged inspection of ports 80 and 443. The helper protocol advances to version 2 and checks for conflicts before applying PF redirects. The CLI reports unavailable ports and available owner details. ChangesLow-port conflict inspection
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant CLI as ports::install
participant Environment as ProcessEnvironment
participant Client as PrivilegedHelperClient
participant Helper as helper dispatch
participant Probe as low_port.rs
CLI->>Environment: inspect_low_ports()
Environment->>Client: inspect_low_ports()
Client->>Helper: LowPortInspect
Helper->>Probe: inspect ports 80 and 443
Probe-->>Helper: availability and owner data
Helper-->>Client: LowPorts payload
Client-->>Environment: LowPortInspection
Environment-->>CLI: inspection results
CLI->>CLI: report unavailable ports and owner details
Merge Risk: ⚪ Minimal · up to The change moves the 80/443 conflict check to a root bind probe and makes the helper reject conflicting PF redirects before changing pf. No concrete merge-blocking issue was found in the supplied evidence. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change strengthens port-conflict protection without broadening access to administrator operations. Remaining uncertainty concerns privileged-service upgrades and recovery under interruption or concurrency, which were not fully validated. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 37.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 135 functions across 32 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Single-port conflicts currently lose their owner diagnostics, and the root conflict guard is not exercised by CI. Both should be addressed before merging.
Reviewed changes The full PR was reviewed, including the privileged inspection path, CLI callers, protocol changes, deleted listener API, and updated tests.
- Root availability checks:
LowPortInspectprobes ports 80/443 through the helper, andPfApplychecks again before changing PF rules. - Conflict diagnostics: Bounded
lsofexecution supplies deduplicated, capped owner details without deciding port availability. - CLI integration:
ports:installchecks before preparing files or accepting current redirects, with setup tests covering apply-time conflict propagation. - Protocol and platform cleanup: Helper version 2.0.0/protocol 2 replace the unused reload operation and private kernel-table listener inspection; fixtures and design guidance are updated accordingly.
Validation: all 38 platform library tests passed on Linux. The CLI integration command stopped in the ports suite at the existing platform boundary (low-port frontend is unsupported on linux); macOS/root execution could not be validated on this host. The lsof selector issue was reproduced locally and checked against Apple's source.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues found in this delta. Approval remains withheld while the existing root acceptance-test finding is open.
Reviewed changes Reviewed 6989b9e against the previous review at 604b72c, with the full PR diff checked for context.
- Replaced separate
lsofselectors with one comma-separated selector, preserving owner diagnostics when only one requested port is occupied without weakening command-failure handling. - Expanded live macOS regression coverage to inspect one held high port alongside one free port and assert owner details and free-port state.
Validation: cargo test --locked -p platform --lib passed all 38 tests, and cargo fmt --all --check passed. A local Linux lsof reproduction confirmed that split selectors return exit 1 with owner output, while the combined selector returns exit 0 with the same output. The macOS-only regression test and privileged helper behavior could not be executed on this Linux host.
gpt-6.1-sol ｜ 𝕏
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6989b9e512
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| #[cfg(target_os = "macos")] | ||
| #[test] | ||
| #[ignore = "requires root and a free loopback port 80"] |
There was a problem hiding this comment.
This is the only test of the root-side PfApply conflict recheck, but #[ignore] excludes it from the workflow's ordinary cargo nextest run (nextest documents that the default runs only non-ignored tests); it also creates the listener in the probing process rather than a separate process. Consequently, no macOS matrix lane verifies the root/cross-process safety boundary even though DESIGN.md:31 now explicitly promises both conditions, so a regression could pass CI and let PF redirect traffic away from an existing service. Add a root CI invocation using a separately spawned listener.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in b64bd7a. Every macOS CI lane now builds as its regular user, verifies the exact ignored platform test in nextest's JSON listing, and runs only that executable/test with sudo. The test holds ports 80 and 443 in a separate native process and checks LowPortInspect, actual PfApply rejection, unchanged PF state, and cleanup. #403 inherits this gate for macOS 27. The root acceptance step passed on macOS 14, 15, 26, and 27 in the combined CI run. The combined CI run also passed the full normal suite in all four macOS lanes, plus Linux and Windows.
There was a problem hiding this comment.
ℹ️ No new issues found in this delta. Approval remains withheld while the existing root acceptance-test finding is open.
Reviewed changes Reviewed 4771790 against the prior review at 6989b9e, with the full PR diff checked for context.
- Added bootstrap readiness waiting to helper installation and rollback restoration, retrying missing or refused socket connections without retrying authentication or invalid-response failures.
- Added macOS socket regression tests for delayed socket creation, a socket that never appears, and invalid lifecycle responses.
- Updated lifecycle design guidance to document socket-availability waiting after registration and restoration.
Validation: cargo fmt --all --check passed, and the final cargo test --locked -p platform --lib run passed all 38 tests on Linux. An intermediate rerun failed the unchanged random-filename test at pf.rs:1661 (assertion failed: !first.file_name().is_some_and(|name| name.contains(&process_id))); it passed on rerun. The new macOS-only socket tests and privileged helper behavior could not be executed on this host.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4771790f29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && let Some(pid) = pid | ||
| && let Some(command) = command | ||
| && let Some((address, port)) = value.rsplit_once(':') | ||
| && matches!(address, "*" | "127.0.0.1" | "[::]") |
There was a problem hiding this comment.
When a service listens on a specific non-loopback IPv4 address such as 192.168.1.10:80, the required 0.0.0.0:80 probe can mark the port unavailable and lsof returns that address, but this whitelist discards the otherwise valid PID and command. The CLI consequently claims that it could not identify the process and makes the user run the same lsof command manually, contrary to the owner-reporting behavior specified in DESIGN.md. The upstream lsof field-output documentation confirms that the n field contains the Internet address, so relevant interface-specific IPv4 records need to be associated with the failed wildcard probe rather than dropped.
AGENTS.md reference: AGENTS.md:L4-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I could not reproduce the proposed bind conflict. On this macOS 27 host, a separate LAN-only IPv4 holder left both 127.0.0.1 and 0.0.0.0 probes free, with SO_REUSEADDR both on and off; the public platform probe returned available. Inspection therefore returns before lsof. These additional measurements used an unprivileged caller, so they do not prove every root/OS combination. I am keeping the address filter: widening it can list an unrelated LAN service during a real loopback conflict. A root or other supported-OS counterexample would justify revisiting that decision. I did fix the related diagnostic ambiguity in b64bd7a: messages now list reported listeners without claiming which one caused the IPv4 bind failure. A mixed IPv4/IPv6-wildcard fixture covers that wording.
4771790 to
b64bd7a
Compare
|
Reviewed CodeRabbit’s documentation-coverage warning and the root acceptance concern. No bulk docstring change is planned for the percentage target. The root test now observes a separate process on each low port and is an explicit gate in every macOS CI lane, including macOS 27 through #403. The behavioral findings and the LAN-owner hypothesis are answered in their threads. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed b64bd7a since the prior review at 4771790, with the full PR diff checked for context; the three rebased predecessor commits have identical patches to their previously reviewed versions.
- Added Root CI coverage that builds as the regular CI user, verifies selection of the exact ignored test, and runs only its executable under
sudoin every declared macOS lane. - Expanded Cross-process acceptance coverage to test ports 80 and 443 separately, verify the holder PID through
LowPortInspect, exercise actualPfApplyrejection, check unchanged active PF state, and confirm port availability after child cleanup. - Revised Listener diagnostics and design guidance to distinguish unavailable IPv4 ports from reported listeners, avoiding attribution that
lsofwildcard notation cannot establish. - Updated Diagnostic snapshots to cover conflict messages, unidentified listeners, and mixed IPv4/IPv6 wildcard output.
Validation: all 38 portable platform library tests passed on Linux, and cargo fmt --all --check passed. The new macOS/root acceptance test cannot run on this host; its CI lanes remain queued, so execution of the new gate is still pending.
gpt-6.1-sol ｜ 𝕏
1 participant
On macOS 27, ad-hoc signing anywhere in PV's ancestry can filter the TCP kernel table to PV's own sockets. The old 80/443 check could then report no conflict and redirect traffic away from another server. Issue #402's claim that production is unaffected was incorrect: the installed CLI is affected too.
The privileged helper now decides low-port availability with the shared bind probe as root.
ports:installchecks before preparing files or returning an already-current result.PfApplychecks again before changing PF. Missing process information never makes a port free. A boundedlsofcommand supplies names and PIDs, with at most eight reported listeners per port; its errors stop the operation. Messages list those listeners without claiming which one caused the IPv4 bind failure, becauselsofuses the same wildcard notation for IPv6-only and dual-stack listeners.This bumps the helper to 2.0.0 and protocol 2, adds the fixed
LowPortInspectrequest, and removes unusedPfReloadand the kernel-table listener API. The daemon never calls the helper. Correctness does not depend on signing identity or ancestry. The application version stays unchanged. Installation and rollback wait within the existing five-second budget for launchd's helper socket; authentication and invalid responses fail immediately.Tests cover field parsing and diagnostic limits, a held high port alongside a free port, protocol round trips and removed-operation rejection, CLI conflicts before file writes and on a repeated install, setup retaining apply-time errors, and delayed/missing/invalid helper sockets. The root acceptance test starts a separate native holder for each of ports 80 and 443, checks its PID through
LowPortInspect, exercises the actualPfApplyguard, verifies unchanged PF state, and confirms the ports are free after cleanup.CI runs that root test in every macOS lane. Cargo builds as the regular CI user; sudo runs only the exact verified test executable. Fixture readiness, system commands, and the CI step are bounded. The wider privileged RC workflow remains a follow-up. #403 adds the macOS 27 lane after this fix.
Validation: formatting, workspace Clippy with warnings denied, cargo-shear, and the full macOS 27 CI-profile suite pass: 1,610 normal tests passed. The earlier privileged installation and Python-server check also passed on this Mac: helper 2.0.0/protocol 2 installed, actual
PfApplyand the CLI refused the root server by PID, andports:installsucceeded after it stopped. The combined stack passed CI on macOS 14, 15, 26, and 27, including the separate-process root acceptance test in every macOS lane. Linux, Windows, benchmark checks, and automated reviews also passed in that run: https://github.com/prvious/pv/actions/runs/37825708765.Based on #408. Closes #402.