Skip to content

feat(security): enforce 0600 config permissions and support DISCORD_TOKEN fallback - #38

Merged
DevRohit06 merged 3 commits into
DevRohit06:mainfrom
canoo:feat/secure-config-and-token-fallback
Sep 6, 2026
Merged

feat(security): enforce 0600 config permissions and support DISCORD_TOKEN fallback#38
DevRohit06 merged 3 commits into
DevRohit06:mainfrom
canoo:feat/secure-config-and-token-fallback

Conversation

@canoo

@canoo canoo commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Closes #37

Summary

Automatically secures ~/.discli/config.json by enforcing 0600 file permissions and 0700 directory permissions on POSIX systems (preventing world-readability on multi-user systems, VPS deployments, and AI agent sandboxes), and adds DISCORD_TOKEN as an environment variable fallback alongside DISCORD_BOT_TOKEN.

Changes

  • src/discli/config.py: Enforce 0700 on ~/.discli and 0600 on config.json during save_config(). Automatically tightens existing loose modes.
  • src/discli/cli.py: Configure @click.option("--token", envvar=["DISCORD_BOT_TOKEN", "DISCORD_TOKEN"]) for seamless token resolution.
  • src/discli/client.py, src/discli/commands/doctor.py, src/discli/commands/setup.py: Recognize DISCORD_TOKEN in diagnostic checks, setup wizard, and error reporting.
  • tests/test_config.py, tests/test_client.py, tests/test_setup.py: Comprehensive test coverage for permission enforcement and token resolution precedence.
  • Documentation: Updated README.md, configuration.mdx, token-resolution.mdx, and cli-commands.mdx.

Test plan

  • Ran uv run pytest tests/ (374 passed, 4 skipped)
  • Verified file mode is 0o600 and directory mode is 0o700 via stat on Linux
  • Tested fallback to DISCORD_TOKEN when DISCORD_BOT_TOKEN is unset
  • Tested precedence of DISCORD_BOT_TOKEN over DISCORD_TOKEN

@DevRohit06

Copy link
Copy Markdown
Owner

Thanks Emi, this is a good direction. Three things before I merge:

  1. In save_config, os.open ignores the mode when config.json already exists, so an old 0644 file gets the token written before the chmod tightens it. That's the exact upgrade case this fixes. os.fchmod before the write, or temp file + os.replace, closes it.

  2. The except OSError: pass falls through to write_text below, so a denied chmod writes the token with default perms and still prints success. Warn on stderr instead.

  3. DISCORD_TOKEN now outranks the config file. Anyone with it exported for another bot silently posts as the wrong one. Can we rank it below the config token? Open to your view.

Also: a few tests only delenv DISCORD_BOT_TOKEN and will hit the network with DISCORD_TOKEN set (test_wave3_search_and_schedule.py:458, test_wave2_visibility_and_permissions.py:266, test_setup.py:206). Clearing both in conftest's no_network fixture covers it. And token-resolution.mdx still shows the old single-envvar diagram and snippet below the table you updated.

Rest of my notes are follow-up material, not blockers.

- Tighten config file permissions with os.fchmod before write
- Warn on stderr if chmod operations fail instead of silently passing
- Rank DISCORD_TOKEN below ~/.discli/config.json as a generic fallback
- Clear both DISCORD_BOT_TOKEN and DISCORD_TOKEN in conftest's no_network fixture
- Update token resolution diagram, code snippets, and docs
@canoo

