feat: Add YNAB budget selector to resolve multi-budget users#51
feat: Add YNAB budget selector to resolve multi-budget users#51
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return None | ||
|
|
||
| # For YNAB, handle budget selection after credentials are set up | ||
| if backend_type == "ynab": |
There was a problem hiding this comment.
The YNAB access token (stored in creds['password']) is passed to a temporary backend instance for budget fetching, but if budget selection fails or is cancelled, the account is deleted while credentials may remain in memory. Consider explicitly clearing the credentials dict or ensuring temp_backend.clear_auth() is called in all error paths to minimize credential exposure window.
Automated security review by Claude 4.5 Sonnet - Human review still required
🔒 Security Review: 1 High/Medium Issue FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
| table = self.query_one("#budget-table", DataTable) | ||
| if table.cursor_row is not None and table.cursor_row < len(self.budgets): | ||
| # Get the budget ID from the row key | ||
| row_key = table.get_row_at(table.cursor_row)[0] # Get the key of the row |
There was a problem hiding this comment.
The code accesses table.get_row_at(table.cursor_row)[0] without proper bounds checking or type validation, which could cause crashes if the table structure changes. While not directly a security issue, crashes during sensitive operations like budget selection could leave the system in an inconsistent state. Add explicit validation that the row exists and the key is the expected type before accessing it.
Automated security review by Claude 4.5 Sonnet - Human review still required
🔒 Security Review: 1 High/Medium Issue FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
moneyflow/app.py
Outdated
| if len(budgets) > 1: | ||
| # Show budget selector | ||
| budget_id = await self.push_screen( | ||
| BudgetSelectorScreen(budgets), wait_for_dismiss=True |
There was a problem hiding this comment.
The exception handler logs errors during budget fetching but doesn't sanitize the exception message, which could contain sensitive budget information or API tokens. Consider using a generic error message or sanitizing exception details before logging. Replace with: logger.error('Failed to fetch YNAB budgets during account setup')
Automated security review by Claude 4.5 Sonnet - Human review still required
🔒 Security Review: 1 High/Medium Issue FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
moneyflow/app.py
Outdated
| # Create temporary backend to fetch budgets | ||
| temp_backend = get_backend("ynab") | ||
| try: | ||
| await temp_backend.login(password=creds["password"]) |
There was a problem hiding this comment.
If an exception occurs during budget fetching (line 617), the temporary backend remains logged in with valid credentials before the cleanup in the except block. This could leave sensitive session data in memory. Consider using a try-finally block or context manager to ensure cleanup happens even if an unexpected exception occurs between login and the except handler.
Automated security review by Claude 4.5 Sonnet - Human review still required
🔒 Security Review: 1 High/Medium Issue FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
moneyflow/app.py
Outdated
| account_manager.delete_account(account.id) | ||
| return None | ||
| finally: | ||
| # Always clear temporary backend auth to minimize credential exposure |
There was a problem hiding this comment.
🚨 Sensitive credentials not cleared after temporary usage (high severity)
The temporary backend uses credentials to fetch budgets, but the credentials dict 'creds' is never cleared after use, remaining in memory. If budget selection fails or is cancelled, credentials stay exposed in memory longer than necessary. Add 'creds.clear()' in the finally block after 'temp_backend.clear_auth()' to minimize credential exposure time.
Automated security review by Claude 4.5 Sonnet - Human review still required
🔒 Security Review: 1 High/Medium Issue FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
moneyflow/app.py
Outdated
| account_manager.delete_account(account.id) | ||
| return None | ||
| finally: | ||
| # Always clear temporary backend auth to minimize credential exposure |
There was a problem hiding this comment.
When budget selection fails, the code clears 'creds' dict but this only clears the local reference - the credentials are still stored in memory via self.stored_credentials. After temp_backend.clear_auth(), credentials should be explicitly cleared from all references to minimize exposure time. Consider adding creds.clear() before returning None on failure paths.
Automated security review by Claude 4.5 Sonnet - Human review still required
moneyflow/app.py
Outdated
| budget_id = await self.push_screen( | ||
| BudgetSelectorScreen(budgets), wait_for_dismiss=True | ||
| ) | ||
|
|
There was a problem hiding this comment.
🚨 YNAB credentials exposed to temporary backend without cleanup (high severity)
Credentials (including access token in password field) are passed to temp_backend.login() for budget fetching. If an exception occurs during budget selection, temp_backend.clear_auth() runs in finally block, but the credentials remain in the 'creds' dict in memory. For a financial app handling API tokens, credential exposure time should be minimized. The creds dict should be cleared or overwritten with dummy values after any error during budget selection.
Automated security review by Claude 4.5 Sonnet - Human review still required
🔒 Security Review: 2 High/Medium Issues FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
|
Thanks for this! I just cut a 0.7.5 bugfix release but I will review this sometime this week |
…of type YNAB, for users with multiple budgets to address issue wesm#48. Previously, moneyflow would pull data from the first budget it finds. When adding a new moneyflow Account with the YNAB backend, this allows selecting one YNAB budget, and persists this choice for the Account over future moneyflow sessions. With my user testing I replaced my existing, default moneyflow account. This PR doesn't address existing accounts that lack the YNAB budget_id (e.g. a flow to "upgrade" them with a selected budget). Although the `accounts.json` can be edited to add the budget_id attribute. ## New Features ### Budget Selection Screen - Interactive modal for selecting from multiple YNAB budgets - Shows budget name and last modified date for informed selection - Keyboard navigation (↑/↓ to navigate, Enter to select, Esc to cancel) - Automatically skipped for single-budget accounts ### Account Management - Added budget_id field to Account class for YNAB-specific budget tracking - Budget selection persisted per account profile - Backward compatible serialization supporting both old and new account formats ### YNAB Backend Integration - New get_budgets() API to retrieve all available budgets - Enhanced login to accept specific budget_id parameter - Budget validation during authentication with clear error messages - Support for budget-specific data fetching ## Workflow 1. **New YNAB Account Setup**: After entering credentials, users with multiple budgets see a selection screen to choose their active budget 2. **Single Budget**: Automatically selects the only available budget 3. **Future Logins**: Uses saved budget_id for consistent data access ## Documentation - Updated YNAB setup guide with budget selection steps - README mentions budget selection for multi-budget users ## Testing - Comprehensive test coverage for budget selection workflows - Account manager budget_id functionality tests - YNAB backend budget validation tests - Error handling for invalid budget scenarios Resolves: wesm#48 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Fix ruff formatting error that was causing CI to fail. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Break long lines in README.md and docs/guide/ynab.md to comply with 120-character line length limit required by markdownlint. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Address security review comment by ensuring temp_backend.clear_auth() and creds.clear() are called when user cancels budget selection or when an exception occurs. Also use generic error message to avoid potentially logging sensitive information. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Refactor YNAB budget selection to use try-finally block, guaranteeing temp_backend.clear_auth() is called even if an unexpected exception occurs between login and the exception handler. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The credentials dict cannot be cleared in the finally block because it's returned and used for the actual backend login. Additionally, credentials are intentionally stored in self.stored_credentials after login for session refresh functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Cleans up backend selection logic in app.py that had become messy with YNAB-specific code scattered throughout. Changes: - Add abstract get_backend_type() method to FinanceBackend base class - Implement get_backend_type() in all backends (monarch, ynab, amazon, demo) - Add centralized get_backend_config() factory function (single source of truth) - Extract YNAB budget selection to dedicated _handle_ynab_budget_selection() - Replace fragile class name string matching with get_backend_type() - Replace 4 duplicated BackendConfig.for_*() mappings with get_backend_config() Benefits: - Open/Closed Principle: new backends just implement get_backend_type() - DRY: single source of truth for backend type → config mapping - SRP: YNAB budget selection isolated in its own method - Cleaner _handle_add_new_account() (reduced from ~115 to ~70 lines) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Simplifies budget_id lookup in initialize_data by extracting the awkward "if account_manager in locals()" pattern into a clean helper method. Before: 15 lines with conditional AccountManager creation After: 2 lines calling self._get_ynab_budget_id(account_id) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
wesm
force-pushed
the
feat/ynab-budget-selector
branch
from
39f7e50 to
0878b8d
Compare
| return None | ||
| finally: | ||
| # Always clear temporary backend auth to minimize credential exposure | ||
| # Note: creds dict is intentionally NOT cleared here on success because |
There was a problem hiding this comment.
In _handle_ynab_budget_selection, if an exception occurs after temp_backend.login(), the creds dictionary is cleared but the temporary backend still holds the token until temp_backend.clear_auth() in finally block. However, if budget selection succeeds, creds is NOT cleared before returning, meaning the YNAB token remains in memory in the creds dict longer than necessary. Consider clearing creds immediately after budget selection succeeds to minimize credential exposure time.
Automated security review by Claude 4.5 Sonnet - Human review still required
🔒 Security Review: 1 High/Medium Issue FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
Move get_backend_config to top-level imports and remove redundant inline imports of AccountManager and get_backend_config within initialize_data. This reduces code noise without changing any logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
wesm
force-pushed
the
feat/ynab-budget-selector
branch
from
0878b8d to
656a069
Compare
| return None | ||
| finally: | ||
| # Always clear temporary backend auth to minimize credential exposure | ||
| # Note: creds dict is intentionally NOT cleared here on success because |
There was a problem hiding this comment.
In _handle_ynab_budget_selection, the broad except catches all exceptions including authentication errors. If an exception occurs after successful login but before budget_id is saved, the creds dict is cleared but the backend may still be authenticated. This could leave the session in an inconsistent state. Consider more specific exception handling and ensure backend.clear_auth() is always called in the finally block even on success until the actual login occurs later.
Automated security review by Claude 4.5 Sonnet - Human review still required
🔒 Security Review: 1 High/Medium Issue FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
|
Thanks for doing this — I rebased (to fix the cache-save failure after adding a new account) and did a little cleaning, merging now |
2 participants
Implements budget selection UI when creating a new moneyflow account of type YNAB, for users with multiple budgets to address issue #48. Previously, moneyflow would pull data from the first budget it finds.
When adding a new moneyflow Account with the YNAB backend, this allows selecting one YNAB budget, and persists this choice for the Account over future moneyflow sessions.
With my user testing I replaced my existing, default moneyflow account. This PR doesn't address existing moneyflow accounts that lack the YNAB budget_id (e.g. a flow to "upgrade" them with a selected budget). Although the
accounts.jsoncan be edited manually to add the budget_id attribute.New Features
Budget Selection Screen
Account Management
YNAB Backend Integration
Workflow
Documentation
Testing
Resolves: #48
🤖 Generated with Claude Code