Avoid predictable /tmp control socket fallback - #11
Conversation
![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: 8156b9d739
ℹ️ 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".
| } | ||
| } | ||
|
|
||
| config_path.parent().unwrap_or_else(|| Path::new(".")).join(".oo-bot-runtime").join(socket_name) |
There was a problem hiding this comment.
When HOME and XDG_RUNTIME_DIR are unset and /run/oo-bot does not exist, this now puts the socket under the config file's parent. For deployments that point OO_CONFIG_PATH at /etc/oo-bot/oo-bot.yaml or another read-only config directory outside the packaged systemd unit, bind_runtime_control_listener will later fail its fs::create_dir_all(parent) call and the bot cannot start; the previous /tmp fallback still worked in that environment. Please choose a fallback that remains writable without requiring the config directory itself to be writable.
Useful? React with 👍 / 👎.
| if let Some(path) = &context.home_dir { | ||
| let trimmed = path.trim(); | ||
| if !trimmed.is_empty() { | ||
| return Path::new(trimmed).join(FALLBACK_RUNTIME_DIR).join(socket_name); |
There was a problem hiding this comment.
When neither /run/oo-bot nor XDG_RUNTIME_DIR is available, adding the full $HOME/.cache/oo-bot/runtime-control/ prefix leaves only about 40 bytes for HOME before the pathname exceeds the Unix-domain socket sun_path limit on Linux. In long-home environments such as CI sandboxes, network-mounted homes, or nested workspaces, UnixListener::bind fails with AF_UNIX path too long, whereas the previous /tmp/oo-bot-control-<hash>.sock fallback stayed short. Please keep the fallback socket path bounded or shorten/hash the directory component.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| config_path.parent().unwrap_or_else(|| Path::new(".")).join(".oo-bot-runtime").join(socket_name) |
There was a problem hiding this comment.
When HOME is unset and the config path is relative (the default is config/oo-bot.yaml), this fallback makes the socket directory relative to whichever cwd runs the command. If the bot is started from one directory and oo-bot control status/stop is invoked from another with the same relative OO_CONFIG_PATH, the client now looks in a different .oo-bot-runtime directory and cannot reach the running bot; the old /tmp/... fallback did not depend on cwd. Please make this fallback absolute or otherwise independent of the caller's working directory.
Useful? React with 👍 / 👎.
1 participant
Motivation
/tmp, allowing a local attacker to pre-create that pathname and prevent the bot from starting (local DoS)./tmpplacement while preserving the ability to disambiguate instances by config fingerprint.Description
HOMEand addFALLBACK_RUNTIME_DIRso the resolver prefers a per-user fallback atHOME/.cache/oo-bot/runtime-controlwhen/run/oo-botandXDG_RUNTIME_DIRare unavailable (preserves the config-hash socket name).HOMEis not available, fall back to a config-scoped runtime dir adjacent to the config file at.oo-bot-runtime/<socket>instead of placing sockets directly under/tmp.OO_CONTROL_SOCKET_PATH,/run/oo-bot, andXDG_RUNTIME_DIRunchanged, and continue to use the config-derived hash suffix for uniqueness.src/control.rstests to cover the new home-based fallback and the config-scoped fallback.Testing
cargo fmt --checkwhich passed without changes reported.cargo test control::testsbut the build was blocked by thelindera-ipadiccrate build script failing to download dictionary assets in this environment, so tests could not complete.cargo test --no-default-features control::testswhich also failed for the same externallindera-ipadicasset-download issue, so test coverage could not be fully validated here.Codex Task