Repository: common-workflow-language/cwltool

Issue:
CommandLineTool output filenames are URL encoded
## Expected Behavior
CommandLineTool outputs whose filenames contain special characters should retain those special characters when copied to the final output directory. For example, if a CommandLineTool writes an output file with the name "(parentheses).txt", then the filename should remain "(parentheses).txt" when copied/moved to the final output directory.

## Actual Behavior
Filenames of CommandLineTool outputs are URL encoded after being copied/moved to the final output directory. For example, an output filename of "(parentheses).txt" becomes "%28parentheses%29.txt" in `command_line_tool.collect_output()`. 

## Cursory Analysis
This happens because `fs_access.glob()` returns a file URI, which is itself URL encoded, and `command_line_tool.collect_output()` properly calls `urllib.parse.unquote()` to obtain the URL decoded **path** but fails to do so for the **basename** and **nameroot** fields. This record's **basename** is then used as the destination name for outputs copied/moved to final output directory.

## Workflow Code
### url_encode_test.cwl
```
class: CommandLineTool
cwlVersion: v1.0

requirements: 
    - class: InlineJavascriptRequirement

baseCommand: touch

inputs:
    inp:
        type: string
        inputBinding:
            position: 0
outputs:
    out_file:
        type: File
        outputBinding:
            glob: $(inputs.inp)
```

### job.json
`{"inp": "(parentheses).txt"}`

## Full Traceback
N/A

