Skip to main content
added 426 characters in body
Source Link
l0b0
  • 9.1k
  • 22
  • 36

Some general tips:

  1. The runner should use argparse to parse arguments. It most definitely should not hardcode passwords.

  2. (object) is redundant in Python 3 class definitions.

  3. I'd recommend running any Python code through Black, flake8 and mypy with a strict configuration like this one:

     [flake8]
     doctests = true
     exclude =
         .git
     max-complexity = 5
     max-line-length = 120
     ignore = W503,E203
    
     [mypy]
     check_untyped_defs = true
     disallow_untyped_defs = true
     ignore_missing_imports = true
     no_implicit_optional = true
     warn_redundant_casts = true
     warn_return_any = true
     warn_unused_ignores = true
    
  4. You reuse variable names with completely different semantics. This is a really bad idea for understanding what the code is doing and following along even otherwise trivial logic. For example, settings = json.loads(settings) means that settings is originally a str, effectively a serialized JSON object, and afterwards a dict. These have completely different semantics and interaction patterns. The easiest way to deal with this is to treat almost every variable as immutable, and naming the variables according to what they really are. For example, settings = json.loads(serialized_settings).

  5. Names should be descriptive, for example password_database = PasswordDatabase().

  6. Don't use *args and **kwargs unless you need dynamic parameter lists. Rather than indexing *args you should use named parameters. If they have default values those should go in the method signature.

  7. .get(foo, None) can be simplified to .get(foo) - get() returns None by default.

  8. if foo is None can in the vast majority of cases be changed to the more idiomatic if foo.

  9. I would highly recommend using a well-known open format such as the KeePass one for storing this data.

  10. This should not be in there:

     if not sample == settings["enc_sample_content"]:
         raise ValueError(
             "Cannot open PassDB: incorrect password provided")
    
  11. There is a lot of encoding and decoding happening, which greatly obfuscates the state and looks unnecessary in several places.

  12. I would not trust this sort of code without a comprehensive test suite.

With the caveat that I'm not a cryptographer:

  1. Salting does not make sense unless you're hashing the password (which you don't want to do in this case). I'll refrain from any other comments on how the salting is done unless someone corrects this.

Some general tips:

  1. The runner should use argparse to parse arguments. It most definitely should not hardcode passwords.

  2. (object) is redundant in Python 3 class definitions.

  3. I'd recommend running any Python code through Black, flake8 and mypy with a strict configuration like this one:

     [flake8]
     doctests = true
     exclude =
         .git
     max-complexity = 5
     max-line-length = 120
     ignore = W503,E203
    
     [mypy]
     check_untyped_defs = true
     disallow_untyped_defs = true
     ignore_missing_imports = true
     no_implicit_optional = true
     warn_redundant_casts = true
     warn_return_any = true
     warn_unused_ignores = true
    
  4. You reuse variable names with completely different semantics. This is a really bad idea for understanding what the code is doing and following along even otherwise trivial logic.

  5. Names should be descriptive, for example password_database = PasswordDatabase().

  6. Don't use *args and **kwargs unless you need dynamic parameter lists. Rather than indexing *args you should use named parameters. If they have default values those should go in the method signature.

  7. .get(foo, None) can be simplified to .get(foo) - get() returns None by default.

  8. if foo is None can in the vast majority of cases be changed to the more idiomatic if foo.

  9. I would highly recommend using a well-known open format such as the KeePass one for storing this data.

  10. This should not be in there:

     if not sample == settings["enc_sample_content"]:
         raise ValueError(
             "Cannot open PassDB: incorrect password provided")
    
  11. There is a lot of encoding and decoding happening, which greatly obfuscates the state and looks unnecessary in several places.

  12. I would not trust this sort of code without a comprehensive test suite.

With the caveat that I'm not a cryptographer:

  1. Salting does not make sense unless you're hashing the password (which you don't want to do in this case). I'll refrain from any other comments on how the salting is done unless someone corrects this.

