Skip to content

fix(computer): guard Unix-only pwd import in site_proxy - #519

Open
saiyam-jain-dev wants to merge 3 commits into
scaleapi:mainfrom
saiyam-jain-dev:fix/site-proxy-pwd-import
Open

saiyam-jain-dev wants to merge 3 commits into
scaleapi:mainfrom
saiyam-jain-dev:fix/site-proxy-pwd-import

Conversation

@saiyam-jain-dev

@saiyam-jain-dev saiyam-jain-dev commented Nov 6, 2026 •

Copy link
Copy Markdown

What and why

site_proxy.py imported pwd unconditionally at module load. Because pwd is a Unix-specific module, importing site_proxy or anything importing it transitively (such as computer.utils.sites and computer.env) fails with ModuleNotFoundError on non-Unix platforms.

This change:

  • Wraps the pwd import in try/except ImportError, matching the pattern in src/agent_env/bundle/_fs.py.
  • Guards pwd and Unix-specific os attributes (geteuid, setgroups, setgid, setuid, O_NOFOLLOW) across serve_user, drop_privileges, trust_nss, as_user, install, and open_log.
  • Adds regression tests in test_computer_site_proxy.py to verify fallback behavior when pwd is unavailable.

How it was tested

  • Ran pytest on the site_proxy unit tests (test_the_proxy_serves_as_nobody_where_the_base_has_it_else_as_the_session_user, test_the_proxy_falls_back_when_pwd_is_unavailable, test_the_proxy_is_standard_library_alone_so_the_bases_own_python_runs_it).
  • Ran ruff check on modified files.
  • Ran check_plugin_api.py against origin/main (no plugin surface changes).

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; no new actionable issues were found.

Summary

The PR lets site_proxy load when pwd is unavailable and adds a fresh-process regression test.

  • The site proxy loads without Python’s pwd module.
  • The local object store imports when Python lacks fcntl.

Reviews (3) · Last reviewed commit: "fix(computer): revert runtime privilege ..." · Reviewed by Greptile

@saiyam-jain-dev
saiyam-jain-dev requested a review from a team as a code owner November 6, 2026 22:21
@saiyam-jain-dev

Copy link
Copy Markdown
Author

Added a fresh-import test (test_the_proxy_imports_when_pwd_is_unavailable) that blocks pwd in sys.modules and reloads site_proxy to verify it imports cleanly without the module.

@andrewpark-scaleai andrewpark-scaleai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes, mainly because this doesn't achieve its stated goal.

It still doesn't import on non-Unix. I blocked every Unix-only module (pwd, fcntl, grp, termios, resource, pty, tty) and imported agent_env.env.envs.computer.utils.sites and computer.env at this head. Both still fail at src/agent_env/store/object_store/local/store.py:27, which does an unconditional import fcntl. The PR only gets past pwd, so the same import chain still breaks on Windows. bundle/_fs.py:18 also says agent-env doesn't target Windows. Which platform hit this? If there is a real non-Unix caller, fcntl needs a guard too. If there isn't one, I'd close this.

Keep the change to the import itself. serve, drop_privileges, install and trust_nss only run as root inside the Linux computer, on the base's own python3. The only host-side import of this module is sites.py:36, which takes is_local and matches. A try/except around import pwd, or moving those two functions into a module with no pwd dependency, covers the host case. That makes the runtime guards unnecessary. Details are inline.

os.setgroups([])
os.setgid(entry.pw_gid)
os.setuid(entry.pw_uid)
if hasattr(os, "setgroups"):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These hasattr checks can't be false. This branch is only reachable after os.geteuid() == 0, which only happens on a Unix host where setgroups, setgid and setuid all exist. And if one of them ever were missing, the privilege drop would be silently skipped while the code carried on as root. A privilege drop shouldn't fail that way. Please revert to the unconditional calls.

return []
try:
home = Path(pwd.getpwnam(user).pw_dir)
except KeyError:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes behavior on Linux, and the description doesn't mention it. Before, an unknown session user raised KeyError and install failed loudly. Now it returns [], so a misnamed user silently skips NSS trust, and the first symptom is a certificate error in Chromium much later. The pwd is None check above is enough for the import case. Please drop this except KeyError.

monkeypatch.setattr(site_proxy, "pwd", None)
assert site_proxy.serve_user("agent") == "agent"
assert site_proxy.trust_nss("agent", "/fake/ca.pem") == []
site_proxy.drop_privileges("agent")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is asserted after this call, so the test only shows that it doesn't raise. If the hasattr guards are removed as suggested above, this line goes too.

site_proxy.drop_privileges("agent")


def test_the_proxy_imports_when_pwd_is_unavailable(monkeypatch):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reloading site_proxy partway through the suite swaps out the module's classes. After the reload, site_proxy.Config is not the Config this file imported at the top. I saw no failures locally (the file and the rest of tst/unit/env/envs pass), but this is fragile. If the import guard is the only change left, running python -c in a subprocess with sys.modules['pwd'] = None tests the fresh import without reloading the module in the test process.

@saiyam-jain-dev

Copy link
Copy Markdown
Author

Thanks for the detailed review and guidance @andrewpark-scaleai.

Updated the branch with the following changes:

  1. Reverted all runtime guards in drop_privileges, trust_nss, as_user, install, and open_log back to their original implementations, preserving unconditional privilege drop behavior and explicit failure on unknown users.
  2. In site_proxy.py, kept only the import fallback (try ... except ImportError: pwd = None) and updated serve_user to catch (KeyError, AttributeError) when pwd is absent.
  3. Swapped store.py:26 from if os.name != "nt": import fcntl to try: import fcntl except ImportError: fcntl = None so downstream imports through agent_env.store do not fail when fcntl is unavailable or blocked.
  4. Updated test_the_proxy_imports_when_pwd_is_unavailable to run in an isolated subprocess (python -c) without reloading site_proxy in-process, and removed the redundant fallback test.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants