Vendor new safety (#5217)

[matteius]

• Vendor safety==2.1.1 cleanly with ruamel.

• Apply more minimal patch to safety.

Thank you for contributing to Pipenv!

The issue

The new version of safety has strange output:

matteius@matteius-VirtualBox:~/pipenv$ pipenv check
Checking PEP 508 requirements...
Passed!
Checking installed package safety...
r: r e resolved (p installed)!
o

n: s c resolved (a installed)!
n

c: a f resolved (f installed)!
e

u: a n resolved (n installed)!
o

e: v u resolved (l installed)!
n

r: i g resolved (n installed)!
o

d: r e resolved (m installed)!
e

[yeisonvargasf]

Hi @matteius!

I'm Yeison from the https://pyup.io team.

Thank you for the effort in integrating this new Safety major version with more features for the users.

The issue is related to the new JSON structure used in Safety 2.x.x that includes further information like remediations for users using an API Key.

You can know more about the JSON output in the following docs

The parsing needs to be fixed in the following line:

pipenv/pipenv/core.py

Line 2782 in ca38db6

for (package, resolved, installed, description, vuln, *_) in results:

It would be nice if we can output the Safety screen output to the Pipenv user as we won't need to make a double effort rendering the results.

On the other side, I think we will need to define more options here, as Safety 2.x.x includes now better handling for exit codes and a policy file that will be useful for the users.

I'll be happy to contribute to this PR if you want to.

[matteius]

@yeisonvargasf Thanks! Sure -- if you have some spare cycles to help with this, probably the way to do this is you can fork pipenv and create a branch based on this branch vendor-latest-safety2 and open a PR back to this branch. I'll be vendoring in the updated safety package when it gets released next to this same PR.

[yeisonvargasf]

Hi @matteius!

I created a draft PR with the work in progress. I'll release a new Safety version in the coming days that makes some adjustments for the integration; therefore, we will need vendor Safety again using that version.

Please note the output change; as this Safety version includes more information, I removed the pipenv output in favor of the new Safety output.

On the other side, there is an issue when Safety tries to read a policy file because Safety uses ruamel to parse it, and the vendor version throws the error module 'ruamel.yaml' has no attribute 'resolver':

pipenv/pipenv/vendor/ruamel/yaml/main.py

Line 77 in 026be80

self.Resolver = ruamel.yaml.resolver.VersionedResolver # type: Any

Could you look at that issue when you return to this pull request?

Thanks!

[matteius]

@yeisonvargasf If you change that issue line ruamel.yaml.resolver.VersionedResolver to pipenv.vendor.ruamel.yaml.resolver.VersionedResolver does it resolve the module? Actually it is probably that top level import of import ruamel.yaml -- this is problematic for the way the pipenv vendoring process re-writes imports. It would be better if that library just did from ruamel.yaml.resolver impoty VersionedResolver because then the imports would be re-written correctly by the pipenv vendoring scripts. It is unfortunate that we are vendoring in ruamel just for this purpose of the safety command -- for what its worth, it would be more ideal if safety did not have this dependency and used instead tomlkit or tomli.

[matteius]

@yeisonvargasf Actually I see now you are using ruamel not for parsing toml files, but for the response of the safety json data I think -- why not use the built in python module yaml?

[yeisonvargasf]

@matteius

If you change that issue line ruamel.yaml.resolver.VersionedResolver to pipenv.vendor.ruamel.yaml.resolver.VersionedResolver does it resolve the module?

Yes, it's related to the top-level import. Any suggestion about how to address it? I'm not sure how works the pipenv vendoring process; any clue to go in the right direction would be great.

why not use the built in python module yaml?

We are using YAML 1.2 for our policy file; sadly, as far as I'm aware pyyaml doesn't support yet YAML 1.2 and yaml doesn't belong to the python standard library so we would need to vendor it like ruamel.

By the way, I agree it would be ideal if safety did not have this dependency, but I don't see a way to achieve that at the moment.

Thanks for the quick response on this, let me know your thoughts about the ruamel dependency.

[matteius]

@yeisonvargasf Thanks for the explanations -- I think the only way around that path import issue is to either add a patch file, which assuredly will work, or we possibly add the right regex to the GLOBAL_REPLACEMENT -- both things are within the tasks directory -- the current set of patches are in pipenv/tasks/vendoring/patches -- I can help with this part, but first I am going to merge your PR into this branch and then resolve conflicts/update with main.

[matteius]

@yeisonvargasf I think I got the vendoring of ruamel sorted out the best that I can without doing an explicit patch file, which should be avoided if possible. Please have a look at this branch again as its the latest main + your changes + my vendoring fixes.

matteius@matteius-VirtualBox:~/pipenv$ pipenv check
Loading .env environment variables...
Checking PEP 508 requirements...
Passed!
Checking installed package safety...
Running the command
+==============================================================================+

                               /$$$$$$            /$$
                              /$$__  $$          | $$
           /$$$$$$$  /$$$$$$ | $$  \__//$$$$$$  /$$$$$$   /$$   /$$
          /$$_____/ |____  $$| $$$$   /$$__  $$|_  $$_/  | $$  | $$
         |  $$$$$$   /$$$$$$$| $$_/  | $$$$$$$$  | $$    | $$  | $$
          \____  $$ /$$__  $$| $$    | $$_____/  | $$ /$$| $$  | $$
          /$$$$$$$/|  $$$$$$$| $$    |  $$$$$$$  |  $$$$/|  $$$$$$$
         |_______/  \_______/|__/     \_______/   \___/   \____  $$
                                                          /$$  | $$
                                                         |  $$$$$$/
  by pyup.io                                              \______/

+==============================================================================+

 REPORT 

  Safety v2.1.1 is scanning for Vulnerabilities...
  Scanning dependencies in your environment:

  -> /home/matteius/.virtualenvs/pipenv/lib/python3.10/site-packages

  Using local file https://raw.githubusercontent.com/pyupio/safety-
    db/master/data/ database
  Found and scanned 118 packages
  Timestamp 2022-09-15 09:54:41
  0 vulnerabilities found
  0 vulnerabilities ignored
+==============================================================================+

 No known security vulnerabilities found. 

+==============================================================================+

[matteius]

Since this runs in a subprocess, we will need an importlib patch similar to: https://github.com/pypa/pipenv/blob/main/tasks/vendoring/patches/patched/_post_pip_import.patch#L9-L15
But also I am wondering, maybe we can invoke it directly without using sub-process -- I was testing out doing something similar with the graph command recently, and it may be possible using a similar approach to this change:

+++ b/pipenv/core.py
@@ -2885,27 +2885,9 @@ def do_check(
 def do_graph(project, bare=False, json=False, json_tree=False, reverse=False):
     import json as jsonlib
 
-    from pipenv.vendor import pipdeptree
-
-    pipdeptree_path = os.path.dirname(pipdeptree.__file__.rstrip("cdo"))
-    try:
-        python_path = project._which("python")
-    except AttributeError:
-        click.echo(
-            "{}: {}".format(
-                click.style("Warning", fg="red", bold=True),
-                "Unable to display currently-installed dependency graph information here. "
-                "Please run within a Pipenv project.",
-            ),
-            err=True,
-        )
-        sys.exit(1)
-    except RuntimeError:
-        pass
-    else:
-        if not os.name == "nt":  # bugfix #4388
-            python_path = Path(python_path).as_posix()
-            pipdeptree_path = Path(pipdeptree_path).as_posix()
+    from pipenv.vendor.pipdeptree import main
 
     if reverse and json:
         click.echo(
@@ -2956,70 +2938,25 @@ def do_graph(project, bare=False, json=False, json_tree=False, reverse=False):
             err=True,
         )
         sys.exit(1)
-    cmd_args = [python_path, pipdeptree_path, "-l"]
+
+    cmd_args = ["-l"]
     if flag:
         cmd_args.append(flag)
-    c = run_command(cmd_args, is_verbose=project.s.is_verbose())
-    # Run dep-tree.
-    if not bare:
-        if json:
-            data = []
-            try:
-                parsed = simplejson.loads(c.stdout.strip())
-            except jsonlib.JSONDecodeError:
-                raise exceptions.JSONParseError(c.stdout, c.stderr)
-            else:
-                for d in parsed:
-                    if d["package"]["key"] not in BAD_PACKAGES:
-                        data.append(d)
-            click.echo(simplejson.dumps(data, indent=4))
-            sys.exit(0)
-        elif json_tree:
-
-            def traverse(obj):
-                if isinstance(obj, list):
-                    return [
-                        traverse(package)
-                        for package in obj
-                        if package["key"] not in BAD_PACKAGES
-                    ]
-                else:
-                    obj["dependencies"] = traverse(obj["dependencies"])
-                    return obj
-
-            try:
-                parsed = simplejson.loads(c.stdout.strip())
-            except jsonlib.JSONDecodeError:
-                raise exceptions.JSONParseError(c.stdout, c.stderr)
-            else:
-                data = traverse(parsed)
-                click.echo(simplejson.dumps(data, indent=4))
-                sys.exit(0)
-        else:
-            for line in c.stdout.strip().split("\n"):
-                # Ignore bad packages as top level.
-                # TODO: This should probably be a "==" in + line.partition
-                if line.split("==")[0] in BAD_PACKAGES and not reverse:
-                    continue
-
-                # Bold top-level packages.
-                if not line.startswith(" "):
-                    click.echo(click.style(line, bold=True))
-                # Echo the rest.
-                else:
-                    click.echo(click.style(line, bold=False))
-    else:
-        click.echo(c.stdout)
-    if c.returncode != 0:
+    sys.argv[1:] = cmd_args
+    try:
+        main()
+    except Exception as e:
         click.echo(
             "{} {}".format(
                 click.style("ERROR: ", fg="red", bold=True),
-                click.style(f"{c.stderr}", fg="white"),
+                click.style(f"{e}", fg="white"),
             ),
             err=True,
         )
+        sys.exit(1)
+
     # Return its return code.
-    sys.exit(c.returncode)
+    sys.exit(0)
 
 
 def do_sync(
diff --git a/tasks/vendoring/__init__.py b/tasks/vendoring/__init__.py
index bbd1a8cc..49bb98e7 100644
--- a/tasks/vendoring/__init__.py
+++ b/tasks/vendoring/__init__.py
@@ -64,6 +64,7 @@ LIBRARY_RENAMES = {
     "packaging": "pipenv.patched.pip._vendor.packaging",
     "urllib3": "pipenv.patched.pip._vendor.urllib3",
     "zipp": "pipenv.vendor.zipp",
+    "pipdeptree": "pipenv.vendor.pipdeptree",
 }

[derula]

Quick opinion: I've always found default safety output overly verbose, especially the 15 lines of logo. Part of why I liked the pipenv check command is because it made the output more compact. This will get lost by just showing safety's output directly.

That said, in my CI builds I have since replaced pipenv check with a small Python script that gets a JSON from safety and formats it manually, so this change wouldn't affect me personally. Just saying, it may seem like a "removed feature" to some users.

[yeisonvargasf]

Hi @matteius! Glad to be in touch with you again; I'm sorry for the delay; we were working on improving our databases and data.

I am wondering, maybe we can invoke it directly without using sub-process

Yes, it's possible; I'll make a PR with these changes. So you can review them.

@matteius, once I make the PR, I'll release a new Safety version, and it'll be great if you can help me with the vendoring of that latest Safety version, so we can get everything ready to get this update done.

@derula, thank you for your observation; I'll include a new non-verbose option in the output formats. However, I'll keep the Safety screen report as the default because it now offers more information about the scanned packages and more resources to read about the vulnerabilities.

[yeisonvargasf]

Hi @matteius, I made a PR with the changes to follow your idea not to use a subprocess, and also I added a minimal output following the @derula opinion.

@matteius, please check if all looks good from your side with these new changes; I'll release a new Safety version in the coming days, addressing an issue related to ruamel inside of the pipenv check command; after that, we'll need to vendoring the latest Safety version.

Thanks for your patience. If you have any comments, let me know. 👍

[derula]

@yeisonvargasf nice, thanks for the consideration! Defaulting to Safety's standard output definitely seems like a good idea, and having the option I think is useful for getting a quick overview of problems, You can always run the command in default mode again to get detailed information.

[yeisonvargasf]

@matteius, @oz123 approved #5480 PR. Could you review it and merge it if you consider it okay?

If you find anything to be fixed, let me know. Thanks!

[yeisonvargasf]

@matteius, thanks for the quick merging! Could you vendoring Safety 2.3.2? This integration needs Safety 2.3.2 and is the most secure version.

Let me know if you want me to help in the vendoring (Happy to help if you share documentation about how to do it).

[matteius]

@yeisonvargasf I have updated the vendoring. If you have to do this again in the future, the process is somewhat straightforward:
1.) Update the patched/patched.txt (or vendor/vendor.txt) file
2.) run pipenv sync -d and then pipenv run invoke vendoring.update and that revendors everything based on those specifiers
3.) Sometimes a patch for the patched version of vendoring breaks and needs to be updated as a result -- safety does have a patch file -- these live in tasks/vendoring/patches/

[matteius]

@yeisonvargasf The one safety test in the project is failing because safety scans the root project virtualenv and not the one created by the test. I marked the test as skip for now, but if you have a chance can you take a look at it? I won't let it hold up this PR since the functionality appears to be working now but it would be good to figure out how to reinstate this test.

[yeisonvargasf]

Thank you @matteius, for your work and quick responses!

I left a PR for your review: #5492.

After fixing #5492 locally, I continued testing and found #5493. Could you help me to take a look at it?

From my side, I'm looking at a solution for #5488 and #4819.