docs(worker): say that wildcard admin-users is open remote code execution

[nilsmechtel]

--admin-users '*' is documented as a permissions convenience — "don't bother maintaining an admin list". What it actually does is make anyone on the internet a full admin of your worker, able to run arbitrary code on the deployment and to destroy it. Nothing in the guide or the --help text says so, and there is no warning at roll time. This PR closes that gap in what the operator is told. It does not change any behaviour.

Why the wildcard is more than open read access

Four things line up. The worker's Hypha service is registered with visibility: public, and a service's authorization gates invocation, not discovery. The admin operations are exposed on that service. They are gated on the admin_users list, and check_permissions honours a * entry — of the nineteen check_permissions call sites in the codebase, only add_admin_user and remove_admin_user pass the allow_wildcard=False that the admin-list persistence work introduced. Everything else still honours it. run_code then deserialises a caller-supplied cloudpickle payload and runs it in an unsandboxed Ray task.

So the effective grant on a wildcard worker is arbitrary execution plus destruction, to callers who never authenticated. run_code is the sharpest door but not the only one: deploy_app and upload_app are equally arbitrary code execution, and stop_worker, stop_all_apps and delete_app hand an anonymous caller the ability to tear the deployment down. A reader who concluded that disabling run_code fixes this would be wrong, so the warning names the wider surface.

This is pre-existing and was found while auditing the admin-list persistence work. That fix is sound and correctly closed the admin-list edit path against wildcard callers — which is why a stranger still cannot make the grant permanent or lock the real admin out. These are the sibling doors gated on the same list.

What changed

The deployment guide gains a Who can control the worker section near the top, before the mode-specific instructions, so operators of all three modes meet it — the parameter table it would otherwise have lived under is Mode 1's only. Mode 1's table row links to it. The --admin-users help text carries a short version of the same.

Because neither is read at roll time, BioEngineWorker now logs a warning once at startup whenever * is in the resolved admin list — after the persisted overlay and the worker's own identity injection, so it reflects what is actually in force. It is placed in start() rather than _connect_to_server, which also runs on the registration-repair path when Hypha stops serving the worker, so it fires once per worker start rather than once per reconnect.

The warning is deliberately qualified by mode. An earlier draft asserted execution "as the operating system user running this process (uid N)" unconditionally, which is wrong under --mode external-cluster: the worker sits in its own pod and the task lands in separate Ray worker pods, so that uid is not the uid the code runs as. An inaccurate security warning is worse than a vague one — it hands the reader a concrete false model. It now says the code runs with whatever access the Ray cluster is given, and names the host and user only for single-machine mode, where that is true.

Two incidental corrections: the guide said the flag takes comma-separated emails. It is nargs="+" with no comma post-processing anywhere, so it is space-separated, as the module's own usage examples and --help output (EMAIL [EMAIL ...]) already showed.

On the alternative fix

The other way to resolve this is to stop honouring the wildcard on the execution path, so the dangerous operations require a named admin even on a wildcarded worker — mirroring what was done for the admin-list edit path. That is deliberately not in this PR. It is a real behaviour change that would break any deployment currently relying on the wildcard, so it is a maintainer's call rather than something to slip into a documentation change. For what it is worth, I think it is the right end state: a switch whose honest description is "this deployment runs untrusted code from strangers" is one almost nobody would knowingly enable, and a docs-only fix relies on the operator reading and believing the warning. But shipping it should be a deliberate, separately announced decision.

Tests

Three tests in tests/worker/test_admin_users.py: the warning fires and names the exposure when the wildcard is present, it stays silent on a named-only list, and it is actually emitted during startup.

That third test was initially an inspect.getsource grep for the call inside start(), and it was hollow — review caught it and mutation confirmed it. Moving the call to the end of start(), after the shutdown wait, left all 24 tests green while making the warning functionally dead: production runs blocking=True, so an operator would not see it until the worker was already shutting down. The grep proved the string was inside start(), not that it ran at startup, which is the whole claim. It is now behavioural, following the pattern in tests/worker/test_startup_resilience.py — build a bare worker, stub the cluster start and the connect, and raise a sentinel at the step after the call site so the assertion can only pass if the warning already fired.

All three mutations are proven against the shipped text. Deleting the warning body reddens the "fires" test and the startup test; removing the wildcard guard so it warns unconditionally reddens only the "stays silent" test; moving the call after the shutdown wait reddens only the startup test, and previously reddened nothing.

Suite on this branch: 521 passed, 25 skipped, across two runs — baseline 518/25 plus the three new tests, no regressions.

🤖 Generated with Claude Code