canoo commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review, Rohit! All points make total sense and are addressed in the latest commit:

  1. fchmod before writing: Updated save_config to invoke os.fchmod(fd, 0o600) immediately after opening the file descriptor and before writing any data. Upgrading an existing 0644 file now tightens permissions while the file is truncated to 0 bytes, before the token hits disk.
  2. Warn on denied permissions: Removed the silent except OSError: pass and now emit explicit warnings to sys.stderr if directory or file permission modifications fail.
  3. DISCORD_TOKEN ranking: Completely agree on the shadowing concern. Re-ranked DISCORD_TOKEN below the config file:
    • Priority 1: --token CLI flag
    • Priority 2: DISCORD_BOT_TOKEN environment variable
    • Priority 3: ~/.discli/config.json config file
    • Priority 4: DISCORD_TOKEN environment variable (generic fallback)
  4. Test isolation: Added monkeypatch.delenv("DISCORD_TOKEN", raising=False) alongside DISCORD_BOT_TOKEN in conftest.py's autouse no_network fixture to protect all tests from ambient tokens.
  5. Docs & diagrams: Updated the Mermaid flowchart, priority table, code snippets, and rationale in token-resolution.mdx (and synced configuration.mdx, cli-commands.mdx, and README.md).

All 381 tests pass cleanly, including when run with external DISCORD_TOKEN and DISCORD_BOT_TOKEN variables populated.

…en unprotected

Two problems in the new save_config() write path.

The recovery block called os.close(fd) after the `with` had already closed
it. The swallowed EBADF is the harmless case; the harmful one is a
concurrent thread claiming that number in between and losing its own file
to the stray close. Narrow the guard to fdopen() itself, which is the only
point where nothing else owns the descriptor.

The os.open() failure path fell back to path.write_text(), writing the
token with whatever the umask gave it and warning about none of it -- the
exact exposure this function exists to prevent, and silent, unlike every
other failure path here. os.open() and write_text() open with the same
flags, so the fallback could not succeed where the primary had failed
anyway. Let the error propagate.

Regression tests cover both, plus descriptor release when fdopen() fails.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@DevRohit06

Copy link
Copy Markdown
Owner

Thanks for this — the security intent is right and test_save_config_fchmod_before_write pins the genuinely subtle part (tightening the mode while the file is still empty after O_TRUNC, so the secret is never on disk at the looser mode). That's the assertion most implementations of this skip.

I pushed one commit (e2057f8) to fix two things in the save_config() write path before merging:

1. Double close of the descriptor. The recovery block called os.close(fd) after the with had already closed it. The swallowed EBADF is the harmless outcome; the harmful one is a concurrent thread claiming that fd number in between and losing its own file to the stray close — and serve.py runs threads. I verified this against your original code with a harness that adopts the fd the way io does: it recorded os.close() on fd 99 twice. The guard is now narrowed to fdopen() itself, which is the only point where nothing else owns the descriptor.

2. The os.open failure path wrote the token unprotected, silently. It fell back to path.write_text(), which writes with whatever the umask gives — the exact exposure this function exists to prevent — and warned about none of it, unlike every other failure path here. It was also effectively unreachable: write_text opens with the same O_WRONLY|O_CREAT|O_TRUNC, so it could not succeed where the primary had failed. The error now propagates.

Three regression tests cover both, plus descriptor release when fdopen() fails.

Two smaller things I did not touch, since they're outside the fix and worth your call — I'll open a follow-up:

  • docs/getting-started/configuration.mdx lost its lead-in sentence ("This writes to ~/.discli/config.json:") along with the Set token. output, so there's now a bare json block with nothing introducing it.
  • CLAUDE.md still lists only DISCORD_BOT_TOKEN, and its note that "examples/meeting_transcriber.py reads DISCORD_TOKEN, not DISCORD_BOT_TOKEN" changes meaning now that DISCORD_TOKEN is a supported fallback.

One note on the PR description: it says the option became envvar=["DISCORD_BOT_TOKEN", "DISCORD_TOKEN"], but cli.py:64 is unchanged and the fallback is a manual lookup after the config file. That's not cosmetic — the Click-list form would rank DISCORD_TOKEN above config.json, the opposite of what shipped. Your ordering is the better call and the rationale in token-resolution.mdx is convincing; it's just a deliberate deviation from what #37 proposed rather than the thing #37 asked for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security & Feature] Enforce 0600 permissions on config.json and support DISCORD_TOKEN fallback

2 participants