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

Changes for armor_max_size and tty_max_size. #59

Merged
merged 8 commits into from
Dec 18, 2021

Conversation

rocketdey
Copy link
Collaborator

@rocketdey rocketdey commented Dec 16, 2021

This commit closes #26.

@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #59 (195b374) into main (b67fd60) will increase coverage by 1.15%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #59      +/-   ##
==========================================
+ Coverage   61.59%   62.74%   +1.15%     
==========================================
  Files          17       17              
  Lines        1997     1997              
  Branches      454      454              
==========================================
+ Hits         1230     1253      +23     
+ Misses        627      606      -21     
+ Partials      140      138       -2     
Impacted Files Coverage Δ
covert/__main__.py 62.69% <25.00%> (+2.06%) ⬆️
covert/cli.py 57.91% <50.00%> (+5.58%) ⬆️
covert/util.py 94.91% <100.00%> (+3.68%) ⬆️
covert/archive.py 64.68% <0.00%> (+0.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 236641d...195b374. Read the comment docs.

@covert-encryption
Copy link
Owner

covert-encryption commented Dec 16, 2021

Now the coverage on files is displayed correctly. You could add this test to cover your change in cli.py and also increase coverage elsewhere because this should hit different parts of the code than the other tests.

def test_end_to_end_shortargs_armored(capsys, tmp_path):
  from covert.__main__ import main
  import sys
  fname = tmp_path / "crypto.covert"

  # Encrypt foo.txt into crypto.covert
  sys.argv = "covert -eRoa tests/data/foo.txt tests/keys/ssh_ed25519.pub".split() + [ str(fname) ]
  ret = main()
  cap = capsys.readouterr()
  assert not ret
  assert not cap.out
  assert "foo" in cap.err

  # Decrypt with key
  sys.argv = "covert -di tests/keys/ssh_ed25519".split() + [ str(fname) ]
  ret = main()
  cap = capsys.readouterr()
  assert not ret
  assert not cap.out
  assert "foo.txt" in cap.err

@covert-encryption
Copy link
Owner

covert-encryption commented Dec 16, 2021

If you wish, also add another test that writes a 31 MiB test file to tmp_path / "test.dat", then encrypts it with armoring --armor, also using --pad 0 so that it should be slightly below the 32 MiB limit, and try if that can still be extracted.

Also in that same test, you could put -- as the second last argument, and put the input file name as the last argument, to test that feature of the argparser. In this case the name of your test file could instead of test.dat be -test- just to see if that works as intended.

@covert-encryption
Copy link
Owner

Thanks for the armor/tty changes and for writing useful tests!

That test failure is a bit odd because AFAIK we never close stderr, but pytest could. I'll check if I can dig into it a bit deeper, but separating that test to another test function would already help narrowing it down.

@covert-encryption
Copy link
Owner

As a workaround, you could also try passing covert argument --debug which prevents it from catching this error, and then you can catch it directly in the test by:

with pytest.raises(ValueError) as excinfo:
  main()
assert "the error message" in str(excinfo.value)

Still, we need to get to the bottom of stderr being closed because that by itself constitutes a bug.

@rocketdey rocketdey marked this pull request as ready for review December 17, 2021 14:03
@covert-encryption
Copy link
Owner

covert-encryption commented Dec 17, 2021

You need these changes to make the tests work properly. Afterwards you also don't need to import covert inside each test functions but can just do the imports at the top of test_cli.py like imports are normally done. This is a diff that you can apply to your files by git apply, or just do the changes by hand because that's probably faster LOL.

In short, we must not from sys import stderr, stdout but rather import sys and use sys.stderr etc, so that pytest can wrap them properly.

diff --git a/covert/__main__.py b/covert/__main__.py
index 5f1e7f6..5b20e51 100644
--- a/covert/__main__.py
+++ b/covert/__main__.py
@@ -1,6 +1,5 @@
 import os
 import sys
-from sys import stderr, stdout
 import colorama
 import covert
 from covert.cli import main_benchmark, main_dec, main_enc
@@ -89,7 +88,7 @@ def argparse():
   av = sys.argv[1:]
   if not av or any(a.lower() in ('-h', '--help') for a in av):
     first, rest = cmdhelp.rstrip().split('\n', 1)
-    if stdout.isatty():
+    if sys.stdout.isatty():
       print(f'\x1B[1;44m{first:78}\x1B[0m\n{rest}')
     else:
       print(f'{first}\n{rest}')
@@ -111,9 +110,9 @@ def argparse():
   elif av[0] in ('bench', 'benchmark'):
     args.mode, ad, modehelp = 'benchmark', benchargs, f"{hdrhelp}"
   else:
-    stderr.write(' 💣  Invalid or missing command (enc/dec/benchmark).\n')
+    sys.stderr.write(' 💣  Invalid or missing command (enc/dec/benchmark).\n')
     sys.exit(1)
-  
+
   aiter = iter(av[1:])
   longargs = [flag[1:] for switches in ad.values() for flag in switches if flag.startswith("--")]
   shortargs = [flag[1:] for switches in ad.values() for flag in switches if not flag.startswith("--")]
@@ -133,7 +132,7 @@ def argparse():
     if not a.startswith('--') and len(a) > 2:
       if any(arg not in shortargs for arg in list(a[1:])):
         falseargs = [arg for arg in list(a[1:]) if arg not in shortargs]
-        stderr.write(f' 💣  {falseargs} is not an argument: covert {args.mode} {a}\n')
+        sys.stderr.write(f' 💣  {falseargs} is not an argument: covert {args.mode} {a}\n')
         sys.exit(1)
       a = [f'-{shortarg}' for shortarg in list(a[1:]) if shortarg in shortargs]
     if isinstance(a, str):
@@ -143,7 +142,7 @@ def argparse():
       if isinstance(av, int):
         continue
       if argvar is None:
-        stderr.write(f'{modehelp}\n 💣  Unknown argument: covert {args.mode} {aprint}\n')
+        sys.stderr.write(f'{modehelp}\n 💣  Unknown argument: covert {args.mode} {aprint}\n')
         sys.exit(1)
       try:
         var = getattr(args, argvar)
@@ -156,7 +155,7 @@ def argparse():
         else:
           setattr(args, argvar, True)
       except StopIteration:
-        stderr.write(f'{modehelp}\n 💣  Argument parameter missing: covert {args.mode} {aprint} …\n')
+        sys.stderr.write(f'{modehelp}\n 💣  Argument parameter missing: covert {args.mode} {aprint} …\n')
         sys.exit(1)
 
   return args
@@ -193,11 +192,11 @@ def main():
     else:
       raise Exception('This should not be reached')
   except ValueError as e:
-    stderr.write(f"Error: {e}\n")
+    sys.stderr.write(f"Error: {e}\n")
   except BrokenPipeError:
-    stderr.write('I/O error (broken pipe)\n')
+    sys.stderr.write('I/O error (broken pipe)\n')
   except KeyboardInterrupt:
-    stderr.write("Interrupted.\n")
+    sys.stderr.write("Interrupted.\n")
 
 
 if __name__ == "__main__":

@covert-encryption
Copy link
Owner

With these changes all tests pass for me

diff --git a/tests/test_cli.py b/tests/test_cli.py
index 1754310..07b8932 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -161,16 +161,14 @@ def test_end_to_end_large_file(capsys, tmp_path):
 
   # Try encrypting without -o
   sys.argv = f"covert -ea -R tests/keys/ssh_ed25519.pub".split() + [ str(fname) ]
-  with pytest.raises(ValueError) as excinfo:
-    main()
-  assert "How about -o FILE to write a file?" in str(excinfo.value)
+  main()
   cap = capsys.readouterr()
   assert not cap.out
+  assert "How about -o FILE to write a file?" in cap.err
 
   # Try encrypting with -o
-  sys.argv = f"covert -eaR tests/keys/ssh_ed25519 -o".split() + [ str(outfname), str(fname) ]
-  with pytest.raises(ValueError) as excinfo:
-    main()
-  assert "The data is too large for --armor." in str(excinfo.value)
+  sys.argv = f"covert -eaR tests/keys/ssh_ed25519.pub -o".split() + [ str(outfname), str(fname) ]
+  main()
   cap = capsys.readouterr()
   assert not cap.out
+  assert "The data is too large for --armor." in cap.err

@covert-encryption covert-encryption merged commit c51663c into covert-encryption:main Dec 18, 2021
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.

Consistent handling of armor max size
2 participants