Some general tips:

  1. The runner should use argparse to parse arguments. It most definitely should not hardcode passwords.

  2. (object) is redundant in Python 3 class definitions.

  3. I'd recommend running any Python code through Black, flake8 and mypy with a strict configuration like this one:

     [flake8]
     doctests = true
     exclude =
         .git
     max-complexity = 5
     max-line-length = 120
     ignore = W503,E203
    
     [mypy]
     check_untyped_defs = true
     disallow_untyped_defs = true
     ignore_missing_imports = true
     no_implicit_optional = true
     warn_redundant_casts = true
     warn_return_any = true
     warn_unused_ignores = true
    
  4. You reuse variable names with completely different semantics. This is a really bad idea for understanding what the code is doing and following along even otherwise trivial logic. For example, settings = json.loads(settings) means that settings is originally a str, effectively a serialized JSON object, and afterwards a dict. These have completely different semantics and interaction patterns. The easiest way to deal with this is to treat almost every variable as immutable, and naming the variables according to what they really are. For example, settings = json.loads(serialized_settings).

  5. Names should be descriptive, for example password_database = PasswordDatabase().

  6. Don't use *args and **kwargs unless you need dynamic parameter lists. Rather than indexing *args you should use named parameters. If they have default values those should go in the method signature.

  7. .get(foo, None) can be simplified to .get(foo) - get() returns None by default.

  8. if foo is None can in the vast majority of cases be changed to the more idiomatic if foo.

  9. I would highly recommend using a well-known open format such as the KeePass one for storing this data.

  10. This should not be in there:

     if not sample == settings["enc_sample_content"]:
         raise ValueError(
             "Cannot open PassDB: incorrect password provided")
    
  11. There is a lot of encoding and decoding happening, which greatly obfuscates the state and looks unnecessary in several places.

  12. I would not trust this sort of code without a comprehensive test suite.

With the caveat that I'm not a cryptographer:

  1. Salting does not make sense unless you're hashing the password (which you don't want to do in this case). I'll refrain from any other comments on how the salting is done unless someone corrects this.
added 207 characters in body
Source Link
l0b0
  • 9.1k
  • 22
  • 36

Some general tips:

  1. The runner should use argparse to parse arguments. It most definitely should not hardcode passwords.

  2. (object) is redundant in Python 3 class definitions.

  3. I'd recommend running any Python code through Black, pyflakesflake8 and mypy with a strict configuration like this one:

     [flake8]
     doctests = true
     exclude =
         .git
     max-complexity = 5
     max-line-length = 120
     ignore = W503,E203
    
     [mypy]
     check_untyped_defs = true
     disallow_untyped_defs = true
     ignore_missing_imports = true
     no_implicit_optional = true
     warn_redundant_casts = true
     warn_return_any = true
     warn_unused_ignores = true
    
  4. You reuse variable names with completely different semantics. This is a really bad idea for understanding what the code is doing and following along even otherwise trivial logic.

  5. Names should be descriptive, for example password_database = PasswordDatabase().

  6. Don't use *args and **kwargs unless you need dynamic parameter lists. Rather than indexing *args you should use named parameters. If they have default values those should go in the method signature.

  7. .get(foo, None) can be simplified to .get(foo) - get() returns None by default.

  8. if foo is None can in the vast majority of cases be changed to the more idiomatic if foo.

  9. I would highly recommend using a well-known open format such as the KeePass one for storing this data.

  10. This should not be in there:

     if not sample == settings["enc_sample_content"]:
         raise ValueError(
             "Cannot open PassDB: incorrect password provided")
    
  11. There is a lot of encoding and decoding happening, which greatly obfuscates the state and looks unnecessary in several places.

  12. I would not trust this sort of code without a comprehensive test suite.

With the caveat that I'm not a cryptographer:

  1. Salting does not make sense unless you're hashing the password (which you don't want to do in this case). I'll refrain from any other comments on how the salting is done unless someone corrects this.

