Merge pull request #1102 from common-workflow-language/bandit

turn on  static security analysis using bandit

Author: mr-c

Makefile

@@ -27,7 +27,7 @@ MODULE=cwltool
 # `[[` conditional expressions.
 PYSOURCES=$(wildcard ${MODULE}/**.py tests/*.py) setup.py
 DEVPKGS=pycodestyle diff_cover autopep8 pylint coverage pydocstyle flake8 \
-	pytest-xdist isort wheel -rtest-requirements.txt
+	pytest-xdist==1.27.0 isort wheel -rtest-requirements.txt
 DEBDEVPKGS=pep8 python-autopep8 pylint python-coverage pydocstyle sloccount \
 	   python-flake8 python-mock shellcheck
 VERSION=1.0.$(shell TZ=UTC git log --first-parent --max-count=1 \

appveyor.yml

@@ -32,7 +32,7 @@ install:
   - ps: 'Install-Product node 0.12 x64'
   - "set PATH=%PYTHON%\\Scripts;%PATH%"
   - "%PYTHON%\\python.exe -m pip install -U pip setuptools^>=20.3 wheel"
-  - "%PYTHON%\\python.exe -m pip install -U codecov -rtest-requirements.txt pytest-xdist "
+  - "%PYTHON%\\python.exe -m pip install -U codecov -rtest-requirements.txt pytest-xdist==1.27 "
     # Note the use of a `^` to escape the `>`
 
 build_script:

cwltool/builder.py

@@ -207,13 +207,12 @@ def bind_input(self,
         value_from_expression = False
         if "inputBinding" in schema and isinstance(schema["inputBinding"], MutableMapping):
             binding = CommentedMap(schema["inputBinding"].items())
-            assert binding is not None
 
             bp = list(aslist(lead_pos))
             if "position" in binding:
                 position = binding["position"]
-                if isinstance(position, str):  # no need to test the CWL Version
-                                               # the schema for v1.0 only allow ints
+                if isinstance(position, str):   # no need to test the CWL Version
+                                                # the schema for v1.0 only allow ints
                     binding['position'] = self.do_eval(position, context=datum)
                     bp.append(binding['position'])
                 else:
@@ -238,7 +237,6 @@ def bind_input(self,
                     avsc = self.names.get_name(t["name"], "")
                 if not avsc:
                     avsc = make_avsc_object(convert_to_dict(t), self.names)
-                assert avsc is not None
                 if validate.validate(avsc, datum):
                     schema = copy.deepcopy(schema)
                     schema["type"] = t
@@ -379,7 +377,6 @@ def tostr(self, value):  # type: (Any) -> Text
                 # docker_req is none only when there is no dockerRequirement
                 # mentioned in hints and Requirement
                 path = docker_windows_path_adjust(value["path"])
-                assert path is not None
                 return path
             return value["path"]
         else:
@@ -424,8 +421,7 @@ def generate_arg(self, binding):  # type: (Dict[Text, Any]) -> List[Text]
             if sep:
                 args.extend([prefix, self.tostr(j)])
             else:
-                assert prefix is not None
-                args.append(prefix + self.tostr(j))
+                args.append("" if not prefix else prefix + self.tostr(j))
 
         return [a for a in args if a is not None]
 

cwltool/command_line_tool.py

@@ -157,7 +157,8 @@ def revmap_file(builder, outdir, f):
         if "basename" not in f:
             f["basename"] = os.path.basename(path)
 
-        assert builder.pathmapper is not None
+        if not builder.pathmapper:
+            raise ValueError("Do not call revmap_file using a builder that doesn't have a pathmapper.")
         revmap_f = builder.pathmapper.reversemap(path)
 
         if revmap_f and not builder.pathmapper.mapper(revmap_f[0]).type.startswith("Writable"):
@@ -204,7 +205,8 @@ def check_adjust(builder, file_o):
     doesn't reach everything in builder.bindings
     """
 
-    assert builder.pathmapper is not None
+    if not builder.pathmapper:
+            raise ValueError("Do not call check_adjust using a builder that doesn't have a pathmapper.")
     file_o["path"] = docker_windows_path_adjust(
         builder.pathmapper.mapper(file_o["location"])[1])
     dn, bn = os.path.split(file_o["path"])
@@ -300,7 +302,7 @@ def job(self,
         if runtimeContext.cachedir and enableReuse:
             cachecontext = runtimeContext.copy()
             cachecontext.outdir = "/out"
-            cachecontext.tmpdir = "/tmp"
+            cachecontext.tmpdir = "/tmp"  # nosec
             cachecontext.stagedir = "/stage"
             cachebuilder = self._init_job(job_order, cachecontext)
             cachebuilder.pathmapper = PathMapper(cachebuilder.files,
@@ -354,7 +356,8 @@ def job(self,
 
             keydictstr = json_dumps(keydict, separators=(',', ':'),
                                     sort_keys=True)
-            cachekey = hashlib.md5(keydictstr.encode('utf-8')).hexdigest()
+            cachekey = hashlib.md5(  # nosec
+                keydictstr.encode('utf-8')).hexdigest()
 
             _logger.debug("[job %s] keydictstr is %s -> %s", jobname,
                           keydictstr, cachekey)
@@ -477,24 +480,24 @@ def rm_pending_output_callback(output_callbacks, jobcachepending,
         if self.tool.get("stdin"):
             with SourceLine(self.tool, "stdin", validate.ValidationException, debug):
                 j.stdin = builder.do_eval(self.tool["stdin"])
-                assert j.stdin is not None
-                reffiles.append({"class": "File", "path": j.stdin})
+                if j.stdin:
+                    reffiles.append({"class": "File", "path": j.stdin})
 
         if self.tool.get("stderr"):
             with SourceLine(self.tool, "stderr", validate.ValidationException, debug):
                 j.stderr = builder.do_eval(self.tool["stderr"])
-                assert j.stderr is not None
-                if os.path.isabs(j.stderr) or ".." in j.stderr:
-                    raise validate.ValidationException(
-                        "stderr must be a relative path, got '%s'" % j.stderr)
+                if j.stderr:
+                    if os.path.isabs(j.stderr) or ".." in j.stderr:
+                        raise validate.ValidationException(
+                            "stderr must be a relative path, got '%s'" % j.stderr)
 
         if self.tool.get("stdout"):
             with SourceLine(self.tool, "stdout", validate.ValidationException, debug):
                 j.stdout = builder.do_eval(self.tool["stdout"])
-                assert j.stdout is not None
-                if os.path.isabs(j.stdout) or ".." in j.stdout or not j.stdout:
-                    raise validate.ValidationException(
-                        "stdout must be a relative path, got '%s'" % j.stdout)
+                if j.stdout:
+                    if os.path.isabs(j.stdout) or ".." in j.stdout or not j.stdout:
+                        raise validate.ValidationException(
+                            "stdout must be a relative path, got '%s'" % j.stdout)
 
         if debug:
             _logger.debug(u"[job %s] command line bindings is %s", j.name,
@@ -717,7 +720,7 @@ def collect_output(self,
                                 files["contents"] = content_limit_respected_read_bytes(f).decode("utf-8")
                         if compute_checksum:
                             with fs_access.open(rfile["location"], "rb") as f:
-                                checksum = hashlib.sha1()
+                                checksum = hashlib.sha1()  # nosec
                                 contents = f.read(1024 * 1024)
                                 while contents != b"":
                                     checksum.update(contents)

cwltool/cwlrdf.py

@@ -27,7 +27,6 @@ def printrdf(wflow, ctx, style):  # type: (Process, ContextType, Text) -> Text
     rdf = gather(wflow, ctx).serialize(format=style, encoding='utf-8')
     if not rdf:
         return u""
-    assert rdf is not None
     return rdf.decode('utf-8')
 
 

cwltool/docker.py

@@ -155,7 +155,7 @@ def get_image(docker_requirement,     # type: Dict[Text, Text]
                 else:
                     loadproc = subprocess.Popen(cmd, stdin=subprocess.PIPE,
                                                 stdout=sys.stderr)
-                    assert loadproc.stdin is not None
+                    assert loadproc.stdin is not None  # nosec
                     _logger.info(u"Sending GET request to %s", docker_requirement["dockerLoad"])
                     req = requests.get(docker_requirement["dockerLoad"], stream=True)
                     size = 0
@@ -296,7 +296,8 @@ def create_runtime(self,
             runtime = [u"docker", u"run", u"-i"]
         self.append_volume(runtime, os.path.realpath(self.outdir),
                            self.builder.outdir, writable=True)
-        self.append_volume(runtime, os.path.realpath(self.tmpdir), "/tmp",
+        tmpdir = "/tmp"  # nosec
+        self.append_volume(runtime, os.path.realpath(self.tmpdir), tmpdir,
                            writable=True)
         self.add_volumes(self.pathmapper, runtime, any_path_okay=True,
                          secret_store=runtimeContext.secret_store,

cwltool/executors.py

@@ -167,17 +167,17 @@ def run_jobs(self,
                         self.output_dirs.add(job.outdir)
                     if runtime_context.research_obj is not None:
                         if not isinstance(process, Workflow):
-                            runtime_context.prov_obj = process.provenance_object
+                            prov_obj = process.provenance_object
                         else:
-                            runtime_context.prov_obj = job.prov_obj
-                        assert runtime_context.prov_obj
-                        runtime_context.prov_obj.evaluate(
-                            process, job, job_order_object,
-                            runtime_context.research_obj)
-                        process_run_id =\
-                            runtime_context.prov_obj.record_process_start(
-                                process, job)
-                        runtime_context = runtime_context.copy()
+                            prov_obj = job.prov_obj
+                        if prov_obj:
+                            runtime_context.prov_obj = prov_obj
+                            prov_obj.evaluate(
+                                process, job, job_order_object,
+                                runtime_context.research_obj)
+                            process_run_id =\
+                                prov_obj.record_process_start(process, job)
+                            runtime_context = runtime_context.copy()
                         runtime_context.process_run_id = process_run_id
                     job.run(runtime_context)
                 else:

cwltool/expression.py

@@ -261,8 +261,8 @@ def do_eval(ex,                       # type: Union[Text, Dict]
            ):  # type: (...) -> Any
 
     runtime = copy.deepcopy(resources)  # type: Dict[str, Any]
-    runtime["tmpdir"] = docker_windows_path_adjust(tmpdir)
-    runtime["outdir"] = docker_windows_path_adjust(outdir)
+    runtime["tmpdir"] = docker_windows_path_adjust(tmpdir) if tmpdir else None
+    runtime["outdir"] = docker_windows_path_adjust(outdir) if outdir else None
 
     rootvars = {
         u"inputs": jobinput,
@@ -274,8 +274,7 @@ def do_eval(ex,                       # type: Union[Text, Dict]
     if six.PY3:
         rootvars = bytes2str_in_dicts(rootvars)  # type: ignore
 
-    if needs_parsing(ex):
-        assert isinstance(ex, string_types)
+    if isinstance(ex, string_types) and needs_parsing(ex):
         fullJS = False
         jslib = u""
         for r in reversed(requirements):

cwltool/job.py

@@ -257,10 +257,14 @@ def _execute(self,
                      u' 2> %s' % os.path.join(self.outdir, self.stderr) if self.stderr else '')
         if self.joborder is not None and runtimeContext.research_obj is not None:
             job_order = self.joborder
-            assert runtimeContext.process_run_id is not None
-            assert runtimeContext.prov_obj is not None
-            runtimeContext.prov_obj.used_artefacts(
-                job_order, runtimeContext.process_run_id, str(self.name))
+            if runtimeContext.process_run_id is not None \
+                    and runtimeContext.prov_obj is not None:
+                runtimeContext.prov_obj.used_artefacts(
+                    job_order, runtimeContext.process_run_id, str(self.name))
+            else:
+                _logger.warning("research_obj set but one of process_run_id "
+                        "or prov_obj is missing from runtimeContext: "
+                        "{}". format(runtimeContext))
         outputs = {}  # type: MutableMapping[Text,Any]
         try:
             stdin_path = None
@@ -323,10 +327,13 @@ def _execute(self,
                 processStatus = "permanentFail"
 
             if 'listing' in self.generatefiles:
-                assert self.generatemapper is not None
-                relink_initialworkdir(
-                    self.generatemapper, self.outdir, self.builder.outdir,
-                    inplace_update=self.inplace_update)
+                if self.generatemapper:
+                    relink_initialworkdir(
+                        self.generatemapper, self.outdir, self.builder.outdir,
+                        inplace_update=self.inplace_update)
+                else:
+                    raise ValueError("'lsiting' in self.generatefiles but no "
+                        "generatemapper was setup.")
 
             outputs = self.collect_outputs(self.outdir, rcode)
             outputs = bytes2str_in_dicts(outputs)  # type: ignore

cwltool/load_tool.py

@@ -108,22 +108,18 @@ def fetch_document(argsworkflow,        # type: Union[Text, Dict[Text, Any]]
         if loadingContext.loader is None:
             loadingContext.loader = default_loader(loadingContext.fetcher_constructor)
 
-    uri = None  # type: Optional[Text]
-    workflowobj = None  # type: Optional[CommentedMap]
     if isinstance(argsworkflow, string_types):
         uri, fileuri = resolve_tool_uri(argsworkflow,
                                         resolver=loadingContext.resolver,
                                         document_loader=loadingContext.loader)
         workflowobj = loadingContext.loader.fetch(fileuri)
-    elif isinstance(argsworkflow, dict):
+        return loadingContext, workflowobj, uri
+    if isinstance(argsworkflow, dict):
         uri = argsworkflow["id"] if argsworkflow.get("id") else "_:" + Text(uuid.uuid4())
         workflowobj = cast(CommentedMap, cmap(argsworkflow, fn=uri))
         loadingContext.loader.idx[uri] = workflowobj
-    else:
-        raise ValidationException("Must be URI or object: '%s'" % argsworkflow)
-    assert workflowobj is not None
-
-    return loadingContext, workflowobj, uri
+        return loadingContext, workflowobj, uri
+    raise ValidationException("Must be URI or object: '%s'" % argsworkflow)
 
 
 def _convert_stdstreams_to_files(workflowobj):
@@ -152,10 +148,9 @@ def _convert_stdstreams_to_files(workflowobj):
                                 filename = workflowobj[streamtype]
                             else:
                                 filename = Text(
-                                    hashlib.sha1(json_dumps(workflowobj,
-                                                            sort_keys=True
-                                                           ).encode('utf-8')
-                                                ).hexdigest())
+                                    hashlib.sha1(  # nosec
+                                        json_dumps(workflowobj, sort_keys=True
+                                                  ).encode('utf-8')).hexdigest())
                                 workflowobj[streamtype] = filename
                             out['type'] = 'File'
                             out['outputBinding'] = cmap({'glob': filename})