Escape Beancount strings at the add_account writer boundary, not just the metadata-value branch #52
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found in the high-effort code review of PR #46.
_escape_beancount_string(added in PR #46) is applied only to the metadata value branch ofadd_account(fava_client.py). Theaccount_nameand metadata keys are written into the same Open directive source text unescaped.Today this is latent, not exploitable: the admin endpoint validates
account_name, and all callers pass system-controlled metadata keys (added_by,source,description,entry-id) and safe descriptions. Butadd_accountis shared infrastructure (callers: the admin endpoint andcrud.get_or_create_user_account), and its source-construction contract currently trusts every caller for the name and keys. A future caller — or user-derived text flowing into a name/key — would corruptchart.beancountand 500 every subsequent/api/sourcewrite.Suggested fix
Make the writer responsible for its own output: escape (or assert-valid)
account_nameand metadata keys where the directive text is assembled, so safety is a property ofadd_accountrather than of each call site. Account names can't legally contain quotes, so an assert/validate is appropriate there; keys should be escaped or constrained to the metadata-key grammar.