## Your Environment
* cwltool version: 3.1.20210426140515


Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language) Report
> Merging [#1446](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language) (e43942b) into [main](https://codecov.io/gh/common-workflow-language/cwltool/commit/7bdc74dd20cdad688e8b46862aabc52afdbacbb2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language) (7bdc74d) will **decrease** coverage by `14.84%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/graphs/tree.svg?width=650&height=150&src=pr&token=xGYp2paYv4&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language)](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language)

```diff
@@             Coverage Diff             @@
##             main    #1446       +/-   ##
===========================================
- Coverage   65.88%   51.04%   -14.85%     
===========================================
  Files          89       45       -44     
  Lines       15492     7774     -7718     
  Branches     3903     1957     -1946     
===========================================
- Hits        10207     3968     -6239     
+ Misses       4199     3255      -944     
+ Partials     1086      551      -535     
```


| [Impacted Files](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language) | Coverage Δ | |
|---|---|---|
| [cwltool/cwltool/singularity.py](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-Y3dsdG9vbC9jd2x0b29sL3Npbmd1bGFyaXR5LnB5) | `17.82% <0.00%> (-39.11%)` | :arrow_down: |
| [cwltool/cwltool/checker.py](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-Y3dsdG9vbC9jd2x0b29sL2NoZWNrZXIucHk=) | `65.42% <0.00%> (-19.69%)` | :arrow_down: |
| [cwltool/cwltool/workflow\_job.py](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-Y3dsdG9vbC9jd2x0b29sL3dvcmtmbG93X2pvYi5weQ==) | `67.71% <0.00%> (-12.79%)` | :arrow_down: |
| [cwltool/cwltool/command\_line\_tool.py](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-Y3dsdG9vbC9jd2x0b29sL2NvbW1hbmRfbGluZV90b29sLnB5) | `59.24% <0.00%> (-10.30%)` | :arrow_down: |
| [cwltool/cwltool/mutation.py](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-Y3dsdG9vbC9jd2x0b29sL211dGF0aW9uLnB5) | `69.23% <0.00%> (-10.26%)` | :arrow_down: |
| [cwltool/cwltool/argparser.py](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-Y3dsdG9vbC9jd2x0b29sL2FyZ3BhcnNlci5weQ==) | `66.36% <0.00%> (-9.55%)` | :arrow_down: |
| [cwltool/cwltool/builder.py](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-Y3dsdG9vbC9jd2x0b29sL2J1aWxkZXIucHk=) | `79.17% <0.00%> (-8.84%)` | :arrow_down: |
| [cwltool/cwltool/update.py](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-Y3dsdG9vbC9jd2x0b29sL3VwZGF0ZS5weQ==) | `55.55% <0.00%> (-7.19%)` | :arrow_down: |
| [cwltool/cwltool/job.py](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-Y3dsdG9vbC9jd2x0b29sL2pvYi5weQ==) | `55.76% <0.00%> (-6.38%)` | :arrow_down: |
| [cwltool/cwltool/expression.py](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-Y3dsdG9vbC9jd2x0b29sL2V4cHJlc3Npb24ucHk=) | `75.70% <0.00%> (-6.08%)` | :arrow_down: |
| ... and [51 more](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language). Last update [7bdc74d...e43942b](https://codecov.io/gh/common-workflow-language/cwltool/pull/1446?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language).

AlexTate: Of course. I can address both of these requests but it will be a little over a week before I have a chance to take a look.

Patch:
diff --git a/cwltool/command_line_tool.py b/cwltool/command_line_tool.py
index 4554f501..08b10385 100644
--- a/cwltool/command_line_tool.py
+++ b/cwltool/command_line_tool.py
@@ -200,9 +200,7 @@ def revmap_file(
     outside the container. Recognizes files in the pathmapper or remaps
     internal output directories to the external directory.
     """
-    split = urllib.parse.urlsplit(outdir)
-    if not split.scheme:
-        outdir = file_uri(str(outdir))
+    outdir_uri, outdir_path = file_uri(str(outdir)), outdir
 
     # builder.outdir is the inner (container/compute node) output directory
     # outdir is the outer (host/storage system) output directory
@@ -236,21 +234,20 @@ def revmap_file(
         ):
             f["location"] = revmap_f[1]
         elif (
-            uripath == outdir
-            or uripath.startswith(outdir + os.sep)
-            or uripath.startswith(outdir + "/")
+            uripath == outdir_uri
+            or uripath.startswith(outdir_uri + os.sep)
+            or uripath.startswith(outdir_uri + "/")
         ):
-            f["location"] = file_uri(path)
+            f["location"] = uripath
         elif (
             path == builder.outdir
             or path.startswith(builder.outdir + os.sep)
             or path.startswith(builder.outdir + "/")
         ):
-            f["location"] = builder.fs_access.join(
-                outdir, path[len(builder.outdir) + 1 :]
+            joined_path = builder.fs_access.join(
+                outdir_path, path[len(builder.outdir) + 1 :]
             )
-        elif not os.path.isabs(path):
-            f["location"] = builder.fs_access.join(outdir, path)
+            f["location"] = file_uri(joined_path)
         else:
             raise WorkflowException(
                 "Output file path %s must be within designated output directory (%s) or an input "
@@ -1337,6 +1334,15 @@ class CommandLineTool(Process):
                             )
                         try:
                             prefix = fs_access.glob(outdir)
+                            sorted_glob_result = sorted(
+                                fs_access.glob(fs_access.join(outdir, gb)),
+                                key=cmp_to_key(
+                                    cast(
+                                        Callable[[str, str], int],
+                                        locale.strcoll,
+                                    )
+                                ),
+                            )
                             r.extend(
                                 [
                                     {
@@ -1347,24 +1353,24 @@ class CommandLineTool(Process):
                                                 g[len(prefix[0]) + 1 :]
                                             ),
                                         ),
-                                        "basename": os.path.basename(g),
-                                        "nameroot": os.path.splitext(
-                                            os.path.basename(g)
-                                        )[0],
-                                        "nameext": os.path.splitext(
-                                            os.path.basename(g)
-                                        )[1],
+                                        "basename": decoded_basename,
+                                        "nameroot": os.path.splitext(decoded_basename)[
+                                            0
+                                        ],
+                                        "nameext": os.path.splitext(decoded_basename)[
+                                            1
+                                        ],
                                         "class": "File"
                                         if fs_access.isfile(g)
                                         else "Directory",
                                     }
-                                    for g in sorted(
-                                        fs_access.glob(fs_access.join(outdir, gb)),
-                                        key=cmp_to_key(
-                                            cast(
-                                                Callable[[str, str], int],
-                                                locale.strcoll,
-                                            )
+                                    for g, decoded_basename in zip(
+                                        sorted_glob_result,
+                                        map(
+                                            lambda x: os.path.basename(
+                                                urllib.parse.unquote(x)
+                                            ),
+                                            sorted_glob_result,
                                         ),
                                     )
                                 ]


Test patch:
diff --git a/tests/test_path_checks.py b/tests/test_path_checks.py
index e8a300fe..df9d7f6a 100644
--- a/tests/test_path_checks.py
+++ b/tests/test_path_checks.py
@@ -1,11 +1,19 @@
-from pathlib import Path
-
 import pytest
+import urllib.parse
 
-from cwltool.main import main
+from pathlib import Path
+from typing import cast
+from ruamel.yaml.comments import CommentedMap
+from schema_salad.sourceline import cmap
 
+from cwltool.main import main
+from cwltool.command_line_tool import CommandLineTool
+from cwltool.context import LoadingContext, RuntimeContext
+from cwltool.update import INTERNAL_VERSION
+from cwltool.utils import CWLObjectType
 from .util import needs_docker
 
+
 script = """
 #!/usr/bin/env cwl-runner
 cwlVersion: v1.0
@@ -95,3 +103,67 @@ def test_unicode_in_output_files(tmp_path: Path, filename: str) -> None:
         filename,
     ]
     assert main(params) == 0
+
+
+def test_clt_returns_specialchar_names(tmp_path: Path) -> None:
+    """Confirm that special characters in filenames do not cause problems."""
+    loading_context = LoadingContext(
+        {
+            "metadata": {
+                "cwlVersion": INTERNAL_VERSION,
+                "http://commonwl.org/cwltool#original_cwlVersion": INTERNAL_VERSION,
+            }
+        }
+    )
+    clt = CommandLineTool(
+        cast(
+            CommentedMap,
+            cmap(
+                {
+                    "cwlVersion": INTERNAL_VERSION,
+                    "class": "CommandLineTool",
+                    "inputs": [],
+                    "outputs": [],
+                    "requirements": [],
+                }
+            ),
+        ),
+        loading_context,
+    )
+
+    # Reserved characters will be URL encoded during the creation of a file URI
+    # Internal references to files are in URI form, and are therefore URL encoded
+    # Final output files should not retain their URL encoded filenames
+    rfc_3986_gen_delims = [":", "/", "?", "#", "[", "]", "@"]
+    rfc_3986_sub_delims = ["!", "$", "&", "'", "(", ")", "*", "+", ",", ";", "="]
+    unix_reserved = ["/", "\0"]
+    reserved = [
+        special_char
+        for special_char in (rfc_3986_gen_delims + rfc_3986_sub_delims)
+        if special_char not in unix_reserved
+    ]
+
+    # Mock an "output" file with the above special characters in its name
+    special = "".join(reserved)
+    output_schema = cast(
+        CWLObjectType, {"type": "File", "outputBinding": {"glob": special}}
+    )
+    mock_output = tmp_path / special
+    mock_output.touch()
+
+    # Prepare minimal arguments for CommandLineTool.collect_output()
+    builder = clt._init_job({}, RuntimeContext())
+    builder.pathmapper = clt.make_path_mapper(
+        builder.files, builder.stagedir, RuntimeContext(), True
+    )
+    fs_access = builder.make_fs_access(str(tmp_path))
+
+    result = cast(
+        CWLObjectType,
+        clt.collect_output(output_schema, builder, str(tmp_path), fs_access),
+    )
+
+    assert result["class"] == "File"
+    assert result["basename"] == special
+    assert result["nameroot"] == special
+    assert str(result["location"]).endswith(urllib.parse.quote(special))