Skip to content

Secure cc-switch database and backup file permissions#221

Open
feiyehua wants to merge 18 commits into
SaladDay:mainfrom
feiyehua:feiyehua/database-permissions
Open

Secure cc-switch database and backup file permissions#221
feiyehua wants to merge 18 commits into
SaladDay:mainfrom
feiyehua:feiyehua/database-permissions

Conversation

@feiyehua

Copy link
Copy Markdown
Collaborator

Description
This PR tightens permissions around the cc-switch application database and related backup files.

Changes

  • Create the app config directory and database file with restrictive permissions on Unix:
    • config/backups directories: 0700
    • database and backup files: 0600
  • Detect existing insecure permissions before opening the database.
  • Prompt interactive users before fixing existing permissions, and warn in non-interactive mode.
  • Validate CC_SWITCH_CONFIG_DIR before database initialization, so library callers and non-CLI entry points also reject dangerous system directories like /, /etc, /usr, and /tmp.
  • Ensure SQL backup files created by ConfigService::create_backup are written under secure backup directories and chmodded to owner-only access.

Limitations
All the permissions related code are for Unix. Permissions on Windows left onchanged (probably not a big problem?)

@unive3sal

Copy link
Copy Markdown
Collaborator

Hi @feiyehua

I'm doing a refactor for CI/CD. It will execute test cases on a sandbox (usually the directory will be set to /tmp). So Would you like to investigate if your commit will reject /tmp directory during cargo test?

Thanks

@feiyehua

feiyehua commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator Author

Hi @feiyehua

I'm doing a refactor for CI/CD. It will execute test cases on a sandbox (usually the directory will be set to /tmp). So Would you like to investigate if your commit will reject /tmp directory during cargo test?

Thanks

Seems CC_SWITCH_CONFIG_DIR=/tmp will be rejected, as we should not change the permissions of system folders (or store database in system folder, especially public ones.).
However current tests set CC_SWITCH_CONFIG_DIR a subfolder under /tmp so it works fine.
Currently some proxy related test fails, but they seems to be pre-existing.

@unive3sal

Copy link
Copy Markdown
Collaborator

hi @feiyehua

I don't think executing test cases under /tmp directory is a dangerous behavior. The reasons as follows:

  • the default permission for /tmp directory is "1777". which means all of the user CAN read, write, and execution.
  • The integration test cases should be executed in a sandbox, otherwise it will break the host configuration and corrupt the test processes.
  • My current approach for CI/CD is setting all environment under /tmp
env HOME="$(mktemp -d)" CC_SWITCH_CONFIG_DIR="$HOME/.cc-switch" CLAUDE_CONFIG_DIR="$HOME/.claude" CODEX_HOME="$HOME/.codex" XDG_CONFIG_HOME="$HOME/.config" XDG_RUNTIME_DIR="$HOME/.runtime" XDG_STATE_HOME="$HOME/.state"

Therefore, I think /tmp directory should NOT be rejected.

@feiyehua

feiyehua commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator Author

hi @feiyehua

I don't think executing test cases under /tmp directory is a dangerous behavior. The reasons as follows:

  • the default permission for /tmp directory is "1777". which means all of the user CAN read, write, and execution.
  • The integration test cases should be executed in a sandbox, otherwise it will break the host configuration and corrupt the test processes.
  • My current approach for CI/CD is setting all environment under /tmp
env HOME="$(mktemp -d)" CC_SWITCH_CONFIG_DIR="$HOME/.cc-switch" CLAUDE_CONFIG_DIR="$HOME/.claude" CODEX_HOME="$HOME/.codex" XDG_CONFIG_HOME="$HOME/.config" XDG_RUNTIME_DIR="$HOME/.runtime" XDG_STATE_HOME="$HOME/.state"

Therefore, I think /tmp directory should NOT be rejected.

This commit only rejects CC_SWITCH_CONFIG_DIR=/tmp. If this is allowed, database such as /tmp/cc-switch.db will be created, which is strange. If the users use CC_SWITCH_CONFIG_DIR=/tmp and cc-switch-cli creates something with incorrect permissions, then it is exposed to all other users.
However, subfolders under /tmp, such as ones that created by mktemp -d, are perfectly fine and will not be rejected.
Tested this folder and it works fine:

echo $HOME && echo $CC_SWITCH_CONFIG_DIR
/tmp/tmp.CkI8wm9hdm
/tmp/tmp.CkI8wm9hdm/.cc-switch

@feiyehua

feiyehua commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed a new version that only checks permissions when app starts up. This ensures permission check runs only once and will not broke TUI.
Also add tests

@SaladDay

SaladDay commented Jun 3, 2026

Copy link
Copy Markdown
Owner

Thanks for the work here. The direction makes sense, but I do not think this PR is ready to merge yet. I found two blocking issues around the permission boundary.

Blockers:

  1. CC_SWITCH_CONFIG_DIR validation can be bypassed with .., and can chmod an unexpected parent directory.

validate_config_dir() falls back to the original path when canonicalize() fails. A path like <tmp>/child/.. passes validation if child does not exist yet. Later, create_secure_dir_all() creates child, then treats child/.. as an existing directory and applies restrict_dir_permissions() to it.

I reproduced this safely in a temporary directory:

config_dir=<tmp>/child/..
validate=Ok("ok")
before=0755
Database::init()=Ok("ok")
after=0700

So the current check rejects literal /tmp, but an equivalent path such as /tmp/new-child/.. can still pass and may end up chmodding /tmp itself. This needs robust normalization/canonicalization before any permission mutation, and paths containing unresolved .. should not be allowed to drive chmod behavior.

  1. The dangerous-directory validation is only wired into the CLI entrypoint, not the database initialization boundary.

The PR description says validation happens before database initialization so library callers and non-CLI entry points also reject dangerous directories. In the current code, validate_config_dir() is called from main.rs, but Database::init() itself does not call it.

I verified this with a minimal external caller:

CC_SWITCH_CONFIG_DIR=/tmp
cc_switch_lib::Database::init() -> INIT_OK

There is also a CLI-internal bypass: main.rs excludes Internal commands from database_access_required(), but internal capture-codex-temp calls AppState::try_new(), which reaches Database::init() without the new validation/prompt path.

This validation should live at the core DB/app-state initialization boundary, or every DB-opening entry point must be proven to go through the same guard.

Non-blocking but worth fixing while here: existing backup files under backups/ are not checked or repaired. The PR secures newly created backup files, but check_permissions() only checks the config dir, cc-switch.db, and the backups directory. Existing world-readable .sql / .db backups would remain exposed without warning.

I would hold merge until the two blockers above are fixed and covered by focused tests.

@feiyehua

feiyehua commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator Author

@SaladDay fixed these problems. Now:

  • Extracted a new resolve_existing_or_new_child_path() function that handles the case where the final
    path component doesn't exist:
    • Splits the path into parent + filename.
    • Canonicalizes the parent (which must exist), then re-appends the filename.
    • If the parent also fails to canonicalize, returns an error.
  • Log if Database::Init() detect insecure permissions

2 tests failing, seems caused by incorrectly dropping newly created temp test home:
cli::tui::app::tests::tests::prompt_create_runtime_clears_filter_when_new_prompt_is_not_visible
cli::tui::app::tests::tests::prompt_save_runtime_creates_prompt_from_one_page_form
fixed

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.

3 participants