Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Support 1PIF file format for 1Password v7.9.11 (macOS) #207

Merged
merged 0 commits into from Jan 24, 2024

Conversation

davidandreoletti
Copy link
Contributor

@davidandreoletti davidandreoletti commented Jan 23, 2024

Rule out CSV file format when a data line looks like json formatted line

With 1Password 7.9.11, pass-import erroneous detects a 1PIF file as a CSV file. This PR improves the CSV detection capability by checking if the CSV file look like a valid JSON file. IFF it does, then the file is not considered a CSV file.

Removed empty line when reading 1PIF file

1PIF exported from 1Password 7.9.11 contains an extra empty line at the end of the file. This empty line is now removed.

Encoded special characters properly when reading JSON strings

1PIF exported from 1Password 7.9.11 does not encode special character. These characters are now properly encoded.

PS: thank you for starting this project and help reduce vendor lock-in. 1Password :-(

@roddhjav
Copy link
Owner

roddhjav commented Jan 23, 2024

Thanks for your PR.

Don't worry about the failed CI, this is unrelated to you PR.
However, when I run the test locally, test_main_decrypt_without_manager fail with:

Error in tests.main.test_pass.TestMainPass.test_main_decrypt_without_manager
  File "/usr/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/usr/lib/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
  File "/usr/lib/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
       ^^^^^^^^
  File "/home/***/pass-import/tests/main/test_pass.py", line 124, in test_main_decrypt_without_manager
    self.main(cmd)
  File "/home/***/pass-import/tests/__init__.py", line 213, in main
    pass_import.__main__.main()
  File "/home/***/pass-import/pass_import/__main__.py", line 456, in main
    cls_import = detectmanager(conf)
                 ^^^^^^^^^^^^^^^^^^^
  File "/home/***/pass-import/pass_import/__main__.py", line 307, in detectmanager
    pm = detect.manager(to_detect)
         ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/***/pass-import/pass_import/auto.py", line 147, in manager
    if file.is_format():
       ^^^^^^^^^^^^^^^^
  File "/home/***/pass-import/pass_import/formats/csv.py", line 91, in is_format
    _ = next(self.reader)
        ^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/csv.py", line 111, in __next__
    row = next(self.reader)
          ^^^^^^^^^^^^^^^^^

Can you have a look a it. You can run the test suite locally or simply test it manually with pass import tests/assets/db/andotp.gpg

@davidandreoletti
Copy link
Contributor Author

davidandreoletti commented Jan 23, 2024

File "/usr/lib/python3.11/csv.py", line 111, in next
row = next(self.reader)

@roddhjav Running the whole test suites locally depends on a lot of password manager binaries available on PATH. As a result, many test suite failures unrelated to this PR show up. Not a criticism, just a remark ;-)

This issue got buried in the noise. I pushed a fix.

Hopefully, CI will confirm its the last one.

@roddhjav
Copy link
Owner

Thanks a lot. Merged!

I have added so exception to not run all tests if some deps are not meet.

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.

None yet

2 participants