Repository navigation
fix(config): heal and diagnose an unreadable config.toml instead of dead-ending sign-in - #5190
Conversation
…ead-ending sign-in
A container whose workspace volume carries a config.toml owned by a uid other
than the runtime user failed every config-dependent RPC with
Failed to read config file: /home/openhuman/.openhuman/config.toml:
Permission denied (os error 13)
while /health kept returning 200 and Sentry saw nothing. The raw anyhow chain,
absolute path included, was painted straight onto the sign-in screen.
The entrypoint's heal was the root gap: it chowned the workspace directory but
never its contents, so a config.toml written by another uid (a root `docker
exec`, an older image, a restored volume) stayed unreadable forever. The core
writes that file at mode 0600, which turns the uid mismatch from cosmetic into
fatal — at 0644 the read would have succeeded.
- docker-entrypoint-core.sh: chown recursively, skipping correctly-owned
entries and using -h so a symlink planted in the workspace cannot redirect a
root chown at an arbitrary target. Log numeric owner/mode before repairing,
tolerate a capability-denied chown instead of dying under `set -e`, exec
directly when already non-root, and refuse to boot with an unreadable config
rather than serving a green /health over a broken core.
- config loader: attach file uid/gid/mode and process euid/egid to the read
failure, and only auto-harden a config we own — chmod-ing a foreign-owned
file with CAP_FOWNER strips the read bit from the uid that actually serves
requests.
- observability: stop demoting an ownership mismatch to a non-paging warning.
That case has a local lever, unlike the ACL / antivirus denials the demotion
was written for, which keep their existing treatment.
- sign-in UI: classify the failure and show actionable, path-free copy instead
of the raw Rust chain. The OAuth path previously bucketed it as a retryable
'other' and told the user to try again forever.
- docker-compose.yml: grant CHOWN/SETUID/SETGID. Without them `cap_drop: ALL`
leaves gosu unable to drop privileges at all, so the shipped stack could not
start the current image.
Also hardens Config::save: the temp file was created at 0o666 & ~umask and the
atomic rename carried that mode onto the live config, so every settings change
re-widened a file holding enc2: provider keys and channel tokens to
world-readable, and fs::copy propagated the same mode to config.toml.bak. The
load-time auto-fix only ever repaired it on the next startup.
📝 WalkthroughWalkthroughThe change adds config ownership diagnostics and secure save permissions, repairs Docker volume ownership during startup, classifies unreadable core configuration failures, and replaces raw frontend errors with localized, path-free guidance. ChangesConfig ownership and startup flow
Frontend error classification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DockerEntrypoint
participant ConfigFile
participant Core
participant DesktopListener
participant Welcome
DockerEntrypoint->>ConfigFile: repair ownership and check readability
DockerEntrypoint->>Core: start through gosu
Core-->>DesktopListener: return config.toml read failure
DesktopListener->>DesktopListener: classify config_unreadable
DesktopListener-->>Welcome: provide localized message key
Welcome-->>Welcome: render actionable login error
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe3f2b4eb8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
| Filename | Overview |
|---|---|
| scripts/docker-entrypoint-core.sh | Major overhaul: recursive symlink-safe workspace healing, two preflights (gosu privilege-drop, then config readability), non-root direct-exec path, and FATAL exit with actionable remedy instead of a silently broken container boot. |
| src/openhuman/config/schema/load/impl_load.rs | Adds describe_config_ownership (uid/gid/mode + process euid/egid on read failure), restricts the auto-chmod to files the process owns, and hardens the temp file to 0600 before writing secrets in Config::save. |
| src/core/observability.rs | Adds marker-keyed exclusion so ownership-mismatch denials page in Sentry while bare ACL/antivirus denials remain demoted; pinned by two complementary tests. |
| app/src/utils/coreConfigFailure.ts | New classifier requiring both a config-read context anchor and a denial signal, preventing unrelated permission errors from triggering the config-unreadable copy; anchored errno regexes prevent prefix collisions. |
| app/src/pages/Welcome.tsx | Routes unreadable-config errors to classified i18n copy instead of painting the raw anyhow chain; unrecognised errors keep their original message; deep-link key propagation wired correctly. |
| app/src/utils/desktopDeepLinkListener.ts | Adds config_unreadable bucket to classifyAuthStoreFailure (most-specific-first, ahead of transport buckets) and propagates an i18n key rather than a literal so the rendering component can localise it. |
| docker-compose.yml | Adds the three capabilities (CHOWN, SETUID, SETGID) the entrypoint requires under cap_drop: ALL; deny-by-default preserved, alternative pinned-user path documented. |
| app/src/store/deepLinkAuthState.ts | Adds errorMessageKey field and threads it correctly through begin/complete/fail transitions; stale-key clearing on every transition is tested. |
| src/openhuman/config/schema/load_tests.rs | Adds two Unix-only tests: ownership diagnostics in the error message, and owner-only (0600) mode enforced on both config.toml and its backup after Config::save. |
| Dockerfile | Adds a documentation comment warning that docker exec lands as root (no entrypoint) and creates a root-owned config.toml; no functional change. |
Sequence Diagram
sequenceDiagram
participant E as docker-entrypoint-core.sh
participant G as gosu
participant C as openhuman-core
participant L as Config::load_or_init
participant O as observability.rs
participant UI as Welcome.tsx / deepLinkListener
Note over E: Container start (root)
E->>E: heal_dir(): find ! -user openhuman -exec lchown
E->>G: Preflight 1 — gosu openhuman true
G-->>E: ok (CAP_SETUID/SETGID present)
E->>G: Preflight 2 — gosu openhuman test -r config.toml
alt config unreadable after heal
G-->>E: exit 1 (FATAL + remedy logged)
E-->>E: container exits — no silent broken boot
else config readable
E->>G: exec gosu openhuman openhuman-core
G->>C: drops privileges to uid 10001
end
Note over UI: User clicks Continue Locally or OAuth deep-link
UI->>C: auth_store_session RPC
C->>L: Config::load_or_init
L->>L: fs::read_to_string(config.toml)
alt EACCES (uid mismatch)
L->>L: describe_config_ownership() appends file uid/gid/mode + process euid + marker
L-->>C: Err with [config owner mismatch] context
C-->>O: expected_error_kind sees marker — keeps paging
C-->>UI: error propagated to client
UI->>UI: isCoreConfigUnreadableError() true
UI->>UI: classifyAuthStoreFailure() config_unreadable
UI->>UI: render t('welcome.coreConfigUnreadable') — path-free, localised
else ok
L-->>C: Config struct
C-->>UI: session token
end
Reviews (6): Last reviewed commit: "Merge remote-tracking branch 'refs/remot..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/docker-entrypoint-core.sh (1)
91-92: 📐 Maintainability & Code Quality | 🔵 TrivialUse the computed
OPENHUMAN_UID:OPENHUMAN_GIDinstead of resolving names for chown.
chown -h "${OPENHUMAN_USER}:${OPENHUMAN_USER}"depends on a group literally namedopenhumanexisting and mapping to the same GID asOPENHUMAN_GID(computed at line 41 but otherwise only used for logging). Using the numeric IDs directly removes that indirection and matches the numeric form already used in the remedy message at line 127.
[recommended_refactor: low_effort_and_high_reward]♻️ Use numeric IDs for chown
- if find "${_dir}" ! -user "${OPENHUMAN_USER}" -exec \ - chown -h "${OPENHUMAN_USER}:${OPENHUMAN_USER}" {} + 2>/dev/null; then + if find "${_dir}" ! -user "${OPENHUMAN_USER}" -exec \ + chown -h "${OPENHUMAN_UID}:${OPENHUMAN_GID}" {} + 2>/dev/null; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/docker-entrypoint-core.sh` around lines 91 - 92, Update the chown invocation in the find ownership-fix block to use the computed numeric OPENHUMAN_UID:OPENHUMAN_GID values instead of OPENHUMAN_USER name resolution, matching the numeric IDs used by the remedy message while preserving the existing ownership-fix behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/utils/coreConfigFailure.ts`:
- Around line 45-48: Replace the ownership-specific
CORE_CONFIG_UNREADABLE_MESSAGE literal with a propagated failure key or kind
that covers all classified configuration-read failures, including ACL denials.
Resolve that key at the UI boundary via useT(), add the translation to en.ts and
every real locale, and ensure no translation value contains an em dash.
In `@gitbooks/features/cloud-deploy.md`:
- Around line 667-678: Update the Docker repair instructions to derive the
application’s runtime UID and GID from the container instead of hard-coding
10001:10001 or relying on docker exec’s default root user. Adjust the
ownership-inspection command to query the runtime user explicitly, then reuse
those discovered numeric values in the recursive chown command while preserving
the restart step.
In `@scripts/docker-entrypoint-core.sh`:
- Around line 119-129: Update Preflight 2 to validate the config.toml located in
the directory returned by resolve_config_dir_for_workspace(), rather than
checking only WORKSPACE_DIR/config.toml. Preserve the existing readability test,
diagnostics, remedy message, and exit behavior for the resolved fallback path.
---
Nitpick comments:
In `@scripts/docker-entrypoint-core.sh`:
- Around line 91-92: Update the chown invocation in the find ownership-fix block
to use the computed numeric OPENHUMAN_UID:OPENHUMAN_GID values instead of
OPENHUMAN_USER name resolution, matching the numeric IDs used by the remedy
message while preserving the existing ownership-fix behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b327e6db-4b4d-4638-acbd-61c16b12d1a1
📒 Files selected for processing (15)
Dockerfileapp/src/pages/Welcome.tsxapp/src/pages/__tests__/Welcome.test.tsxapp/src/utils/__tests__/coreConfigFailure.test.tsapp/src/utils/__tests__/desktopDeepLinkListener.test.tsapp/src/utils/coreConfigFailure.tsapp/src/utils/desktopDeepLinkListener.tsdocker-compose.ymlgitbooks/features/cloud-deploy.mdscripts/docker-entrypoint-core.shsrc/core/observability.rssrc/openhuman/config/schema/load/impl_load.rssrc/openhuman/config/schema/load/mod.rssrc/openhuman/config/schema/load_tests.rssrc/openhuman/config/schema/mod.rs
… harden the heal Review findings from CodeRabbit, Greptile, and Codex on the config-read denial fix. - Localize the user-facing copy. It was a hardcoded English literal in a util, so non-English users would always have seen English and the i18n gates could not see it. The classifier now yields an i18n key that the rendering component resolves via useT(); `welcome.coreConfigUnreadable` and `welcome.localSessionErrorFallback` are added to en.ts and all 13 locales. Deep-link auth runs outside React, so DeepLinkAuthState carries an optional `errorMessageKey` alongside the literal `errorMessage` and Welcome resolves it. - Hedge the wording. The classifier also matches denials with no ownership marker (cores predating it, Windows DACLs, antivirus locks), where asserting "config.toml is owned by a different user account" is factually wrong and points at the wrong remedy. The copy now says it may belong to another account or be inaccessible for another reason. - Widen preflight 2 to every candidate config dir. `resolve_config_dir_for_workspace` falls back to `<parent-of-workspace>/.openhuman` when the workspace holds no config.toml, so a custom OPENHUMAN_WORKSPACE could resolve a config the guard never checked. The entrypoint now builds one deduplicated candidate list and both heals and preflights every entry. - chown by numeric uid:gid rather than `openhuman:openhuman`. The name pair only works while the primary group happens to share the user's name, and this makes the heal symmetric with the numeric remedy printed on failure. Also fail fast with a clear message when the user does not exist in the image. - Derive the repair ids from the container in the docs instead of hardcoding 10001, and query `id openhuman` rather than a bare `id`, which reports root because `docker exec` does not drop privileges. - Pass the raw message to `isCoreConfigUnreadableError` in `classifyAuthStoreFailure`; the predicate normalises its own input and every other call site passes raw.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/utils/__tests__/coreConfigFailure.test.ts`:
- Around line 63-66: Update the assertion in the CORE_CONFIG_UNREADABLE_I18N_KEY
test to verify that the English copy is truthy and is not equal to
CORE_CONFIG_UNREADABLE_I18N_KEY, preserving the guarantee that the UI receives
real translated text rather than the bare key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9705255f-3677-4ef9-ab56-e7a7d0c6856e
📒 Files selected for processing (24)
app/src/components/oauth/__tests__/OAuthProviderButton.test.tsxapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/pages/Welcome.tsxapp/src/pages/__tests__/Welcome.test.tsxapp/src/store/deepLinkAuthState.tsapp/src/utils/__tests__/coreConfigFailure.test.tsapp/src/utils/__tests__/desktopDeepLinkListener.test.tsapp/src/utils/coreConfigFailure.tsapp/src/utils/desktopDeepLinkListener.tsgitbooks/features/cloud-deploy.mdscripts/docker-entrypoint-core.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/docker-entrypoint-core.sh
- app/src/pages/tests/Welcome.test.tsx
- gitbooks/features/cloud-deploy.md
`toBeTruthy()` passes on a value equal to the key itself, which is precisely the "bare key reached the UI" case the test claims to rule out.
…geKey field The store's own test asserts the whole state object with toEqual, so adding errorMessageKey broke 8 assertions there. Caught by CI's vitest-related run, not locally, because only the directly-edited suites were run. Also covers the new field's behaviour: a key is carried through, and a stale key is cleared on the next transition, including when a later literal-message failure follows a keyed one.
M3gA-Mind
left a comment
There was a problem hiding this comment.
PR #5190 — fix(config): heal and diagnose an unreadable config.toml instead of dead-ending sign-in
Walkthrough
This PR fixes a container/cloud failure mode where a config.toml owned by a different uid than the runtime process failed every config-dependent RPC (including sign-in) with Permission denied (os error 13) while /health stayed green and Sentry saw nothing. The fix spans five layers: the Docker entrypoint now heals the workspace recursively (symlink-safe via find … -exec chown -h), refuses to boot on an unreadable config, and logs numeric owner/mode; the Rust loader appends uid/gid/mode diagnostics to the read failure and only auto-hardens configs it owns; Config::save now chmods to 0600 before writing secrets (an adjacent, real defect); observability stops demoting the ownership-mismatch case out of Sentry; and the client surfaces classified, path-free, localized copy instead of a raw anyhow chain. The reasoning is unusually well-documented in code comments and the PR body, coverage is thorough (frontend + Rust unit tests, both-ways observability tests), and i18n is complete across all 14 locales.
Overall assessment: clean and mergeable. No blockers. A few small polish items and one trade-off worth confirming, below.
Changes
| File | Summary |
|---|---|
scripts/docker-entrypoint-core.sh |
Recursive symlink-safe heal across all candidate config dirs; two preflights (gosu privilege-drop, config readability); direct-exec when already non-root; numeric pre-heal diagnostics. |
docker-compose.yml |
Adds CHOWN/SETUID/SETGID — without them cap_drop: ALL blocks gosu's privilege drop and the stack cannot boot the current image. |
Dockerfile |
Comment warning that docker exec lands as root and can create a root-owned config.toml. |
src/openhuman/config/schema/load/impl_load.rs |
describe_config_ownership diagnostics + [config owner mismatch] marker; auto-fix now requires meta.uid()==euid; Config::save hardens temp file to 0600 before writing. |
src/openhuman/config/schema/{load/mod,mod}.rs |
Re-export CONFIG_OWNER_MISMATCH_MARKER as the producer/consumer contract. |
src/core/observability.rs |
Ownership-mismatch denials keep paging (marker-keyed); ACL/AV denials stay demoted. Both-ways tests. |
src/openhuman/config/schema/load_tests.rs |
Tests for ownership diagnostics + owner-only save/backup mode. |
app/src/utils/coreConfigFailure.ts (new) |
Classifier requiring both a config-read anchor and a denial signal; exported i18n key. |
app/src/store/deepLinkAuthState.ts |
Adds errorMessageKey so non-React deep-link code can hand a translatable key to the renderer. |
app/src/utils/desktopDeepLinkListener.ts |
New config_unreadable bucket in classifyAuthStoreFailure, routed to the i18n key. |
app/src/pages/Welcome.tsx |
Substitutes localized copy for the raw chain on local-login + deep-link errors. |
app/src/lib/i18n/*.ts (14 files) |
welcome.coreConfigUnreadable + welcome.localSessionErrorFallback in every locale. |
gitbooks/features/cloud-deploy.md |
Docs for the UID-drift heal, the FATAL preflight, and docker exec -u recovery. |
*_tests.* (Welcome/deepLink/OAuth) |
Assertions extended for the new errorMessageKey field. |
Actionable comments (4)
💡 Refactor / suggestion
1. app/src/utils/coreConfigFailure.ts:22 — PERMISSION_SIGNAL matches os error 13/os error 5 unanchored
os error 5 is a substring of os error 50, os error 512, etc., and os error 13 of os error 130+. The Rust side that produces these strings always wraps them in parens ((os error 5), (os error 13)), so the client can match the exact same shape and avoid a spurious match on an unrelated errno that happens to share a prefix. Low impact (it only decides whether nicer copy vs. the raw message is shown, and only for config-read-anchored messages), but trivially tightened:
// before
const PERMISSION_SIGNAL = /permission denied|access is denied|os error 13|os error 5/;
// after — match the parenthesised form the loader actually emits
const PERMISSION_SIGNAL = /permission denied|access is denied|\(os error 13\)|\(os error 5\)/;❓ Question / trade-off
2. scripts/docker-entrypoint-core.sh:65-66 — pinned non-root user skips the readability preflight
The already running non-root → exec openhuman-core fast path (correctly) skips the heal, but it also skips preflight 2 (the readability check that emits the one-line FATAL remedy). So an operator who pins user: "10001:10001" with a mis-owned config.toml gets exactly the old silent dead-end this PR set out to eliminate — the core boots and every config RPC returns EACCES with no remedy in the log. This is inherent (you can't gosu-test as another user without being root), and the PR body documents the "loses the ability to repair a root-owned volume" trade-off, but it doesn't call out that this path also loses the diagnosis. Consider a cheap self-readability check on the direct-exec path so the pinned-user case still fails fast with guidance:
if [ "$(id -u)" -ne 0 ]; then
for _dir in "${WORKSPACE_DIR}" "${HOME_OPENHUMAN_DIR}"; do
if [ -e "${_dir}/config.toml" ] && [ ! -r "${_dir}/config.toml" ]; then
echo "[docker-entrypoint] FATAL ${_dir}/config.toml not readable by uid=$(id -u)" >&2
ls -ln "${_dir}/config.toml" >&2 || true
exit 1
fi
done
echo "[docker-entrypoint] already running non-root — skipping heal, exec direct"
exec openhuman-core "$@"
fiNot a blocker — happy to accept the documented trade-off if you'd rather keep this path minimal.
Nitpicks (2)
scripts/docker-entrypoint-core.sh:73(viaadd_config_dirat :141) —heal_dirrunsmkdir -pon the derived legacy dir$(dirname "$WORKSPACE_DIR")/.openhuman. For a customOPENHUMAN_WORKSPACEthis creates a sibling.openhumandirectory that the core may never resolve into, as a side effect of healing. Harmless (the core would create it on demand anyway), but consider guarding the legacy candidate with[ -d "$_dir" ]so a healthy run doesn't materialize an empty owned dir.src/openhuman/config/schema/load/impl_load.rs:273— thetracing::warn!+describe_config_ownership().awaitsit inside theretry_with_backoff_asyncclosure. Today EACCES isn't transient on unix so this fires once; on Windows a sharing violation (os error 32) is retry-eligible, which would emit up to 5 warns + 5 metadata stats for one failure. Consider logging once after the retry loop resolves, or only on the final attempt.
Verified / looks good
Config::savehardens the temp file to 0600 beforewrite_all, so no secret byte is ever world-readable; the non-fatalset_permissions(CIFS/exFAT/FUSE) rationale is sound and matches the first-init path.save_keeps_config_and_backup_owner_only_readablepins bothconfig.tomland.bak.- The load-time auto-fix correctly gates on
meta.uid() == geteuid()— no longer strips the read bit off a foreign-owned config viaCAP_FOWNER. - Producer/consumer marker coupling is compile-time (
CONFIG_OWNER_MISMATCH_MARKERre-exported, referenced by both the loader andobservability), and both-ways tests (does_not_demote…/still_demotes…) pin the polarity. is_config_read_io_failure_messagemarker exclusion is placed correctly relative to the directory/NotFound guards; the marker is lowercase so it matches the lowered message verbatim.- The error chain preserves the underlying
io::Errorviaanyhow::Error::new(error).context(...), sois_transient_fs_errorcan still downcast and{:#}still renders the cause — the #3962 contract holds. isCoreConfigUnreadableErrorcorrectly requires both a config-read anchor and a denial signal, so unrelated permission errors keep their own message; empty/null-safe; covered for the reported chain, the pre-marker shape, and Windows.- i18n copy added to all 14 locales, no em dashes, hedged wording ("may belong to"), no path/uid/errno leakage — pinned by the
CORE_CONFIG_UNREADABLE_I18N_KEYtest. docker-compose.ymlgrant is minimal and deny-by-default is preserved;read_only/no-new-privilegesunchanged. Rationale documented inline.find … -exec chown -h … +is symlink-safe by construction (-Pdefault traversal +lchown), and the reasoning about hardlinks not crossing the volume filesystem is correct.
…th, tighten errno match
- Keep the diagnosis on the pinned non-root path. The `already running non-root
-> exec` fast path skipped preflight 2 along with the heal, so an operator
running `user: "10001:10001"` over a mis-owned config.toml got exactly the
silent dead-end this script exists to prevent: the core boots and every config
RPC returns EACCES with nothing in the log. The heal genuinely is unavailable
there (no CAP_CHOWN), but `test -r` as ourselves needs no privileges, so that
path now fails fast with a remedy naming the host-side chown. The candidate
dir list moved above the branch so both paths share it, and the
user-must-exist guard moved below it, since only the root path consumes those
ids.
- Stop materializing a spurious legacy dir. `heal_dir` runs `mkdir -p`, so the
DERIVED `<parent-of-workspace>/.openhuman` candidate was created on every
start even when the core would never resolve into it. It is now a candidate
only when it already exists; the configured dirs are still created as before.
- Log the config-read failure once instead of per retry attempt. On Windows a
sharing violation is retry-eligible, so the warn inside the retry closure
could emit five lines and five metadata stats for one failure. The ownership
detail is already in the error chain, so a single post-loop warn with `{:#}`
carries the same information.
- Match the parenthesised errno form. `os error 5` unanchored also matches
`os error 50`/`512`, and `os error 13` matches `os error 130`. The loader
always emits `(os error N)`, so the classifier keys on that shape; covered by
a new test over the near-miss errnos.
# Conflicts: # app/src/utils/__tests__/desktopDeepLinkListener.test.ts # src/openhuman/config/schema/load/impl_load.rs # src/openhuman/config/schema/load_tests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31a39d7a5c
ℹ️ 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".
| if find "${_dir}" ! -user "${OPENHUMAN_USER}" -exec \ | ||
| chown -h "${OPENHUMAN_UID}:${OPENHUMAN_GID}" {} + 2>/dev/null; then |
There was a problem hiding this comment.
Traverse restrictive stale-owned directories during healing
When a restored or UID-drifted volume root is owned by the old UID with mode 0700, the shipped Compose capability set (cap_drop: ALL plus only CHOWN, SETUID, and SETGID) does not let the root entrypoint bypass directory permissions. This find chowns the directory entry to UID 10001 but then receives Permission denied before reaching its children; afterward the root-side [ -e .../config.toml ] preflight also cannot traverse the now-10001-owned directory and silently skips the check, so the core starts as UID 10001 with the still-foreign 0600 config and reproduces the sign-in dead end this change intends to heal. The healing path needs sufficient traversal access, and the preflight should test existence/readability as the target user rather than short-circuiting on root's -e.
Useful? React with 👍 / 👎.
| for _dir in ${CONFIG_DIRS}; do | ||
| heal_dir "${_dir}" |
There was a problem hiding this comment.
Preserve whitespace in custom workspace paths
When OPENHUMAN_WORKSPACE contains whitespace, this unquoted iteration splits the configured path into multiple directories. The previous implementation consistently quoted WORKSPACE_DIR, but the new CONFIG_DIRS representation causes the entrypoint to create/chown unrelated path fragments and never heal or preflight the actual workspace; the core then starts with the original mounted directory still mis-owned. Store and iterate candidates without shell word splitting, or reject unsupported paths before modifying anything.
Useful? React with 👍 / 👎.
Summary
config.tomlowned by a different uid than the runtime process failed every config-dependent RPC withPermission denied (os error 13)— including sign-in — while/healthstill returned 200 and Sentry saw nothing.scripts/docker-entrypoint-core.shnow heals the workspace recursively (it only ever chowned the directory, never its contents), refuses to boot on an unreadable config instead of serving a green health check, and logs the numeric owner/mode that makes the denial diagnosable.file uid/gid/mode+process euid/egidto the read failure, and only auto-hardens a config it owns.anyhowchain (absolute path and errno included) at the user; the OAuth path no longer classifies this permanent fault as a retryableother.docker-compose.ymlgrantsCHOWN/SETUID/SETGID; without themcap_drop: ALLleavesgosuunable to drop privileges, so the shipped stack cannot start the current image at all.Problem
Reported from the desktop sign-in screen, under the Continue Locally (Experimental) button:
Failure chain, verified end to end:
Welcome.tsx:249-253renderslocalLoginErrorverbatim fromerr.message(Welcome.tsx:73-75).storeSessionToken→ RPCauth_store_session, whose first statement isconfig_rpc::load_config_with_timeout()(credentials/schemas.rs:328-330).Config::load_or_init→fs::read_to_stringatconfig/schema/load/impl_load.rs:198returnsEACCES.loader.rs:51renders the full chain with{e:#}and it reaches the DOM unmodified.The path carries no
users/<id>segment, which only happens whenOPENHUMAN_WORKSPACEis set — i.e. a containerized core (Dockerfile:116,docker-compose.yml:43,.fly/fly.toml:22).config_path.exists()andcreate_dir_allboth succeeded, so the directory is healthy; the file is owned by a uid other than 10001 at mode 0600.Why nothing recovered:
scripts/docker-entrypoint-core.sh:42chowned the workspace directory only — not its contents. Its own header claimed "no manualdocker volume rmis required when upgrading from a previously broken image", which was true for the directory inode and false for every file inside it. Aconfig.tomlwritten by another uid (a rootdocker exec, which does not run the entrypoint; an older image; a restored volume) stayed unreadable forever.config.tomlat 0600 (impl_load.rs:279), so our own hardening converts a survivable uid mismatch into a hard denial — at 0644 the read would have succeeded.impl_load.rs:142already holds the file'sMetadata, yet the failure logged neither the file's uid/gid/mode nor the process euid — the three numbers that fully explain anEACCESon a file that just passed anexists()check.core::observabilityclassifies this string asConfigReadIoFailureand demotes it to a non-pagingwarn!, justified as "user-environment state with zero local lever". For the container case that premise is false: we choose the runtime uid and we set the mode.is_transient_fs_error(util.rs:645-650) classifies nothing outsidecfg(windows), so the POSIX denial fails on the first attempt.Not a recent regression in behaviour — only in wording.
9281b630c(#3962) switchedloader.rs:51toformat!("{e:#}"); before it, the same denial rendered without the: Permission denied (os error 13)tail.Solution
Entrypoint (
scripts/docker-entrypoint-core.sh)find "$dir" ! -user openhuman -exec chown -h ... +— only mis-owned entries are touched, so a healthy volume costs one traversal and no ctime churn.-h(lchown) is load-bearing: this runs as root over a directory the unprivileged core can write, so a plainchownwould dereference a symlink planted at$dir/x -> /etc/shadowand hand uid 10001 the target.finddoes not follow symlinks (-Pdefault), and-hcloses the last dereference. Hardlinks cannot cross the volume's filesystem boundary.set -edeath.gosu … trueproves privileges can actually be dropped — otherwise a missingCAP_SETUIDwould be misreported as a file-ownership problem and send the operator after the wrongchown; (2) the config is readable by the runtime user, or the container exits with the exact remedy in the log rather than serving a green/healthover a core that fails every config RPC.execwhen already non-root, so a deliberateuser: "10001:10001"works. It previously died, because the chown failed underset -e.pre-healowner/mode lines for the dir andconfig.toml; dropped$*from the pre-exec echo (raw CLI args in container logs).Config loader (
src/openhuman/config/schema/load/impl_load.rs)describe_config_ownershipappends(file uid=… gid=… mode=…; process euid=… egid=…)to the read-failure context, plus a[config owner mismatch]marker when the uids differ. Numeric ids and mode bits are not PII. Building it needs anawait, so the syncwith_contextclosure is replaced with an explicitanyhow::Error::new(io_err).context(..)— theio::Errorstays in the chain, sois_transient_fs_errorstill downcasts and{:#}still renders the cause.meta.uid() == geteuid(). Chmod-ing a foreign-owned config either fails withEPERM(log noise) or — withCAP_FOWNER, e.g. a root-run CLI in a container — succeeds and strips the read bit from the uid that actually serves requests. A foreign 0644 config still loads; repairing ownership is the entrypoint's job.Config::savehardening (adjacent defect, fixed here). The temp file was openedcreate_new(true).write(true)with no mode →0o666 & ~umask(0644), and the atomic rename carries the temp file's mode onto the live config. Every settings change therefore silently re-widened a file holdingenc2:provider keys and channel tokens to world-readable, andfs::copypropagated the same mode toconfig.toml.bak; the load-time auto-fix only repaired it on the next startup. Now chmod'd to 0600 before any secret byte is written. Deliberately non-fatal — filesystems without chmod (CIFS/SMB, exFAT, some FUSE) would otherwise turn a working save into a hard error, and a failed hardening leaves the file exactly as permissive as it was before this call existed.Observability (
src/core/observability.rs)is_config_read_io_failure_messagereturnsfalsewhen the marker is present, so an ownership mismatch keeps paging. Denials without the marker (ACL, antivirus, OneDrive placeholder) keep the existing demotion. The matcher references the loader's constant rather than a copied literal, making the producer/consumer coupling compile-time.Client
app/src/utils/coreConfigFailure.tsrecognises the failure (requires both a config-read context and a denial signal, so unrelated permission errors keep their own message) and supplies path-free, actionable copy.Welcome.tsxsubstitutes it for the raw chain; unrecognised errors are still surfaced verbatim, so nothing else changes.classifyAuthStoreFailuregains aconfig_unreadablebucket, andauthStoreFailureUserMessagehandles it in both local and cloud mode — this fault is a property of whichever core answered, and no amount of retrying clears it. It previously fell through toother→ "Sign-in failed. Please try again."Deliberately NOT done: falling back to
Config::default()on an unreadable config.config.tomlcarriesenc2:secrets. A silent default would let the nextconfig.save()on the sign-in path rename a default config over the user's real one, destroying every provider key, channel token, and agent setting — unrecoverably, since.bakis a copy of the new bytes, not a rollback. The repo already encodes this reasoning in the corrupted-config path ("skipping save to protect the .bak"). An unreadable config must be fatal to the operation. Nor does the corechownanything itself: it lacksCAP_CHOWNundercap_drop: ALL, and silently taking ownership of a foreign file is a privilege bug, not a fix.Submission Checklist
pnpm teston the three affected suites, 60/60). The Rust half could not be measured locally because the lib does not currently compile against the vendoredtinyagents/tinyflowssubmodules in my checkout; CI resolves them at the pinned revisions. Every changed Rust line is exercised by one of the four added tests.docs/TEST-COVERAGE-MATRIX.mdCloses #NNN— no issue filed; reported directly by a user. Happy to open one and link it if preferred.Impact
CAP_CHOWN) now fails the container start instead of booting into a state where every config RPC returnsEACCESbehind a 200/health. That is the intended behaviour change and the one thing to weigh in review — a restart loop carrying a one-line remedy is easier to diagnose than a green container with a dead sign-in screen.docker-compose.yml: the shipped stack could not boot the current image (cap_drop: ALLdeniesgosuthesetuidit needs). Deny-by-default is preserved; the three added capabilities are the entire grant.read_only: trueandno-new-privileges: trueare unchanged. Operators preferring zero capabilities can pinuser: "10001:10001"and use the new direct-exec path, at the cost of losing the ability to repair a root-owned volume.config.tomlandconfig.toml.bakstop being world-readable between saves. The recursive chown is symlink-safe by construction. The auto-chmod no longer acts on files the process does not own.auth_store_failure: config_unreadabletag. Existing ACL/antivirus denials keep their current demoted treatment — pinned by a test.cfg(unix); on Windows the error text and behaviour are byte-identical to before. The desktop path is unaffected except that an unreadable config now yields a classified message instead of a raw Rust chain.Related
docker-volume-permissionssmoke job deleted by04175ad78;gitbooks/features/cloud-deploy.md:662-665still documents it as active. Adocker-volume-uid-driftsibling (pre-seed aroot:root 0600 config.toml, assert the container heals or exits non-zero) is the direct regression test for this bug and the only coverage the shell change can get./healthdoes not track config readability (health/core.rs:129CRITICAL_COMPONENTS), so Docker/Fly route traffic to a core that fails every config RPC.is_transient_fs_errorhas no non-Windows classification at all, soretry_with_backoff_asyncnever retries on Linux/macOS for any filesystem error.authStoreFailureUserMessageand its neighbours return hardcoded English rather than going throughuseT(); the new copy follows the existing convention in that module rather than introducing a mixed one.gitbooks/features/cloud-deploy.md:639-642documents a GHCR image at UID 1000; no core runtime image is published from this repo's workflows, so that claim needs confirming or correcting at the source.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/unreadable-config-blocks-signin31a39d7a5Validation Run
pnpm --filter openhuman-app format:check— and repo-widepnpm format:check(Prettier +cargo fmt --all --check, both manifests), rc=0pnpm typecheck— rc=0vitest run src/utils/__tests__/coreConfigFailure.test.ts src/utils/__tests__/desktopDeepLinkListener.test.ts src/pages/__tests__/Welcome.test.tsx→ 3 files, 60 tests, all passingpnpm lint— rc=0 (102 pre-existing warnings, 0 errors; none in changed files)pnpm i18n:check,pnpm i18n:english:check,pnpm docs:check— all rc=0sh -n scripts/docker-entrypoint-core.sh— cleancargo fmt --checkpasses;cargo check --lib --testsreports zero diagnostics in any changed file, but the lib does not link — see belowValidation Blocked
command:post-mergecargo test --lib openhuman::config::schema::load_testsand the pre-pushpnpm rust:clippyerror:alsa-syscannot find the host system package metadataalsa.pcimpact:The merge-resolution validation reaches the native dependency build but this checkout host does not have the ALSA development package installed. The pre-push hook was bypassed for this environment-only failure after its formatting, frontend lint, and TypeScript compile stages passed. CI's Linux image supplies the native build dependencies and is the authoritative Rust test/check lane.Behavior Changes
config.tomlis unreadable by the runtime user now fails to start (with a one-line remedy in the log) rather than booting into a state where every config RPC returnsEACCESbehind a healthy/health. Mis-owned workspaces that can be repaired are repaired silently.Parity Contract
#3962context/{:#}contract tests still hold. Denials without an ownership mismatch keep their existing Sentry demotion — pinned bystill_demotes_config_read_denial_without_owner_mismatch. Unrecognised client errors still render verbatim — pinned by the pre-existingshows error when storeSessionToken rejectstest, which is unmodified.contains()anchor inexpected_error_kind; only"being used by another process"mentions "process", and the appended text readsprocess euid=, so no earlier or later matcher can be tripped by the new metadata. The directory-at-config guard and theNotFoundexclusion are untouched.Duplicate / Superseded PR Handling
Summary by CodeRabbit
config.tomlis unreadable due to ownership/permissions, the runtime stops and shows localized recovery guidance without exposing sensitive filesystem details.0600) and improved detection/classification of config ownership/permission issues.docker execconfig.tomlscenario and the correct recovery commands/logs to monitor.