Support AES/mixed keys. Add tests & new configs.#72
Support AES/mixed keys. Add tests & new configs.#72gamesguru wants to merge 6 commits intolouisabraham:masterfrom
Conversation
- GH Actions on Windows/macOS
- Add argcomplete (optional end-user add-on).
- Add fixes & tests for master password.
- tests: Generate mixed key test data. Use some mocks.
- tests: Avoid modifying .json files, use temp storage.
- tests: Add test for AES export/import (AES, 3DES, and mixed keys).
- Tidy up, lint, and format.
- Fix edge cases with argcomplete.
- Windows: Fix profile discovery and CSV printing.
- Better loop logic (for imports w/ empty row or missing header).
- Config files.
- Add stuff back that was removed accidentally in a frenzy.
gamesguru
left a comment
gamesguru
left a comment
There was a problem hiding this comment.
Initial self-review with suggested follow-ups and shipstops in comments.
ffpass/__init__.py
Outdated
| from Crypto.Cipher import AES, DES3 | ||
|
|
||
| # noqa: W503 | ||
| # line break before binary operator |
There was a problem hiding this comment.
this is redundant and i meant to remove
| if: always() | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: python -m coveralls --service=github |
There was a problem hiding this comment.
This may break coverage reporting, and should possibly be reverted. It's just what I had a working reference copy of at the time that I could use, sorry.
| logging.debug(f"row: {row}") | ||
|
|
||
| # Peek at the first line to detect if it is a header or normal row | ||
| if first_row is None: |
There was a problem hiding this comment.
This is convoluted logic, which is perhaps best left to the user as a --header flag, without which input is assumed to be headerless (i.e., first row is already an (url, username, password) tuple/list?
Or default the other way if you prefer.
But this feels excessive, when I look at it with fresh eyes. Not tested on export to file, either, unsure if there is/was a test for that.
| dirs = { | ||
| "darwin": "~/Library/Application Support/Firefox/Profiles", | ||
| "linux": "~/.mozilla/firefox", | ||
| "win32": os.path.expandvars(r"%LOCALAPPDATA%\Mozilla\Firefox\Profiles"), |
There was a problem hiding this comment.
This did not work for me on Windows 10 nor Hiren's USB. It's possible though we may need logic for supporting/checking both local and roaming. I am a Linux user and defer to your knowledge on this matter.
| argcomplete.autocomplete(parser) | ||
|
|
||
| except (ImportError, ModuleNotFoundError): | ||
| sys.stderr.write( |
There was a problem hiding this comment.
logger uninitialized here (in effort to avoid messy circular logic edge cases)...... so, this is only available method to print to stderr at this point.
| try: | ||
| main() | ||
| except SystemExit: | ||
| pass |
There was a problem hiding this comment.
This can potentially be wrapped in an assertRaises() call, while reflecting overall on the ratio of unit:e2e integration tests.
| HEADER = 'url,username,password' | ||
| IMPORT_CREDENTIAL = 'https://www.example.com,foo,bar' | ||
| EXPECTED_EXPORT_OUTPUT = [HEADER, 'http://www.stealmylogin.com,test,test'] | ||
| EXPECTED_IMPORT_OUTPUT = EXPECTED_EXPORT_OUTPUT + [IMPORT_CREDENTIAL] |
There was a problem hiding this comment.
Ugly, but it works. Open to alternate suggestions!
|
|
||
| if mode == 'import': | ||
| ffpass_input = HEADER + IMPORT_CREDENTIAL | ||
| ffpass_input = "\n".join([HEADER, IMPORT_CREDENTIAL]) |
There was a problem hiding this comment.
tested without issue in both Windows 10 cmd.exe as well as Git Bash shell.
|
|
||
| def test_legacy_firefox_export(): | ||
| r = run_ffpass('export', 'tests/firefox-70') | ||
| def stdout_splitter(input_text): |
There was a problem hiding this comment.
May be unnecessary and can be reverted, as per earlier comments regarding fixes at the csv module level in the source code.
tests/test_run.py
Outdated
| profile_path = clean_profile('firefox-70') | ||
|
|
||
| # modifies the temp file, not the original | ||
| r = run_ffpass_cmd('import', profile_path) |
There was a problem hiding this comment.
This is an important fix imo. It's best not to modify files in the source tree that are used for tests!
Someone could accidentally commit changes and break it.
Better to use a temp copy.
use --reveal-keys flag use git-sqlite-filter for *.db files
1 participant
Large PR with many enhancements.
• v0.6.0: Import & export for AES and mixed keys.
• Fix errors w/ Windows & w/ GitHub runner. Configs.