Some general tips:

  1. The runner should use argparse to parse arguments. It most definitely should not hardcode passwords.

  2. (object) is redundant in Python 3 class definitions.

  3. I'd recommend running any Python code through Black, pyflakes and mypy with a strict configuration like this one:

     [flake8]
     doctests = true
     exclude =
         .git
     max-complexity = 5
     max-line-length = 120
     ignore = W503,E203
    
     [mypy]
     check_untyped_defs = true
     disallow_untyped_defs = true
     ignore_missing_imports = true
     no_implicit_optional = true
     warn_redundant_casts = true
     warn_return_any = true
     warn_unused_ignores = true
    
  4. You reuse variable names with completely different semantics. This is a really bad idea for understanding what the code is doing and following along even otherwise trivial logic.

  5. Names should be descriptive, for example password_database = PasswordDatabase().

  6. Don't use *args and **kwargs unless you need dynamic parameter lists. Rather than indexing *args you should use named parameters. If they have default values those should go in the method signature.

  7. .get(foo, None) can be simplified to .get(foo) - get() returns None by default.

  8. if foo is None can in the vast majority of cases be changed to the more idiomatic if foo.

  9. I would highly recommend using a well-known open format such as the KeePass one for storing this data.

  10. This should not be in there:

     if not sample == settings["enc_sample_content"]:
         raise ValueError(
             "Cannot open PassDB: incorrect password provided")
    

With the caveat that I'm not a cryptographer:

  1. Salting does not make sense unless you're hashing the password (which you don't want to do in this case). I'll refrain from any other comments on how the salting is done unless someone corrects this.

Some general tips:

  1. The runner should use argparse to parse arguments. It most definitely should not hardcode passwords.

  2. (object) is redundant in Python 3 class definitions.

  3. I'd recommend running any Python code through Black, flake8 and mypy with a strict configuration like this one:

     [flake8]
     doctests = true
     exclude =
         .git
     max-complexity = 5
     max-line-length = 120
     ignore = W503,E203
    
     [mypy]
     check_untyped_defs = true
     disallow_untyped_defs = true
     ignore_missing_imports = true
     no_implicit_optional = true
     warn_redundant_casts = true
     warn_return_any = true
     warn_unused_ignores = true
    
  4. You reuse variable names with completely different semantics. This is a really bad idea for understanding what the code is doing and following along even otherwise trivial logic.

  5. Names should be descriptive, for example password_database = PasswordDatabase().

  6. Don't use *args and **kwargs unless you need dynamic parameter lists. Rather than indexing *args you should use named parameters. If they have default values those should go in the method signature.

  7. .get(foo, None) can be simplified to .get(foo) - get() returns None by default.

  8. if foo is None can in the vast majority of cases be changed to the more idiomatic if foo.

  9. I would highly recommend using a well-known open format such as the KeePass one for storing this data.

  10. This should not be in there:

     if not sample == settings["enc_sample_content"]:
         raise ValueError(
             "Cannot open PassDB: incorrect password provided")
    
  11. There is a lot of encoding and decoding happening, which greatly obfuscates the state and looks unnecessary in several places.

  12. I would not trust this sort of code without a comprehensive test suite.

With the caveat that I'm not a cryptographer:

  1. Salting does not make sense unless you're hashing the password (which you don't want to do in this case). I'll refrain from any other comments on how the salting is done unless someone corrects this.
added 401 characters in body
Source Link
l0b0
  • 9.1k
  • 22
  • 36

