PR https://github.com/Keelvar/SimpleSecurity

[psmgeelen]

Hi FHPyhtonUtils,
My organization has forked your repo and adapted your library to our needs. We would love to contribute our developments back into your repo. It probably makes sense to align with one another before committing a PR. This is a first for me, so how would you like to proceed?

Regards Pieter

[FredHappyface]

Hi thank you for opening the issue!

Sounds interesting! I'll take a look at what you've done. Really glad that the project has proven useful 😊

[pieterge]

Hi FHPyhtonUtils,
Please let me know if there is any feedback :) Regards

[FredHappyface]

Hi just got around to this. Ended up being a semi-detailed code review so hope that's alright :D

Going to break down into the following:

• May need some changes to be included in upstream
• Would be good to see in upstream
• Out of scope/ no thank you
• Misc

May need some changes to be included in upstream

1. My projects use tabs, understandable that you've used spaces (pep8 compliant and tbh makes linting/ formatting easier). But that'd be something to swap back temporarily to merge back
2. I like the use of rst docstrings, though typing has been sacrificed (there's a handy vscode ext that fills them in for you if your org uses that)

Would be good to see in upstream

1. Trivy looks pretty cool too. I've seen it be used for scanning images in a harbor instance (a docker repo) and wasn't aware of python uses. Definitely would be great in upstream
2. Really like the use of scan_dir, think that'd be really useful in upstream
3. dlint implementation is far nicer and more resillient than doing string splits and all that sorta stuff

Out of scope/ no thank you

1. Git integration is interesting: though this tightly couples a consumer to github so probably wants to stay out of upstream
2. Like the use of typical linters (or tools in linting mode such as black). It's not clear to me how best to include these as the lib has a security focus. Whilst regular bugs can be security related, a lot of pylint issues are just noise in that context and formatting is unlikely to indicate a security issue
3. Note: when you use from __future__ import annotations you can avoid using from typing import Dict, List when targeting python 3.7+
4. Sphinx doc generator (happy using handsdown to generate .md for renderer (supported by main git hosting providers and readable in vim for those that thing web browsers are evil XD

Misc

1. RE "TODO Do we need to run poetry lock before commiting to poetry show? " -> Answer is 'yes' (had this confirmed by another project. IMO there must be a better way of doing this but not sure what that is yet)

Thanks

[FredHappyface]

Hi @pieterge just checking you've seen the previous message and curious on how you'd like to proceed? I realize there'd be a fair bit involved in getting changes back upstream in this case. Few options I can see would be

1. Close the issue as won't do (there's a couple changes I'd be keen on merging so am happy to do that manually)
2. Assign resource to merge. As noted I appreciate this would involve some overhead
3. Negotiate any changes. Can't think of what this would entail tbh

Please let me know if you have any preference. Thanks 🙂

[pieterge]

Very much so! Schedule is not being nice to atm. I should have time by the end of next week to assess and commit a PR. Would that fit your timeline?

[FredHappyface]

Yeah that'd work great. Thanks very much! 🙂

[pieterge]

Sorry for my delay, lots going on. Current the development of simplesecurity has been deprioritized I will have more clarity on Wednesday.

[psmgeelen]

Hey @FredHappyface, I decided to help this project along privately. To answer your questions:

1. I can accommodate for the tabs :)
2. What do you mean by this comment? Isn't the typing handled on declarative level when writing the objects?
3. Currently, Trivy is installed via an additional Shell script and is not integrated as a pyproject.toml dependency. I am second guessing the value of Trivy when compared to SemGrep atm, as they seem similar.
4. I will keep the scan_dir
5. Thank you for your comments on dlint implementation.
6. The Git integration has been implemented in a modular way. So we can add or remove it when we want to easily.
7. I am currently looking into ruff https://github.com/charliermarsh/ruff, which seems to make far more sense than all those other linters as it combines linters, is more performant as it is implemented in rust and has a similar, integrated approach as SimpleSecurity. I would keep linters out of scope for now.
8. I will add future imports
9. I would love to have sphinx in this project. I would be great if we can have excellent documentation that is interactive. If you are open to discuss this further, I would love to do so :)
10. I dont fully understand your Misc remark
    I will setup a PR and we can work on it together, if that is OK with you. I am assuming that this wil live a little longer, as we have quiet a few details to work through :).

[FredHappyface]

Hey cheers for the response @psmgeelen !

1. Awesome thanks
2. In python you can use loads of different docstring format, here are a few:

   def test1(a: int, b: str) -> str:
      """Convert a to a string and append b
   
      :param int a: some number, eg 10
      :param str b: some string, eg 'hello'
      :return str: a to a string and b, eg '10hello'
      """
      return str(a) + b
   
   
   def test2(a: int, b: str) -> str:
      """Convert a to a string and append b
   
      :param a: some number, eg 10
      :param b: some string, eg 'hello'
      :return: a to a string and b, eg '10hello'
      """
      return str(a) + b
   
   
   def test3(a: int, b: str) -> str:
      """Convert a to a string and append b
   
      Args:
          a (int): some number, eg 10
          b (str): some string, eg 'hello'
   
      Returns:
          str: a to a string and b, eg '10hello'
      """
      return str(a) + b

3. Hmm one of my concerns is that this tool is supposed to be cross platform (mostly) though happy to keep for now :) 
4. Thank you :) 
5. No worries, though does this require an extra dependency? Is this widely used? (Happy to pull in other dependencies but always adds a maintenance concern)
6. Oh awesome, ta 
7. Heard a bit about this, seems promising for typical use. Though you'll probably want to consider adding a type checker if you're using it to replace pylint and find the type related warnings useful, as ruff doesn't support typing 
8. 👍
9. Problem with sphinx for me is hosting it, happy to include gh actions for it. Though I'd like to keep md docs (happy to regenerate these post merge to keep things simple)
10. There was a comment in code for a todo and iirc it was in response to that. Though I must admit I cannot remember the details