Security remediation: credentials hygiene, HTTP safety, output safety, typed errors, test suite#2
Open
mikesmith-stack wants to merge 2 commits into
Open
Conversation
…, typed errors, test suite Addresses several classes of defects in the existing codebase. None of these changes alter the public CLI surface beyond removing the --password flag. Credentials & logging - Load the OAuth client secret from SIMPLIFI_CLIENT_SECRET instead of embedding it in source. Missing env var raises a clear RuntimeError. - Remove --password CLI argument. Password is resolved from SIMPLIFI_PASSWORD or, on a TTY, prompted via getpass.getpass(). - MFA code is now read via getpass.getpass() rather than input(), so it does not echo or enter readline history. - The bearer token, refresh token, MFA channel (a masked but still user-identifying string), and Simplifi user id are no longer logged at any level. HTTP hygiene & SSRF - Every requests call passes timeout=30. No more indefinite hangs. - Pagination follows an upstream-supplied nextLink: that value is now validated to be either a relative path or an absolute URL on https://services.quicken.com/ before the bearer token is sent. - The get_datasets query-string used the variable name 'limit' as a dict key instead of the string literal 'limit', so the limit was never actually sent. Fixed. - Every JSON decode is wrapped: malformed bodies raise a typed SimplifiAPIError instead of bubbling a raw JSONDecodeError. Typed exception hierarchy - New simplifiapi.exceptions module defines SimplifiAPIError and an AuthenticationError subclass. - Client methods raise these instead of returning None / False on failure (verify_token now raises on failure and installs the bearer header on success). - The CLI main() catches SimplifiAPIError at the boundary and SystemExits with a one-line message — no Python traceback that could echo URL params or partial headers. Output safety - --filename runs through os.path.basename so '../../tmp/evil' cannot escape CWD. - CSV cells whose first character is = + - or @ are prefixed with a single quote (formula-injection escape). - JSON output is opened 'w' with utf-8 encoding (was 'w+', no encoding, which mangled non-ASCII on Windows cp1252 locales). Quality cleanups - Collapse four near-identical resource getters (get_accounts/transactions/tags/categories) into _get_resource. - Library is logging-pure: __init__ attaches only a NullHandler; real log config lives in cli.main's _configure_logging() and honours LOG_LEVEL. - Migrate project metadata to pyproject.toml [project] (PEP 621), drop the broken 'name = setuptools' setup.cfg metadata block. - Pin runtime deps: requests>=2.31,<3, pandas>=2,<3, configargparse>=1.7,<2. - Add .env.example template for SIMPLIFI_CLIENT_SECRET and SIMPLIFI_PASSWORD. Tests - New pytest + responses suite covering the cases above (auth happy path, MFA, pagination key, SSRF rejection, timeout presence, CSV formula escape, --filename basename strip, JSON-decode safety, typed exception boundary). 33 tests; coverage configured to fail under 80% (currently 91%). README is rewritten to document the trust model, the new CLI surface, the typed-exception API, and a development setup.
570827a to
94cc9d0
Compare
The trust-model section already noted the clientSecret cannot be rotated per-user, but didn't explain what the value actually is, how the original author obtained it, or how a new user is supposed to set it. Future users hit this question on first run. Add a 'Where SIMPLIFI_CLIENT_SECRET comes from' section between Trust Model and Install covering: - Quicken does not issue this; there is no developer program for Simplifi - The value is the acme_web OAuth client secret used by Quicken's own Simplifi web app at app.simplifimoney.com - The value was originally extracted from Quicken's frontend, then hardcoded in client.py; this change moves it out of source - Three practical paths to set it: pull from previously-committed history at 27fcc74, extract via DevTools, or don't use the tool - Why per-user rotation isn't possible The actual secret value is deliberately not republished in the README; readers are pointed at 'git show 27fcc74:simplifiapi/client.py' instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses several classes of defects in the current codebase. It does not change the public CLI surface beyond removing the
--passwordflag (which was unsafe to keep — see below). The Python API gets a small breaking change:verify_tokennow raisesAuthenticationErroron failure and returnsNoneon success, instead of returningFalse/True. The justification is that the previous return value was silently ignored at the only existing call site incli.py(assert client.verify_token(token)), so a failed verify was effectively a no-op.Credentials & logging
SIMPLIFI_CLIENT_SECRETinstead of embedding it in source. Missing env var raises a clearRuntimeError.--passwordCLI argument. Password is resolved fromSIMPLIFI_PASSWORDor, on a TTY, prompted viagetpass.getpass(). Passing passwords onargvleaked them viaps, shell history, and syslog.getpass.getpass()rather thaninput(), so it doesn't echo or enter readline history.HTTP hygiene & SSRF
requestscall passestimeout=30. No more indefinite hangs.nextLink: that value is now validated to be either a relative path or an absolute URL onhttps://services.quicken.com/before the bearer token is sent.get_datasetsused the variable namelimitas a dict key instead of the string literal\"limit\", so the limit was never actually sent on the query string. Fixed.SimplifiAPIErrorinstead of bubbling a rawJSONDecodeError.Typed exception hierarchy
simplifiapi.exceptionsmodule definesSimplifiAPIErrorand anAuthenticationErrorsubclass.Clientmethods raise these instead of returningNone/Falseon failure.main()catchesSimplifiAPIErrorat the boundary andSystemExits with a one-line message — no Python traceback that could echo URL params or partial headers.Output safety
--filenameruns throughos.path.basenameso../../tmp/evilcannot escape CWD.=,+,-, or@are prefixed with a single quote (formula-injection escape).\"w\"withutf-8encoding (was\"w+\"with no encoding, which mangled non-ASCII on Windows cp1252 locales).Quality cleanups
get_accounts/transactions/tags/categories) into a shared_get_resourcehelper.__init__attaches only aNullHandler; real log config lives incli.main's_configure_logging()and honoursLOG_LEVEL.pyproject.toml [project](PEP 621); drop the brokenname = setuptoolsblock fromsetup.cfg(the package was effectively distributing under the namesetuptools).requests>=2.31,<3,pandas>=2,<3,configargparse>=1.7,<2..env.exampletemplate forSIMPLIFI_CLIENT_SECRETandSIMPLIFI_PASSWORD.Tests
pytest+responsestest suite, 33 tests, covering the cases above (auth happy path, MFA challenge, pagination key, SSRF rejection, timeout presence, CSV formula escape,--filenamebasename strip, JSON-decode safety, typed-exception boundary).fail_under=80(currently 91% locally).README
Rewritten to document the trust model, the new CLI surface, the typed-exception API, and a development setup.
Test plan
pip install -e '.[dev]'pytest— 33 passedpytest --cov— coverage 91%, gate passesNotes for the maintainer
clientSecretcannot be rotated per-user. Because this is an unofficial client, Quicken does not own a per-user relationship for theclientSecret. If the secret leaks, every user is affected and Quicken's only remediation is invalidating the secret for the entire client. The README now states this explicitly so users opt in with their eyes open. Let me know if you'd prefer different framing.DeprecationWarnings remain from theresponseslibrary (match_querystringis deprecated in favour ofresponses.matchers.query_param_matcher). I left the migration out to keep this PR focused; happy to add a follow-up.