feat(pam tunnel): foreground/background/run + cross-process registry (rebase onto upstream master)#15
feat(pam tunnel): foreground/background/run + cross-process registry (rebase onto upstream master)#15msawczynk wants to merge 4 commits into
Conversation
…ss-process tunnel registry
…urn, port safety - Add Windows hard-termination warning to --background and --run banners (was only on --foreground) - Use clean_stale=False in --background polling loop to avoid filesystem churn every 0.5s - Guard port comparison in register_tunnel against non-int-coercible values - Document normalize_bind_host limitation (0.0.0.0 vs 127.0.0.1 caught at OS level) - Note thread-safety characteristic of _registry_dir_initialized flag Made-with: Cursor
… in tunnel_helpers The original feat commit was authored against release 17.2.10, which did not yet contain parse_keeper_webrtc_version_from_sdp / set_remote_description_and_parse_version. During the feat-commit refactor those helpers were removed. Upstream has since added both (commit efdf580 and its precursor) and pam_launch/terminal_connection.py imports them. After rebasing this PR onto current upstream/release, the removal became a regression: ImportError: cannot import name 'parse_keeper_webrtc_version_from_sdp' from 'keepercommander.commands.tunnel.port_forward.tunnel_helpers' Restore both helpers verbatim from upstream/release so the import chain (pam_launch -> terminal_connection -> tunnel_helpers) resolves again. Unit tests (15) + full import chain verified locally. Made-with: Cursor
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 636faee. Configure here.
| # return | ||
|
|
||
| automators = json.loads(self.dump_automators(params,fmt='json')) | ||
| conflict_automators = [automator['name'] for automator in automators if automator['node_id']==1067368092533492 and automator['enabled']] |
There was a problem hiding this comment.
Hardcoded node_id instead of using resolved variable
High Severity
The conflict check on line 292 compares automator['node_id'] against the hardcoded value 1067368092533492 instead of the dynamically resolved matched_node_id. This means the "existing automators in this node" warning only fires for one specific node (likely a developer's test environment) and silently skips the check for every other node. The filter expression needs to use matched_node_id instead of the literal integer.
Reviewed by Cursor Bugbot for commit 636faee. Configure here.
| global _gateway_cache_result, _gateway_cache_time | ||
| now = time.time() | ||
| if _gateway_cache_result is not None and (now - _gateway_cache_time) < _GATEWAY_CACHE_TTL: | ||
| return _gateway_cache_result |
There was a problem hiding this comment.
Gateway cache reads globals without holding lock
Low Severity
get_all_gateways reads both _gateway_cache_result and _gateway_cache_time outside the lock on lines 38–40. A concurrent call to invalidate_gateway_cache could set _gateway_cache_result = None after the is not None check passes but before the return, causing the function to return None instead of a valid controller list. The pre-lock check is an optimization but introduces a TOCTOU window with invalidate_gateway_cache.
Reviewed by Cursor Bugbot for commit 636faee. Configure here.
| cli_scale: Optional[int] = None, | ||
| connect_banner_title: Optional[str] = None, | ||
| pre_connect_spinner: Optional[PamLaunchSpinner] = None, | ||
| preserve_crlf: bool = True, |
There was a problem hiding this comment.
Parameter preserve_crlf accepted but never used
Low Severity
The preserve_crlf parameter is declared in _start_cli_session and passed from execute(), but it is never read or referenced anywhere in the method body. The CRLF normalization flag actually flows through kwargs → launch_terminal_connection → _build_guacamole_connection_settings → PythonHandler. This dead parameter suggests either missing logic in _start_cli_session or an unnecessary indirection that could confuse future maintainers.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 636faee. Configure here.
1 participant
Reapplies the PAM non-interactive tunnel work in four commits, rebased onto current
Keeper-Security/Commandermaster(fetched asupstream/master).Contents
pam tunnel start:--foreground/--background/--run/--pid-file/--timeout, file-basedkeeper-tunnel-sessionsregistry, list/stop across processeskeepercommander/commands/tunnel_registry.pyunit-tests/test_tunnel_registry.py(15 unit tests;pytest unit-tests/test_tunnel_registry.pypasses)tunnel_helpers:parse_keeper_webrtc_version_from_sdphelpers for import chain compatibilityNext step for upstream
msawczynk/Commandercursor/pam-tunnel-rebase-upstream-c427→Keeper-Security/Commander:master(orreleaseif that is the integration branch).Related context: original Keeper-Security#1848.