Some general tips:

  1. The runner should use argparse to parse arguments. It most definitely should not hardcode passwords.

  2. (object) is redundant in Python 3 class definitions.

  3. I'd recommend running any Python code through Black, pyflakes and mypy with a strict configuration like this one:

     [flake8]
     doctests = true
     exclude =
         .git
     max-complexity = 5
     max-line-length = 120
     ignore = W503,E203
    
     [mypy]
     check_untyped_defs = true
     disallow_untyped_defs = true
     ignore_missing_imports = true
     no_implicit_optional = true
     warn_redundant_casts = true
     warn_return_any = true
     warn_unused_ignores = true
    
  4. You reuse variable names with completely different semantics. This is a really bad idea for understanding what the code is doing and following along even otherwise trivial logic.

  5. Names should be descriptive, for example password_database = PasswordDatabase().

  6. Don't use *args and **kwargs unless you need dynamic parameter lists. Rather than indexing *args you should use named parameters. If they have default values those should go in the method signature.

  7. .get(foo, None) can be simplified to .get(foo) - get() returns None by default.

  8. if foo is None can in the vast majority of cases be changed to the more idiomatic if foo.

  9. I would highly recommend using a well-known open format such as the KeePass one for storing this data.

  10. This should not be in there:

     if not sample == settings["enc_sample_content"]:
         raise ValueError(
             "Cannot open PassDB: incorrect password provided")
    

With the caveat that I'm not a cryptographer:

  1. Salting does not make sense unless you're hashing the password (which you don't want to do in this case). I'll refrain from any other comments on how the salting is done unless someone corrects this.

Some general tips:

  1. The runner should use argparse to parse arguments. It most definitely should not hardcode passwords.

  2. (object) is redundant in Python 3 class definitions.

  3. I'd recommend running any Python code through Black, pyflakes and mypy with a strict configuration like this one:

     [flake8]
     doctests = true
     exclude =
         .git
     max-complexity = 5
     max-line-length = 120
     ignore = W503,E203
    
     [mypy]
     check_untyped_defs = true
     disallow_untyped_defs = true
     ignore_missing_imports = true
     no_implicit_optional = true
     warn_redundant_casts = true
     warn_return_any = true
     warn_unused_ignores = true
    
  4. You reuse variable names with completely different semantics. This is a really bad idea for understanding what the code is doing and following along even otherwise trivial logic.

  5. Names should be descriptive, for example password_database = PasswordDatabase().

With the caveat that I'm not a cryptographer:

  1. Salting does not make sense unless you're hashing the password (which you don't want to do in this case).

Some general tips:

  1. The runner should use argparse to parse arguments. It most definitely should not hardcode passwords.

  2. (object) is redundant in Python 3 class definitions.

  3. I'd recommend running any Python code through Black, pyflakes and mypy with a strict configuration like this one:

     [flake8]
     doctests = true
     exclude =
         .git
     max-complexity = 5
     max-line-length = 120
     ignore = W503,E203
    
     [mypy]
     check_untyped_defs = true
     disallow_untyped_defs = true
     ignore_missing_imports = true
     no_implicit_optional = true
     warn_redundant_casts = true
     warn_return_any = true
     warn_unused_ignores = true
    
  4. You reuse variable names with completely different semantics. This is a really bad idea for understanding what the code is doing and following along even otherwise trivial logic.

  5. Names should be descriptive, for example password_database = PasswordDatabase().

  6. Don't use *args and **kwargs unless you need dynamic parameter lists. Rather than indexing *args you should use named parameters. If they have default values those should go in the method signature.

  7. .get(foo, None) can be simplified to .get(foo) - get() returns None by default.

  8. if foo is None can in the vast majority of cases be changed to the more idiomatic if foo.

  9. I would highly recommend using a well-known open format such as the KeePass one for storing this data.

  10. This should not be in there:

     if not sample == settings["enc_sample_content"]:
         raise ValueError(
             "Cannot open PassDB: incorrect password provided")
    

With the caveat that I'm not a cryptographer:

  1. Salting does not make sense unless you're hashing the password (which you don't want to do in this case). I'll refrain from any other comments on how the salting is done unless someone corrects this.
Source Link
l0b0
  • 9.1k
  • 22
  • 36
Loading