From 0c085fb359b67d5f6c325aa5f5e6f29ebbcd76c4 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sun, 17 May 2026 07:04:03 -0400 Subject: [PATCH 1/7] feat: migrate credential storage to cli-common/credstore (Phase B pilot) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the hand-rolled internal/keychain (macOS `security` shell-out + Linux/Windows plaintext file) with the shared cli-common/credstore library, per the Open CLI Collective Secret-Handling Standard §2.4 + §1.3/§1.4/§1.7/ §1.8/§1.11/§1.12. slck is the first real credstore consumer; this PR's structure is the reusable per-CLI template for gro/atlassian. - internal/keychain is now a credstore adapter (OS keyring only; no shell-out, no plaintext file). Ref is authoritative: ParseRef(config credential_ref) -> Open(parsed-service) against parsed-profile. - New internal/config: config.yml (credential_ref, workspace, keyring.backend). - One-time legacy migration (§1.8/§2.4): old `slck` AND `slack-chat-api` Keychain services + the legacy plaintext credentials file -> the new bundle, with the api_token->bot_token rename. All conflicts detected before any write/delete; conflicts fail loud naming all locations and never print a value (§1.8/§1.12). Idempotent; --overwrite resolution. - _migration JSON signal (object-merge / non-object-wrap, consume-once) + one-line stderr notice. - New `slck set-credential` (stdin / --from-env ingress, §1.5.2). - §1.12 no-leak acceptance suite; all credential tests hermetic (file backend in a temp HOME), fixing the B0-deferred TestRunInit_WrongTokenType non-hermetic flake. BREAKING CHANGE: SLACK_API_TOKEN / SLACK_USER_TOKEN are no longer read at runtime (only as setup ingress via `init --bot-token-from-env`). Positional `config set-token ` and `init --bot-token ` value-flags are removed (use `slck set-credential --stdin` / `slck init`). Linux/Windows plaintext fallback is replaced by the OS keyring (fail-closed; opt-in encrypted-file backend). Closes #157 [INT-437] --- CLAUDE.md | 18 +- README.md | 113 +++-- go.mod | 14 +- go.sum | 35 +- internal/client/client.go | 14 +- internal/client/client_flag_test.go | 193 ++----- internal/cmd/config/clear.go | 73 +-- internal/cmd/config/config_test.go | 281 +++------- internal/cmd/config/delete_token.go | 68 +-- internal/cmd/config/noleak_test.go | 76 +++ internal/cmd/config/set_token.go | 98 +--- internal/cmd/config/show.go | 88 +++- internal/cmd/config/test_test.go | 13 +- internal/cmd/initcmd/init.go | 201 +++++--- internal/cmd/initcmd/init_test.go | 196 +++---- internal/cmd/root/root.go | 10 +- internal/cmd/setcred/setcred.go | 104 ++++ internal/cmd/setcred/setcred_test.go | 87 ++++ internal/cmd/whoami/whoami_test.go | 13 +- internal/config/config.go | 103 ++++ internal/keychain/keychain.go | 379 ++++++-------- internal/keychain/keychain_test.go | 733 ++++++--------------------- internal/keychain/migrate.go | 284 +++++++++++ internal/keychain/passphrase.go | 54 ++ internal/output/migration.go | 66 +++ internal/output/migration_test.go | 82 +++ internal/output/output.go | 18 +- internal/testutil/testutil.go | 32 ++ 28 files changed, 1839 insertions(+), 1607 deletions(-) create mode 100644 internal/cmd/config/noleak_test.go create mode 100644 internal/cmd/setcred/setcred.go create mode 100644 internal/cmd/setcred/setcred_test.go create mode 100644 internal/config/config.go create mode 100644 internal/keychain/migrate.go create mode 100644 internal/keychain/passphrase.go create mode 100644 internal/output/migration.go create mode 100644 internal/output/migration_test.go create mode 100644 internal/testutil/testutil.go diff --git a/CLAUDE.md b/CLAUDE.md index bf9042f..3085082 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -116,16 +116,28 @@ The `internal/client` package wraps the Slack API: ## Common Issues -- **Token not found**: Run `slck config set-token` or set `SLACK_API_TOKEN` +- **Token not found**: Run `slck init` or `slck set-credential --key bot_token --stdin`. Environment variables are NOT read at runtime (only as setup ingress, e.g. `init --bot-token-from-env`). - **Permission denied**: Check bot token scopes in Slack app settings - **Lint failures**: Run `make lint` locally before pushing - **golangci-lint version**: CI uses v2.0.2 with v2 config format +## Credentials + +slck stores credentials in the OS keyring via `cli-common/credstore` (Open +CLI Collective Secret-Handling Standard §2.4). The `internal/keychain` +package is a credstore adapter (no `security` shell-out, no plaintext file). +Non-secret config (`credential_ref`, `workspace`, `keyring.backend`) lives in +`~/.config/slack-chat-api/config.yml`. Ingress is only `slck init` / +`slck set-credential` (stdin or `--from-env`); never a flag/positional value. + ## Dependencies -- `github.com/slack-go/slack` - Slack API client +- `github.com/open-cli-collective/cli-common` - shared credstore (OS keyring) - `github.com/spf13/cobra` - CLI framework -- `github.com/zalando/go-keyring` - Cross-platform keychain +- `golang.org/x/term` - no-echo passphrase prompt (file-backend opt-in) + +(The HTTP Slack client is hand-rolled in `internal/client`; there is no +`slack-go`/`zalando` dependency.) ## Commit Conventions diff --git a/README.md b/README.md index c418540..7a61b12 100644 --- a/README.md +++ b/README.md @@ -129,22 +129,31 @@ make build ## Platform Support +Credentials are stored in the OS keyring via the shared `cli-common/credstore` +library: + | Platform | Credential Storage | |----------|-------------------| -| macOS | Secure (Keychain) | -| Linux | Config file (`~/.config/slack-chat-api/credentials`) | -| Windows | Config file (`%USERPROFILE%\.config\slack-chat-api\credentials`) | +| macOS | Keychain | +| Windows | Credential Manager | +| Linux | Secret Service (D-Bus); fails closed if a working keyring is locked. Encrypted-file backend only when there is no keyring at all, or by explicit opt-in (`keyring.backend: file` in `config.yml`). | -**Note:** On Linux and Windows, credentials are stored in a file with restricted permissions (0600). While not as secure as macOS Keychain, this is standard practice for CLI tools. +Tokens are **never** written to a plaintext file. Non-secret config +(`credential_ref`, `workspace`) lives in `~/.config/slack-chat-api/config.yml`. ### Credential Resolution -Credentials are resolved in this order (first match wins): - -1. **Environment variables** (highest priority) — `SLACK_API_TOKEN`, `SLACK_USER_TOKEN` -2. **Stored credentials** — Keychain (macOS) or config file (Linux/Windows) +At runtime the **only** source of a token is the OS keyring. Environment +variables are **not** read as credentials at runtime (per the Open CLI +Collective Secret-Handling Standard §1.11) — they are accepted only as +*ingress* during setup, e.g. `slck init --bot-token-from-env SLACK_BOT_TOKEN` +or `op read ... | slck set-credential --key bot_token --stdin`. -This means environment variables always override stored credentials, allowing automation tools to inject their own tokens without conflicting with locally stored ones. +> **Migrating from an older slck?** On first run, any tokens from a previous +> version (old macOS Keychain items under service `slck`/`slack-chat-api`, or +> the legacy `~/.config/slack-chat-api/credentials` file) are moved into the +> keyring automatically, once, then the originals are removed. A one-line +> notice is printed to stderr (and a `_migration` block to JSON output). ## Authentication @@ -303,47 +312,47 @@ This means environment variables always override stored credentials, allowing au } ``` - **Upgrading an existing install?** Paste the full manifest above into your app's **Features → App Manifest** tab (replacing the previous manifest), click **Save Changes**, then click **Install App → Reinstall to Workspace** to grant the new scopes. Copy the fresh `xoxb-…` token and run `slck config set-token`. + **Upgrading an existing install?** Paste the full manifest above into your app's **Features → App Manifest** tab (replacing the previous manifest), click **Save Changes**, then click **Install App → Reinstall to Workspace** to grant the new scopes. Copy the fresh `xoxb-…` token and run `slck init` (or `slck set-credential`). 4. Click **Create** → **Install to Workspace** → **Allow** 5. Copy the **Bot User OAuth Token** (starts with `xoxb-`) -6. Run: +6. Run the interactive setup: ```bash - slck config set-token - # Paste your token when prompted + slck init + # Paste your token when prompted (input is not echoed back) ``` -Your token is stored securely in macOS Keychain, or in a config file on Linux and Windows. +Your token is stored in the OS keyring (Keychain / Credential Manager / +Secret Service). It is never written to a plaintext file. **NOTE:** If you plan on sending messages or taking actions using your user token _(See: Choosing Between Bot and User Tokens)_, you'll need to adjust the manifest above to have all the same scopes configured for your user as your bot (with the exception of the `"channels:manage"` scope, which only applies to bots). -### Alternative: Environment Variable - -```bash -export SLACK_API_TOKEN=xoxb-your-token-here -``` - -### Alternative: 1Password Integration +### Scripted / non-interactive setup -Use a shell function to lazy-load your token from 1Password on first use: +The token is read only from stdin or a named env var — never a flag or +positional value (so it can't leak via shell history or `ps`): ```bash -# Add to ~/.zshrc or ~/.bashrc -slack-chat() { - if [[ -z "$SLACK_API_TOKEN" ]]; then - export SLACK_API_TOKEN="$(op read 'op://Personal/slck/api_token')" - fi - command slck "$@" -} +# From a pipe (preferred for automation): +op read 'op://Personal/slck/bot_token' | slck set-credential --key bot_token --stdin + +# Or, during full init, from a named env var (the env var is consumed at +# setup time only; it is NOT read on subsequent runs): +slck init --bot-token-from-env SLACK_BOT_TOKEN --user-token-from-env SLACK_USER_TOKEN ``` -Or create an alias that always fetches fresh: +### 1Password integration + +Because the token lives in the keyring after setup, you do **not** wrap every +invocation in `op`. Stage it once: ```bash -alias slack-chat='SLACK_API_TOKEN="$(op read '\''op://Personal/slck/api_token'\'')" slck' +op read 'op://Personal/slck/bot_token' | slck set-credential --key bot_token --stdin +op read 'op://Personal/slck/user_token' | slck set-credential --key user_token --stdin ``` -Replace `op://Personal/slck/api_token` with your 1Password secret reference. +Replace the `op://…` references with your own 1Password secret references. +Re-run only when the token rotates. ### Required Scopes @@ -394,14 +403,14 @@ Most commands use the **bot token**. Search commands require a **user token**. **Setting up both tokens:** ```bash -# Set bot token (for channels, users, messages, workspace) -slck config set-token xoxb-your-bot-token +# Bot token (for channels, users, messages, workspace) +op read 'op://Personal/slck/bot_token' | slck set-credential --key bot_token --stdin -# Set user token (for search) -slck config set-token xoxp-your-user-token +# User token (for search) +op read 'op://Personal/slck/user_token' | slck set-credential --key user_token --stdin ``` -The `set-token` command automatically detects the token type and stores it appropriately. +Or run `slck init` for a guided, interactive setup of both. **Getting a user token:** @@ -410,12 +419,12 @@ The `set-token` command automatically detects the token type and stores it appro 3. Reinstall app to workspace (if already installed) 4. Copy the **User OAuth Token** (starts with `xoxp-`) -**Environment variables:** +**Setup-time env-var ingress** (read once during `slck init`, never at runtime): -| Variable | Token Type | Description | -|----------|------------|-------------| -| `SLACK_API_TOKEN` | Bot | Bot token for most commands | -| `SLACK_USER_TOKEN` | User | User token for search commands | +| Flag | Description | +|------|-------------| +| `slck init --bot-token-from-env NAME` | Read the bot token from env var `NAME` at setup | +| `slck init --user-token-from-env NAME` | Read the user token from env var `NAME` at setup | ## Global Flags @@ -743,16 +752,16 @@ slck whoami ### Config ```bash -# Set API token (interactive prompt) -slck config set-token +# Guided interactive setup (bot + optional user token) +slck init -# Set API token directly -slck config set-token xoxb-your-token-here +# Set one credential from stdin (scriptable; no value on the command line) +op read 'op://Personal/slck/bot_token' | slck set-credential --key bot_token --stdin -# Show current config status +# Show current config status (backend, ref, which keys present — never values) slck config show -# Delete stored token +# Delete stored token(s) slck config delete-token ``` @@ -820,12 +829,16 @@ Commands have convenient aliases: | Variable | Description | |----------|-------------| -| `SLACK_API_TOKEN` | Bot token (overrides stored bot token) | -| `SLACK_USER_TOKEN` | User token for search (overrides stored user token) | | `SLCK_AS_USER` | Set to `true` or `1` to default to user token instead of bot token | +| `SLACK_CHAT_API_KEYRING_BACKEND` | Force the keyring backend (e.g. `file`) — non-secret selector (§1.4) | +| `SLACK_CHAT_API_KEYRING_PASSPHRASE` | Passphrase for the encrypted-file backend (the one runtime secret-env exception, §1.4) | | `NO_COLOR` | Disable colored output when set | | `XDG_CONFIG_HOME` | Custom config directory (default: `~/.config`) | +> `SLACK_API_TOKEN` / `SLACK_USER_TOKEN` are **no longer read at runtime** +> (§1.11). Use `slck init --bot-token-from-env` / `slck set-credential` to +> stage a token into the keyring instead. + ## Known Limitations ### Message Length Limits diff --git a/go.mod b/go.mod index fda2993..fc91b81 100644 --- a/go.mod +++ b/go.mod @@ -1,16 +1,26 @@ module github.com/open-cli-collective/slack-chat-api -go 1.24 +go 1.25.0 require ( + github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 github.com/spf13/cobra v1.8.0 github.com/stretchr/testify v1.11.1 + golang.org/x/term v0.43.0 + gopkg.in/yaml.v3 v3.0.1 ) require ( + github.com/99designs/go-keychain v0.0.0-20191008050251-8e49817e8af4 // indirect + github.com/99designs/keyring v1.2.2 // indirect + github.com/danieljoos/wincred v1.1.2 // indirect github.com/davecgh/go-spew v1.1.1 // indirect + github.com/dvsekhvalnov/jose2go v1.5.0 // indirect + github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2 // indirect + github.com/gsterjov/go-libsecret v0.0.0-20161001094733-a6f4afe4910c // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect + github.com/mtibben/percent v0.2.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/spf13/pflag v1.0.5 // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect + golang.org/x/sys v0.44.0 // indirect ) diff --git a/go.sum b/go.sum index ed04719..7d61249 100644 --- a/go.sum +++ b/go.sum @@ -1,8 +1,30 @@ +github.com/99designs/go-keychain v0.0.0-20191008050251-8e49817e8af4 h1:/vQbFIOMbk2FiG/kXiLl8BRyzTWDw7gX/Hz7Dd5eDMs= +github.com/99designs/go-keychain v0.0.0-20191008050251-8e49817e8af4/go.mod h1:hN7oaIRCjzsZ2dE+yG5k+rsdt3qcwykqK6HVGcKwsw4= +github.com/99designs/keyring v1.2.2 h1:pZd3neh/EmUzWONb35LxQfvuY7kiSXAq3HQd97+XBn0= +github.com/99designs/keyring v1.2.2/go.mod h1:wes/FrByc8j7lFOAGLGSNEg8f/PaI3cgTBqhFkHUrPk= github.com/cpuguy83/go-md2man/v2 v2.0.3/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= +github.com/danieljoos/wincred v1.1.2 h1:QLdCxFs1/Yl4zduvBdcHB8goaYk9RARS2SgLLRuAyr0= +github.com/danieljoos/wincred v1.1.2/go.mod h1:GijpziifJoIBfYh+S7BbkdUTU4LfM+QnGqR5Vl2tAx0= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/dvsekhvalnov/jose2go v1.5.0 h1:3j8ya4Z4kMCwT5nXIKFSV84YS+HdqSSO0VsTQxaLAeM= +github.com/dvsekhvalnov/jose2go v1.5.0/go.mod h1:QsHjhyTlD/lAVqn/NSbVZmSCGeDehTB/mPZadG+mhXU= +github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2 h1:ZpnhV/YsD2/4cESfV5+Hoeu/iUR3ruzNvZ+yQfO03a0= +github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2/go.mod h1:bBOAhwG1umN6/6ZUMtDFBMQR8jRg9O75tm9K00oMsK4= +github.com/gsterjov/go-libsecret v0.0.0-20161001094733-a6f4afe4910c h1:6rhixN/i8ZofjG1Y75iExal34USq5p+wiN1tpie8IrU= +github.com/gsterjov/go-libsecret v0.0.0-20161001094733-a6f4afe4910c/go.mod h1:NMPJylDgVpX0MLRlPy15sqSwOFv/U1GZ2m21JhFfek0= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= +github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= +github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= +github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= +github.com/mtibben/percent v0.2.1 h1:5gssi8Nqo8QU/r2pynCm+hBQHpkB/uNK7BJCFogWdzs= +github.com/mtibben/percent v0.2.1/go.mod h1:KG9uO+SZkUp+VkRHsCdYQV3XSZrrSpR3O9ibNBTZrns= +github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e h1:fD57ERR4JtEqsWbfPhv4DMiApHyliiK5xCTNVSPiaAs= +github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= +github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 h1:78EW5uCbAzbAO32+oY4HDaFOqS2sPYnc4AT+G5UjdL0= +github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14/go.mod h1:5i4MkFToMVPLBW29O01lsHS9d1m9pC0BxSOYjFDz7ds= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= @@ -10,9 +32,20 @@ github.com/spf13/cobra v1.8.0 h1:7aJaZx1B85qltLMc546zn58BxxfZdR/W22ej9CFoEf0= github.com/spf13/cobra v1.8.0/go.mod h1:WXLWApfZ71AjXPya3WOlMsY9yMs7YeiHhFVlvLyhcho= github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= -gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +golang.org/x/sys v0.0.0-20210819135213-f52c844e1c1c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.44.0 h1:ildZl3J4uzeKP07r2F++Op7E9B29JRUy+a27EibtBTQ= +golang.org/x/sys v0.44.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/term v0.43.0 h1:S4RLU2sB31O/NCl+zFN9Aru9A/Cq2aqKpTZJ6B+DwT4= +golang.org/x/term v0.43.0/go.mod h1:lrhlHNdQJHO+1qVYiHfFKVuVioJIheAc3fBSMFYEIsk= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b h1:QRR6H1YWRnHb4Y/HeNFCTJLFVxaq6wH4YuVdsUOr75U= +gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/client/client.go b/internal/client/client.go index c8fb666..17cb56e 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -41,7 +41,12 @@ type Client struct { // NewBotClient creates a new Slack client using the bot token func NewBotClient() (*Client, error) { - token, err := keychain.GetAPIToken() + st, err := keychain.Open() + if err != nil { + return nil, err + } + defer func() { _ = st.Close() }() + token, err := st.BotToken() if err != nil { return nil, err } @@ -91,7 +96,12 @@ func NewWithConfig(baseURL, token string, httpClient *http.Client) *Client { // NewUserClient creates a new Slack client using the user token (for search) func NewUserClient() (*Client, error) { - token, err := keychain.GetUserToken() + st, err := keychain.Open() + if err != nil { + return nil, err + } + defer func() { _ = st.Close() }() + token, err := st.UserToken() if err != nil { return nil, fmt.Errorf("user token required for search: %w", err) } diff --git a/internal/client/client_flag_test.go b/internal/client/client_flag_test.go index dac91f3..79ee5d2 100644 --- a/internal/client/client_flag_test.go +++ b/internal/client/client_flag_test.go @@ -1,183 +1,88 @@ package client import ( - "os" - "strings" "testing" + "github.com/stretchr/testify/require" + "github.com/open-cli-collective/slack-chat-api/internal/keychain" + "github.com/open-cli-collective/slack-chat-api/internal/testutil" ) -func TestNew(t *testing.T) { - // Save and restore environment and token pointer - originalEnv := os.Getenv("SLCK_AS_USER") - originalPtr := useUserToken - defer func() { - if originalEnv != "" { - os.Setenv("SLCK_AS_USER", originalEnv) - } else { - os.Unsetenv("SLCK_AS_USER") - } - useUserToken = originalPtr - }() - - // Check if we have tokens available for testing - hasAPIToken := keychain.HasStoredToken() || os.Getenv("SLACK_API_TOKEN") != "" - hasUserToken := keychain.HasStoredUserToken() || os.Getenv("SLACK_USER_TOKEN") != "" - - trueVal := true - falseVal := false +const ( + flagBotTok = "xoxb-flagtest-bot" + flagUserTok = "xoxp-flagtest-user" +) - tests := []struct { - name string - tokenPtr *bool - envValue string - expectUser bool - description string +// TestNew exercises the --as-bot/--as-user flag and SLCK_AS_USER precedence +// in New(). Tokens are seeded into a hermetic file-backed keyring so every +// branch resolves and we can assert which token New() selected. +func TestNew(t *testing.T) { + testutil.Setup(t) + st, err := keychain.Open() + require.NoError(t, err) + require.NoError(t, st.SetBotToken(flagBotTok)) + require.NoError(t, st.SetUserToken(flagUserTok)) + require.NoError(t, st.Close()) + + trueVal, falseVal := true, false + cases := []struct { + name string + ptr *bool + env string + wantToken string }{ - { - name: "explicit user token", - tokenPtr: &trueVal, - envValue: "", - expectUser: true, - description: "When explicitly set to user, should use user token", - }, - { - name: "explicit bot token", - tokenPtr: &falseVal, - envValue: "", - expectUser: false, - description: "When explicitly set to bot, should use bot token", - }, - { - name: "unset with no env uses bot token", - tokenPtr: nil, - envValue: "", - expectUser: false, - description: "When unset and no env var, should use bot token", - }, - { - name: "unset with env true uses user token", - tokenPtr: nil, - envValue: "true", - expectUser: true, - description: "When unset and SLCK_AS_USER=true, should use user token", - }, - { - name: "unset with env 1 uses user token", - tokenPtr: nil, - envValue: "1", - expectUser: true, - description: "When unset and SLCK_AS_USER=1, should use user token", - }, - { - name: "unset with env false uses bot token", - tokenPtr: nil, - envValue: "false", - expectUser: false, - description: "When unset and SLCK_AS_USER=false, should use bot token", - }, - { - name: "explicit user ignores env var", - tokenPtr: &trueVal, - envValue: "false", - expectUser: true, - description: "Explicit user should ignore env var", - }, - { - name: "explicit bot overrides env var", - tokenPtr: &falseVal, - envValue: "true", - expectUser: false, - description: "Explicit bot should override SLCK_AS_USER=true", - }, + {"explicit user", &trueVal, "", flagUserTok}, + {"explicit bot", &falseVal, "", flagBotTok}, + {"unset, no env -> bot", nil, "", flagBotTok}, + {"unset, env true -> user", nil, "true", flagUserTok}, + {"unset, env 1 -> user", nil, "1", flagUserTok}, + {"unset, env false -> bot", nil, "false", flagBotTok}, + {"explicit user ignores env", &trueVal, "false", flagUserTok}, + {"explicit bot overrides env", &falseVal, "true", flagBotTok}, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Set token pointer and environment - useUserToken = tt.tokenPtr - if tt.envValue != "" { - os.Setenv("SLCK_AS_USER", tt.envValue) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + useUserToken = tc.ptr + t.Cleanup(func() { useUserToken = nil }) + if tc.env != "" { + t.Setenv("SLCK_AS_USER", tc.env) } else { - os.Unsetenv("SLCK_AS_USER") - } - - // Call the function - may succeed if tokens are present, or fail if not - client, err := New() - - // Case 1: If we have the appropriate token, client should be created successfully - if tt.expectUser && hasUserToken { - if err != nil { - t.Errorf("Expected success with user token, got error: %v", err) - } - if client == nil { - t.Errorf("Expected client to be created") - } - return - } - - if !tt.expectUser && hasAPIToken { - if err != nil { - t.Errorf("Expected success with bot token, got error: %v", err) - } - if client == nil { - t.Errorf("Expected client to be created") - } - return - } - - // Case 2: If we don't have the appropriate token, should get specific error - if err == nil { - t.Errorf("Expected error due to missing token, got nil") - return + t.Setenv("SLCK_AS_USER", "") } - // Check error message indicates correct token type was attempted - errMsg := err.Error() - if tt.expectUser { - // Should try to get user token - if !strings.Contains(errMsg, "user token") && !strings.Contains(errMsg, "xoxp-") { - t.Errorf("Expected user token error, got: %s", errMsg) - } - } else { - // Should try to get bot token (but NOT specifically request user token) - if strings.Contains(errMsg, "user token") || strings.Contains(errMsg, "xoxp-") { - t.Errorf("Expected bot token error, got user token error: %s", errMsg) - } + c, err := New() + require.NoError(t, err) + require.NotNil(t, c) + if c.token != tc.wantToken { + t.Fatalf("New() selected token %q, want %q", c.token, tc.wantToken) } }) } } func TestSetAsUser(t *testing.T) { - // Save original value original := useUserToken - defer func() { - useUserToken = original - }() + defer func() { useUserToken = original }() SetAsUser(true) if useUserToken == nil || !*useUserToken { - t.Errorf("Expected useUserToken to point to true after SetAsUser(true)") + t.Errorf("Expected useUserToken true after SetAsUser(true)") } - SetAsUser(false) if useUserToken == nil || *useUserToken { - t.Errorf("Expected useUserToken to point to false after SetAsUser(false)") + t.Errorf("Expected useUserToken false after SetAsUser(false)") } } func TestResetTokenMode(t *testing.T) { - // Set to user mode SetAsUser(true) if useUserToken == nil { - t.Errorf("Expected useUserToken to be non-nil after SetAsUser(true)") + t.Errorf("Expected useUserToken non-nil after SetAsUser(true)") } - - // Reset ResetTokenMode() if useUserToken != nil { - t.Errorf("Expected useUserToken to be nil after reset, got %v", useUserToken) + t.Errorf("Expected useUserToken nil after reset, got %v", useUserToken) } } diff --git a/internal/cmd/config/clear.go b/internal/cmd/config/clear.go index 3a152e8..2353e53 100644 --- a/internal/cmd/config/clear.go +++ b/internal/cmd/config/clear.go @@ -1,58 +1,65 @@ package config import ( + "os" + "github.com/spf13/cobra" + appconfig "github.com/open-cli-collective/slack-chat-api/internal/config" "github.com/open-cli-collective/slack-chat-api/internal/keychain" "github.com/open-cli-collective/slack-chat-api/internal/output" ) +type clearOptions struct { + all bool +} + func newClearCmd() *cobra.Command { - return &cobra.Command{ + opts := &clearOptions{} + cmd := &cobra.Command{ Use: "clear", - Short: "Remove all stored credentials", - Long: `Remove all stored Slack API tokens at once. - -This is equivalent to running: - slck config delete-token --type all --force - -Note: Environment variables (SLACK_API_TOKEN, SLACK_USER_TOKEN) are not affected.`, + Short: "Remove stored credentials for the active profile", + Long: `Remove the keyring credentials under the active credential_ref +(§1.7). Scope is the active profile only — other profiles and other CLIs are +untouched. With --all, also remove config.yml (return to a pre-init state). +Idempotent and non-interactive.`, RunE: func(cmd *cobra.Command, args []string) error { - return runClear() + return runClear(opts) }, } + cmd.Flags().BoolVar(&opts.all, "all", false, "Also remove config.yml (pre-init state)") + return cmd } -func runClear() error { - clearedAny := false +func runClear(opts *clearOptions) error { + st, err := keychain.Open() + if err != nil { + return err + } + defer func() { _ = st.Close() }() - if keychain.HasStoredToken() { - if err := keychain.DeleteAPIToken(); err != nil { - return err + removed, err := st.Clear() + if err != nil { + return err + } + if len(removed) == 0 { + output.Printf("No keyring credentials under %s.\n", st.Ref()) + } else { + for _, k := range removed { + output.Printf("Removed %s from %s\n", k, st.Ref()) } - output.Println("Cleared bot token") - clearedAny = true } - if keychain.HasStoredUserToken() { - if err := keychain.DeleteUserToken(); err != nil { + if opts.all { + p := appconfig.Path() + switch err := os.Remove(p); { + case err == nil: + output.Printf("Removed %s\n", p) + case os.IsNotExist(err): + // idempotent + default: return err } - output.Println("Cleared user token") - clearedAny = true - } - - if !clearedAny { - output.Println("No stored tokens to clear.") } - - // Warn about env vars - hasEnvBot := keychain.GetTokenSource() == "environment variable" - hasEnvUser := keychain.GetUserTokenSource() == "environment variable" - if hasEnvBot || hasEnvUser { - output.Println() - output.Println("Note: Environment variable SLACK_API_TOKEN and/or SLACK_USER_TOKEN will still be used if set.") - } - return nil } diff --git a/internal/cmd/config/config_test.go b/internal/cmd/config/config_test.go index 4556c8d..8e0a25c 100644 --- a/internal/cmd/config/config_test.go +++ b/internal/cmd/config/config_test.go @@ -1,191 +1,70 @@ package config import ( + "bytes" "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/open-cli-collective/cli-common/credstore" + "github.com/open-cli-collective/slack-chat-api/internal/keychain" + "github.com/open-cli-collective/slack-chat-api/internal/output" + "github.com/open-cli-collective/slack-chat-api/internal/testutil" ) -func TestRunSetToken_Success(t *testing.T) { - if keychain.IsSecureStorage() { - // On macOS, tokens go to Keychain - just verify no error - opts := &setTokenOptions{} - err := runSetToken("xoxb-test-token-12345678", opts) - require.NoError(t, err) - - // Clean up - _ = keychain.DeleteAPIToken() - } else { - // On Linux, use temp directory - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - opts := &setTokenOptions{} - err := runSetToken("xoxb-test-token-12345678", opts) - require.NoError(t, err) - } -} +const distinctiveTok = "xoxb-DISTINCTIVE-secret-value-9999" -func TestRunSetToken_EmptyToken_WithInput(t *testing.T) { - // Test with a provided token (not empty string that triggers stdin read) - opts := &setTokenOptions{} - - // The runSetToken function checks for empty after stdin read, - // so we can't test the empty validation without stdin - // Instead, test that a valid token works - if keychain.IsSecureStorage() { - err := runSetToken("xoxb-valid-token-12345678", opts) - require.NoError(t, err) - _ = keychain.DeleteAPIToken() - } else { - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - err := runSetToken("xoxb-valid-token-12345678", opts) - require.NoError(t, err) - } +func captureOutput(t *testing.T, fn func() error) (string, error) { + t.Helper() + var buf bytes.Buffer + orig := output.Writer + output.Writer = &buf + t.Cleanup(func() { output.Writer = orig }) + err := fn() + return buf.String(), err } -func TestRunDeleteToken_Success(t *testing.T) { - if keychain.IsSecureStorage() { - // Set then delete via keychain - setOpts := &setTokenOptions{} - err := runSetToken("xoxb-test-token-12345678", setOpts) - require.NoError(t, err) - - deleteOpts := &deleteTokenOptions{tokenType: "all"} - err = runDeleteToken(deleteOpts) - require.NoError(t, err) - } else { - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - setOpts := &setTokenOptions{} - err := runSetToken("xoxb-test-token-12345678", setOpts) - require.NoError(t, err) - - deleteOpts := &deleteTokenOptions{tokenType: "all"} - err = runDeleteToken(deleteOpts) - require.NoError(t, err) +// set-token is hard-deprecated (§2.4): every invocation fails and points at +// set-credential, accepting the value via no path. +func TestSetTokenHardDeprecated(t *testing.T) { + cmd := newSetTokenCmd() + for _, args := range [][]string{{}, {"xoxb-leak-attempt"}} { + cmd.SetArgs(args) + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "set-credential") + assert.Contains(t, err.Error(), "removed") } } func TestRunShow_NoToken(t *testing.T) { - if keychain.IsSecureStorage() { - // Make sure no token is set - _ = keychain.DeleteAPIToken() - } - - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - opts := &showOptions{} - - // Should not error, just show "Not configured" - err := runShow(opts) + testutil.Setup(t) + out, err := captureOutput(t, func() error { return runShow(&showOptions{}) }) require.NoError(t, err) + assert.Contains(t, out, "not configured") } -func TestRunShow_WithToken(t *testing.T) { - if keychain.IsSecureStorage() { - // Set a token - setOpts := &setTokenOptions{} - err := runSetToken("xoxb-test-token-12345678901234567890", setOpts) - require.NoError(t, err) - - showOpts := &showOptions{} - err = runShow(showOpts) - require.NoError(t, err) - - // Clean up - _ = keychain.DeleteAPIToken() - } else { - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - setOpts := &setTokenOptions{} - err := runSetToken("xoxb-test-token-12345678901234567890", setOpts) - require.NoError(t, err) - - showOpts := &showOptions{} - err = runShow(showOpts) - require.NoError(t, err) - } -} - -func TestRunDeleteToken_WhenNoToken(t *testing.T) { - if keychain.IsSecureStorage() { - // First ensure no token exists - _ = keychain.DeleteAPIToken() - } - - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - opts := &deleteTokenOptions{} - - // On keychain it may error if no item found, on file-based it's fine - err := runDeleteToken(opts) - // We don't assert on error since behavior varies by platform - _ = err -} - -func TestRunSetToken_TokenFormats(t *testing.T) { - tests := []struct { - name string - token string - }{ - {"bot token", "xoxb-fake-token-for-testing-only"}, - {"user token", "xoxp-fake-token-for-testing-only"}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if keychain.IsSecureStorage() { - opts := &setTokenOptions{} - err := runSetToken(tt.token, opts) - require.NoError(t, err) - _ = keychain.DeleteAPIToken() - _ = keychain.DeleteUserToken() - } else { - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - opts := &setTokenOptions{} - err := runSetToken(tt.token, opts) - assert.NoError(t, err) - } - }) - } -} - -func TestRunSetToken_InvalidTokenFormat(t *testing.T) { - invalidTokens := []struct { - name string - token string - }{ - {"app token", "xapp-fake-token-for-testing-only"}, - {"unknown prefix", "invalid-token-format"}, - } +func TestRunShow_WithToken_NeverPrintsValue(t *testing.T) { + testutil.Setup(t) + st, err := keychain.Open() + require.NoError(t, err) + require.NoError(t, st.SetBotToken(distinctiveTok)) + _ = st.Close() - for _, tt := range invalidTokens { - t.Run(tt.name, func(t *testing.T) { - opts := &setTokenOptions{} - err := runSetToken(tt.token, opts) - assert.Error(t, err) - assert.Contains(t, err.Error(), "unrecognized token format") - }) + out, err := captureOutput(t, func() error { return runShow(&showOptions{}) }) + require.NoError(t, err) + assert.Contains(t, out, "present") + // §1.11 item 3 / §1.12: not even a masked prefix of the value. + if leak := credstore.NoLeakAssertion([]byte(out), distinctiveTok); leak != nil { + t.Fatalf("config show leaked: %v", leak) } } -// Confirmation prompt tests for delete-token command - func TestRunDeleteToken_Confirmation(t *testing.T) { - tests := []struct { + cases := []struct { name string input string force bool @@ -193,50 +72,52 @@ func TestRunDeleteToken_Confirmation(t *testing.T) { }{ {"force skips prompt", "", true, true}, {"y confirms", "y\n", false, true}, - {"yes confirms", "yes\n", false, true}, - {"YES confirms (case insensitive)", "YES\n", false, true}, + {"YES confirms", "YES\n", false, true}, {"n cancels", "n\n", false, false}, - {"no cancels", "no\n", false, false}, - {"empty input cancels", "\n", false, false}, - {"other input cancels", "maybe\n", false, false}, - {"whitespace y confirms", " y \n", false, true}, + {"empty cancels", "\n", false, false}, + {"whitespace y", " y \n", false, true}, } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Use temp dir to avoid affecting real keychain - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - // Skip keychain tests on macOS since we can't easily mock it - if keychain.IsSecureStorage() { - t.Skip("Skipping on macOS - keychain can't be easily mocked") - } - - // First set a token - setOpts := &setTokenOptions{} - err := runSetToken("xoxb-test-token-12345678", setOpts) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + testutil.Setup(t) + seed, err := keychain.Open() require.NoError(t, err) - - // Now test delete with confirmation - deleteOpts := &deleteTokenOptions{ - force: tt.force, - stdin: strings.NewReader(tt.input), - tokenType: "all", - } - - err = runDeleteToken(deleteOpts) + require.NoError(t, seed.SetBotToken("xoxb-test-token-12345678")) + _ = seed.Close() + + _, err = captureOutput(t, func() error { + return runDeleteToken(&deleteTokenOptions{ + force: tc.force, stdin: strings.NewReader(tc.input), tokenType: "all", + }) + }) require.NoError(t, err) - // Check if token was actually deleted - _, tokenErr := keychain.GetAPIToken() - if tt.expectDelete { - assert.Error(t, tokenErr, "token should have been deleted") - } else { - assert.NoError(t, tokenErr, "token should still exist") - // Clean up for next test - _ = keychain.DeleteAPIToken() - } + chk, err := keychain.Open() + require.NoError(t, err) + defer func() { _ = chk.Close() }() + assert.Equal(t, !tc.expectDelete, chk.HasBotToken()) }) } } + +func TestRunDeleteToken_WhenNoToken(t *testing.T) { + testutil.Setup(t) + _, err := captureOutput(t, func() error { + return runDeleteToken(&deleteTokenOptions{tokenType: "all"}) + }) + require.NoError(t, err) +} + +func TestRunClear_Idempotent(t *testing.T) { + testutil.Setup(t) + seed, err := keychain.Open() + require.NoError(t, err) + require.NoError(t, seed.SetBotToken("xoxb-test-token-12345678")) + _ = seed.Close() + + _, err = captureOutput(t, func() error { return runClear(&clearOptions{}) }) + require.NoError(t, err) + // Second clear is a no-op, still succeeds (§1.7 idempotent). + _, err = captureOutput(t, func() error { return runClear(&clearOptions{}) }) + require.NoError(t, err) +} diff --git a/internal/cmd/config/delete_token.go b/internal/cmd/config/delete_token.go index e45ef92..03abdc8 100644 --- a/internal/cmd/config/delete_token.go +++ b/internal/cmd/config/delete_token.go @@ -24,8 +24,8 @@ func newDeleteTokenCmd() *cobra.Command { cmd := &cobra.Command{ Use: "delete-token", - Short: "Delete stored Slack API token(s)", - Long: `Delete stored Slack API token(s). + Short: "Delete stored Slack token(s) from the keyring", + Long: `Delete stored Slack token(s) from the OS keyring. Use --type to specify which token to delete: - bot: Delete the bot token (xoxb-*) @@ -43,58 +43,43 @@ Use --type to specify which token to delete: } func runDeleteToken(opts *deleteTokenOptions) error { - // Validate token type if opts.tokenType != "bot" && opts.tokenType != "user" && opts.tokenType != "all" { return fmt.Errorf("invalid token type: %s (must be bot, user, or all)", opts.tokenType) } - // Check what tokens exist - hasBotToken := keychain.HasStoredToken() - hasUserToken := keychain.HasStoredUserToken() + st, err := keychain.Open() + if err != nil { + return err + } + defer func() { _ = st.Close() }() - // Determine what we're deleting - deleteBot := (opts.tokenType == "bot" || opts.tokenType == "all") && hasBotToken - deleteUser := (opts.tokenType == "user" || opts.tokenType == "all") && hasUserToken + hasBot := st.HasBotToken() + hasUser := st.HasUserToken() + deleteBot := (opts.tokenType == "bot" || opts.tokenType == "all") && hasBot + deleteUser := (opts.tokenType == "user" || opts.tokenType == "all") && hasUser if !deleteBot && !deleteUser { - switch opts.tokenType { - case "bot": - output.Println("No bot token stored to delete.") - if os.Getenv("SLACK_API_TOKEN") != "" { - output.Println("Note: Bot token is set via SLACK_API_TOKEN environment variable.") - } - case "user": - output.Println("No user token stored to delete.") - if os.Getenv("SLACK_USER_TOKEN") != "" { - output.Println("Note: User token is set via SLACK_USER_TOKEN environment variable.") - } - default: - output.Println("No tokens stored to delete.") - if os.Getenv("SLACK_API_TOKEN") != "" || os.Getenv("SLACK_USER_TOKEN") != "" { - output.Println("Note: Tokens may be set via environment variables.") - } - } + output.Println("No matching tokens stored to delete.") return nil } - // Prompt for confirmation unless --force if !opts.force { reader := opts.stdin if reader == nil { reader = os.Stdin } - var tokenDesc string + var desc string switch { case deleteBot && deleteUser: - tokenDesc = "bot and user tokens" + desc = "bot and user tokens" case deleteBot: - tokenDesc = "bot token" + desc = "bot token" case deleteUser: - tokenDesc = "user token" + desc = "user token" } - output.Printf("About to delete the stored %s.\n", tokenDesc) + output.Printf("About to delete the stored %s.\n", desc) output.Printf("Are you sure? [y/N]: ") scanner := bufio.NewScanner(reader) @@ -107,28 +92,17 @@ func runDeleteToken(opts *deleteTokenOptions) error { } } - // Delete tokens if deleteBot { - if err := keychain.DeleteAPIToken(); err != nil { + if err := st.DeleteBotToken(); err != nil { return fmt.Errorf("failed to delete bot token: %w", err) } - if keychain.IsSecureStorage() { - output.Println("Bot token deleted from Keychain") - } else { - output.Println("Bot token deleted from config file") - } + output.Printf("Deleted bot_token from %s\n", st.Ref()) } - if deleteUser { - if err := keychain.DeleteUserToken(); err != nil { + if err := st.DeleteUserToken(); err != nil { return fmt.Errorf("failed to delete user token: %w", err) } - if keychain.IsSecureStorage() { - output.Println("User token deleted from Keychain") - } else { - output.Println("User token deleted from config file") - } + output.Printf("Deleted user_token from %s\n", st.Ref()) } - return nil } diff --git a/internal/cmd/config/noleak_test.go b/internal/cmd/config/noleak_test.go new file mode 100644 index 0000000..7709373 --- /dev/null +++ b/internal/cmd/config/noleak_test.go @@ -0,0 +1,76 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/open-cli-collective/cli-common/credstore" + + "github.com/open-cli-collective/slack-chat-api/internal/keychain" + "github.com/open-cli-collective/slack-chat-api/internal/output" + "github.com/open-cli-collective/slack-chat-api/internal/testutil" +) + +// TestNoLeak_CredentialSurface is the §1.12 acceptance "no-leak" test for +// slck's credential-surface commands: load a known-distinctive token into +// the keyring, run every credential command class, capture stdout+stderr, +// and fail if the token (or any prefix of it) appears. The API commands +// never echo the token (it travels only in the Authorization header), so +// the leak surface is exactly these commands. +func TestNoLeak_CredentialSurface(t *testing.T) { + const secret = "xoxb-NOLEAK-canary-7f3a9c2e1d8b4a6f" + + run := func(name string, fn func() error) string { + t.Helper() + out, err := captureOutput(t, fn) + // JSON mode toggles a global; reset so it can't bleed. + t.Cleanup(func() { output.JSON = false }) + require.NoError(t, err, name) + if leak := credstore.NoLeakAssertion([]byte(out), secret); leak != nil { + t.Fatalf("%s leaked the secret: %v\noutput:\n%s", name, leak, out) + } + return out + } + + seed := func() { + st, err := keychain.Open() + require.NoError(t, err) + require.NoError(t, st.SetBotToken(secret)) + require.NoError(t, st.Close()) + } + + t.Run("show text", func(t *testing.T) { + testutil.Setup(t) + seed() + out := run("config show", func() error { return runShow(&showOptions{}) }) + require.Contains(t, out, "present") + }) + + t.Run("show json", func(t *testing.T) { + testutil.Setup(t) + seed() + output.JSON = true + run("config show -o json", func() error { return runShow(&showOptions{}) }) + }) + + t.Run("delete-token", func(t *testing.T) { + testutil.Setup(t) + seed() + run("config delete-token", func() error { + return runDeleteToken(&deleteTokenOptions{force: true, tokenType: "all"}) + }) + }) + + t.Run("clear", func(t *testing.T) { + testutil.Setup(t) + seed() + run("config clear", func() error { return runClear(&clearOptions{}) }) + }) + + t.Run("clear --all", func(t *testing.T) { + testutil.Setup(t) + seed() + run("config clear --all", func() error { return runClear(&clearOptions{all: true}) }) + }) +} diff --git a/internal/cmd/config/set_token.go b/internal/cmd/config/set_token.go index 21c699d..3c26e4d 100644 --- a/internal/cmd/config/set_token.go +++ b/internal/cmd/config/set_token.go @@ -1,95 +1,35 @@ package config import ( - "bufio" "fmt" - "os" - "strings" "github.com/spf13/cobra" - - "github.com/open-cli-collective/slack-chat-api/internal/keychain" - "github.com/open-cli-collective/slack-chat-api/internal/output" ) -type setTokenOptions struct{} - +// set-token is hard-deprecated (§2.4 / §1.5). It accepted a secret as a +// positional argument — the worst ingress form (no `=` to spot in shell +// history, no hint the arg is sensitive). It now accepts the value via NO +// path (flag, positional, or stdin) and exits nonzero pointing at the +// sanctioned ingress, `slck set-credential`. func newSetTokenCmd() *cobra.Command { - opts := &setTokenOptions{} - return &cobra.Command{ - Use: "set-token [token]", - Short: "Set a Slack API token", - Long: `Set a Slack API token for authentication. - -Token types are detected automatically: - - Bot tokens (xoxb-*): Used for channels, users, messages commands - - User tokens (xoxp-*): Used for search commands - -On macOS: Tokens are stored securely in the system Keychain. -On Linux: Tokens are stored in ~/.config/slack-chat-api/credentials (file permissions 0600). - -If no token is provided as an argument, you will be prompted to enter it.`, - Args: cobra.MaximumNArgs(1), + Use: "set-token", + Short: "Removed — use 'slck set-credential' (see message)", + Hidden: true, + // Accept and ignore any args so we can emit our own guidance + // rather than a generic cobra usage error. + Args: cobra.ArbitraryArgs, RunE: func(cmd *cobra.Command, args []string) error { - var token string - if len(args) > 0 { - token = args[0] - } - return runSetToken(token, opts) - }, - } -} - -func runSetToken(token string, opts *setTokenOptions) error { - // Warn Linux users about file-based storage - if !keychain.IsSecureStorage() { - output.Println("Warning: On Linux, your token will be stored in a config file") - output.Println(" (~/.config/slack-chat-api/credentials) with restricted permissions (0600).") - output.Println(" This is less secure than macOS Keychain storage.") - output.Println() - } - - if token == "" { - fmt.Print("Enter Slack API token: ") - reader := bufio.NewReader(os.Stdin) - input, err := reader.ReadString('\n') - if err != nil { - return fmt.Errorf("failed to read input: %w", err) - } - token = strings.TrimSpace(input) - } + return fmt.Errorf(`'slck config set-token' has been removed. - if token == "" { - return fmt.Errorf("token cannot be empty") - } +It passed a secret as a positional argument, which leaks via shell history +and process listings. Use 'slck set-credential' instead, which reads the +value from stdin or a named env var only: - // Detect token type and store appropriately - tokenType := keychain.DetectTokenType(token) + slck set-credential --ref slack-chat-api/default --key bot_token --stdin + op read 'op://Vault/Slack/bot_token' | slck set-credential --key bot_token --stdin - switch tokenType { - case "bot": - if err := keychain.SetAPIToken(token); err != nil { - return fmt.Errorf("failed to store bot token: %w", err) - } - if keychain.IsSecureStorage() { - output.Println("Bot token stored securely in Keychain") - } else { - output.Println("Bot token stored in ~/.config/slack-chat-api/credentials") - } - case "user": - if err := keychain.SetUserToken(token); err != nil { - return fmt.Errorf("failed to store user token: %w", err) - } - if keychain.IsSecureStorage() { - output.Println("User token stored securely in Keychain") - } else { - output.Println("User token stored in ~/.config/slack-chat-api/credentials") - } - output.Println("Note: This token will be used for search commands.") - default: - return fmt.Errorf("unrecognized token format (expected xoxb-* for bot or xoxp-* for user)") +For full guided setup: slck init --bot-token-from-env SLACK_BOT_TOKEN`) + }, } - - return nil } diff --git a/internal/cmd/config/show.go b/internal/cmd/config/show.go index 72a5cc1..336d7a0 100644 --- a/internal/cmd/config/show.go +++ b/internal/cmd/config/show.go @@ -1,10 +1,11 @@ package config import ( - "strings" - "github.com/spf13/cobra" + "github.com/open-cli-collective/cli-common/credstore" + + appconfig "github.com/open-cli-collective/slack-chat-api/internal/config" "github.com/open-cli-collective/slack-chat-api/internal/keychain" "github.com/open-cli-collective/slack-chat-api/internal/output" ) @@ -17,45 +18,76 @@ func newShowCmd() *cobra.Command { return &cobra.Command{ Use: "show", Short: "Show current configuration status", + Long: `Show credential configuration: backend, ref, and which keys are +present. Secret values are never displayed (§1.11).`, RunE: func(cmd *cobra.Command, args []string) error { return runShow(opts) }, } } -func maskToken(token string) string { - if len(token) < 12 { - return strings.Repeat("*", len(token)) - } - return token[:8] + strings.Repeat("*", len(token)-12) + token[len(token)-4:] +// showStatus is the non-secret view (§1.11 item 3): presence, backend, ref, +// workspace — never a token value, not even a masked prefix. +type showStatus struct { + Ref string `json:"credential_ref"` + Backend string `json:"backend"` + BackendSource string `json:"backend_source"` + PassphraseSource string `json:"passphrase_source,omitempty"` + Workspace string `json:"workspace,omitempty"` + BotToken bool `json:"bot_token_present"` + UserToken bool `json:"user_token_present"` } -func runShow(opts *showOptions) error { - hasAnyToken := false - - // Bot token - botToken, botErr := keychain.GetAPIToken() - if botErr == nil { - hasAnyToken = true - source := keychain.GetTokenSource() - output.Printf("Bot Token: %s (from %s)\n", maskToken(botToken), source) - } else { - output.Println("Bot Token: Not configured") +func runShow(_ *showOptions) error { + cfg, err := appconfig.Load() + if err != nil { + return err + } + st, err := keychain.Open() + if err != nil { + return err } + defer func() { _ = st.Close() }() - // User token - userToken, userErr := keychain.GetUserToken() - if userErr == nil { - hasAnyToken = true - source := keychain.GetUserTokenSource() - output.Printf("User Token: %s (from %s)\n", maskToken(userToken), source) - } else { - output.Println("User Token: Not configured (required for search)") + backend, src := st.Backend() + status := showStatus{ + Ref: st.Ref(), + Backend: string(backend), + BackendSource: string(src), + Workspace: cfg.Workspace, + BotToken: st.HasBotToken(), + UserToken: st.HasUserToken(), + } + if backend == credstore.BackendFile { + svc, _, _ := credstore.ParseRef(st.Ref()) + status.PassphraseSource = keychain.PassphraseSource(svc) } - if !hasAnyToken { - output.Println("\nRun 'slck config set-token' to configure") + if output.IsJSON() { + return output.PrintJSON(status) } + output.Printf("Credential ref: %s\n", status.Ref) + output.Printf("Backend: %s (%s)\n", status.Backend, status.BackendSource) + if status.PassphraseSource != "" { + output.Printf("Passphrase: %s\n", status.PassphraseSource) + } + if status.Workspace != "" { + output.Printf("Workspace: %s\n", status.Workspace) + } + output.Printf("bot_token: %s\n", presence(status.BotToken)) + output.Printf("user_token: %s\n", presence(status.UserToken)) + if !status.BotToken && !status.UserToken { + output.Println() + output.Println("No credentials configured. Run 'slck init' or") + output.Println("'slck set-credential --key bot_token --stdin'.") + } return nil } + +func presence(ok bool) string { + if ok { + return "present" + } + return "not configured" +} diff --git a/internal/cmd/config/test_test.go b/internal/cmd/config/test_test.go index 50324e4..7140707 100644 --- a/internal/cmd/config/test_test.go +++ b/internal/cmd/config/test_test.go @@ -10,7 +10,7 @@ import ( "github.com/stretchr/testify/require" "github.com/open-cli-collective/slack-chat-api/internal/client" - "github.com/open-cli-collective/slack-chat-api/internal/keychain" + "github.com/open-cli-collective/slack-chat-api/internal/testutil" ) func TestRunTest_Success(t *testing.T) { @@ -174,15 +174,8 @@ func TestRunTest_NetworkErrors(t *testing.T) { } func TestRunTest_NoTokenConfigured(t *testing.T) { - if keychain.IsSecureStorage() { - t.Skip("Skipping on macOS - keychain may have stored token") - } - - // Use temp dir with no token set - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - t.Setenv("SLACK_API_TOKEN", "") // Ensure bot env var is empty - t.Setenv("SLACK_USER_TOKEN", "") // Ensure user env var is empty + // Hermetic empty keyring (file backend, temp HOME) — no real keychain. + testutil.Setup(t) opts := &testOptions{} diff --git a/internal/cmd/initcmd/init.go b/internal/cmd/initcmd/init.go index 5baaf99..e51e3b0 100644 --- a/internal/cmd/initcmd/init.go +++ b/internal/cmd/initcmd/init.go @@ -10,42 +10,51 @@ import ( "github.com/spf13/cobra" "github.com/open-cli-collective/slack-chat-api/internal/client" + appconfig "github.com/open-cli-collective/slack-chat-api/internal/config" "github.com/open-cli-collective/slack-chat-api/internal/keychain" "github.com/open-cli-collective/slack-chat-api/internal/output" ) +// initOptions: tokens are NOT accepted as flag/positional values (§1.5/§1.12). +// They arrive only via a named env var, single-secret stdin, or an +// interactive prompt — all valid *ingress* mechanisms during setup. type initOptions struct { - botToken string - userToken string + botEnv string // --bot-token-from-env NAME + userEnv string // --user-token-from-env NAME + botStdin bool // --bot-token-stdin (single-secret stdin) noVerify bool - stdin io.Reader // For testing - newClient func(baseURL, token string) *client.Client // For testing + overwrite bool // --overwrite: resolve a §1.8 legacy/keyring conflict + + stdin io.Reader // test seam (prompts / --bot-token-stdin) + newClient func(baseURL, token string) *client.Client // test seam } -// NewCmd creates the init command +// NewCmd creates the init command. func NewCmd() *cobra.Command { opts := &initOptions{} cmd := &cobra.Command{ Use: "init", Short: "Interactive setup wizard", - Long: `Set up slck with guided configuration. + Long: `Set up slck. -This wizard walks you through configuring bot and user tokens -for Slack API access. Tokens are verified against the Slack API -unless --no-verify is passed. +Tokens are read from a named environment variable, single-secret stdin, or +an interactive prompt — never from a flag value (which would leak via shell +history / process listings, §1.5/§1.12). Examples: -For non-interactive use, provide tokens via flags: - slck init --bot-token xoxb-... --user-token xoxp-...`, + slck init --bot-token-from-env SLACK_BOT_TOKEN --user-token-from-env SLACK_USER_TOKEN + op read 'op://Vault/Slack/bot' | slck init --bot-token-stdin + slck init # interactive`, RunE: func(cmd *cobra.Command, args []string) error { return runInit(opts) }, } - cmd.Flags().StringVar(&opts.botToken, "bot-token", "", "Bot token (xoxb-*) for non-interactive setup") - cmd.Flags().StringVar(&opts.userToken, "user-token", "", "User token (xoxp-*) for non-interactive setup") + cmd.Flags().StringVar(&opts.botEnv, "bot-token-from-env", "", "Read the bot token from this env var") + cmd.Flags().StringVar(&opts.userEnv, "user-token-from-env", "", "Read the user token from this env var") + cmd.Flags().BoolVar(&opts.botStdin, "bot-token-stdin", false, "Read the bot token from stdin") cmd.Flags().BoolVar(&opts.noVerify, "no-verify", false, "Skip token verification") - + cmd.Flags().BoolVar(&opts.overwrite, "overwrite", false, "Resolve a legacy/keyring migration conflict by forcing the legacy value") return cmd } @@ -67,118 +76,142 @@ func runInit(opts *initOptions) error { output.Println("Slack CLI Setup") output.Println() - // Check for existing config - hasBotToken := keychain.HasStoredToken() - hasUserToken := keychain.HasStoredUserToken() - if hasBotToken || hasUserToken { - output.Println("Existing configuration detected.") - if !promptYesNo(opts.reader(), "Overwrite existing configuration?", false) { + // Opening the store runs the one-time legacy migration (§1.8). With + // --overwrite a legacy/keyring conflict is resolved by forcing legacy. + open := keychain.Open + if opts.overwrite { + open = keychain.OpenForMigrationOverwrite + } + st, err := open() + if err != nil { + return err + } + defer func() { _ = st.Close() }() + + if (st.HasBotToken() || st.HasUserToken()) && !opts.overwrite && opts.interactive() { + if !promptYesNo(opts.reader(), "Credentials already exist. Overwrite?", false) { output.Println("Setup cancelled.") return nil } - output.Println() } - output.Println("This CLI supports both bot tokens (xoxb-*) and user tokens (xoxp-*).") - output.Println("Bot tokens are recommended for most use cases.") - output.Println() - - // Bot token - botToken := opts.botToken - if botToken == "" { - var err error - botToken, err = promptToken(opts.reader(), "Bot Token (xoxb-...)") - if err != nil { - return err - } + botToken, err := opts.resolveBot() + if err != nil { + return err } + userToken, err := opts.resolveUser() + if err != nil { + return err + } + if botToken == "" && userToken == "" { + output.Println("No tokens provided. Setup cancelled.") + return nil + } + + workspace := "" if botToken != "" { - tokenType := keychain.DetectTokenType(botToken) - if tokenType != "bot" { - return fmt.Errorf("expected bot token (xoxb-*), got %s token", tokenType) + if t := keychain.DetectTokenType(botToken); t != "bot" { + return fmt.Errorf("expected bot token (xoxb-*), got %s token", t) } - if !opts.noVerify { - output.Println() - output.Println("Testing connection...") - c := opts.makeClient(botToken) - info, err := c.AuthTest() + info, err := opts.verify(botToken, "Bot") if err != nil { return fmt.Errorf("bot token verification failed: %w", err) } - output.Println(" Bot token valid") - output.Printf(" Connected to workspace: %s\n", info.Team) - output.Printf(" User: %s\n", info.User) - if info.BotID != "" { - output.Printf(" Bot ID: %s\n", info.BotID) - } + workspace = info.TeamID } - - if err := keychain.SetAPIToken(botToken); err != nil { - return fmt.Errorf("failed to store bot token: %w", err) + if err := st.SetBotToken(botToken); err != nil { + return err } - output.Println() output.Println("Bot token saved.") } - // User token - userToken := opts.userToken - if userToken == "" { - output.Println() - if promptYesNo(opts.reader(), "Would you like to add a user token as well? (needed for search)", false) { - var err error - userToken, err = promptToken(opts.reader(), "User Token (xoxp-...)") - if err != nil { - return err - } - } - } - if userToken != "" { - tokenType := keychain.DetectTokenType(userToken) - if tokenType != "user" { - return fmt.Errorf("expected user token (xoxp-*), got %s token", tokenType) + if t := keychain.DetectTokenType(userToken); t != "user" { + return fmt.Errorf("expected user token (xoxp-*), got %s token", t) } - if !opts.noVerify { - output.Println() - output.Println("Testing connection...") - c := opts.makeClient(userToken) - info, err := c.AuthTest() + info, err := opts.verify(userToken, "User") if err != nil { return fmt.Errorf("user token verification failed: %w", err) } - output.Println(" User token valid") - output.Printf(" Connected to workspace: %s\n", info.Team) - output.Printf(" User: %s\n", info.User) + if workspace == "" { + workspace = info.TeamID + } } - - if err := keychain.SetUserToken(userToken); err != nil { - return fmt.Errorf("failed to store user token: %w", err) + if err := st.SetUserToken(userToken); err != nil { + return err } - output.Println() output.Println("User token saved.") } - if botToken == "" && userToken == "" { - output.Println("No tokens provided. Setup cancelled.") - return nil + // Persist non-secret config (credential_ref + workspace, §1.2/§2.4). + cfg, err := appconfig.Load() + if err != nil { + return err + } + if workspace != "" { + cfg.Workspace = workspace + } + if err := cfg.Save(); err != nil { + return err } output.Println() output.Println("Configuration saved. Try it out:") if botToken != "" { output.Println(" slck channels list") - output.Println(" slck users list") } if userToken != "" { output.Println(" slck search messages \"hello\"") } - return nil } +// interactive reports whether init will prompt (no ingress flags given). +func (o *initOptions) interactive() bool { + return o.botEnv == "" && o.userEnv == "" && !o.botStdin +} + +func (o *initOptions) verify(token, label string) (*client.AuthTestResponse, error) { + output.Printf("Verifying %s token...\n", label) + info, err := o.makeClient(token).AuthTest() + if err != nil { + return nil, err + } + output.Printf(" Connected to workspace: %s\n", info.Team) + return info, nil +} + +func (o *initOptions) resolveBot() (string, error) { + switch { + case o.botEnv != "": + return os.Getenv(o.botEnv), nil + case o.botStdin: + b, err := io.ReadAll(o.reader()) + if err != nil { + return "", fmt.Errorf("read bot token from stdin: %w", err) + } + return strings.TrimRight(string(b), "\r\n"), nil + default: + return promptToken(o.reader(), "Bot Token (xoxb-...)") + } +} + +func (o *initOptions) resolveUser() (string, error) { + if o.userEnv != "" { + return os.Getenv(o.userEnv), nil + } + if !o.interactive() { + return "", nil // non-interactive run only sets what was supplied + } + if !promptYesNo(o.reader(), "Add a user token as well? (needed for search)", false) { + return "", nil + } + return promptToken(o.reader(), "User Token (xoxp-...)") +} + func promptToken(reader io.Reader, prompt string) (string, error) { fmt.Printf("%s: ", prompt) scanner := bufio.NewScanner(reader) diff --git a/internal/cmd/initcmd/init_test.go b/internal/cmd/initcmd/init_test.go index 0045b2d..dcd914b 100644 --- a/internal/cmd/initcmd/init_test.go +++ b/internal/cmd/initcmd/init_test.go @@ -12,175 +12,123 @@ import ( "github.com/open-cli-collective/slack-chat-api/internal/client" "github.com/open-cli-collective/slack-chat-api/internal/keychain" + "github.com/open-cli-collective/slack-chat-api/internal/testutil" ) func newMockServer(t *testing.T) *httptest.Server { t.Helper() return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - resp := map[string]interface{}{ - "ok": true, - "team": "Test Workspace", - "user": "testbot", - "team_id": "T123", - "user_id": "U123", - "bot_id": "B123", - } w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(resp) + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, "team": "Test Workspace", "user": "testbot", + "team_id": "T123", "user_id": "U123", "bot_id": "B123", + }) })) } -func TestRunInit_NonInteractive_BotOnly(t *testing.T) { - if keychain.IsSecureStorage() { - t.Skip("Skipping on macOS - keychain can't be easily mocked") - } - - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - server := newMockServer(t) - defer server.Close() - - opts := &initOptions{ - botToken: "xoxb-test-token-12345678", - newClient: func(_, token string) *client.Client { - return client.NewWithConfig(server.URL, token, nil) - }, - } +func mockClient(s *httptest.Server) func(string, string) *client.Client { + return func(_, token string) *client.Client { return client.NewWithConfig(s.URL, token, nil) } +} - err := runInit(opts) +func hasBot(t *testing.T) bool { + t.Helper() + st, err := keychain.Open() require.NoError(t, err) - - // Verify token was stored - assert.True(t, keychain.HasStoredToken()) + defer func() { _ = st.Close() }() + return st.HasBotToken() } -func TestRunInit_NonInteractive_BothTokens(t *testing.T) { - if keychain.IsSecureStorage() { - t.Skip("Skipping on macOS - keychain can't be easily mocked") - } - - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) +func TestRunInit_FromEnv_BotOnly(t *testing.T) { + testutil.Setup(t) + s := newMockServer(t) + defer s.Close() + t.Setenv("BOT_TOK", "xoxb-test-token-12345678") - server := newMockServer(t) - defer server.Close() + err := runInit(&initOptions{botEnv: "BOT_TOK", newClient: mockClient(s)}) + require.NoError(t, err) + assert.True(t, hasBot(t)) +} - opts := &initOptions{ - botToken: "xoxb-test-token-12345678", - userToken: "xoxp-test-token-12345678", - newClient: func(_, token string) *client.Client { - return client.NewWithConfig(server.URL, token, nil) - }, - } +func TestRunInit_FromEnv_BothTokens(t *testing.T) { + testutil.Setup(t) + s := newMockServer(t) + defer s.Close() + t.Setenv("BOT_TOK", "xoxb-test-token-12345678") + t.Setenv("USR_TOK", "xoxp-test-token-12345678") - err := runInit(opts) + err := runInit(&initOptions{botEnv: "BOT_TOK", userEnv: "USR_TOK", newClient: mockClient(s)}) require.NoError(t, err) - assert.True(t, keychain.HasStoredToken()) - assert.True(t, keychain.HasStoredUserToken()) + st, err := keychain.Open() + require.NoError(t, err) + defer func() { _ = st.Close() }() + assert.True(t, st.HasBotToken()) + assert.True(t, st.HasUserToken()) } -func TestRunInit_NonInteractive_NoVerify(t *testing.T) { - if keychain.IsSecureStorage() { - t.Skip("Skipping on macOS - keychain can't be easily mocked") - } - - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - opts := &initOptions{ - botToken: "xoxb-test-token-12345678", +func TestRunInit_Stdin_NoVerify(t *testing.T) { + testutil.Setup(t) + err := runInit(&initOptions{ + botStdin: true, + stdin: strings.NewReader("xoxb-test-token-12345678\n"), noVerify: true, - } - - err := runInit(opts) + }) require.NoError(t, err) - - assert.True(t, keychain.HasStoredToken()) + assert.True(t, hasBot(t)) } +// Fixes the B0-deferred non-hermetic flake: testutil.Setup forces the file +// backend in a temp HOME, so this no longer races the real macOS keychain. func TestRunInit_WrongTokenType(t *testing.T) { - opts := &initOptions{ - botToken: "xoxp-this-is-a-user-token", + testutil.Setup(t) + err := runInit(&initOptions{ + botStdin: true, + stdin: strings.NewReader("xoxp-this-is-a-user-token\n"), noVerify: true, - } - - err := runInit(opts) - assert.Error(t, err) + }) + require.Error(t, err) assert.Contains(t, err.Error(), "expected bot token") } func TestRunInit_WrongUserTokenType(t *testing.T) { - if keychain.IsSecureStorage() { - t.Skip("Skipping on macOS - keychain can't be easily mocked") - } - - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - opts := &initOptions{ - botToken: "xoxb-test-token-12345678", - userToken: "xoxb-this-is-a-bot-token", - noVerify: true, - } - - err := runInit(opts) - assert.Error(t, err) + testutil.Setup(t) + t.Setenv("BOT_TOK", "xoxb-test-token-12345678") + t.Setenv("USR_TOK", "xoxb-this-is-a-bot-token") + err := runInit(&initOptions{botEnv: "BOT_TOK", userEnv: "USR_TOK", noVerify: true}) + require.Error(t, err) assert.Contains(t, err.Error(), "expected user token") } func TestRunInit_Interactive_NoTokensProvided(t *testing.T) { - // Simulate pressing enter (empty) for bot token, then "n" for user token prompt - opts := &initOptions{ - stdin: strings.NewReader("\nn\n"), - noVerify: true, - } - - err := runInit(opts) + testutil.Setup(t) + // empty bot prompt, then "n" to the add-user prompt. + err := runInit(&initOptions{stdin: strings.NewReader("\nn\n"), noVerify: true}) require.NoError(t, err) + assert.False(t, hasBot(t)) } func TestRunInit_Interactive_CancelOverwrite(t *testing.T) { - if keychain.IsSecureStorage() { - t.Skip("Skipping on macOS - keychain can't be easily mocked") - } - - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - - // Set an existing token first - require.NoError(t, keychain.SetAPIToken("xoxb-existing-token")) - - // Simulate "n" for overwrite prompt - opts := &initOptions{ - stdin: strings.NewReader("n\n"), - noVerify: true, - } + testutil.Setup(t) + st, err := keychain.Open() + require.NoError(t, err) + require.NoError(t, st.SetBotToken("xoxb-existing-token")) + _ = st.Close() - err := runInit(opts) + // Existing creds + interactive + "n" to overwrite → cancelled. + err = runInit(&initOptions{stdin: strings.NewReader("n\n"), noVerify: true}) require.NoError(t, err) } func TestRunInit_VerificationFailed(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - resp := map[string]interface{}{ - "ok": false, - "error": "invalid_auth", - } + testutil.Setup(t) + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(resp) + _ = json.NewEncoder(w).Encode(map[string]interface{}{"ok": false, "error": "invalid_auth"}) })) - defer server.Close() - - opts := &initOptions{ - botToken: "xoxb-bad-token-12345678", - newClient: func(_, token string) *client.Client { - return client.NewWithConfig(server.URL, token, nil) - }, - } + defer s.Close() + t.Setenv("BOT_TOK", "xoxb-bad-token-12345678") - err := runInit(opts) - assert.Error(t, err) + err := runInit(&initOptions{botEnv: "BOT_TOK", newClient: mockClient(s)}) + require.Error(t, err) assert.Contains(t, err.Error(), "verification failed") } diff --git a/internal/cmd/root/root.go b/internal/cmd/root/root.go index 7514838..4b182e4 100644 --- a/internal/cmd/root/root.go +++ b/internal/cmd/root/root.go @@ -15,6 +15,7 @@ import ( "github.com/open-cli-collective/slack-chat-api/internal/cmd/initcmd" "github.com/open-cli-collective/slack-chat-api/internal/cmd/messages" "github.com/open-cli-collective/slack-chat-api/internal/cmd/search" + "github.com/open-cli-collective/slack-chat-api/internal/cmd/setcred" "github.com/open-cli-collective/slack-chat-api/internal/cmd/users" "github.com/open-cli-collective/slack-chat-api/internal/cmd/whoami" "github.com/open-cli-collective/slack-chat-api/internal/cmd/workspace" @@ -34,10 +35,12 @@ var rootCmd = &cobra.Command{ It provides commands for managing channels, users, messages, and other Slack workspace operations. -Configure your API token with: - slck config set-token +Configure credentials with: + slck init + slck set-credential --key bot_token --stdin (e.g. via 'op read | ...') -Or set the SLACK_API_TOKEN environment variable.`, +Credentials are stored in the OS keyring; environment variables are not +read at runtime (they are accepted only as ingress during setup).`, Version: version.Version, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { // Parse and validate output format @@ -91,4 +94,5 @@ func init() { rootCmd.AddCommand(emoji.NewCmd()) rootCmd.AddCommand(files.NewCmd()) rootCmd.AddCommand(initcmd.NewCmd()) + rootCmd.AddCommand(setcred.NewCmd()) } diff --git a/internal/cmd/setcred/setcred.go b/internal/cmd/setcred/setcred.go new file mode 100644 index 0000000..81890ec --- /dev/null +++ b/internal/cmd/setcred/setcred.go @@ -0,0 +1,104 @@ +// Package setcred implements `slck set-credential` — the low-level, +// single-secret, scriptable credential ingress (§1.5.2). It accepts the +// value only via stdin or a named env var, never as a flag/positional +// value, and only for allowed keys. +package setcred + +import ( + "fmt" + "io" + "os" + "strings" + + "github.com/spf13/cobra" + + "github.com/open-cli-collective/slack-chat-api/internal/keychain" + "github.com/open-cli-collective/slack-chat-api/internal/output" +) + +type options struct { + ref string + key string + stdin bool + fromEnv string + in io.Reader // test seam +} + +// NewCmd builds the `slck set-credential` command. +func NewCmd() *cobra.Command { + opts := &options{} + cmd := &cobra.Command{ + Use: "set-credential --key (--stdin | --from-env NAME)", + Short: "Store one credential in the keyring (stdin or env ingress)", + Long: `Store a single secret in the OS keyring (§1.5.2). + +The value is read ONLY from stdin (--stdin) or a named environment variable +(--from-env NAME) — never from a flag or positional argument. Only the +keys 'bot_token' and 'user_token' are accepted. + + op read 'op://Vault/Slack/bot_token' | slck set-credential --key bot_token --stdin + slck set-credential --key user_token --from-env SLACK_USER_TOKEN`, + RunE: func(cmd *cobra.Command, args []string) error { + return run(opts) + }, + } + cmd.Flags().StringVar(&opts.ref, "ref", "", "Credential ref (default: config.yml credential_ref)") + cmd.Flags().StringVar(&opts.key, "key", "", "Key to set: bot_token or user_token") + cmd.Flags().BoolVar(&opts.stdin, "stdin", false, "Read the secret value from stdin") + cmd.Flags().StringVar(&opts.fromEnv, "from-env", "", "Read the secret value from this env var") + return cmd +} + +func run(opts *options) error { + if opts.key == "" { + return fmt.Errorf("--key is required (bot_token or user_token)") + } + if opts.stdin == (opts.fromEnv != "") { + return fmt.Errorf("exactly one of --stdin or --from-env is required") + } + + value, err := readValue(opts) + if err != nil { + return err + } + if value == "" { + return fmt.Errorf("empty secret value rejected") + } + + st, err := keychain.OpenRef(opts.ref) + if err != nil { + return err + } + defer func() { _ = st.Close() }() + + switch opts.key { + case keychain.KeyBotToken: + err = st.SetBotToken(value) + case keychain.KeyUserToken: + err = st.SetUserToken(value) + default: + return fmt.Errorf("key %q not allowed (allowed: %s, %s)", + opts.key, keychain.KeyBotToken, keychain.KeyUserToken) + } + if err != nil { + return err + } + // Never echo the value (§1.12); naming the key/ref is fine. + output.Printf("Stored %s in %s\n", opts.key, st.Ref()) + return nil +} + +func readValue(opts *options) (string, error) { + if opts.fromEnv != "" { + return os.Getenv(opts.fromEnv), nil + } + r := opts.in + if r == nil { + r = os.Stdin + } + b, err := io.ReadAll(r) + if err != nil { + return "", fmt.Errorf("read secret from stdin: %w", err) + } + return strings.TrimRight(string(b), "\r\n"), nil +} diff --git a/internal/cmd/setcred/setcred_test.go b/internal/cmd/setcred/setcred_test.go new file mode 100644 index 0000000..36bd806 --- /dev/null +++ b/internal/cmd/setcred/setcred_test.go @@ -0,0 +1,87 @@ +package setcred + +import ( + "bytes" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/open-cli-collective/cli-common/credstore" + + "github.com/open-cli-collective/slack-chat-api/internal/keychain" + "github.com/open-cli-collective/slack-chat-api/internal/output" + "github.com/open-cli-collective/slack-chat-api/internal/testutil" +) + +func capture(t *testing.T, fn func() error) (string, error) { + t.Helper() + var buf bytes.Buffer + orig := output.Writer + output.Writer = &buf + t.Cleanup(func() { output.Writer = orig }) + return buf.String(), fn() +} + +func TestSetCredential_Stdin(t *testing.T) { + testutil.Setup(t) + const secret = "xoxb-SETCRED-stdin-canary-12345" + out, err := capture(t, func() error { + return run(&options{key: keychain.KeyBotToken, stdin: true, + in: strings.NewReader(secret + "\n")}) + }) + require.NoError(t, err) + if leak := credstore.NoLeakAssertion([]byte(out), secret); leak != nil { + t.Fatalf("set-credential echoed the value: %v", leak) + } + st, err := keychain.Open() + require.NoError(t, err) + defer func() { _ = st.Close() }() + got, err := st.BotToken() + require.NoError(t, err) + require.Equal(t, secret, got) +} + +func TestSetCredential_FromEnv(t *testing.T) { + testutil.Setup(t) + t.Setenv("MY_SLACK_SECRET", "xoxp-fromenv-value") + _, err := capture(t, func() error { + return run(&options{key: keychain.KeyUserToken, fromEnv: "MY_SLACK_SECRET"}) + }) + require.NoError(t, err) + st, _ := keychain.Open() + defer func() { _ = st.Close() }() + v, err := st.UserToken() + require.NoError(t, err) + require.Equal(t, "xoxp-fromenv-value", v) +} + +func TestSetCredential_RejectsDisallowedKey(t *testing.T) { + testutil.Setup(t) + _, err := capture(t, func() error { + return run(&options{key: "totally_bogus", stdin: true, in: strings.NewReader("x")}) + }) + require.Error(t, err) + require.Contains(t, err.Error(), "not allowed") +} + +func TestSetCredential_RequiresExactlyOneSource(t *testing.T) { + testutil.Setup(t) + // neither + _, err := capture(t, func() error { return run(&options{key: keychain.KeyBotToken}) }) + require.Error(t, err) + // both + _, err = capture(t, func() error { + return run(&options{key: keychain.KeyBotToken, stdin: true, fromEnv: "X"}) + }) + require.Error(t, err) +} + +func TestSetCredential_EmptyRejected(t *testing.T) { + testutil.Setup(t) + _, err := capture(t, func() error { + return run(&options{key: keychain.KeyBotToken, stdin: true, in: strings.NewReader("\n")}) + }) + require.Error(t, err) + require.Contains(t, err.Error(), "empty") +} diff --git a/internal/cmd/whoami/whoami_test.go b/internal/cmd/whoami/whoami_test.go index 6d2afd2..ffe700b 100644 --- a/internal/cmd/whoami/whoami_test.go +++ b/internal/cmd/whoami/whoami_test.go @@ -10,7 +10,7 @@ import ( "github.com/stretchr/testify/require" "github.com/open-cli-collective/slack-chat-api/internal/client" - "github.com/open-cli-collective/slack-chat-api/internal/keychain" + "github.com/open-cli-collective/slack-chat-api/internal/testutil" ) func TestRunWhoami_BotTokenOnly(t *testing.T) { @@ -93,15 +93,8 @@ func TestRunWhoami_BothTokens(t *testing.T) { } func TestRunWhoami_NoTokens(t *testing.T) { - if keychain.IsSecureStorage() { - t.Skip("Skipping on macOS - keychain may have stored token") - } - - // Use temp dir with no token set - tempDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tempDir) - t.Setenv("SLACK_API_TOKEN", "") - t.Setenv("SLACK_USER_TOKEN", "") + // Hermetic empty keyring (file backend, temp HOME) — no real keychain. + testutil.Setup(t) opts := &whoamiOptions{} diff --git a/internal/config/config.go b/internal/config/config.go new file mode 100644 index 0000000..0c4ff36 --- /dev/null +++ b/internal/config/config.go @@ -0,0 +1,103 @@ +// Package config holds slck's non-secret on-disk configuration per the Open +// CLI Collective Secret-Handling Standard §1.2 / §2.4. No access secret is +// ever written here — secrets live only in the OS keyring via cli-common's +// credstore. This file owns config.yml: the authoritative credential_ref +// (§1.3), the non-secret workspace identifier, and the optional §1.4 +// file-backend opt-in. +package config + +import ( + "fmt" + "os" + "path/filepath" + + "gopkg.in/yaml.v3" +) + +const ( + // DefaultCredentialRef applies when config.yml is absent or omits + // credential_ref. It is still parsed through credstore.ParseRef by + // callers — never assumed structurally (§1.3). + DefaultCredentialRef = "slack-chat-api/default" + + appDirName = "slack-chat-api" + configFileName = "config.yml" +) + +// Config is slck's config.yml. Everything here is safe for an org to commit +// to a private/MDM-controlled store (§1.2); none of it is an access secret. +type Config struct { + // CredentialRef is the authoritative / keyring ref + // (§1.3). Callers resolve it via credstore.ParseRef; the service and + // profile are never hard-coded from a convention. + CredentialRef string `yaml:"credential_ref"` + // Workspace is the non-secret Slack workspace identifier captured at + // init (verification by `slck me`, shown in `config show`, assertable + // by org deployment scripts). Not needed for API calls. + Workspace string `yaml:"workspace,omitempty"` + // Keyring carries the optional §1.4 explicit file-backend opt-in. + Keyring KeyringConfig `yaml:"keyring,omitempty"` +} + +// KeyringConfig is the §1.4 backend selector. Backend == "file" forces the +// encrypted-file backend unconditionally (the supported path for users who +// genuinely prefer it / headless Linux); empty means OS default selection. +type KeyringConfig struct { + Backend string `yaml:"backend,omitempty"` +} + +// Dir is the cross-platform config directory: $XDG_CONFIG_HOME/slack-chat-api +// else ~/.config/slack-chat-api. Identical on Linux, macOS, and Windows — +// this matches the released layout (the legacy code has no %APPDATA% branch), +// so config.yml sits beside the legacy credentials file it supersedes. +func Dir() string { + if xdg := os.Getenv("XDG_CONFIG_HOME"); xdg != "" { + return filepath.Join(xdg, appDirName) + } + home, _ := os.UserHomeDir() + return filepath.Join(home, ".config", appDirName) +} + +// Path is the config.yml location. +func Path() string { return filepath.Join(Dir(), configFileName) } + +// Load reads config.yml. An absent file is not an error: defaults are applied +// (CredentialRef = DefaultCredentialRef) and a usable Config is returned. +func Load() (*Config, error) { + c := &Config{} + data, err := os.ReadFile(Path()) + if err != nil { + if os.IsNotExist(err) { + c.applyDefaults() + return c, nil + } + return nil, fmt.Errorf("read config %s: %w", Path(), err) + } + if err := yaml.Unmarshal(data, c); err != nil { + return nil, fmt.Errorf("parse config %s: %w", Path(), err) + } + c.applyDefaults() + return c, nil +} + +func (c *Config) applyDefaults() { + if c.CredentialRef == "" { + c.CredentialRef = DefaultCredentialRef + } +} + +// Save writes config.yml at 0600 under a 0700 directory. Non-secret, but +// there is no reason for it to be world-readable. +func (c *Config) Save() error { + if err := os.MkdirAll(Dir(), 0o700); err != nil { + return fmt.Errorf("create config dir: %w", err) + } + data, err := yaml.Marshal(c) + if err != nil { + return fmt.Errorf("marshal config: %w", err) + } + if err := os.WriteFile(Path(), data, 0o600); err != nil { + return fmt.Errorf("write config %s: %w", Path(), err) + } + return nil +} diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index 2b54b16..1a72eca 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -1,273 +1,218 @@ +// Package keychain is slck's credential adapter. Despite the historical +// package name, it no longer shells out to macOS `security` or writes a +// plaintext file: it is a thin wrapper over cli-common's credstore, which +// owns OS-keyring storage, §1.4 backend selection (incl. Linux fail-closed +// and the encrypted-file fallback), and the §1.5.2 allowed-key allowlist. +// The name is retained only to avoid churning every importer during the +// Phase B pilot (Open CLI Collective Secret-Handling Standard §2.4). +// +// All runtime credential resolution goes through here and reads the OS +// keyring only — never an environment variable, never a config field +// (§1.11 acceptance item 2). Environment variables carry secret material +// into slck solely as *ingress* during `init` / `set-credential`. package keychain import ( + "errors" "fmt" - "os" - "os/exec" - "path/filepath" - "runtime" "strings" + + "github.com/open-cli-collective/cli-common/credstore" + + "github.com/open-cli-collective/slack-chat-api/internal/config" ) +// Bundle key names (§2.4). Note bot_token, not the legacy api_token — the +// rename is performed by the one-time migration (migrate.go). const ( - serviceName = "slack-chat-api" - apiTokenKey = "api_token" - userTokenKey = "user_token" + KeyBotToken = "bot_token" + KeyUserToken = "user_token" ) -// GetAPIToken retrieves the Slack API token from environment or keychain/config -func GetAPIToken() (string, error) { - // Check environment variable first (allows override for automation) - if token := os.Getenv("SLACK_API_TOKEN"); token != "" { - return token, nil - } - - // Fallback to secure storage (keychain on macOS, config file on Linux) - token, err := getCredential(apiTokenKey) - if err == nil && token != "" { - return token, nil +// allowedKeys is slck's §1.5.2 allowlist: exactly the two bundle keys. +var allowedKeys = []string{KeyBotToken, KeyUserToken} + +// Store is an open handle to slck's credential bundle. Construct with Open, +// always Close. It carries the resolved ref so callers can report it in +// `config show` / errors without re-deriving it (§1.12: ref is not secret). +type Store struct { + cs *credstore.Store + service string + profile string + ref string +} + +// Open resolves the authoritative credential_ref from config.yml (§1.3 — +// the service/profile are parsed, never assumed), opens the backing +// credstore, and runs the one-time legacy migration (§1.8) before returning. +// The returned Store reads/writes the OS keyring only. A legacy-vs-keyring +// conflict surfaces here as a §1.8 error; `slck init --overwrite` calls +// OpenForMigrationOverwrite to force the legacy value instead. +func Open() (*Store, error) { return open(false) } + +// OpenForMigrationOverwrite is Open with the §1.8 `--overwrite` resolution: +// a legacy value is forced over an existing keyring entry. It still cannot +// resolve a legacy-vs-legacy disagreement (the user must pick). +func OpenForMigrationOverwrite() (*Store, error) { return open(true) } + +func open(overwrite bool) (*Store, error) { + cfg, err := config.Load() + if err != nil { + return nil, err } - - return "", fmt.Errorf("no API token found - run 'slck config set-token' or set SLACK_API_TOKEN") -} - -// SetAPIToken stores the Slack API token -func SetAPIToken(token string) error { - return setCredential(apiTokenKey, token) + return openWith(cfg, overwrite) } -// DeleteAPIToken removes the Slack API token -func DeleteAPIToken() error { - return deleteCredential(apiTokenKey) -} - -// IsSecureStorage returns true if using secure storage (macOS Keychain) -func IsSecureStorage() bool { - return runtime.GOOS == "darwin" -} - -// HasStoredToken returns true if a token is stored in keychain/config (not env var) -func HasStoredToken() bool { - token, err := getCredential(apiTokenKey) - return err == nil && token != "" -} - -// GetTokenSource returns where the current token is stored -func GetTokenSource() string { - if os.Getenv("SLACK_API_TOKEN") != "" { - return "environment variable" +// OpenRef opens a store against an explicit ref instead of config.yml's +// credential_ref — used by `slck set-credential --ref` (§1.5.2 ingress). +// An empty ref falls back to the configured/default ref. +func OpenRef(ref string) (*Store, error) { + cfg, err := config.Load() + if err != nil { + return nil, err } - if token, err := getCredential(apiTokenKey); err == nil && token != "" { - if runtime.GOOS == "darwin" { - return "Keychain" - } - return "config file" + if ref != "" { + cfg.CredentialRef = ref } - return "" + return openWith(cfg, false) } -// --- User Token (for search) --- - -// GetUserToken retrieves the user token from keychain/config or environment -func GetUserToken() (string, error) { - // Check environment variable first - if token := os.Getenv("SLACK_USER_TOKEN"); token != "" { - return token, nil +// openWith is the seam unit tests drive with an injected config (e.g. a +// memory-backend opt-in via Keyring.Backend) so they never touch a real +// keyring (§1.12 test obligation, and hermeticity). +func openWith(cfg *config.Config, overwrite bool) (*Store, error) { + service, profile, err := credstore.ParseRef(cfg.CredentialRef) + if err != nil { + return nil, fmt.Errorf("invalid credential_ref %q: %w", cfg.CredentialRef, err) } - // Try secure storage (keychain on macOS, config file on Linux) - token, err := getCredential(userTokenKey) - if err == nil && token != "" { - return token, nil + opts := &credstore.Options{AllowedKeys: allowedKeys} + if b, ok := configBackend(cfg.Keyring.Backend); ok { + opts.ConfigBackend = b } + opts.FilePassphrase = passphraseFunc(service) - return "", fmt.Errorf("no user token found - run 'slck config set-token ' or set SLACK_USER_TOKEN") -} - -// SetUserToken stores a user token -func SetUserToken(token string) error { - return setCredential(userTokenKey, token) -} - -// DeleteUserToken removes the stored user token -func DeleteUserToken() error { - return deleteCredential(userTokenKey) -} + cs, err := credstore.Open(service, opts) + if err != nil { + return nil, err + } -// HasStoredUserToken returns true if a user token is stored in keychain/config (not env var) -func HasStoredUserToken() bool { - token, err := getCredential(userTokenKey) - return err == nil && token != "" -} + s := &Store{cs: cs, service: service, profile: profile, ref: cfg.CredentialRef} -// GetUserTokenSource returns where the user token comes from -func GetUserTokenSource() string { - if token, err := getCredential(userTokenKey); err == nil && token != "" { - if runtime.GOOS == "darwin" { - return "Keychain" - } - return "config file" + if err := migrateLegacyOverwrite(s, cfg, overwrite); err != nil { + _ = cs.Close() + return nil, err } - if os.Getenv("SLACK_USER_TOKEN") != "" { - return "environment variable" - } - return "" + return s, nil } -// DetectTokenType returns "bot" for xoxb-*, "user" for xoxp-*, or "unknown" -func DetectTokenType(token string) string { - if strings.HasPrefix(token, "xoxb-") { - return "bot" - } - if strings.HasPrefix(token, "xoxp-") { - return "user" +// configBackend maps the config.yml keyring.backend value to a credstore +// Backend. Only the §1.4 "file" opt-in is honored; anything else is left to +// credstore's fail-closed default selection (ok=false). Empty is the +// common case (auto-select). +func configBackend(v string) (credstore.Backend, bool) { + if strings.TrimSpace(v) == "file" { + return credstore.BackendFile, true } - return "unknown" + return "", false } -// --- Platform-specific implementations --- - -func getCredential(key string) (string, error) { - if runtime.GOOS == "darwin" { - return getFromKeychain(key) +// Close releases the backing store. Safe on a nil receiver. +func (s *Store) Close() error { + if s == nil || s.cs == nil { + return nil } - return getFromConfigFile(key) + return s.cs.Close() } -func setCredential(key, value string) error { - if runtime.GOOS == "darwin" { - return setInKeychain(key, value) - } - return setInConfigFile(key, value) -} +// Ref returns the resolved credential ref (non-secret; safe to display). +func (s *Store) Ref() string { return s.ref } -func deleteCredential(key string) error { - if runtime.GOOS == "darwin" { - return deleteFromKeychain(key) - } - return deleteFromConfigFile(key) -} +// Backend reports the credstore backend and how it was selected, for +// `config show` (§1.11 item 7). Neither value is secret. +func (s *Store) Backend() (credstore.Backend, credstore.Source) { return s.cs.Backend() } -// --- macOS Keychain --- +// BotToken returns the bot token from the keyring. ErrMissingBotToken (an +// errors.Is-matchable wrapper of credstore.ErrNotFound) when unset. +func (s *Store) BotToken() (string, error) { return s.get(KeyBotToken, ErrMissingBotToken) } -func getFromKeychain(account string) (string, error) { - cmd := exec.Command("security", "find-generic-password", - "-s", serviceName, - "-a", account, - "-w") +// UserToken returns the user token from the keyring. +func (s *Store) UserToken() (string, error) { return s.get(KeyUserToken, ErrMissingUserToken) } - output, err := cmd.Output() +func (s *Store) get(key string, missing error) (string, error) { + v, err := s.cs.Get(s.profile, key) + if errors.Is(err, credstore.ErrNotFound) || (err == nil && v == "") { + return "", missing + } if err != nil { - return "", err + // Never embed the value; naming ref/key/op is allowed (§1.12). + return "", fmt.Errorf("read %s from %s: %w", key, s.ref, err) } - - return strings.TrimSpace(string(output)), nil -} - -func setInKeychain(account, value string) error { - // First try to delete any existing item (ignore errors) - _ = deleteFromKeychain(account) - - cmd := exec.Command("security", "add-generic-password", - "-s", serviceName, - "-a", account, - "-w", value, - "-U") // Update if exists - - return cmd.Run() + return v, nil } -func deleteFromKeychain(account string) error { - cmd := exec.Command("security", "delete-generic-password", - "-s", serviceName, - "-a", account) +// SetBotToken / SetUserToken are ingress-only writes (called from init / +// set-credential after the value arrived via stdin/env per §1.5). +func (s *Store) SetBotToken(v string) error { return s.set(KeyBotToken, v) } +func (s *Store) SetUserToken(v string) error { return s.set(KeyUserToken, v) } - return cmd.Run() -} - -// --- Config File (Linux fallback) --- - -func getConfigDir() string { - if xdgConfig := os.Getenv("XDG_CONFIG_HOME"); xdgConfig != "" { - return filepath.Join(xdgConfig, "slack-chat-api") +func (s *Store) set(key, v string) error { + if err := s.cs.Set(s.profile, key, v, credstore.WithOverwrite()); err != nil { + return fmt.Errorf("store %s at %s: %w", key, s.ref, err) } - home, _ := os.UserHomeDir() - return filepath.Join(home, ".config", "slack-chat-api") + return nil } -func getConfigFilePath() string { - return filepath.Join(getConfigDir(), "credentials") -} +// DeleteBotToken / DeleteUserToken remove a single key (idempotent: a +// missing key is not an error — §1.7). +func (s *Store) DeleteBotToken() error { return s.del(KeyBotToken) } +func (s *Store) DeleteUserToken() error { return s.del(KeyUserToken) } -func getFromConfigFile(key string) (string, error) { - data, err := os.ReadFile(getConfigFilePath()) - if err != nil { - return "", err +func (s *Store) del(key string) error { + // Idempotent by construction (§1.7): an absent key is success. The + // Exists pre-check is backend-agnostic — credstore's file backend + // surfaces a raw os "not found" rather than ErrNotFound on Delete. + if ok, _ := s.cs.Exists(s.profile, key); !ok { + return nil } - - lines := strings.Split(string(data), "\n") - for _, line := range lines { - parts := strings.SplitN(line, "=", 2) - if len(parts) == 2 && parts[0] == key { - return parts[1], nil - } + if err := s.cs.Delete(s.profile, key); err != nil && !errors.Is(err, credstore.ErrNotFound) { + return fmt.Errorf("delete %s at %s: %w", key, s.ref, err) } - - return "", fmt.Errorf("key not found") + return nil } -func setInConfigFile(key, value string) error { - configDir := getConfigDir() - if err := os.MkdirAll(configDir, 0700); err != nil { - return err - } - - configPath := getConfigFilePath() - - // Read existing config - existing := make(map[string]string) - if data, err := os.ReadFile(configPath); err == nil { - lines := strings.Split(string(data), "\n") - for _, line := range lines { - parts := strings.SplitN(line, "=", 2) - if len(parts) == 2 { - existing[parts[0]] = parts[1] - } - } - } - - // Update value - existing[key] = value +// HasBotToken / HasUserToken report presence without returning the value +// (used by `config show` / `init` overwrite prompts — §1.11 item 3). +func (s *Store) HasBotToken() bool { return s.has(KeyBotToken) } +func (s *Store) HasUserToken() bool { return s.has(KeyUserToken) } - // Write back - var lines []string - for k, v := range existing { - lines = append(lines, fmt.Sprintf("%s=%s", k, v)) - } - - return os.WriteFile(configPath, []byte(strings.Join(lines, "\n")+"\n"), 0600) +func (s *Store) has(key string) bool { + ok, err := s.cs.Exists(s.profile, key) + return err == nil && ok } -func deleteFromConfigFile(key string) error { - configPath := getConfigFilePath() - - data, err := os.ReadFile(configPath) - if err != nil { - return err - } +// Clear removes the whole bundle under the active profile (config clear, +// §1.7). Idempotent; scope is the active profile only. +func (s *Store) Clear() ([]string, error) { + return s.cs.DeleteBundle(s.profile) +} - var newLines []string - lines := strings.Split(string(data), "\n") - for _, line := range lines { - parts := strings.SplitN(line, "=", 2) - if len(parts) == 2 && parts[0] != key { - newLines = append(newLines, line) - } - } +// Sentinel "missing" errors. errors.Is(err, ErrMissingBotToken) lets the +// CLI print an actionable setup hint without leaking anything. +var ( + ErrMissingBotToken = errors.New("slck: no bot token in keyring — run `slck init` or `slck set-credential --key bot_token --stdin`") + ErrMissingUserToken = errors.New("slck: no user token in keyring — run `slck init` or `slck set-credential --key user_token --stdin`") +) - if len(newLines) == 0 { - return os.Remove(configPath) +// DetectTokenType returns "bot" for xoxb-*, "user" for xoxp-*, else +// "unknown". Pure; used by init to validate the slot a token was given to. +func DetectTokenType(token string) string { + switch { + case strings.HasPrefix(token, "xoxb-"): + return "bot" + case strings.HasPrefix(token, "xoxp-"): + return "user" + default: + return "unknown" } - - return os.WriteFile(configPath, []byte(strings.Join(newLines, "\n")+"\n"), 0600) } diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go index 13e8995..f3b1fdf 100644 --- a/internal/keychain/keychain_test.go +++ b/internal/keychain/keychain_test.go @@ -1,667 +1,262 @@ package keychain import ( + "errors" "os" "path/filepath" - "runtime" + "strings" "testing" -) - -func TestGetAPIToken_FromEnvVar(t *testing.T) { - // Clear any existing env var first - originalValue := os.Getenv("SLACK_API_TOKEN") - defer func() { - if originalValue != "" { - _ = os.Setenv("SLACK_API_TOKEN", originalValue) - } else { - _ = os.Unsetenv("SLACK_API_TOKEN") - } - }() - - t.Setenv("SLACK_API_TOKEN", "xoxb-test-token-from-env") - - token, err := GetAPIToken() - if err != nil { - // On macOS, keychain might have a token which takes precedence - // So we only check the env var path on non-darwin or when keychain fails - if runtime.GOOS != "darwin" { - t.Fatalf("unexpected error: %v", err) - } - } - - // If we got a token and we're not on darwin (where keychain takes precedence) - if runtime.GOOS != "darwin" && token != "xoxb-test-token-from-env" { - t.Errorf("expected token from env, got %s", token) - } -} -func TestGetAPIToken_NoToken(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain may have stored token") - } + "github.com/open-cli-collective/cli-common/credstore" - // Clear env var - originalValue := os.Getenv("SLACK_API_TOKEN") - defer func() { - if originalValue != "" { - _ = os.Setenv("SLACK_API_TOKEN", originalValue) - } else { - _ = os.Unsetenv("SLACK_API_TOKEN") - } - }() - _ = os.Unsetenv("SLACK_API_TOKEN") - - // Use a temp dir that doesn't have any config - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - _, err := GetAPIToken() - if err == nil { - t.Error("expected error when no token is configured") - } -} + appconfig "github.com/open-cli-collective/slack-chat-api/internal/config" + "github.com/open-cli-collective/slack-chat-api/internal/output" + "github.com/open-cli-collective/slack-chat-api/internal/testutil" +) -func TestIsSecureStorage(t *testing.T) { - expected := runtime.GOOS == "darwin" - actual := IsSecureStorage() +const ( + botTok = "xoxb-1111-distinctive-bot-secret" + userTok = "xoxp-2222-distinctive-user-secret" +) - if actual != expected { - t.Errorf("IsSecureStorage() = %v, expected %v", actual, expected) +func TestDetectTokenType(t *testing.T) { + cases := map[string]string{ + "xoxb-abc": "bot", "xoxp-abc": "user", "xoxc-abc": "unknown", "": "unknown", } -} - -func TestGetConfigDir_Default(t *testing.T) { - // Clear XDG_CONFIG_HOME to test default - originalValue := os.Getenv("XDG_CONFIG_HOME") - defer func() { - if originalValue != "" { - _ = os.Setenv("XDG_CONFIG_HOME", originalValue) - } else { - _ = os.Unsetenv("XDG_CONFIG_HOME") + for in, want := range cases { + if got := DetectTokenType(in); got != want { + t.Fatalf("DetectTokenType(%q)=%q want %q", in, got, want) } - }() - _ = os.Unsetenv("XDG_CONFIG_HOME") - - home, _ := os.UserHomeDir() - expected := filepath.Join(home, ".config", "slack-chat-api") - actual := getConfigDir() - - if actual != expected { - t.Errorf("getConfigDir() = %s, expected %s", actual, expected) - } -} - -func TestGetConfigDir_XDGConfigHome(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - expected := filepath.Join(tmpDir, "slack-chat-api") - actual := getConfigDir() - - if actual != expected { - t.Errorf("getConfigDir() = %s, expected %s", actual, expected) } } -func TestConfigFile_SetAndGet(t *testing.T) { - // This tests the config file functions directly - // which work on all platforms (used as fallback on macOS) - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - // Test set - err := setInConfigFile("test_key", "test_value") +func TestStoreRoundTripAndClear(t *testing.T) { + testutil.Setup(t) + st, err := Open() if err != nil { - t.Fatalf("setInConfigFile failed: %v", err) + t.Fatalf("Open: %v", err) } + defer func() { _ = st.Close() }() - // Verify config directory was created - configDir := filepath.Join(tmpDir, "slack-chat-api") - info, err := os.Stat(configDir) - if err != nil { - t.Fatalf("config directory not created: %v", err) + if _, err := st.BotToken(); !errors.Is(err, ErrMissingBotToken) { + t.Fatalf("missing bot token err=%v want ErrMissingBotToken", err) } - if !info.IsDir() { - t.Error("expected config path to be a directory") + if err := st.SetBotToken(botTok); err != nil { + t.Fatalf("SetBotToken: %v", err) } - - // Test get - value, err := getFromConfigFile("test_key") - if err != nil { - t.Fatalf("getFromConfigFile failed: %v", err) + if err := st.SetUserToken(userTok); err != nil { + t.Fatalf("SetUserToken: %v", err) } - if value != "test_value" { - t.Errorf("expected test_value, got %s", value) + if v, err := st.BotToken(); err != nil || v != botTok { + t.Fatalf("BotToken=%q,%v", v, err) } -} - -func TestConfigFile_GetNonExistent(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - _, err := getFromConfigFile("nonexistent_key") - if err == nil { - t.Error("expected error for nonexistent key") + if !st.HasBotToken() || !st.HasUserToken() { + t.Fatalf("Has*Token false after set") } -} - -func TestConfigFile_Delete(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - // Set a value first - err := setInConfigFile("delete_test", "value") - if err != nil { - t.Fatalf("setInConfigFile failed: %v", err) + if err := st.DeleteUserToken(); err != nil { + t.Fatalf("DeleteUserToken: %v", err) } - - // Verify it exists - _, err = getFromConfigFile("delete_test") - if err != nil { - t.Fatalf("key not found after set: %v", err) + if st.HasUserToken() { + t.Fatalf("user token still present after delete") } - - // Delete it - err = deleteFromConfigFile("delete_test") + if err := st.DeleteUserToken(); err != nil { + t.Fatalf("idempotent delete: %v", err) + } + removed, err := st.Clear() if err != nil { - t.Fatalf("deleteFromConfigFile failed: %v", err) + t.Fatalf("Clear: %v", err) } - - // Verify it's gone - _, err = getFromConfigFile("delete_test") - if err == nil { - t.Error("expected error after delete") + if len(removed) == 0 || st.HasBotToken() { + t.Fatalf("Clear left state: removed=%v", removed) } } -func TestConfigFile_MultipleKeys(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - // Set multiple keys - err := setInConfigFile("key1", "value1") - if err != nil { - t.Fatalf("setInConfigFile key1 failed: %v", err) - } - err = setInConfigFile("key2", "value2") +// TestRefAuthoritative proves nothing is hard-coded: a non-default ref in +// config.yml drives the service/profile, and a value written under it is not +// visible under the default ref (§1.3 — Codex Blocker). +func TestRefAuthoritative(t *testing.T) { + testutil.Setup(t) + st, err := openWith(&appconfig.Config{CredentialRef: "slack-chat-api/work"}, false) if err != nil { - t.Fatalf("setInConfigFile key2 failed: %v", err) + t.Fatalf("openWith: %v", err) } - err = setInConfigFile("key3", "value3") - if err != nil { - t.Fatalf("setInConfigFile key3 failed: %v", err) + if st.Ref() != "slack-chat-api/work" { + t.Fatalf("Ref=%q", st.Ref()) } - - // Verify all exist - v1, _ := getFromConfigFile("key1") - v2, _ := getFromConfigFile("key2") - v3, _ := getFromConfigFile("key3") - - if v1 != "value1" || v2 != "value2" || v3 != "value3" { - t.Errorf("multiple keys not stored correctly: %s, %s, %s", v1, v2, v3) + if err := st.SetBotToken(botTok); err != nil { + t.Fatalf("set: %v", err) } + _ = st.Close() - // Delete middle key - err = deleteFromConfigFile("key2") + def, err := openWith(&appconfig.Config{CredentialRef: appconfig.DefaultCredentialRef}, false) if err != nil { - t.Fatalf("deleteFromConfigFile failed: %v", err) + t.Fatalf("open default: %v", err) } - - // Verify key1 and key3 still exist - v1, err = getFromConfigFile("key1") - if err != nil || v1 != "value1" { - t.Errorf("key1 not preserved after deleting key2") - } - v3, err = getFromConfigFile("key3") - if err != nil || v3 != "value3" { - t.Errorf("key3 not preserved after deleting key2") - } - - // Verify key2 is gone - _, err = getFromConfigFile("key2") - if err == nil { - t.Error("key2 should be deleted") + defer func() { _ = def.Close() }() + if def.HasBotToken() { + t.Fatalf("value leaked across profiles") } } -func TestConfigFile_UpdateExistingKey(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - // Set initial value - err := setInConfigFile("update_test", "original") - if err != nil { - t.Fatalf("setInConfigFile failed: %v", err) +func writeLegacyFile(t *testing.T, kv map[string]string) string { + t.Helper() + dir := filepath.Join(os.Getenv("XDG_CONFIG_HOME"), "slack-chat-api") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) } - - // Update value - err = setInConfigFile("update_test", "updated") - if err != nil { - t.Fatalf("setInConfigFile update failed: %v", err) - } - - // Verify updated value - value, err := getFromConfigFile("update_test") - if err != nil { - t.Fatalf("getFromConfigFile failed: %v", err) + p := filepath.Join(dir, "credentials") + var b strings.Builder + for k, v := range kv { + b.WriteString(k + "=" + v + "\n") } - if value != "updated" { - t.Errorf("expected updated, got %s", value) + if err := os.WriteFile(p, []byte(b.String()), 0o600); err != nil { + t.Fatal(err) } + return p } -func TestConfigFile_Permissions(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("File permissions test not applicable on Windows") - } +func TestMigratePlaintextFileRenamesAndCleansUp(t *testing.T) { + testutil.Setup(t) + legacy := writeLegacyFile(t, map[string]string{"api_token": botTok, "user_token": userTok}) - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - // Set a value to create the file - err := setInConfigFile("perm_test", "value") + st, err := Open() if err != nil { - t.Fatalf("setInConfigFile failed: %v", err) + t.Fatalf("Open(migrate): %v", err) } + defer func() { _ = st.Close() }() - // Check file permissions (should be 0600) - configPath := filepath.Join(tmpDir, "slack-chat-api", "credentials") - info, err := os.Stat(configPath) - if err != nil { - t.Fatalf("stat failed: %v", err) + if v, _ := st.BotToken(); v != botTok { + t.Fatalf("api_token did not migrate to bot_token: %q", v) } - - mode := info.Mode().Perm() - if mode != 0600 { - t.Errorf("expected file permissions 0600, got %o", mode) + if v, _ := st.UserToken(); v != userTok { + t.Fatalf("user_token did not migrate: %q", v) } - - // Check directory permissions (should be 0700) - dirInfo, err := os.Stat(filepath.Join(tmpDir, "slack-chat-api")) - if err != nil { - t.Fatalf("stat dir failed: %v", err) + if _, err := os.Stat(legacy); !os.IsNotExist(err) { + t.Fatalf("legacy plaintext file not removed: %v", err) } - - dirMode := dirInfo.Mode().Perm() - if dirMode != 0700 { - t.Errorf("expected directory permissions 0700, got %o", dirMode) + if _, err := os.Stat(appconfig.Path()); err != nil { + t.Fatalf("config.yml not written: %v", err) } } -func TestConfigFile_ValueWithEquals(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - // Test value containing equals sign - valueWithEquals := "token=part=with=equals" - err := setInConfigFile("equals_test", valueWithEquals) +func TestMigrateIdempotent(t *testing.T) { + testutil.Setup(t) + writeLegacyFile(t, map[string]string{"api_token": botTok}) + s1, err := Open() if err != nil { - t.Fatalf("setInConfigFile failed: %v", err) + t.Fatalf("first Open: %v", err) } + _ = s1.Close() + output.ResetMigration() - value, err := getFromConfigFile("equals_test") + s2, err := Open() if err != nil { - t.Fatalf("getFromConfigFile failed: %v", err) + t.Fatalf("second Open: %v", err) } - if value != valueWithEquals { - t.Errorf("expected %s, got %s", valueWithEquals, value) + defer func() { _ = s2.Close() }() + if v, _ := s2.BotToken(); v != botTok { + t.Fatalf("value lost on idempotent re-open: %q", v) } } -func TestHasStoredToken_WithToken(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") - } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - // Initially no token - if HasStoredToken() { - t.Error("expected no stored token initially") - } - - // Set a token - err := setInConfigFile(apiTokenKey, "xoxb-test-token") +func TestMigrateConflictFailsLoudWithoutLeaking(t *testing.T) { + testutil.Setup(t) + pre, err := Open() if err != nil { - t.Fatalf("setInConfigFile failed: %v", err) - } - - // Now should have a token - if !HasStoredToken() { - t.Error("expected stored token after set") + t.Fatal(err) } -} - -func TestHasStoredToken_EnvVarOnly(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") + if err := pre.SetBotToken("xoxb-KEYRING-existing-value"); err != nil { + t.Fatal(err) } + _ = pre.Close() - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - t.Setenv("SLACK_API_TOKEN", "xoxb-env-token") + legacy := writeLegacyFile(t, map[string]string{"api_token": "xoxb-LEGACY-different-value"}) - // HasStoredToken should return false when only env var is set - if HasStoredToken() { - t.Error("expected false when only env var is set") + _, err = Open() + if !errors.Is(err, credstore.ErrMigrationConflict) { + t.Fatalf("want ErrMigrationConflict, got %v", err) } -} - -func TestGetTokenSource_ConfigFile(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") + msg := err.Error() + if !strings.Contains(msg, legacy) || !strings.Contains(msg, "slack-chat-api/default") { + t.Fatalf("conflict msg missing locations: %q", msg) } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - // Clear env var - originalValue := os.Getenv("SLACK_API_TOKEN") - defer func() { - if originalValue != "" { - _ = os.Setenv("SLACK_API_TOKEN", originalValue) - } else { - _ = os.Unsetenv("SLACK_API_TOKEN") - } - }() - _ = os.Unsetenv("SLACK_API_TOKEN") - - // No token - should return empty string - source := GetTokenSource() - if source != "" { - t.Errorf("expected empty string for no token, got %s", source) + if leak := credstore.NoLeakAssertion([]byte(msg), + "xoxb-KEYRING-existing-value", "xoxb-LEGACY-different-value"); leak != nil { + t.Fatalf("conflict message leaked a secret value: %v", leak) } - - // Set a token in config file - err := setInConfigFile(apiTokenKey, "xoxb-test-token") - if err != nil { - t.Fatalf("setInConfigFile failed: %v", err) - } - - // Should return "config file" on non-darwin - source = GetTokenSource() - if source != "config file" { - t.Errorf("expected 'config file', got %s", source) + if _, statErr := os.Stat(legacy); statErr != nil { + t.Fatalf("legacy file deleted despite conflict: %v", statErr) } } -func TestGetTokenSource_EnvVar(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") - } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - t.Setenv("SLACK_API_TOKEN", "xoxb-env-token") - - // When only env var is set (no config file) - source := GetTokenSource() - if source != "environment variable" { - t.Errorf("expected 'environment variable', got %s", source) - } -} - -// --- User Token Tests --- - -func TestUserToken_SetAndGet(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") - } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - // Clear env var - _ = os.Unsetenv("SLACK_USER_TOKEN") - - // Set user token - err := SetUserToken("xoxp-test-user-token") +func TestMigrateConflictResolvedByOverwrite(t *testing.T) { + testutil.Setup(t) + pre, err := Open() if err != nil { - t.Fatalf("SetUserToken failed: %v", err) + t.Fatal(err) } - - // Get user token - token, err := GetUserToken() - if err != nil { - t.Fatalf("GetUserToken failed: %v", err) + if err := pre.SetBotToken("xoxb-OLD-keyring"); err != nil { + t.Fatal(err) } - if token != "xoxp-test-user-token" { - t.Errorf("expected xoxp-test-user-token, got %s", token) - } -} - -func TestUserToken_Delete(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") - } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - _ = os.Unsetenv("SLACK_USER_TOKEN") + _ = pre.Close() + legacy := writeLegacyFile(t, map[string]string{"api_token": "xoxb-NEW-legacy-forced"}) - // Set a token first - err := SetUserToken("xoxp-delete-test") + st, err := OpenForMigrationOverwrite() if err != nil { - t.Fatalf("SetUserToken failed: %v", err) + t.Fatalf("overwrite migrate: %v", err) } - - // Verify it exists - if !HasStoredUserToken() { - t.Error("expected stored user token after set") + defer func() { _ = st.Close() }() + if v, _ := st.BotToken(); v != "xoxb-NEW-legacy-forced" { + t.Fatalf("overwrite did not force legacy: %q", v) } - - // Delete it - err = DeleteUserToken() - if err != nil { - t.Fatalf("DeleteUserToken failed: %v", err) - } - - // Verify it's gone - if HasStoredUserToken() { - t.Error("expected no stored user token after delete") + if _, e := os.Stat(legacy); !os.IsNotExist(e) { + t.Fatalf("legacy not removed after forced migrate") } } -func TestUserToken_EnvVar(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") - } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - t.Setenv("SLACK_USER_TOKEN", "xoxp-env-user-token") - - // Should get token from env var - token, err := GetUserToken() - if err != nil { - t.Fatalf("GetUserToken failed: %v", err) - } - if token != "xoxp-env-user-token" { - t.Errorf("expected xoxp-env-user-token, got %s", token) - } -} - -func TestUserToken_NoToken(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") - } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - _ = os.Unsetenv("SLACK_USER_TOKEN") - - _, err := GetUserToken() - if err == nil { - t.Error("expected error when no user token is configured") - } -} - -func TestHasStoredUserToken(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") - } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - _ = os.Unsetenv("SLACK_USER_TOKEN") - - // Initially no token - if HasStoredUserToken() { - t.Error("expected no stored user token initially") - } - - // Set a token - err := SetUserToken("xoxp-test-token") +func TestMigrateEqualValueCleansUpSilently(t *testing.T) { + testutil.Setup(t) + pre, err := Open() if err != nil { - t.Fatalf("SetUserToken failed: %v", err) - } - - // Now should have a token - if !HasStoredUserToken() { - t.Error("expected stored user token after set") - } -} - -func TestHasStoredUserToken_EnvVarOnly(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") - } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - t.Setenv("SLACK_USER_TOKEN", "xoxp-env-token") - - // HasStoredUserToken should return false when only env var is set - if HasStoredUserToken() { - t.Error("expected false when only env var is set") - } -} - -func TestGetUserTokenSource_ConfigFile(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") + t.Fatal(err) } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - _ = os.Unsetenv("SLACK_USER_TOKEN") - - // No token - should return empty string - source := GetUserTokenSource() - if source != "" { - t.Errorf("expected empty string for no token, got %s", source) + if err := pre.SetBotToken(botTok); err != nil { + t.Fatal(err) } + _ = pre.Close() + legacy := writeLegacyFile(t, map[string]string{"api_token": botTok}) + output.ResetMigration() - // Set a token in config file - err := SetUserToken("xoxp-test-token") + st, err := Open() if err != nil { - t.Fatalf("SetUserToken failed: %v", err) + t.Fatalf("Open: %v", err) } - - // Should return "config file" on non-darwin - source = GetUserTokenSource() - if source != "config file" { - t.Errorf("expected 'config file', got %s", source) + defer func() { _ = st.Close() }() + if _, e := os.Stat(legacy); !os.IsNotExist(e) { + t.Fatalf("equal-value legacy not cleaned up") } -} - -func TestGetUserTokenSource_EnvVar(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") - } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - t.Setenv("SLACK_USER_TOKEN", "xoxp-env-token") - - // When only env var is set (no config file) - source := GetUserTokenSource() - if source != "environment variable" { - t.Errorf("expected 'environment variable', got %s", source) + if v, _ := st.BotToken(); v != botTok { + t.Fatalf("value changed: %q", v) } } -func TestDetectTokenType(t *testing.T) { - tests := []struct { - token string - expected string - }{ - {"xoxb-123-456-789", "bot"}, - {"xoxp-123-456-789", "user"}, - {"invalid-token", "unknown"}, - {"", "unknown"}, - {"xoxb", "unknown"}, - {"xoxp", "unknown"}, - {"xoxb-", "bot"}, - {"xoxp-", "user"}, - } - - for _, tt := range tests { - t.Run(tt.token, func(t *testing.T) { - result := DetectTokenType(tt.token) - if result != tt.expected { - t.Errorf("DetectTokenType(%q) = %q, expected %q", tt.token, result, tt.expected) +func TestDiscoverFileBranch(t *testing.T) { + testutil.Setup(t) + writeLegacyFile(t, map[string]string{"api_token": botTok, "ignored_key": "x"}) + got := discover("slack-chat-api", credstore.BackendFile) + var sawBot bool + for _, c := range got { + if c.newKey == KeyBotToken { + sawBot = true + if c.legacyField != "api_token" { + t.Fatalf("legacyField=%q want api_token", c.legacyField) } - }) - } -} - -func TestBothTokenTypes_Coexist(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("Skipping on macOS - keychain tests require manual setup") - } - - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - _ = os.Unsetenv("SLACK_API_TOKEN") - _ = os.Unsetenv("SLACK_USER_TOKEN") - - // Set both tokens - err := SetAPIToken("xoxb-bot-token") - if err != nil { - t.Fatalf("SetAPIToken failed: %v", err) - } - err = SetUserToken("xoxp-user-token") - if err != nil { - t.Fatalf("SetUserToken failed: %v", err) - } - - // Verify both exist independently - botToken, err := GetAPIToken() - if err != nil { - t.Fatalf("GetAPIToken failed: %v", err) - } - if botToken != "xoxb-bot-token" { - t.Errorf("expected xoxb-bot-token, got %s", botToken) - } - - userToken, err := GetUserToken() - if err != nil { - t.Fatalf("GetUserToken failed: %v", err) - } - if userToken != "xoxp-user-token" { - t.Errorf("expected xoxp-user-token, got %s", userToken) - } - - // Delete bot token, user token should remain - err = DeleteAPIToken() - if err != nil { - t.Fatalf("DeleteAPIToken failed: %v", err) - } - - // User token should still exist - userToken, err = GetUserToken() - if err != nil { - t.Fatalf("GetUserToken after bot delete failed: %v", err) - } - if userToken != "xoxp-user-token" { - t.Errorf("user token should persist after bot token delete") + } + if c.legacyField == "ignored_key" { + t.Fatalf("non-credential key discovered") + } } - - // Bot token should be gone - if HasStoredToken() { - t.Error("bot token should be deleted") + if !sawBot { + t.Fatalf("api_token not discovered from file: %+v", got) } } diff --git a/internal/keychain/migrate.go b/internal/keychain/migrate.go new file mode 100644 index 0000000..1688403 --- /dev/null +++ b/internal/keychain/migrate.go @@ -0,0 +1,284 @@ +package keychain + +import ( + "bufio" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "sort" + "strings" + + "github.com/open-cli-collective/cli-common/credstore" + + "github.com/open-cli-collective/slack-chat-api/internal/config" + "github.com/open-cli-collective/slack-chat-api/internal/output" +) + +// One-time legacy migration (§1.8 / §2.4). Reads any legacy credential +// location that still exists, writes the new credstore bundle, surfaces the +// signal (stderr for humans, _migration for JSON), then deletes the legacy +// originals. Idempotent: once the originals are gone there is nothing to do +// and no signal fires. Conflicts (legacy vs legacy, or legacy vs an existing +// keyring value) fail loudly per §1.8 — all conflicts are detected before +// any write or delete, and on conflict every legacy and keyring entry is +// left exactly as it was. + +// legacy keychain service names slck has used historically (§2.4): the +// current "slack-chat-api" and the older "slck". Account names are the +// legacy field names; "api_token" is renamed to bot_token in the new layout. +var legacyKeychainServices = []string{"slck", "slack-chat-api"} + +// legacyFields maps a legacy field/account name to the new bundle key. +var legacyFields = map[string]string{"api_token": KeyBotToken, "user_token": KeyUserToken} + +// candidate is one discovered legacy value for a logical new key. +type candidate struct { + newKey string // bot_token | user_token + legacyField string // api_token | user_token (what §1.8 `field` must name) + location string // non-secret descriptor (never the value) + value string + deleter func() error // removes this specific legacy original +} + +// migrateLegacyOverwrite runs the one-time migration. overwrite (the §1.8 +// `--overwrite` path, reached via keychain.OpenForMigrationOverwrite) forces +// a legacy value over an existing keyring entry; it cannot resolve a +// legacy-vs-legacy disagreement — the user must still pick. +func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error { + backend, _ := s.cs.Backend() + cands := discover(s.service, backend) + if len(cands) == 0 { + return nil // nothing legacy on disk/keychain — the steady state + } + + // Group candidates by new key, in deterministic key order. + byKey := map[string][]candidate{} + for _, c := range cands { + byKey[c.newKey] = append(byKey[c.newKey], c) + } + keys := make([]string, 0, len(byKey)) + for k := range byKey { + keys = append(keys, k) + } + sort.Strings(keys) + + // Phase 1: resolve every key, detecting ALL conflicts before mutating. + writes := map[string]string{} // newKey -> value to SetBundle + changes := []credstore.MigrationChange{} // for the _migration block + humanField := map[string]string{} // newKey -> legacyField (stderr) + var cleanups []func() error // run only after a clean write + + for _, k := range keys { + group := byKey[k] + distinct := map[string]bool{} + for _, c := range group { + distinct[c.value] = true + } + + target, hasTarget := currentValue(s, k) + + switch { + case len(distinct) > 1 && (!overwrite || hasTarget): + // Legacy sources disagree: overwrite can't pick among them either. + return conflict(s, group, target, hasTarget) + case hasTarget && !overwrite && disagrees(distinct, target): + return conflict(s, group, target, hasTarget) + case hasTarget && !disagrees(distinct, target): + // Already migrated (values match): no write, just clean up the + // leftover originals from an interrupted prior run. No signal. + for _, c := range group { + cleanups = append(cleanups, c.deleter) + } + default: + // Resolvable migration: one value (or overwrite forcing one). + val := group[0].value + writes[k] = val + lf := group[0].legacyField + humanField[k] = lf + changes = append(changes, credstore.MigrationJSONEntry( + lf, group[0].location, + fmt.Sprintf("keyring:%s/%s/%s", s.service, s.profile, k))) + for _, c := range group { + cleanups = append(cleanups, c.deleter) + } + } + } + + // Phase 2: write the new bundle (no overwrite needed when target absent; + // WithOverwrite only when the caller forced it). + if len(writes) > 0 { + var opts []credstore.SetOpt + if overwrite { + opts = append(opts, credstore.WithOverwrite()) + } + if _, err := s.cs.SetBundle(s.profile, writes, opts...); err != nil { + return fmt.Errorf("migrate to keyring %s: %w", s.ref, err) + } + } + + // Phase 3: surface the signal (only for keys actually moved this run). + if len(changes) > 0 { + output.RecordMigration(credstore.NewMigrationBlock(changes...)) + if !output.IsJSON() { + for _, k := range keys { + if lf, ok := humanField[k]; ok { + credstore.EmitMigrationStderr(lf, s.ref) + } + } + } + } + + // Phase 4: delete the legacy originals, then persist credential_ref so + // the migration is not re-attempted (§1.8 "adding credential_ref"). + for _, del := range cleanups { + if err := del(); err != nil { + return fmt.Errorf("migration wrote the keyring but could not remove a legacy original (%s): %w", s.ref, err) + } + } + if err := cfg.Save(); err != nil { + return fmt.Errorf("migration succeeded but writing config.yml failed: %w", err) + } + return nil +} + +// currentValue reports the existing credstore value for a key (post-Open). +func currentValue(s *Store, key string) (string, bool) { + v, err := s.cs.Get(s.profile, key) + if err != nil || v == "" { + return "", false + } + return v, true +} + +func disagrees(distinct map[string]bool, target string) bool { + for v := range distinct { + if v != target { + return true + } + } + return false +} + +// conflict builds the §1.8 error: names every legacy source and the keyring +// target, never a value (masked or not). Reports the legacy field name. +func conflict(s *Store, group []candidate, target string, hasTarget bool) error { + locs := make([]string, 0, len(group)+1) + for _, c := range group { + locs = append(locs, c.location) + } + if hasTarget { + _ = target // value intentionally unused — never printed + locs = append(locs, fmt.Sprintf("keyring:%s/%s/%s", s.service, s.profile, group[0].newKey)) + } + return credstore.MigrationConflictError("slck", group[0].legacyField, strings.Join(locs, ", "), s.ref) +} + +// discover enumerates every legacy source that currently exists. macOS +// keychain reads are migration-only and darwin-only (the sole sanctioned +// `security` shell-out) — and only when the *selected* backend is the OS +// Keychain. If the user forced the file/memory backend (config/env/Options), +// they have opted out of the OS keychain entirely, so probing it for legacy +// items would be wrong; this also keeps tests (which force the file backend +// in a temp HOME) hermetic and free of `security` shell-outs. The plaintext +// file path is the released layout — $XDG_CONFIG_HOME/slack-chat-api/ +// credentials else ~/.config/... — on Linux AND Windows alike (the legacy +// code has no %APPDATA% branch). +func discover(service string, backend credstore.Backend) []candidate { + var out []candidate + + if runtime.GOOS == "darwin" && backend == credstore.BackendKeychain { + for _, svc := range legacyKeychainServices { + for field, newKey := range legacyFields { + svc, field, newKey := svc, field, newKey + if v, ok := keychainRead(svc, field); ok { + out = append(out, candidate{ + newKey: newKey, + legacyField: field, + location: fmt.Sprintf("keychain:%s/%s", svc, field), + value: v, + deleter: func() error { return keychainDelete(svc, field) }, + }) + } + } + } + } + + path := legacyCredentialsPath() + for field, v := range readLegacyFile(path) { + field, v := field, v + newKey, known := legacyFields[field] + if !known { + continue + } + out = append(out, candidate{ + newKey: newKey, + legacyField: field, + location: fmt.Sprintf("file:%s#%s", path, field), + value: v, + // Secrets-only file: both keys share one deleter target, so it + // must be idempotent — the first key's cleanup removes the file + // and the second must treat already-gone as success. + deleter: func() error { + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + return err + } + return nil + }, + }) + } + return out +} + +func legacyCredentialsPath() string { + if xdg := os.Getenv("XDG_CONFIG_HOME"); xdg != "" { + return filepath.Join(xdg, "slack-chat-api", "credentials") + } + home, _ := os.UserHomeDir() + return filepath.Join(home, ".config", "slack-chat-api", "credentials") +} + +// readLegacyFile parses the legacy key=value credentials file. Missing file +// → empty map (not an error: the steady state). +func readLegacyFile(path string) map[string]string { + f, err := os.Open(path) + if err != nil { + return nil + } + defer func() { _ = f.Close() }() + m := map[string]string{} + sc := bufio.NewScanner(f) + for sc.Scan() { + line := sc.Text() + if i := strings.IndexByte(line, '='); i > 0 { + m[line[:i]] = line[i+1:] + } + } + return m +} + +func keychainRead(service, account string) (string, bool) { + out, err := exec.Command("security", "find-generic-password", + "-s", service, "-a", account, "-w").Output() + if err != nil { + return "", false + } + v := strings.TrimRight(string(out), "\r\n") + if v == "" { + return "", false + } + return v, true +} + +func keychainDelete(service, account string) error { + err := exec.Command("security", "delete-generic-password", + "-s", service, "-a", account).Run() + // Already-absent is success for an idempotent cleanup. + var ee *exec.ExitError + if err != nil && errors.As(err, &ee) { + return nil + } + return err +} diff --git a/internal/keychain/passphrase.go b/internal/keychain/passphrase.go new file mode 100644 index 0000000..1008db5 --- /dev/null +++ b/internal/keychain/passphrase.go @@ -0,0 +1,54 @@ +package keychain + +import ( + "fmt" + "os" + "strings" + + "golang.org/x/term" +) + +// passphraseEnvVar is the §1.4 named exception: _KEYRING_PASSPHRASE, +// SERVICE being the upper-snake-cased service segment of the credential_ref +// (slack-chat-api -> SLACK_CHAT_API_KEYRING_PASSPHRASE). Service segments are +// [A-Za-z0-9_-], so only '-' needs translating. +func passphraseEnvVar(service string) string { + return strings.ToUpper(strings.ReplaceAll(service, "-", "_")) + "_KEYRING_PASSPHRASE" +} + +// passphraseFunc is credstore Options.FilePassphrase: consulted only for the +// encrypted-file backend, and only after credstore has already checked +// _KEYRING_PASSPHRASE itself. So this is the interactive fallback: +// a no-echo TTY prompt. Headless with no env var set is a hard, actionable +// error — never a silent empty passphrase (that would create an +// effectively-unencrypted keyring). +func passphraseFunc(service string) func() (string, error) { + return func() (string, error) { + if !term.IsTerminal(int(os.Stdin.Fd())) { + return "", fmt.Errorf( + "file keyring backend needs a passphrase: set %s, or run interactively", + passphraseEnvVar(service)) + } + fmt.Fprintf(os.Stderr, "Passphrase for the %s file keyring: ", service) + b, err := term.ReadPassword(int(os.Stdin.Fd())) + fmt.Fprintln(os.Stderr) + if err != nil { + return "", fmt.Errorf("read passphrase: %w", err) + } + p := strings.TrimRight(string(b), "\r\n") + if p == "" { + return "", fmt.Errorf("empty passphrase rejected") + } + return p, nil + } +} + +// PassphraseSource describes, for `config show`, where the file-backend +// passphrase would come from (§1.4: the user must understand their posture). +// Only meaningful when the file backend is in use. +func PassphraseSource(service string) string { + if os.Getenv(passphraseEnvVar(service)) != "" { + return "env (" + passphraseEnvVar(service) + ")" + } + return "interactive prompt" +} diff --git a/internal/output/migration.go b/internal/output/migration.go new file mode 100644 index 0000000..51c64fe --- /dev/null +++ b/internal/output/migration.go @@ -0,0 +1,66 @@ +package output + +import ( + "bytes" + "encoding/json" + "sync" +) + +// The §1.8 machine-readable migration signal. When the one-time legacy +// migration runs, it records a block here; the next JSON emit splices it in +// and clears it (run-scoped, consume-once) so it appears exactly once and +// never leaks into a later response or a parallel test. +// +// Policy (Codex-reviewed, template for B2/B3): +// - object responses -> "_migration" merged as the first top-level field, +// original fields preserved verbatim and in order. +// - non-object responses (slck emits arrays for list/history endpoints) +// -> wrapped as {"_migration": ..., "data": }. +var ( + migMu sync.Mutex + migPending []byte // marshaled migration block value, or nil if none +) + +// RecordMigration stores the §1.8 block (anything that marshals to the +// `_migration` *value*, e.g. credstore.MigrationBlock). nil/empty changes +// must not call this — absence means "no migration this run". +func RecordMigration(block interface{}) { + b, err := json.Marshal(block) + if err != nil { + return // a marshal failure here must not break the actual command + } + migMu.Lock() + migPending = b + migMu.Unlock() +} + +// takeMigration returns the pending block and clears it (consume-once). +func takeMigration() []byte { + migMu.Lock() + defer migMu.Unlock() + b := migPending + migPending = nil + return b +} + +// ResetMigration drops any pending block without emitting it. Test hook so +// one test's recorded migration can never bleed into another. +func ResetMigration() { + migMu.Lock() + migPending = nil + migMu.Unlock() +} + +// spliceMigration applies the object-merge / non-object-wrap policy to an +// already-marshaled response body, returning compact JSON. +func spliceMigration(body, mig []byte) []byte { + t := bytes.TrimSpace(body) + if len(t) > 0 && t[0] == '{' && t[len(t)-1] == '}' { + inner := bytes.TrimSpace(t[1 : len(t)-1]) + if len(inner) == 0 { + return []byte(`{"_migration":` + string(mig) + `}`) + } + return []byte(`{"_migration":` + string(mig) + `,` + string(inner) + `}`) + } + return []byte(`{"_migration":` + string(mig) + `,"data":` + string(t) + `}`) +} diff --git a/internal/output/migration_test.go b/internal/output/migration_test.go new file mode 100644 index 0000000..3e19832 --- /dev/null +++ b/internal/output/migration_test.go @@ -0,0 +1,82 @@ +package output + +import ( + "bytes" + "encoding/json" + "strings" + "testing" +) + +type migBlock struct { + Version int `json:"version"` + Changes []string `json:"changes"` +} + +func capture(t *testing.T, data interface{}) string { + t.Helper() + var buf bytes.Buffer + orig := Writer + Writer = &buf + t.Cleanup(func() { Writer = orig }) + if err := PrintJSON(data); err != nil { + t.Fatalf("PrintJSON: %v", err) + } + return buf.String() +} + +func TestPrintJSON_ObjectMergesMigrationFirst(t *testing.T) { + ResetMigration() + RecordMigration(migBlock{Version: 1, Changes: []string{"bot_token"}}) + out := capture(t, map[string]string{"result": "ok"}) + + var m map[string]json.RawMessage + if err := json.Unmarshal([]byte(out), &m); err != nil { + t.Fatalf("not valid JSON object: %v\n%s", err, out) + } + if _, ok := m["_migration"]; !ok { + t.Fatalf("_migration not merged: %s", out) + } + if _, ok := m["result"]; !ok { + t.Fatalf("original field lost: %s", out) + } + // Consume-once: a second emit has no _migration. + out2 := capture(t, map[string]string{"result": "ok"}) + if strings.Contains(out2, "_migration") { + t.Fatalf("_migration leaked into a later response: %s", out2) + } +} + +func TestPrintJSON_NonObjectWrapped(t *testing.T) { + ResetMigration() + RecordMigration(migBlock{Version: 1, Changes: []string{"user_token"}}) + out := capture(t, []string{"a", "b"}) + + var w struct { + Migration migBlock `json:"_migration"` + Data []json.RawMessage `json:"data"` + } + if err := json.Unmarshal([]byte(out), &w); err != nil { + t.Fatalf("array not wrapped into object: %v\n%s", err, out) + } + if len(w.Data) != 2 || w.Migration.Version != 1 { + t.Fatalf("wrap shape wrong: %s", out) + } +} + +func TestPrintJSON_EmptyObjectStillValid(t *testing.T) { + ResetMigration() + RecordMigration(migBlock{Version: 1}) + out := capture(t, map[string]string{}) + var m map[string]json.RawMessage + if err := json.Unmarshal([]byte(out), &m); err != nil || len(m) != 1 { + t.Fatalf("empty-object splice invalid: %v\n%s", err, out) + } +} + +func TestPrintJSON_NoMigrationUnchanged(t *testing.T) { + ResetMigration() + out := capture(t, map[string]string{"k": "v"}) + if strings.Contains(out, "_migration") { + t.Fatalf("unexpected _migration with none recorded: %s", out) + } +} diff --git a/internal/output/output.go b/internal/output/output.go index 2671e60..c366b15 100644 --- a/internal/output/output.go +++ b/internal/output/output.go @@ -1,6 +1,7 @@ package output import ( + "bytes" "encoding/json" "fmt" "io" @@ -37,8 +38,23 @@ func IsJSON() bool { return OutputFormat == FormatJSON || JSON } -// PrintJSON outputs data as formatted JSON +// PrintJSON outputs data as formatted JSON. If a §1.8 migration block was +// recorded this run, it is spliced in (consume-once) per the policy in +// migration.go and never appears again. func PrintJSON(data interface{}) error { + if mig := takeMigration(); mig != nil { + body, err := json.Marshal(data) + if err != nil { + return err + } + var buf bytes.Buffer + if err := json.Indent(&buf, spliceMigration(body, mig), "", " "); err != nil { + return err + } + buf.WriteByte('\n') + _, err = Writer.Write(buf.Bytes()) + return err + } enc := json.NewEncoder(Writer) enc.SetIndent("", " ") return enc.Encode(data) diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go new file mode 100644 index 0000000..ec76672 --- /dev/null +++ b/internal/testutil/testutil.go @@ -0,0 +1,32 @@ +// Package testutil provides a hermetic credential environment for tests +// (§1.12 test obligation). It forces credstore's encrypted-file backend +// inside a per-test temp HOME with a fixed passphrase, so no test ever +// touches the real OS keyring, shells out to `security`, or depends on +// machine state. This is the test pattern B2/B3 reuse. +package testutil + +import ( + "path/filepath" + "testing" + + "github.com/open-cli-collective/slack-chat-api/internal/output" +) + +// Setup isolates HOME/XDG to a temp dir and forces the file backend with a +// known passphrase via the §1.4 named env vars. Returns the temp dir so a +// test can plant legacy artifacts (e.g. a legacy credentials file) under it. +func Setup(t *testing.T) string { + t.Helper() + tmp := t.TempDir() + t.Setenv("HOME", tmp) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, "xdgconfig")) + // Force credstore's encrypted-file backend (never the real Keychain / + // Secret Service), with the passphrase supplied non-interactively. + t.Setenv("SLACK_CHAT_API_KEYRING_BACKEND", "file") + t.Setenv("SLACK_CHAT_API_KEYRING_PASSPHRASE", "test-passphrase") + // Belt-and-suspenders: a prior test's recorded §1.8 block must never + // bleed into this one's JSON output. + output.ResetMigration() + t.Cleanup(output.ResetMigration) + return tmp +} From 535c810afce57e3ae4e6c012c1a8e453a6025f68 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sun, 17 May 2026 07:15:46 -0400 Subject: [PATCH 2/7] fix(credstore): address Codex PR review (4 blockers, 4 majors, 2 minors) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - go.mod back to go 1.24.0 (pin x/term v0.27, x/sys v0.28 — latest needed go 1.25, breaking B0's 1.24 alignment and CI). - migration: legacy-vs-legacy disagreement is ALWAYS a conflict, even with --overwrite (it can't pick a winner; §1.8). - config clear opens WITHOUT migration (keychain.OpenNoMigrate) so the §1.8 remediation it advertises actually works during a conflict. - fail closed on an unrecognized config keyring.backend (no silent degradation to auto-select; §1.4). - init interactive token prompt is now no-echo (term.ReadPassword) with a non-terminal test seam (§1.12). - _migration block recorded ONLY on JSON runs (no stale block leaking into a later JSON response in a long-lived/test process). - purge residual 'config set-token'/plaintext guidance from search, errors, config test, whoami, README, and the Homebrew release caveats. - §1.12 no-leak suite rewritten as internal/noleak: every command class via its real cobra command, capturing stdout+stderr+writer. - narrow security-delete not-found classification to exit 44 only. - clear --all documents the (currently empty) cache scope explicitly. [INT-437] --- .github/workflows/release.yml | 7 +- README.md | 13 ++- go.mod | 6 +- go.sum | 8 +- internal/client/errors.go | 4 +- internal/cmd/config/clear.go | 9 +- internal/cmd/config/noleak_test.go | 76 ---------------- internal/cmd/config/test.go | 2 +- internal/cmd/initcmd/init.go | 12 +++ internal/cmd/search/search.go | 2 +- internal/cmd/whoami/whoami.go | 2 +- internal/keychain/keychain.go | 51 ++++++----- internal/keychain/keychain_test.go | 4 +- internal/keychain/migrate.go | 30 +++++-- internal/noleak/noleak_test.go | 136 +++++++++++++++++++++++++++++ 15 files changed, 235 insertions(+), 127 deletions(-) delete mode 100644 internal/cmd/config/noleak_test.go create mode 100644 internal/noleak/noleak_test.go diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 71fcd1b..07bba35 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -124,10 +124,11 @@ jobs: caveats <<~EOS To configure slck, run: - slck config set-token + slck init - On macOS, your token is stored securely in the system Keychain. - On Linux, your token is stored in ~/.config/slack-chat-api/credentials. + Tokens are stored in the OS keyring (Keychain on macOS, + Credential Manager on Windows, Secret Service on Linux). + They are never written to a plaintext file. EOS end CASKEOF diff --git a/README.md b/README.md index 7a61b12..17a0b99 100644 --- a/README.md +++ b/README.md @@ -767,10 +767,19 @@ slck config delete-token #### Config Command Reference +Ingress is at the top level, not under `config`: + +| Command | Flags | Description | +|---------|-------|-------------| +| `slck init` | `--bot-token-from-env`, `--user-token-from-env`, `--bot-token-stdin`, `--overwrite`, `--no-verify` | Guided setup; stores into the keyring | +| `slck set-credential` | `--key`, `--stdin`, `--from-env`, `--ref` | Set one credential (stdin/env only) | + +`config` subcommands (none accept a secret value): + | Command | Flags | Description | |---------|-------|-------------| -| `set-token [token]` | | Set API token (auto-detects bot/user type) | -| `show` | | Show current configuration status | +| `set-token` | | Removed — errors out, points to `set-credential` | +| `show` | | Show backend, ref, which keys are present (never values) | | `delete-token` | `--force`, `--type` | Delete stored token(s) | | `test` | | Test authentication for configured tokens | diff --git a/go.mod b/go.mod index fc91b81..ff1fa98 100644 --- a/go.mod +++ b/go.mod @@ -1,12 +1,12 @@ module github.com/open-cli-collective/slack-chat-api -go 1.25.0 +go 1.24.0 require ( github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 github.com/spf13/cobra v1.8.0 github.com/stretchr/testify v1.11.1 - golang.org/x/term v0.43.0 + golang.org/x/term v0.27.0 gopkg.in/yaml.v3 v3.0.1 ) @@ -22,5 +22,5 @@ require ( github.com/mtibben/percent v0.2.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/spf13/pflag v1.0.5 // indirect - golang.org/x/sys v0.44.0 // indirect + golang.org/x/sys v0.28.0 // indirect ) diff --git a/go.sum b/go.sum index 7d61249..8589f67 100644 --- a/go.sum +++ b/go.sum @@ -39,10 +39,10 @@ github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= golang.org/x/sys v0.0.0-20210819135213-f52c844e1c1c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.44.0 h1:ildZl3J4uzeKP07r2F++Op7E9B29JRUy+a27EibtBTQ= -golang.org/x/sys v0.44.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= -golang.org/x/term v0.43.0 h1:S4RLU2sB31O/NCl+zFN9Aru9A/Cq2aqKpTZJ6B+DwT4= -golang.org/x/term v0.43.0/go.mod h1:lrhlHNdQJHO+1qVYiHfFKVuVioJIheAc3fBSMFYEIsk= +golang.org/x/sys v0.28.0 h1:Fksou7UEQUWlKvIdsqzJmUmCX3cZuD2+P3XyyzwMhlA= +golang.org/x/sys v0.28.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/term v0.27.0 h1:WP60Sv1nlK1T6SupCHbXzSaN0b9wUmsPoRS9b61A23Q= +golang.org/x/term v0.27.0/go.mod h1:iMsnZpn0cago0GOrHO2+Y7u7JPn5AylBrcoWkElMTSM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b h1:QRR6H1YWRnHb4Y/HeNFCTJLFVxaq6wH4YuVdsUOr75U= gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/internal/client/errors.go b/internal/client/errors.go index e86ed14..80342f8 100644 --- a/internal/client/errors.go +++ b/internal/client/errors.go @@ -9,8 +9,8 @@ import ( var errorHints = map[string]string{ "channel_not_found": "Verify the channel ID is correct. Use 'slck channels list' to find channel IDs.", "not_in_channel": "The bot must be invited to the channel. Use /invite @yourbot in Slack.", - "invalid_auth": "Token is invalid or expired. Run 'slck config set-token' to set a new token.", - "token_revoked": "Token has been revoked. Run 'slck config set-token' to set a new token.", + "invalid_auth": "Token is invalid or expired. Run 'slck init' (or 'slck set-credential --key bot_token --stdin') to set a new token.", + "token_revoked": "Token has been revoked. Run 'slck init' (or 'slck set-credential --key bot_token --stdin') to set a new token.", "ratelimited": "Rate limit exceeded. Wait a moment and try again.", "user_not_found": "Verify the user ID is correct. Use 'slck users list' to find user IDs.", "message_not_found": "Message not found. Verify the channel ID and timestamp are correct.", diff --git a/internal/cmd/config/clear.go b/internal/cmd/config/clear.go index 2353e53..bed81ba 100644 --- a/internal/cmd/config/clear.go +++ b/internal/cmd/config/clear.go @@ -32,7 +32,10 @@ Idempotent and non-interactive.`, } func runClear(opts *clearOptions) error { - st, err := keychain.Open() + // No-migration open: clear is the §1.8 conflict remediation ("run + // config clear then re-run"). Running migration first would return + // ErrMigrationConflict before we could delete the keyring entry. + st, err := keychain.OpenNoMigrate() if err != nil { return err } @@ -51,6 +54,10 @@ func runClear(opts *clearOptions) error { } if opts.all { + // §1.7: --all returns the active profile to a pre-init state — + // keyring bundle (above) + config file + caches + empty dirs. + // slck has NO cache directory today (no command writes one); if + // one is ever added it must also be removed here. p := appconfig.Path() switch err := os.Remove(p); { case err == nil: diff --git a/internal/cmd/config/noleak_test.go b/internal/cmd/config/noleak_test.go deleted file mode 100644 index 7709373..0000000 --- a/internal/cmd/config/noleak_test.go +++ /dev/null @@ -1,76 +0,0 @@ -package config - -import ( - "testing" - - "github.com/stretchr/testify/require" - - "github.com/open-cli-collective/cli-common/credstore" - - "github.com/open-cli-collective/slack-chat-api/internal/keychain" - "github.com/open-cli-collective/slack-chat-api/internal/output" - "github.com/open-cli-collective/slack-chat-api/internal/testutil" -) - -// TestNoLeak_CredentialSurface is the §1.12 acceptance "no-leak" test for -// slck's credential-surface commands: load a known-distinctive token into -// the keyring, run every credential command class, capture stdout+stderr, -// and fail if the token (or any prefix of it) appears. The API commands -// never echo the token (it travels only in the Authorization header), so -// the leak surface is exactly these commands. -func TestNoLeak_CredentialSurface(t *testing.T) { - const secret = "xoxb-NOLEAK-canary-7f3a9c2e1d8b4a6f" - - run := func(name string, fn func() error) string { - t.Helper() - out, err := captureOutput(t, fn) - // JSON mode toggles a global; reset so it can't bleed. - t.Cleanup(func() { output.JSON = false }) - require.NoError(t, err, name) - if leak := credstore.NoLeakAssertion([]byte(out), secret); leak != nil { - t.Fatalf("%s leaked the secret: %v\noutput:\n%s", name, leak, out) - } - return out - } - - seed := func() { - st, err := keychain.Open() - require.NoError(t, err) - require.NoError(t, st.SetBotToken(secret)) - require.NoError(t, st.Close()) - } - - t.Run("show text", func(t *testing.T) { - testutil.Setup(t) - seed() - out := run("config show", func() error { return runShow(&showOptions{}) }) - require.Contains(t, out, "present") - }) - - t.Run("show json", func(t *testing.T) { - testutil.Setup(t) - seed() - output.JSON = true - run("config show -o json", func() error { return runShow(&showOptions{}) }) - }) - - t.Run("delete-token", func(t *testing.T) { - testutil.Setup(t) - seed() - run("config delete-token", func() error { - return runDeleteToken(&deleteTokenOptions{force: true, tokenType: "all"}) - }) - }) - - t.Run("clear", func(t *testing.T) { - testutil.Setup(t) - seed() - run("config clear", func() error { return runClear(&clearOptions{}) }) - }) - - t.Run("clear --all", func(t *testing.T) { - testutil.Setup(t) - seed() - run("config clear --all", func() error { return runClear(&clearOptions{all: true}) }) - }) -} diff --git a/internal/cmd/config/test.go b/internal/cmd/config/test.go index 8ca167e..5493d05 100644 --- a/internal/cmd/config/test.go +++ b/internal/cmd/config/test.go @@ -79,7 +79,7 @@ func runTest(opts *testOptions, botClient *client.Client, userClient *client.Cli if !anySuccess { output.Println() - output.Println("No valid tokens configured. Run 'slck config set-token' to configure.") + output.Println("No valid tokens configured. Run 'slck init' to configure.") } return nil diff --git a/internal/cmd/initcmd/init.go b/internal/cmd/initcmd/init.go index e51e3b0..d47b924 100644 --- a/internal/cmd/initcmd/init.go +++ b/internal/cmd/initcmd/init.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/spf13/cobra" + "golang.org/x/term" "github.com/open-cli-collective/slack-chat-api/internal/client" appconfig "github.com/open-cli-collective/slack-chat-api/internal/config" @@ -212,8 +213,19 @@ func (o *initOptions) resolveUser() (string, error) { return promptToken(o.reader(), "User Token (xoxp-...)") } +// promptToken reads a token interactively. When stdin is a real terminal it +// is read WITHOUT echo (§1.12 — a typed token must not be displayed); the +// test seam (an injected non-terminal reader) falls back to a line read. func promptToken(reader io.Reader, prompt string) (string, error) { fmt.Printf("%s: ", prompt) + if f, ok := reader.(*os.File); ok && term.IsTerminal(int(f.Fd())) { + b, err := term.ReadPassword(int(f.Fd())) + fmt.Println() + if err != nil { + return "", err + } + return strings.TrimSpace(string(b)), nil + } scanner := bufio.NewScanner(reader) if scanner.Scan() { return strings.TrimSpace(scanner.Text()), nil diff --git a/internal/cmd/search/search.go b/internal/cmd/search/search.go index 2f2d2a2..988cb9d 100644 --- a/internal/cmd/search/search.go +++ b/internal/cmd/search/search.go @@ -18,7 +18,7 @@ To set up a user token: 2. Add 'search:read' to User Token Scopes 3. Reinstall the app to your workspace 4. Copy the User OAuth Token (starts with xoxp-) - 5. Run: slck config set-token `, + 5. Run: slck set-credential --key user_token --stdin (or: slck init)`, } cmd.AddCommand(newMessagesCmd()) diff --git a/internal/cmd/whoami/whoami.go b/internal/cmd/whoami/whoami.go index f643f0b..3c47967 100644 --- a/internal/cmd/whoami/whoami.go +++ b/internal/cmd/whoami/whoami.go @@ -100,7 +100,7 @@ func runWhoami(opts *whoamiOptions, botClient *client.Client, userClient *client // Check if any token worked if result.Bot == nil && result.User == nil { output.Println("No valid tokens configured.") - output.Println("Run 'slck config set-token' to configure authentication.") + output.Println("Run 'slck init' to configure authentication.") return nil } diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index 1a72eca..c1e7fef 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -48,19 +48,26 @@ type Store struct { // The returned Store reads/writes the OS keyring only. A legacy-vs-keyring // conflict surfaces here as a §1.8 error; `slck init --overwrite` calls // OpenForMigrationOverwrite to force the legacy value instead. -func Open() (*Store, error) { return open(false) } +func Open() (*Store, error) { return open(false, true) } // OpenForMigrationOverwrite is Open with the §1.8 `--overwrite` resolution: // a legacy value is forced over an existing keyring entry. It still cannot // resolve a legacy-vs-legacy disagreement (the user must pick). -func OpenForMigrationOverwrite() (*Store, error) { return open(true) } +func OpenForMigrationOverwrite() (*Store, error) { return open(true, true) } -func open(overwrite bool) (*Store, error) { +// OpenNoMigrate opens the store WITHOUT running the one-time migration. It +// exists so `config clear` can perform the §1.8 conflict remediation it +// advertises ("run config clear then re-run"): if migration ran first it +// would return ErrMigrationConflict before clear could delete the keyring +// entry, leaving the user with no way out. +func OpenNoMigrate() (*Store, error) { return open(false, false) } + +func open(overwrite, runMigration bool) (*Store, error) { cfg, err := config.Load() if err != nil { return nil, err } - return openWith(cfg, overwrite) + return openWith(cfg, overwrite, runMigration) } // OpenRef opens a store against an explicit ref instead of config.yml's @@ -74,21 +81,28 @@ func OpenRef(ref string) (*Store, error) { if ref != "" { cfg.CredentialRef = ref } - return openWith(cfg, false) + return openWith(cfg, false, true) } // openWith is the seam unit tests drive with an injected config (e.g. a -// memory-backend opt-in via Keyring.Backend) so they never touch a real +// file-backend opt-in via Keyring.Backend) so they never touch a real // keyring (§1.12 test obligation, and hermeticity). -func openWith(cfg *config.Config, overwrite bool) (*Store, error) { +func openWith(cfg *config.Config, overwrite, runMigration bool) (*Store, error) { service, profile, err := credstore.ParseRef(cfg.CredentialRef) if err != nil { return nil, fmt.Errorf("invalid credential_ref %q: %w", cfg.CredentialRef, err) } opts := &credstore.Options{AllowedKeys: allowedKeys} - if b, ok := configBackend(cfg.Keyring.Backend); ok { - opts.ConfigBackend = b + switch b := strings.TrimSpace(cfg.Keyring.Backend); b { + case "": + // Auto-select per §1.4 (credstore decides; fail-closed on Linux). + case "file": + opts.ConfigBackend = credstore.BackendFile + default: + // Fail closed: an unrecognized backend must not silently degrade + // to auto-selection and store credentials somewhere unintended. + return nil, fmt.Errorf("invalid keyring.backend %q in config (only \"file\" is supported)", b) } opts.FilePassphrase = passphraseFunc(service) @@ -99,24 +113,15 @@ func openWith(cfg *config.Config, overwrite bool) (*Store, error) { s := &Store{cs: cs, service: service, profile: profile, ref: cfg.CredentialRef} - if err := migrateLegacyOverwrite(s, cfg, overwrite); err != nil { - _ = cs.Close() - return nil, err + if runMigration { + if err := migrateLegacyOverwrite(s, cfg, overwrite); err != nil { + _ = cs.Close() + return nil, err + } } return s, nil } -// configBackend maps the config.yml keyring.backend value to a credstore -// Backend. Only the §1.4 "file" opt-in is honored; anything else is left to -// credstore's fail-closed default selection (ok=false). Empty is the -// common case (auto-select). -func configBackend(v string) (credstore.Backend, bool) { - if strings.TrimSpace(v) == "file" { - return credstore.BackendFile, true - } - return "", false -} - // Close releases the backing store. Safe on a nil receiver. func (s *Store) Close() error { if s == nil || s.cs == nil { diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go index f3b1fdf..ecb4950 100644 --- a/internal/keychain/keychain_test.go +++ b/internal/keychain/keychain_test.go @@ -76,7 +76,7 @@ func TestStoreRoundTripAndClear(t *testing.T) { // visible under the default ref (§1.3 — Codex Blocker). func TestRefAuthoritative(t *testing.T) { testutil.Setup(t) - st, err := openWith(&appconfig.Config{CredentialRef: "slack-chat-api/work"}, false) + st, err := openWith(&appconfig.Config{CredentialRef: "slack-chat-api/work"}, false, true) if err != nil { t.Fatalf("openWith: %v", err) } @@ -88,7 +88,7 @@ func TestRefAuthoritative(t *testing.T) { } _ = st.Close() - def, err := openWith(&appconfig.Config{CredentialRef: appconfig.DefaultCredentialRef}, false) + def, err := openWith(&appconfig.Config{CredentialRef: appconfig.DefaultCredentialRef}, false, true) if err != nil { t.Fatalf("open default: %v", err) } diff --git a/internal/keychain/migrate.go b/internal/keychain/migrate.go index 1688403..fd2a6fd 100644 --- a/internal/keychain/migrate.go +++ b/internal/keychain/migrate.go @@ -81,8 +81,10 @@ func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error target, hasTarget := currentValue(s, k) switch { - case len(distinct) > 1 && (!overwrite || hasTarget): - // Legacy sources disagree: overwrite can't pick among them either. + case len(distinct) > 1: + // Legacy sources disagree among themselves: --overwrite cannot + // pick a winner here either (§1.8 — the user must). Always a + // conflict, regardless of overwrite or target presence. return conflict(s, group, target, hasTarget) case hasTarget && !overwrite && disagrees(distinct, target): return conflict(s, group, target, hasTarget) @@ -120,9 +122,13 @@ func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error } // Phase 3: surface the signal (only for keys actually moved this run). + // Record the _migration block ONLY on a JSON run — recording it on a + // text run would leave a stale block that a later JSON command in the + // same (or test) process could splice into an unrelated response. if len(changes) > 0 { - output.RecordMigration(credstore.NewMigrationBlock(changes...)) - if !output.IsJSON() { + if output.IsJSON() { + output.RecordMigration(credstore.NewMigrationBlock(changes...)) + } else { for _, k := range keys { if lf, ok := humanField[k]; ok { credstore.EmitMigrationStderr(lf, s.ref) @@ -272,13 +278,21 @@ func keychainRead(service, account string) (string, bool) { return v, true } +// securityErrItemNotFound is `security`'s exit status when the item is +// absent (SecKeychainErrorCode errSecItemNotFound). Only this is treated as +// idempotent success — a denial/locked/other failure must surface so we +// don't silently leave a legacy secret behind after "migration". +const securityErrItemNotFound = 44 + func keychainDelete(service, account string) error { err := exec.Command("security", "delete-generic-password", "-s", service, "-a", account).Run() - // Already-absent is success for an idempotent cleanup. - var ee *exec.ExitError - if err != nil && errors.As(err, &ee) { + if err == nil { return nil } - return err + var ee *exec.ExitError + if errors.As(err, &ee) && ee.ExitCode() == securityErrItemNotFound { + return nil // already absent — fine for idempotent cleanup + } + return fmt.Errorf("remove legacy keychain item %s/%s: %w", service, account, err) } diff --git a/internal/noleak/noleak_test.go b/internal/noleak/noleak_test.go new file mode 100644 index 0000000..5a3aa08 --- /dev/null +++ b/internal/noleak/noleak_test.go @@ -0,0 +1,136 @@ +// Package noleak is the §1.12 acceptance "no-leak" suite. It loads a +// known-distinctive token into a hermetic keyring, drives every +// credential-surface command class through its real cobra command, captures +// EVERY output channel (os.Stdout, os.Stderr, and output.Writer), and fails +// if the token — or any prefix of it — appears anywhere. This is the test +// obligation B2/B3 reuse verbatim. +package noleak + +import ( + "bytes" + "io" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/open-cli-collective/cli-common/credstore" + + cfgcmd "github.com/open-cli-collective/slack-chat-api/internal/cmd/config" + initcmd "github.com/open-cli-collective/slack-chat-api/internal/cmd/initcmd" + "github.com/open-cli-collective/slack-chat-api/internal/cmd/setcred" + "github.com/open-cli-collective/slack-chat-api/internal/cmd/whoami" + "github.com/open-cli-collective/slack-chat-api/internal/keychain" + "github.com/open-cli-collective/slack-chat-api/internal/output" + "github.com/open-cli-collective/slack-chat-api/internal/testutil" +) + +const secret = "xoxb-NOLEAK-canary-7f3a9c2e1d8b4a6f0011" + +// captureAll redirects os.Stdout, os.Stderr and output.Writer, runs fn, and +// returns everything written to any of them. +func captureAll(t *testing.T, stdin string, fn func()) string { + t.Helper() + rOut, wOut, _ := os.Pipe() + rErr, wErr, _ := os.Pipe() + origOut, origErr, origW := os.Stdout, os.Stderr, output.Writer + os.Stdout, os.Stderr = wOut, wErr + var ob bytes.Buffer + output.Writer = &ob + + var inR *os.File + if stdin != "" { + var inW *os.File + inR, inW, _ = os.Pipe() + origIn := os.Stdin + os.Stdin = inR + go func() { _, _ = inW.WriteString(stdin); _ = inW.Close() }() + t.Cleanup(func() { os.Stdin = origIn }) + } + + done := make(chan string, 2) + go func() { b, _ := io.ReadAll(rOut); done <- string(b) }() + go func() { b, _ := io.ReadAll(rErr); done <- string(b) }() + + fn() + + _ = wOut.Close() + _ = wErr.Close() + os.Stdout, os.Stderr, output.Writer = origOut, origErr, origW + out1, out2 := <-done, <-done + return out1 + out2 + ob.String() +} + +func seed(t *testing.T) { + t.Helper() + st, err := keychain.Open() + require.NoError(t, err) + require.NoError(t, st.SetBotToken(secret)) + require.NoError(t, st.Close()) +} + +func assertNoLeak(t *testing.T, name, captured string) { + t.Helper() + if leak := credstore.NoLeakAssertion([]byte(captured), secret); leak != nil { + t.Fatalf("%s leaked the secret (%v).\n--- captured ---\n%s", name, leak, captured) + } +} + +func TestNoLeak_ConfigShowText(t *testing.T) { + testutil.Setup(t) + seed(t) + c := cfgcmd.NewCmd() + c.SetArgs([]string{"show"}) + out := captureAll(t, "", func() { _ = c.Execute() }) + require.Contains(t, out, "present") + assertNoLeak(t, "config show", out) +} + +func TestNoLeak_ConfigShowJSON(t *testing.T) { + testutil.Setup(t) + seed(t) + output.JSON = true + t.Cleanup(func() { output.JSON = false }) + c := cfgcmd.NewCmd() + c.SetArgs([]string{"show"}) + assertNoLeak(t, "config show -o json", captureAll(t, "", func() { _ = c.Execute() })) +} + +func TestNoLeak_DeleteTokenAndClear(t *testing.T) { + for _, args := range [][]string{{"delete-token", "--force"}, {"clear"}, {"clear", "--all"}} { + testutil.Setup(t) + seed(t) + c := cfgcmd.NewCmd() + c.SetArgs(args) + assertNoLeak(t, "config "+strings.Join(args, " "), + captureAll(t, "", func() { _ = c.Execute() })) + } +} + +func TestNoLeak_SetCredentialStdin(t *testing.T) { + testutil.Setup(t) + c := setcred.NewCmd() + c.SetArgs([]string{"--key", "bot_token", "--stdin"}) + out := captureAll(t, secret+"\n", func() { _ = c.Execute() }) + assertNoLeak(t, "set-credential --stdin", out) +} + +func TestNoLeak_InitFromEnv(t *testing.T) { + testutil.Setup(t) + t.Setenv("NOLEAK_BOT", secret) + c := initcmd.NewCmd() + c.SetArgs([]string{"--bot-token-from-env", "NOLEAK_BOT", "--no-verify"}) + out := captureAll(t, "", func() { _ = c.Execute() }) + assertNoLeak(t, "init --bot-token-from-env", out) +} + +func TestNoLeak_Whoami(t *testing.T) { + testutil.Setup(t) + seed(t) + c := whoami.NewCmd() + c.SetArgs([]string{}) + // whoami will fail to reach Slack (hermetic env); we only care that no + // output channel echoes the seeded token. + assertNoLeak(t, "whoami", captureAll(t, "", func() { _ = c.Execute() })) +} From cd751e4045e17de7c495782febf29744c7a262d6 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sun, 17 May 2026 07:22:56 -0400 Subject: [PATCH 3/7] fix(credstore): address Codex re-review (1 blocker, 1 major, 1 minor) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - B: legacy macOS Keychain discovery is now independent of the destination backend (§2.4). A macOS user who opts into keyring.backend:file still gets old slck/slack-chat-api Keychain items migrated. Test hermeticity moved to a documented test-only env seam (SLCK_TEST_DISABLE_LEGACY_KEYCHAIN_SCAN, set by testutil) instead of coupling discovery to the backend. - M: no-leak suite now also asserts masked-prefix/suffix canaries and covers config test + representative API command classes (channels list text+json, messages history). - m: init --bot-token-from-env / --user-token-from-env fail closed on an empty/unset env var instead of silently reporting setup success. [INT-437] --- internal/cmd/initcmd/init.go | 15 ++++++++-- internal/keychain/keychain_test.go | 2 +- internal/keychain/migrate.go | 31 ++++++++++---------- internal/noleak/noleak_test.go | 45 ++++++++++++++++++++++++++++-- internal/testutil/testutil.go | 4 +++ 5 files changed, 78 insertions(+), 19 deletions(-) diff --git a/internal/cmd/initcmd/init.go b/internal/cmd/initcmd/init.go index d47b924..26baec8 100644 --- a/internal/cmd/initcmd/init.go +++ b/internal/cmd/initcmd/init.go @@ -188,7 +188,14 @@ func (o *initOptions) verify(token, label string) (*client.AuthTestResponse, err func (o *initOptions) resolveBot() (string, error) { switch { case o.botEnv != "": - return os.Getenv(o.botEnv), nil + // Explicit env ingress must fail closed: an unset/empty var is an + // error, not a silent "no bot token" that reports setup success + // while storing nothing. + v := os.Getenv(o.botEnv) + if v == "" { + return "", fmt.Errorf("--bot-token-from-env %s is empty or unset", o.botEnv) + } + return v, nil case o.botStdin: b, err := io.ReadAll(o.reader()) if err != nil { @@ -202,7 +209,11 @@ func (o *initOptions) resolveBot() (string, error) { func (o *initOptions) resolveUser() (string, error) { if o.userEnv != "" { - return os.Getenv(o.userEnv), nil + v := os.Getenv(o.userEnv) + if v == "" { + return "", fmt.Errorf("--user-token-from-env %s is empty or unset", o.userEnv) + } + return v, nil } if !o.interactive() { return "", nil // non-interactive run only sets what was supplied diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go index ecb4950..df2c0a9 100644 --- a/internal/keychain/keychain_test.go +++ b/internal/keychain/keychain_test.go @@ -243,7 +243,7 @@ func TestMigrateEqualValueCleansUpSilently(t *testing.T) { func TestDiscoverFileBranch(t *testing.T) { testutil.Setup(t) writeLegacyFile(t, map[string]string{"api_token": botTok, "ignored_key": "x"}) - got := discover("slack-chat-api", credstore.BackendFile) + got := discover("slack-chat-api") var sawBot bool for _, c := range got { if c.newKey == KeyBotToken { diff --git a/internal/keychain/migrate.go b/internal/keychain/migrate.go index fd2a6fd..fd62449 100644 --- a/internal/keychain/migrate.go +++ b/internal/keychain/migrate.go @@ -48,8 +48,7 @@ type candidate struct { // a legacy value over an existing keyring entry; it cannot resolve a // legacy-vs-legacy disagreement — the user must still pick. func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error { - backend, _ := s.cs.Backend() - cands := discover(s.service, backend) + cands := discover(s.service) if len(cands) == 0 { return nil // nothing legacy on disk/keychain — the steady state } @@ -182,20 +181,24 @@ func conflict(s *Store, group []candidate, target string, hasTarget bool) error return credstore.MigrationConflictError("slck", group[0].legacyField, strings.Join(locs, ", "), s.ref) } -// discover enumerates every legacy source that currently exists. macOS -// keychain reads are migration-only and darwin-only (the sole sanctioned -// `security` shell-out) — and only when the *selected* backend is the OS -// Keychain. If the user forced the file/memory backend (config/env/Options), -// they have opted out of the OS keychain entirely, so probing it for legacy -// items would be wrong; this also keeps tests (which force the file backend -// in a temp HOME) hermetic and free of `security` shell-outs. The plaintext -// file path is the released layout — $XDG_CONFIG_HOME/slack-chat-api/ -// credentials else ~/.config/... — on Linux AND Windows alike (the legacy -// code has no %APPDATA% branch). -func discover(service string, backend credstore.Backend) []candidate { +// legacyKeychainScanDisabledEnv is a test-only seam: when set, discover() +// skips the darwin `security` shell-out so the test suite is hermetic and +// never touches the real login Keychain. Production never sets it — legacy +// Keychain discovery must run regardless of the destination backend +// (§2.4: a macOS user who opts into keyring.backend:file must still have +// old `slck`/`slack-chat-api` Keychain items migrated). +const legacyKeychainScanDisabledEnv = "SLCK_TEST_DISABLE_LEGACY_KEYCHAIN_SCAN" + +// discover enumerates every legacy source that currently exists, +// independent of the destination backend (§2.4). macOS Keychain reads are +// migration-only and darwin-only (the sole sanctioned `security` +// shell-out). The plaintext file path is the released layout — +// $XDG_CONFIG_HOME/slack-chat-api/credentials else ~/.config/... — on Linux +// AND Windows alike (the legacy code has no %APPDATA% branch). +func discover(service string) []candidate { var out []candidate - if runtime.GOOS == "darwin" && backend == credstore.BackendKeychain { + if runtime.GOOS == "darwin" && os.Getenv(legacyKeychainScanDisabledEnv) == "" { for _, svc := range legacyKeychainServices { for field, newKey := range legacyFields { svc, field, newKey := svc, field, newKey diff --git a/internal/noleak/noleak_test.go b/internal/noleak/noleak_test.go index 5a3aa08..c1a574b 100644 --- a/internal/noleak/noleak_test.go +++ b/internal/noleak/noleak_test.go @@ -13,12 +13,15 @@ import ( "strings" "testing" + "github.com/spf13/cobra" "github.com/stretchr/testify/require" "github.com/open-cli-collective/cli-common/credstore" + "github.com/open-cli-collective/slack-chat-api/internal/cmd/channels" cfgcmd "github.com/open-cli-collective/slack-chat-api/internal/cmd/config" initcmd "github.com/open-cli-collective/slack-chat-api/internal/cmd/initcmd" + "github.com/open-cli-collective/slack-chat-api/internal/cmd/messages" "github.com/open-cli-collective/slack-chat-api/internal/cmd/setcred" "github.com/open-cli-collective/slack-chat-api/internal/cmd/whoami" "github.com/open-cli-collective/slack-chat-api/internal/keychain" @@ -28,6 +31,11 @@ import ( const secret = "xoxb-NOLEAK-canary-7f3a9c2e1d8b4a6f0011" +// canaries: the full secret AND distinctive masked-prefix / suffix slices, +// so a "first 8 + last 4" style display (§1.11 item 3 — masked prefixes are +// still secret) is also caught, not just a verbatim dump. +var canaries = []string{secret, secret[:16], secret[len(secret)-8:]} + // captureAll redirects os.Stdout, os.Stderr and output.Writer, runs fn, and // returns everything written to any of them. func captureAll(t *testing.T, stdin string, fn func()) string { @@ -72,8 +80,8 @@ func seed(t *testing.T) { func assertNoLeak(t *testing.T, name, captured string) { t.Helper() - if leak := credstore.NoLeakAssertion([]byte(captured), secret); leak != nil { - t.Fatalf("%s leaked the secret (%v).\n--- captured ---\n%s", name, leak, captured) + if leak := credstore.NoLeakAssertion([]byte(captured), canaries...); leak != nil { + t.Fatalf("%s leaked the secret/prefix (%v).\n--- captured ---\n%s", name, leak, captured) } } @@ -134,3 +142,36 @@ func TestNoLeak_Whoami(t *testing.T) { // output channel echoes the seeded token. assertNoLeak(t, "whoami", captureAll(t, "", func() { _ = c.Execute() })) } + +func TestNoLeak_ConfigTest(t *testing.T) { + testutil.Setup(t) + seed(t) + c := cfgcmd.NewCmd() + c.SetArgs([]string{"test"}) + assertNoLeak(t, "config test", captureAll(t, "", func() { _ = c.Execute() })) +} + +// Representative API command classes: with the canary token in the keyring +// and no reachable Slack (hermetic), they take their error path. The token +// travels only in the Authorization header — assert it never reaches any +// output channel (incl. error/verbose output). +func TestNoLeak_APICommandClasses(t *testing.T) { + cases := []struct { + name string + cmd func() *cobra.Command + args []string + }{ + {"channels list", channels.NewCmd, []string{"list"}}, + {"channels list -o json", channels.NewCmd, []string{"list", "-o", "json"}}, + {"messages history", messages.NewCmd, []string{"history", "C123"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + testutil.Setup(t) + seed(t) + c := tc.cmd() + c.SetArgs(tc.args) + assertNoLeak(t, tc.name, captureAll(t, "", func() { _ = c.Execute() })) + }) + } +} diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go index ec76672..085f1f0 100644 --- a/internal/testutil/testutil.go +++ b/internal/testutil/testutil.go @@ -24,6 +24,10 @@ func Setup(t *testing.T) string { // Secret Service), with the passphrase supplied non-interactively. t.Setenv("SLACK_CHAT_API_KEYRING_BACKEND", "file") t.Setenv("SLACK_CHAT_API_KEYRING_PASSPHRASE", "test-passphrase") + // Neutralize the darwin legacy-Keychain `security` probe so the suite + // is hermetic. This is independent of the destination backend: the + // migration's discovery matrix (§2.4) otherwise always runs on macOS. + t.Setenv("SLCK_TEST_DISABLE_LEGACY_KEYCHAIN_SCAN", "1") // Belt-and-suspenders: a prior test's recorded §1.8 block must never // bleed into this one's JSON output. output.ResetMigration() From a045901dc0dd63a0bcef913d437108fd62a55f01 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sun, 17 May 2026 07:31:10 -0400 Subject: [PATCH 4/7] fix(credstore): address Codex re-review #3 (3 majors) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - no-leak suite: the JSON API path no longer passes vacuously. -o is a ROOT persistent flag; toggling output.JSON drives the real IsJSON() path instead, and API-class subtests now require a real (non-usage) execution error so a parser failure can't masquerade as a clean no-leak pass. Added root.Command() so the suite/B2/B3 can drive the real top-level command with persistent flags. - root_test.go is now hermetic (testutil.Setup → file backend in temp HOME; no real keyring / security shell-out) and asserts bot-vs-user resolution by the live error instead of a long-removed exact string. - purge remaining old credential guidance: snap/snapcraft.yaml caveat and integration-tests.md (setup, user-token, env-precedence, token-type, restore, error-table) — now teach init / set-credential --stdin and state env vars are not read at runtime. [INT-437] --- integration-tests.md | 50 ++++++------ internal/cmd/root/root.go | 7 ++ internal/cmd/root/root_test.go | 134 +++++++++------------------------ internal/noleak/noleak_test.go | 25 +++++- snap/snapcraft.yaml | 2 +- 5 files changed, 92 insertions(+), 126 deletions(-) diff --git a/integration-tests.md b/integration-tests.md index 08cf836..ef13a65 100644 --- a/integration-tests.md +++ b/integration-tests.md @@ -2,6 +2,13 @@ Manual integration tests for verifying slck against a live Slack workspace. Tests are organized from safe (read-only) to destructive, so you can stop at any section. +> **Credential model (§1.11/§1.12):** slck stores credentials in the OS +> keyring only. Ingress is `slck init` or `slck set-credential` (stdin / +> `--from-env`) — never a positional/flag value. `slck config set-token` is +> removed. `SLACK_API_TOKEN` / `SLACK_USER_TOKEN` are **not** read at +> runtime (only as `init --bot-token-from-env` setup ingress). Scenarios +> below use the new commands. + --- ## Part 1: Setup @@ -48,18 +55,16 @@ Also add this **User Token Scope** (for Part 3B: Search Tests): # Build the CLI make build -# Set up Bot Token (required for most commands) -# Copy the "Bot User OAuth Token" (starts with xoxb-) -./bin/slck config set-token -# Paste your xoxb-... token when prompted +# Set up Bot + User tokens (guided; input is not echoed) +# Copy the "Bot User OAuth Token" (xoxb-) and "User OAuth Token" (xoxp-) +./bin/slck init +# Or non-interactively, per secret, from a pipe: +# printf '%s' "$XOXB" | ./bin/slck set-credential --key bot_token --stdin +# printf '%s' "$XOXP" | ./bin/slck set-credential --key user_token --stdin # Verify bot token works ./bin/slck workspace info -# Set up User Token (required for search tests in Part 3B) -# Copy the "User OAuth Token" (starts with xoxp-) -./bin/slck config set-token xoxp-your-user-token - # Verify both tokens are configured ./bin/slck config show ./bin/slck config test @@ -234,10 +239,8 @@ Search requires a **user token** (`xoxp-*`), not a bot token. These tests verify ```bash # Get your user token from Slack app settings: # OAuth & Permissions → User OAuth Token (starts with xoxp-) - slck config set-token xoxp-your-user-token - - # Or use environment variable: - export SLACK_USER_TOKEN=xoxp-your-user-token + printf '%s' "$XOXP" | slck set-credential --key user_token --stdin + # (or run `slck init` for the guided flow) ``` 2. Verify both tokens are configured: @@ -312,10 +315,9 @@ First, create a message with a unique identifier that we can search for: | Step | Command | Expected | |------|---------|----------| -| 1 | `unset SLACK_USER_TOKEN` | Clear env var | -| 2 | `slck config delete-token --type user --force` | Delete stored user token | -| 3 | `slck search messages "test"` | Error mentioning user token requirement | -| 4 | `slck config set-token xoxp-your-user-token` | Re-configure user token | +| 1 | `slck config delete-token --type user --force` | Delete stored user token | +| 2 | `slck search messages "test"` | Error mentioning user token requirement | +| 3 | `printf '%s' "$XOXP" \| slck set-credential --key user_token --stdin` | Re-configure user token | ### 3B.9 User Search Tests @@ -477,9 +479,9 @@ Using **NEW_CHANNEL_ID** from step 5.1: | Step | Command | Expected | |------|---------|----------| -| 1 | `slck config set-token xoxb-test-fake-token` | "Bot token stored" | -| 2 | `slck config set-token xoxp-test-fake-token` | "User token stored" | -| 3 | `slck config set-token invalid-token` | Error: unrecognized token format | +| 1 | `printf '%s' xoxb-test-fake-token \| slck set-credential --key bot_token --stdin` | "Stored bot_token" | +| 2 | `printf '%s' xoxp-test-fake-token \| slck set-credential --key user_token --stdin` | "Stored user_token" | +| 3 | `slck config set-token xoxb-x` | Error: removed; points to `slck set-credential` (nonzero exit) | ### 6.4 Selective Token Deletion @@ -496,9 +498,9 @@ Using **NEW_CHANNEL_ID** from step 5.1: | Step | Command | Expected | |------|---------|----------| -| 1 | `slck config set-token` | Prompts for token, stores it (bot) | -| 2 | `slck config set-token xoxp-your-user-token` | Stores user token | -| 3 | `slck config show` | Both tokens configured | +| 1 | `slck init` | Prompts for bot (and optional user) token, stores them | +| 2 | `printf '%s' "$XOXP" \| slck set-credential --key user_token --stdin` | Stores user token | +| 3 | `slck config show` | Both tokens present (values never shown) | | 4 | `slck config test` | Both tokens valid | ### 6.6 Delete All Tokens @@ -528,7 +530,7 @@ These can be run at any time to verify error handling. | Step | Command | Expected | |------|---------|----------| -| 1 | `SLACK_API_TOKEN=invalid slck workspace info` | Error: `invalid_auth` | +| 1 | `printf '%s' invalid \| slck set-credential --key bot_token --stdin` then `slck workspace info` | Error: `invalid_auth` (env vars are not read at runtime) | | 2 | (Use token without `channels:manage`) `slck channels create test` | Error describing missing scope | ### 7.3 Edge Cases @@ -637,7 +639,7 @@ Using **CANVAS_ID₁** from step 10.1: | Error | Cause | Solution | |-------|-------|----------| -| `invalid_auth` | Token invalid or expired | Regenerate token, run `config set-token` | +| `invalid_auth` | Token invalid or expired | Regenerate token, run `slck init` (or `slck set-credential`) | | `not_in_channel` | Bot not in channel | `/invite @botname` in Slack | | `channel_not_found` | Wrong ID or no access | Verify ID, check bot permissions | | `missing_scope` | Token lacks required scope | Add scope in Slack app settings, reinstall | diff --git a/internal/cmd/root/root.go b/internal/cmd/root/root.go index 4b182e4..bfbe777 100644 --- a/internal/cmd/root/root.go +++ b/internal/cmd/root/root.go @@ -65,6 +65,13 @@ read at runtime (they are accepted only as ingress during setup).`, }, } +// Command returns the fully-configured root command (persistent --output +// flag, PersistentPreRunE, all subcommands). Exposed so tests — notably the +// §1.12 no-leak suite — can drive the real top-level command exactly as a +// user would, instead of constructing a subcommand without root's +// persistent flags (which would error out before the command ever runs). +func Command() *cobra.Command { return rootCmd } + // Execute runs the root command func Execute() { if err := rootCmd.Execute(); err != nil { diff --git a/internal/cmd/root/root_test.go b/internal/cmd/root/root_test.go index 1a4e15d..991ed74 100644 --- a/internal/cmd/root/root_test.go +++ b/internal/cmd/root/root_test.go @@ -1,131 +1,71 @@ package root import ( + "strings" "testing" "github.com/open-cli-collective/slack-chat-api/internal/client" + "github.com/open-cli-collective/slack-chat-api/internal/testutil" ) func TestAsUserAndAsBotMutualExclusivity(t *testing.T) { - // Save original flag values - originalAsUser := asUser - originalAsBot := asBot + originalAsUser, originalAsBot := asUser, asBot defer func() { - asUser = originalAsUser - asBot = originalAsBot + asUser, asBot = originalAsUser, originalAsBot client.ResetTokenMode() }() - // Test: both flags set should return error - asUser = true - asBot = true - + asUser, asBot = true, true err := rootCmd.PersistentPreRunE(rootCmd, []string{}) if err == nil { - t.Error("Expected error when both --as-user and --as-bot are set, got nil") + t.Fatal("expected error when both --as-user and --as-bot are set") } - if err != nil && err.Error() != "cannot use both --as-user and --as-bot flags together" { - t.Errorf("Unexpected error message: %s", err.Error()) + if err.Error() != "cannot use both --as-user and --as-bot flags together" { + t.Errorf("unexpected error message: %s", err.Error()) } } -func TestAsUserFlagSetsClientMode(t *testing.T) { - // Save original flag values - originalAsUser := asUser - originalAsBot := asBot - defer func() { - asUser = originalAsUser - asBot = originalAsBot +// resolveErr runs PersistentPreRunE with the given flag state, then +// client.New() against a hermetic empty keyring, returning the resulting +// error text. testutil.Setup forces the file backend in a temp HOME so this +// never touches the real OS keyring (§1.12 hermeticity). +func resolveErr(t *testing.T, user, bot bool) string { + t.Helper() + testutil.Setup(t) + origU, origB := asUser, asBot + t.Cleanup(func() { + asUser, asBot = origU, origB client.ResetTokenMode() - }() - - // Test: --as-user flag should set client to user mode - asUser = true - asBot = false + }) + asUser, asBot = user, bot client.ResetTokenMode() - - err := rootCmd.PersistentPreRunE(rootCmd, []string{}) - if err != nil { - t.Errorf("Unexpected error: %v", err) + if err := rootCmd.PersistentPreRunE(rootCmd, []string{}); err != nil { + t.Fatalf("PersistentPreRunE: %v", err) } - - // Verify by checking that New() would try to use user token - // We can't directly inspect the internal state, but we can verify - // the behavior by attempting to create a client and checking the error - _, err = client.New() + _, err := client.New() if err == nil { - // If no error, client was created (token exists) - return - } - // Error should mention user token since that's what we're trying to use - if err.Error() == "user token not found: set SLACK_USER_TOKEN or run: slck config set-user-token" { - // This confirms --as-user flag correctly set the client mode - return + t.Fatal("expected a missing-credential error against an empty keyring") } - // If we get here with a different error, that's also acceptable - // as long as it's not a bot token error when we expected user mode + return err.Error() } -func TestAsBotFlagSetsClientMode(t *testing.T) { - // Save original flag values - originalAsUser := asUser - originalAsBot := asBot - defer func() { - asUser = originalAsUser - asBot = originalAsBot - client.ResetTokenMode() - }() - - // Test: --as-bot flag should set client to bot mode - asUser = false - asBot = true - client.ResetTokenMode() - - err := rootCmd.PersistentPreRunE(rootCmd, []string{}) - if err != nil { - t.Errorf("Unexpected error: %v", err) +func TestAsUserFlagSelectsUserToken(t *testing.T) { + msg := resolveErr(t, true, false) + if !strings.Contains(msg, "user token") { + t.Errorf("--as-user should resolve the user token; got %q", msg) } +} - // Verify by checking that New() would try to use bot token - _, err = client.New() - if err == nil { - // If no error, client was created (token exists) - return - } - // Error should NOT mention user token since we're using bot mode - if err.Error() == "user token not found: set SLACK_USER_TOKEN or run: slck config set-user-token" { - t.Error("Expected bot token mode but got user token error") +func TestAsBotFlagSelectsBotToken(t *testing.T) { + msg := resolveErr(t, false, true) + if !strings.Contains(msg, "bot token") || strings.Contains(msg, "user token") { + t.Errorf("--as-bot should resolve the bot token; got %q", msg) } } func TestNoFlagsDefaultsBotMode(t *testing.T) { - // Save original flag values - originalAsUser := asUser - originalAsBot := asBot - defer func() { - asUser = originalAsUser - asBot = originalAsBot - client.ResetTokenMode() - }() - - // Test: no flags should default to bot mode - asUser = false - asBot = false - client.ResetTokenMode() - - err := rootCmd.PersistentPreRunE(rootCmd, []string{}) - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - - // With no flags set, client.New() should try bot token by default - _, err = client.New() - if err == nil { - // If no error, client was created (token exists) - return - } - // Error should NOT mention user token since default is bot mode - if err.Error() == "user token not found: set SLACK_USER_TOKEN or run: slck config set-user-token" { - t.Error("Expected bot token mode by default but got user token error") + msg := resolveErr(t, false, false) + if !strings.Contains(msg, "bot token") || strings.Contains(msg, "user token") { + t.Errorf("default should resolve the bot token; got %q", msg) } } diff --git a/internal/noleak/noleak_test.go b/internal/noleak/noleak_test.go index c1a574b..1a5317d 100644 --- a/internal/noleak/noleak_test.go +++ b/internal/noleak/noleak_test.go @@ -160,18 +160,35 @@ func TestNoLeak_APICommandClasses(t *testing.T) { name string cmd func() *cobra.Command args []string + json bool }{ - {"channels list", channels.NewCmd, []string{"list"}}, - {"channels list -o json", channels.NewCmd, []string{"list", "-o", "json"}}, - {"messages history", messages.NewCmd, []string{"history", "C123"}}, + {"channels list", channels.NewCmd, []string{"list"}, false}, + // JSON mode is toggled via output.JSON (the real IsJSON() path), + // NOT a `-o json` arg: -o is a ROOT persistent flag, so passing it + // to a bare subcommand errors during parsing and would make this a + // vacuous no-leak pass. + {"channels list (json)", channels.NewCmd, []string{"list"}, true}, + {"messages history", messages.NewCmd, []string{"history", "C123"}, false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { testutil.Setup(t) seed(t) + if tc.json { + output.JSON = true + t.Cleanup(func() { output.JSON = false }) + } c := tc.cmd() c.SetArgs(tc.args) - assertNoLeak(t, tc.name, captureAll(t, "", func() { _ = c.Execute() })) + var execErr error + out := captureAll(t, "", func() { execErr = c.Execute() }) + // In the hermetic env these must actually run and fail at the + // API/auth boundary — a nil error or a cobra usage/flag error + // would mean the command never executed (vacuous pass). + require.Error(t, execErr, "%s should fail at the API boundary, not pass vacuously", tc.name) + require.NotContains(t, execErr.Error(), "unknown flag") + require.NotContains(t, execErr.Error(), "unknown command") + assertNoLeak(t, tc.name, out) }) } } diff --git a/snap/snapcraft.yaml b/snap/snapcraft.yaml index a66a8a2..9cc4d98 100644 --- a/snap/snapcraft.yaml +++ b/snap/snapcraft.yaml @@ -12,7 +12,7 @@ description: | - Message reactions and threads - JSON output for scripting - Run 'slck config set-token' to configure your Slack token. + Run 'slck init' to configure your Slack credentials (stored in the OS keyring). grade: stable confinement: strict From 1de0f57c6c13f819b45a4444041438ed04887ca4 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sun, 17 May 2026 07:33:50 -0400 Subject: [PATCH 5/7] =?UTF-8?q?docs:=20integration-tests=20config=20show?= =?UTF-8?q?=20reflects=20presence-only=20output=20(=C2=A71.11)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [INT-437] --- integration-tests.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration-tests.md b/integration-tests.md index ef13a65..757b41f 100644 --- a/integration-tests.md +++ b/integration-tests.md @@ -467,7 +467,7 @@ Using **NEW_CHANNEL_ID** from step 5.1: | Step | Command | Expected | |------|---------|----------| -| 1 | `slck config show` | Shows both Bot Token and User Token (masked) with storage location | +| 1 | `slck config show` | `bot_token: present`, `user_token: present`, plus backend + ref (NO token characters, not even masked) | ### 6.2 Config Test (Dual Token) @@ -488,7 +488,7 @@ Using **NEW_CHANNEL_ID** from step 5.1: | Step | Command | Expected | |------|---------|----------| | 1 | `slck config delete-token --type bot --force` | "Bot token deleted" | -| 2 | `slck config show` | Bot Token: Not configured, User Token: still present | +| 2 | `slck config show` | `bot_token: not configured`, `user_token: present` | | 3 | `slck workspace info` | Error: no bot token configured | | 4 | `slck search messages "test"` | Still works (uses user token) | | 5 | `slck config delete-token --type user --force` | "User token deleted" | From 49aef89c82d673a7cb9ae9373ac0263dfa98965d Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sun, 17 May 2026 07:40:52 -0400 Subject: [PATCH 6/7] test(credstore): close TDD-flagged coverage gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract the pure §1.8 resolver planMigration() out of migrateLegacyOverwrite (no I/O; injected current-value lookup) so every branch is unit-testable. TestPlanMigration now covers the legacy-vs-legacy disagreement branch (asserting --overwrite still cannot resolve it, and the error names all sources and leaks no value), plus legacy-vs-target / equal-target / identical-duplicate paths. - TestRuntimeEnvNotReadAsCredential locks the headline §1.11 invariant: SLACK_API_TOKEN/SLACK_USER_TOKEN set, empty keyring -> reads still fail missing (env never consulted at runtime). [INT-437] --- internal/keychain/keychain_test.go | 92 ++++++++++++++++++ internal/keychain/migrate.go | 149 ++++++++++++++++------------- 2 files changed, 176 insertions(+), 65 deletions(-) diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go index df2c0a9..5e1dd30 100644 --- a/internal/keychain/keychain_test.go +++ b/internal/keychain/keychain_test.go @@ -19,6 +19,73 @@ const ( userTok = "xoxp-2222-distinctive-user-secret" ) +// TestPlanMigration exercises the pure §1.8 resolver directly — including +// the legacy-vs-legacy disagreement branch, which the file/keychain +// plumbing can't reach (a key=value file yields one value per key and the +// darwin Keychain scan is disabled in tests). +func TestPlanMigration(t *testing.T) { + cand := func(val, loc string) candidate { + return candidate{newKey: KeyBotToken, legacyField: "api_token", value: val, + location: loc, deleter: func() error { return nil }} + } + none := func(string) (string, bool) { return "", false } + has := func(v string) func(string) (string, bool) { + return func(string) (string, bool) { return v, true } + } + + t.Run("legacy-vs-legacy disagreement is always a conflict", func(t *testing.T) { + for _, ow := range []bool{false, true} { // overwrite cannot resolve it + _, err := planMigration("slack-chat-api", "default", "slack-chat-api/default", + []candidate{cand("xoxb-AAA", "keychain:slck/api_token"), + cand("xoxb-BBB", "file:/p/credentials#api_token")}, none, ow) + if !errors.Is(err, credstore.ErrMigrationConflict) { + t.Fatalf("overwrite=%v: want ErrMigrationConflict, got %v", ow, err) + } + if leak := credstore.NoLeakAssertion([]byte(err.Error()), + "xoxb-AAA", "xoxb-BBB"); leak != nil { + t.Fatalf("conflict leaked a value: %v", leak) + } + if !strings.Contains(err.Error(), "keychain:slck/api_token") || + !strings.Contains(err.Error(), "file:/p/credentials#api_token") { + t.Fatalf("conflict must name all sources: %q", err.Error()) + } + } + }) + + t.Run("legacy-vs-target differs, no overwrite -> conflict", func(t *testing.T) { + _, err := planMigration("svc", "default", "svc/default", + []candidate{cand("xoxb-LEGACY", "file:x#api_token")}, has("xoxb-TARGET"), false) + if !errors.Is(err, credstore.ErrMigrationConflict) { + t.Fatalf("want conflict, got %v", err) + } + }) + + t.Run("legacy-vs-target differs, overwrite -> write", func(t *testing.T) { + p, err := planMigration("svc", "default", "svc/default", + []candidate{cand("xoxb-LEGACY", "file:x#api_token")}, has("xoxb-TARGET"), true) + if err != nil || p.writes[KeyBotToken] != "xoxb-LEGACY" || len(p.changes) != 1 { + t.Fatalf("overwrite should force legacy: plan=%+v err=%v", p, err) + } + }) + + t.Run("equal target -> cleanup only, no write/signal", func(t *testing.T) { + p, err := planMigration("svc", "default", "svc/default", + []candidate{cand("xoxb-SAME", "file:x#api_token")}, has("xoxb-SAME"), false) + if err != nil || len(p.writes) != 0 || len(p.changes) != 0 || len(p.cleanups) != 1 { + t.Fatalf("equal value should clean up silently: plan=%+v err=%v", p, err) + } + }) + + t.Run("identical duplicate legacy values -> single write", func(t *testing.T) { + p, err := planMigration("svc", "default", "svc/default", + []candidate{cand("xoxb-DUP", "keychain:slck/api_token"), + cand("xoxb-DUP", "file:x#api_token")}, none, false) + if err != nil || p.writes[KeyBotToken] != "xoxb-DUP" || len(p.cleanups) != 2 { + t.Fatalf("byte-identical dup should migrate once: plan=%+v err=%v", p, err) + } + }) +} + func TestDetectTokenType(t *testing.T) { cases := map[string]string{ "xoxb-abc": "bot", "xoxp-abc": "user", "xoxc-abc": "unknown", "": "unknown", @@ -30,6 +97,31 @@ func TestDetectTokenType(t *testing.T) { } } +// TestRuntimeEnvNotReadAsCredential locks the headline §1.11 invariant: +// SLACK_API_TOKEN / SLACK_USER_TOKEN must NOT be consulted at runtime, even +// when set. With an empty keyring, reads must still fail-missing. +func TestRuntimeEnvNotReadAsCredential(t *testing.T) { + testutil.Setup(t) + t.Setenv("SLACK_API_TOKEN", "xoxb-env-must-be-ignored") + t.Setenv("SLACK_USER_TOKEN", "xoxp-env-must-be-ignored") + + st, err := Open() + if err != nil { + t.Fatalf("Open: %v", err) + } + defer func() { _ = st.Close() }() + + if _, err := st.BotToken(); !errors.Is(err, ErrMissingBotToken) { + t.Fatalf("SLACK_API_TOKEN was consulted at runtime (err=%v)", err) + } + if _, err := st.UserToken(); !errors.Is(err, ErrMissingUserToken) { + t.Fatalf("SLACK_USER_TOKEN was consulted at runtime (err=%v)", err) + } + if st.HasBotToken() || st.HasUserToken() { + t.Fatal("Has*Token true purely from env — §1.11 violation") + } +} + func TestStoreRoundTripAndClear(t *testing.T) { testutil.Setup(t) st, err := Open() diff --git a/internal/keychain/migrate.go b/internal/keychain/migrate.go index fd62449..787da84 100644 --- a/internal/keychain/migrate.go +++ b/internal/keychain/migrate.go @@ -53,69 +53,20 @@ func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error return nil // nothing legacy on disk/keychain — the steady state } - // Group candidates by new key, in deterministic key order. - byKey := map[string][]candidate{} - for _, c := range cands { - byKey[c.newKey] = append(byKey[c.newKey], c) - } - keys := make([]string, 0, len(byKey)) - for k := range byKey { - keys = append(keys, k) - } - sort.Strings(keys) - - // Phase 1: resolve every key, detecting ALL conflicts before mutating. - writes := map[string]string{} // newKey -> value to SetBundle - changes := []credstore.MigrationChange{} // for the _migration block - humanField := map[string]string{} // newKey -> legacyField (stderr) - var cleanups []func() error // run only after a clean write - - for _, k := range keys { - group := byKey[k] - distinct := map[string]bool{} - for _, c := range group { - distinct[c.value] = true - } - - target, hasTarget := currentValue(s, k) - - switch { - case len(distinct) > 1: - // Legacy sources disagree among themselves: --overwrite cannot - // pick a winner here either (§1.8 — the user must). Always a - // conflict, regardless of overwrite or target presence. - return conflict(s, group, target, hasTarget) - case hasTarget && !overwrite && disagrees(distinct, target): - return conflict(s, group, target, hasTarget) - case hasTarget && !disagrees(distinct, target): - // Already migrated (values match): no write, just clean up the - // leftover originals from an interrupted prior run. No signal. - for _, c := range group { - cleanups = append(cleanups, c.deleter) - } - default: - // Resolvable migration: one value (or overwrite forcing one). - val := group[0].value - writes[k] = val - lf := group[0].legacyField - humanField[k] = lf - changes = append(changes, credstore.MigrationJSONEntry( - lf, group[0].location, - fmt.Sprintf("keyring:%s/%s/%s", s.service, s.profile, k))) - for _, c := range group { - cleanups = append(cleanups, c.deleter) - } - } + plan, err := planMigration(s.service, s.profile, s.ref, cands, + func(k string) (string, bool) { return currentValue(s, k) }, overwrite) + if err != nil { + return err } // Phase 2: write the new bundle (no overwrite needed when target absent; // WithOverwrite only when the caller forced it). - if len(writes) > 0 { + if len(plan.writes) > 0 { var opts []credstore.SetOpt if overwrite { opts = append(opts, credstore.WithOverwrite()) } - if _, err := s.cs.SetBundle(s.profile, writes, opts...); err != nil { + if _, err := s.cs.SetBundle(s.profile, plan.writes, opts...); err != nil { return fmt.Errorf("migrate to keyring %s: %w", s.ref, err) } } @@ -124,18 +75,20 @@ func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error // Record the _migration block ONLY on a JSON run — recording it on a // text run would leave a stale block that a later JSON command in the // same (or test) process could splice into an unrelated response. - if len(changes) > 0 { + if len(plan.changes) > 0 { if output.IsJSON() { - output.RecordMigration(credstore.NewMigrationBlock(changes...)) + output.RecordMigration(credstore.NewMigrationBlock(plan.changes...)) } else { - for _, k := range keys { - if lf, ok := humanField[k]; ok { + for _, k := range plan.keys { + if lf, ok := plan.humanField[k]; ok { credstore.EmitMigrationStderr(lf, s.ref) } } } } + cleanups := plan.cleanups + // Phase 4: delete the legacy originals, then persist credential_ref so // the migration is not re-attempted (§1.8 "adding credential_ref"). for _, del := range cleanups { @@ -149,6 +102,72 @@ func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error return nil } +// migrationPlan is the pure result of resolving discovered candidates: what +// to write, the §1.8 signal, and the legacy originals to delete on success. +type migrationPlan struct { + writes map[string]string // newKey -> value to SetBundle + changes []credstore.MigrationChange // _migration block entries + humanField map[string]string // newKey -> legacyField (stderr) + cleanups []func() error // legacy deleters (post-write) + keys []string // sorted newKeys (deterministic) +} + +// planMigration is the pure §1.8 resolver: group candidates by new key and, +// detecting EVERY conflict before any mutation is proposed, decide writes / +// cleanups / signal. It performs no I/O — `current` injects the existing +// keyring value lookup — so all branches (legacy-vs-legacy disagreement, +// legacy-vs-target, idempotent equal, overwrite) are unit-testable with +// synthetic candidates. +func planMigration(service, profile, ref string, cands []candidate, + current func(key string) (string, bool), overwrite bool) (migrationPlan, error) { + + byKey := map[string][]candidate{} + for _, c := range cands { + byKey[c.newKey] = append(byKey[c.newKey], c) + } + p := migrationPlan{writes: map[string]string{}, humanField: map[string]string{}} + for k := range byKey { + p.keys = append(p.keys, k) + } + sort.Strings(p.keys) + + for _, k := range p.keys { + group := byKey[k] + distinct := map[string]bool{} + for _, c := range group { + distinct[c.value] = true + } + target, hasTarget := current(k) + + switch { + case len(distinct) > 1: + // Legacy sources disagree among themselves: --overwrite cannot + // pick a winner here either (§1.8 — the user must). Always a + // conflict, regardless of overwrite or target presence. + return migrationPlan{}, conflictErr(service, profile, ref, group, hasTarget) + case hasTarget && !overwrite && disagrees(distinct, target): + return migrationPlan{}, conflictErr(service, profile, ref, group, hasTarget) + case hasTarget && !disagrees(distinct, target): + // Already migrated (values match): no write, just clean up the + // leftover originals from an interrupted prior run. No signal. + for _, c := range group { + p.cleanups = append(p.cleanups, c.deleter) + } + default: + // Resolvable migration: one value (or overwrite forcing one). + p.writes[k] = group[0].value + p.humanField[k] = group[0].legacyField + p.changes = append(p.changes, credstore.MigrationJSONEntry( + group[0].legacyField, group[0].location, + fmt.Sprintf("keyring:%s/%s/%s", service, profile, k))) + for _, c := range group { + p.cleanups = append(p.cleanups, c.deleter) + } + } + } + return p, nil +} + // currentValue reports the existing credstore value for a key (post-Open). func currentValue(s *Store, key string) (string, bool) { v, err := s.cs.Get(s.profile, key) @@ -167,18 +186,18 @@ func disagrees(distinct map[string]bool, target string) bool { return false } -// conflict builds the §1.8 error: names every legacy source and the keyring -// target, never a value (masked or not). Reports the legacy field name. -func conflict(s *Store, group []candidate, target string, hasTarget bool) error { +// conflictErr builds the §1.8 error: names every legacy source and the +// keyring target, never a value (masked or not). Reports the legacy field +// name. Pure — no *Store — so it is testable in isolation. +func conflictErr(service, profile, ref string, group []candidate, hasTarget bool) error { locs := make([]string, 0, len(group)+1) for _, c := range group { locs = append(locs, c.location) } if hasTarget { - _ = target // value intentionally unused — never printed - locs = append(locs, fmt.Sprintf("keyring:%s/%s/%s", s.service, s.profile, group[0].newKey)) + locs = append(locs, fmt.Sprintf("keyring:%s/%s/%s", service, profile, group[0].newKey)) } - return credstore.MigrationConflictError("slck", group[0].legacyField, strings.Join(locs, ", "), s.ref) + return credstore.MigrationConflictError("slck", group[0].legacyField, strings.Join(locs, ", "), ref) } // legacyKeychainScanDisabledEnv is a test-only seam: when set, discover() From ea11ef513d607cedf1ac40729be508f432bb1b7c Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sun, 17 May 2026 07:56:09 -0400 Subject: [PATCH 7/7] fix(credstore): address pr-review-daemon findings (remediation escape hatches, ingress) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - delete-token / config show now use OpenNoMigrate so they remain usable during an unresolved §1.8 conflict (same remediation role as config clear); config show is the §1.11 item 3 diagnostic and must not be blocked - OpenRef (set-credential --ref) no longer runs migration: the one-time §1.8 migration only ever targets the canonical configured ref, never an arbitrary --ref (which would discover the default ref's legacy data) - set-credential --from-env now names the empty/unset variable (parity with init resolveBot/resolveUser); never echoes a value (§1.12) - init: non-interactive ingress emits a stderr notice before overwriting an existing credential so an accidental CI re-run is visible - document why workspace stores info.TeamID (stable id) not info.Team - add end-to-end init --overwrite tests: migration conflict fails loud without leaking, and --overwrite forces the legacy value [INT-437] --- internal/cmd/config/delete_token.go | 6 ++- internal/cmd/config/show.go | 6 ++- internal/cmd/initcmd/init.go | 10 +++++ internal/cmd/initcmd/init_test.go | 66 +++++++++++++++++++++++++++++ internal/cmd/setcred/setcred.go | 9 +++- internal/keychain/keychain.go | 9 +++- 6 files changed, 101 insertions(+), 5 deletions(-) diff --git a/internal/cmd/config/delete_token.go b/internal/cmd/config/delete_token.go index 03abdc8..b5e8c5e 100644 --- a/internal/cmd/config/delete_token.go +++ b/internal/cmd/config/delete_token.go @@ -47,7 +47,11 @@ func runDeleteToken(opts *deleteTokenOptions) error { return fmt.Errorf("invalid token type: %s (must be bot, user, or all)", opts.tokenType) } - st, err := keychain.Open() + // OpenNoMigrate (not Open): delete-token is a §1.8 remediation path — + // "delete the conflicting key, then re-run". Running the one-time + // migration first would surface ErrMigrationConflict and block the very + // command meant to resolve it (same reasoning as config clear). + st, err := keychain.OpenNoMigrate() if err != nil { return err } diff --git a/internal/cmd/config/show.go b/internal/cmd/config/show.go index 336d7a0..6e8430a 100644 --- a/internal/cmd/config/show.go +++ b/internal/cmd/config/show.go @@ -43,7 +43,11 @@ func runShow(_ *showOptions) error { if err != nil { return err } - st, err := keychain.Open() + // OpenNoMigrate (not Open): config show is the §1.11 item 3 diagnostic. + // It must remain usable during an unresolved §1.8 conflict so the user + // can see which keys are where before remediating — running migration + // first would fail it with ErrMigrationConflict and hide that state. + st, err := keychain.OpenNoMigrate() if err != nil { return err } diff --git a/internal/cmd/initcmd/init.go b/internal/cmd/initcmd/init.go index 26baec8..f954382 100644 --- a/internal/cmd/initcmd/init.go +++ b/internal/cmd/initcmd/init.go @@ -94,6 +94,12 @@ func runInit(opts *initOptions) error { output.Println("Setup cancelled.") return nil } + } else if (st.HasBotToken() || st.HasUserToken()) && !opts.interactive() { + // Non-interactive ingress (env/stdin) cannot prompt, so an + // accidental CI re-run of `slck init` would silently replace a + // stored credential. Emit a stderr notice first — the ref is not + // secret and the value is never printed (§1.12). + fmt.Fprintf(os.Stderr, "Notice: overwriting existing credential(s) at %s\n", st.Ref()) } botToken, err := opts.resolveBot() @@ -109,6 +115,10 @@ func runInit(opts *initOptions) error { return nil } + // workspace stores info.TeamID — the stable machine identifier (e.g. + // T04AB1234), not the human-readable info.Team display name shown by + // verify(). The ID is durable across workspace renames; config show + // reports it as a non-secret correlation field, not for display polish. workspace := "" if botToken != "" { diff --git a/internal/cmd/initcmd/init_test.go b/internal/cmd/initcmd/init_test.go index dcd914b..2ad200c 100644 --- a/internal/cmd/initcmd/init_test.go +++ b/internal/cmd/initcmd/init_test.go @@ -2,19 +2,40 @@ package initcmd import ( "encoding/json" + "errors" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/open-cli-collective/cli-common/credstore" + "github.com/open-cli-collective/slack-chat-api/internal/client" "github.com/open-cli-collective/slack-chat-api/internal/keychain" "github.com/open-cli-collective/slack-chat-api/internal/testutil" ) +// writeLegacyCreds writes the legacy plaintext credentials file at the path +// the one-time migration scans ($XDG_CONFIG_HOME/slack-chat-api/credentials, +// per testutil.Setup's isolated XDG). +func writeLegacyCreds(t *testing.T, kv map[string]string) string { + t.Helper() + dir := filepath.Join(os.Getenv("XDG_CONFIG_HOME"), "slack-chat-api") + require.NoError(t, os.MkdirAll(dir, 0o700)) + path := filepath.Join(dir, "credentials") + var b strings.Builder + for k, v := range kv { + b.WriteString(k + "=" + v + "\n") + } + require.NoError(t, os.WriteFile(path, []byte(b.String()), 0o600)) + return path +} + func newMockServer(t *testing.T) *httptest.Server { t.Helper() return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -119,6 +140,51 @@ func TestRunInit_Interactive_CancelOverwrite(t *testing.T) { require.NoError(t, err) } +// A legacy value that disagrees with an existing keyring value must, WITHOUT +// --overwrite, fail loud (§1.8) and never leak either secret — exercised +// through the init command's default Open path, not just the resolver. +func TestRunInit_MigrationConflict_FailsLoudWithoutLeaking(t *testing.T) { + testutil.Setup(t) + pre, err := keychain.Open() + require.NoError(t, err) + require.NoError(t, pre.SetBotToken("xoxb-OLD-keyring-value")) + _ = pre.Close() + writeLegacyCreds(t, map[string]string{"api_token": "xoxb-NEW-legacy-value"}) + + err = runInit(&initOptions{stdin: strings.NewReader("\nn\n"), noVerify: true}) + require.Error(t, err) + assert.True(t, errors.Is(err, credstore.ErrMigrationConflict), + "want ErrMigrationConflict, got %v", err) + assert.NotContains(t, err.Error(), "xoxb-OLD-keyring-value") + assert.NotContains(t, err.Error(), "xoxb-NEW-legacy-value") +} + +// `slck init --overwrite` is the only path that forces a legacy value over a +// conflicting keyring entry. Exercise the CLI flag → OpenForMigrationOverwrite +// wiring end to end (the resolver itself is unit-tested separately). +func TestRunInit_OverwriteResolvesMigrationConflict(t *testing.T) { + testutil.Setup(t) + pre, err := keychain.Open() + require.NoError(t, err) + require.NoError(t, pre.SetBotToken("xoxb-OLD-keyring-value")) + _ = pre.Close() + legacy := writeLegacyCreds(t, map[string]string{"api_token": "xoxb-NEW-legacy-value"}) + + // No tokens supplied: init resolves the conflict during Open, then exits + // on "no tokens provided" — the migration side effect is what we assert. + err = runInit(&initOptions{overwrite: true, stdin: strings.NewReader("\nn\n"), noVerify: true}) + require.NoError(t, err) + + st, err := keychain.OpenNoMigrate() + require.NoError(t, err) + defer func() { _ = st.Close() }() + v, err := st.BotToken() + require.NoError(t, err) + assert.Equal(t, "xoxb-NEW-legacy-value", v, "--overwrite must force the legacy value") + _, statErr := os.Stat(legacy) + assert.True(t, os.IsNotExist(statErr), "legacy file must be removed after forced migrate") +} + func TestRunInit_VerificationFailed(t *testing.T) { testutil.Setup(t) s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/cmd/setcred/setcred.go b/internal/cmd/setcred/setcred.go index 81890ec..16711de 100644 --- a/internal/cmd/setcred/setcred.go +++ b/internal/cmd/setcred/setcred.go @@ -90,7 +90,14 @@ func run(opts *options) error { func readValue(opts *options) (string, error) { if opts.fromEnv != "" { - return os.Getenv(opts.fromEnv), nil + v := os.Getenv(opts.fromEnv) + if v == "" { + // Name the variable (parity with init's resolveBot/resolveUser) + // so the user knows which env var to populate — never echo a + // value (§1.12); the var name is not secret. + return "", fmt.Errorf("--from-env %s is empty or unset", opts.fromEnv) + } + return v, nil } r := opts.in if r == nil { diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index c1e7fef..ab232e7 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -72,7 +72,12 @@ func open(overwrite, runMigration bool) (*Store, error) { // OpenRef opens a store against an explicit ref instead of config.yml's // credential_ref — used by `slck set-credential --ref` (§1.5.2 ingress). -// An empty ref falls back to the configured/default ref. +// An empty ref falls back to the configured/default ref. Migration does NOT +// run here: the one-time §1.8 migration only ever targets the canonical +// configured ref (running it against an arbitrary --ref would discover the +// default ref's legacy data and could write it under the wrong +// service/profile). set-credential is pure ingress; migration still runs on +// the next init / first API call via the default Open path. func OpenRef(ref string) (*Store, error) { cfg, err := config.Load() if err != nil { @@ -81,7 +86,7 @@ func OpenRef(ref string) (*Store, error) { if ref != "" { cfg.CredentialRef = ref } - return openWith(cfg, false, true) + return openWith(cfg, false, false) } // openWith is the seam unit tests drive with an injected config (e.g. a