Skip to content

Sprint 1: Critical security and correctness fixes#12

Merged
SuaveIV merged 2 commits intomainfrom
sprint/critical-fixes
Mar 27, 2026
Merged

Sprint 1: Critical security and correctness fixes#12
SuaveIV merged 2 commits intomainfrom
sprint/critical-fixes

Conversation

@SuaveIV
Copy link
Copy Markdown
Owner

@SuaveIV SuaveIV commented Mar 27, 2026

Summary
This PR implements Sprint 1 of the implementation plan, addressing critical security and correctness issues.

Changes

  1. Config Validation at Startup (src/familybot/config.py)
    Added ConfigurationError exception class
    Added �alidate_config() function that checks:
    Discord API key is not empty
    Discord admin ID is a non-zero integer
    Steamworks API key is not empty
    ITAD API key is not empty
    Family Steam ID is a non-zero integer
    All channel IDs are non-zero integers
    Token save path is not empty
    Web UI port is valid (1-65535)
    Validation runs at import time, exits with clear error message on failure
  2. Remove API Key Exposure from URLs (4 files)
    API keys were being embedded directly in URL strings, risking exposure in logs, stack traces, and error messages. Fixed by using the \params=\ argument in HTTP clients.

Files changed:

\src/familybot/plugins/steam_family.py\ — Wishlist API call
\src/familybot/lib/plugin_admin_actions.py\ — Wishlist API call
\src/familybot/lib/admin_commands.py\ — Owned games and wishlist API calls, added \params\ parameter to \make_request_with_retry()
\src/familybot/plugins/common_game.py\ — Owned games API call
3. Task 1.3: Sync I/O Wrapping
Verified that all \get_lowest_price()\ call sites already use \�syncio.to_thread()\ — no changes needed.

Testing
Config validation will now catch missing/placeholder values at startup
API keys no longer appear in URL strings
All existing functionality preserved
Documentation
Added \doc/IMPLEMENTATION_PLAN.md\ with full sprint plan organized by file

- Add config validation at startup (config.py)
  - Validates all required API keys, channel IDs, and paths
  - Raises clear ConfigurationError on missing/placeholder values
  - Exits with error message instead of cryptic runtime failures

- Remove API key exposure from URLs (4 files)
  - steam_family.py: Use params= for wishlist API calls
  - plugin_admin_actions.py: Use params= for wishlist API calls
  - admin_commands.py: Use params= for owned games and wishlist API calls
  - common_game.py: Use params= for owned games API calls
  - Prevents API keys from leaking in logs, stack traces, or error messages

- Task 1.3 (sync I/O wrapping): Already complete
  - All get_lowest_price calls already use asyncio.to_thread()
@SuaveIV SuaveIV self-assigned this Mar 27, 2026
@SuaveIV SuaveIV merged commit 1c955c5 into main Mar 27, 2026
@SuaveIV SuaveIV deleted the sprint/critical-fixes branch March 27, 2026 02:36
@SuaveIV
Copy link
Copy Markdown
Owner Author

SuaveIV commented Mar 27, 2026

Pushed additional fixes for remaining review findings:

  1. close_db_connection: Use contextlib.suppress(sqlite3.Error) instead of ry/except: pass

  2. _mark_migration_run: Use get_write_connection() for serialized writes

  3. load_family_members_from_db migration: Use get_write_connection() for the INSERT OR IGNORE executemany and commit

  4. All write functions now use get_write_connection():

    • cache_user_games
    • cache_discord_user
    • cache_wishlist
    • cache_family_library
    • purge_wishlist_cache
    • purge_family_library_cache
    • cleanup_expired_cache
  5. routes_cache.py purge_cache: Use get_write_connection() instead of raw get_db_connection() with manual conn.close(). Removed unused get_db_connection import.

  6. Removed duplicate function definitions from incremental editing artifacts.

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.

1 participant