Skip to content

Commit f44689e

Browse files
MarkRobboMarkRobbo
authored
Merge pull request #154 from common-workflow-language/doublegit
Ensure generic git handling of github.com gitlab.com URIs
2 parents f8a81a5 + 2d88145 commit f44689e

File tree

9 files changed

+61
-31
lines changed

9 files changed

+61
-31
lines changed

‎src/main/java/org/commonwl/view/cwl/CWLService.java

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,22 @@
1919

2020
package org.commonwl.view.cwl;
2121

22-
import com.fasterxml.jackson.databind.JsonNode;
23-
import com.fasterxml.jackson.databind.ObjectMapper;
24-
import com.fasterxml.jackson.databind.node.ArrayNode;
25-
import com.fasterxml.jackson.databind.node.ObjectNode;
26-
import com.fasterxml.jackson.databind.node.TextNode;
22+
import static org.apache.commons.io.FileUtils.readFileToString;
23+
24+
import java.io.ByteArrayInputStream;
25+
import java.io.File;
26+
import java.io.IOException;
27+
import java.io.StringWriter;
28+
import java.util.ArrayList;
29+
import java.util.HashMap;
30+
import java.util.Iterator;
31+
import java.util.List;
32+
import java.util.Map;
33+
2734
import org.apache.commons.io.FileUtils;
2835
import org.apache.commons.io.FilenameUtils;
36+
import org.apache.jena.iri.IRI;
37+
import org.apache.jena.iri.IRIFactory;
2938
import org.apache.jena.ontology.OntModelSpec;
3039
import org.apache.jena.query.QuerySolution;
3140
import org.apache.jena.query.ResultSet;
@@ -47,15 +56,11 @@
4756
import org.springframework.stereotype.Service;
4857
import org.yaml.snakeyaml.Yaml;
4958

50-
import java.io.ByteArrayInputStream;
51-
import java.io.File;
52-
import java.io.IOException;
53-
import java.io.StringWriter;
54-
import java.nio.file.Path;
55-
import java.nio.file.Paths;
56-
import java.util.*;
57-
58-
import static org.apache.commons.io.FileUtils.readFileToString;
59+
import com.fasterxml.jackson.databind.JsonNode;
60+
import com.fasterxml.jackson.databind.ObjectMapper;
61+
import com.fasterxml.jackson.databind.node.ArrayNode;
62+
import com.fasterxml.jackson.databind.node.ObjectNode;
63+
import com.fasterxml.jackson.databind.node.TextNode;
5964

6065
/**
6166
* Provides CWL parsing for workflows to gather an overview
@@ -65,6 +70,7 @@
6570
public class CWLService {
6671

6772
private final Logger logger = LoggerFactory.getLogger(this.getClass());
73+
private final IRIFactory iriFactory = IRIFactory.iriImplementation();
6874

6975
// Autowired properties/services
7076
private final RDFService rdfService;
@@ -368,8 +374,10 @@ public Workflow parseWorkflowWithCwltool(Workflow basicModel,
368374
// Add new step
369375
CWLStep wfStep = new CWLStep();
370376

371-
Path workflowPath = Paths.get(step.get("wf").toString()).getParent();
372-
Path runPath = Paths.get(step.get("run").toString());
377+
IRI wfStepUri = iriFactory.construct(step.get("wf").asResource().getURI());
378+
IRI workflowPath = wfStepUri.resolve("./");
379+
380+
IRI runPath = iriFactory.construct(step.get("run").asResource().getURI());
373381
wfStep.setRun(workflowPath.relativize(runPath).toString());
374382
wfStep.setRunType(rdfService.strToRuntype(step.get("runtype").toString()));
375383

‎src/main/java/org/commonwl/view/workflow/WorkflowController.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,22 +127,24 @@ public ModelAndView createWorkflow(@Valid WorkflowForm workflowForm, BindingResu
127127
return new ModelAndView("redirect:" + gitInfo.getInternalUrl());
128128
}
129129
} catch (TransportException ex) {
130+
logger.warn("git.sshError " + workflowForm , ex);
130131
bindingResult.rejectValue("url", "git.sshError");
131132
return new ModelAndView("index");
132133
} catch (GitAPIException ex) {
134+
logger.error("git.retrievalError " + workflowForm , ex);
133135
bindingResult.rejectValue("url", "git.retrievalError");
134-
logger.error("Git API Error", ex);
135136
return new ModelAndView("index");
136137
} catch (WorkflowNotFoundException ex) {
138+
logger.warn("git.pathTraversal " + workflowForm , ex);
137139
bindingResult.rejectValue("url", "git.pathTraversal");
138140
return new ModelAndView("index");
139141
} catch (Exception ex) {
142+
logger.warn("url.parsingError " + workflowForm , ex);
140143
bindingResult.rejectValue("url", "url.parsingError");
141144
return new ModelAndView("index");
142145
}
143146
}
144147
gitInfo = workflow.getRetrievedFrom();
145-
146148
// Redirect to the workflow
147149
return new ModelAndView("redirect:" + gitInfo.getInternalUrl());
148150
}
@@ -473,12 +475,16 @@ private ModelAndView getWorkflow(GitDetails gitDetails, RedirectAttributes redir
473475
}
474476
}
475477
} catch (TransportException ex) {
478+
logger.warn("git.sshError " + workflowForm , ex);
476479
errors.rejectValue("url", "git.sshError", "SSH URLs are not supported, please provide a HTTPS URL for the repository or submodules");
477480
} catch (GitAPIException ex) {
481+
logger.error("git.retrievalError " + workflowForm, ex);
478482
errors.rejectValue("url", "git.retrievalError", "The workflow could not be retrieved from the Git repository using the details given");
479483
} catch (WorkflowNotFoundException ex) {
484+
logger.warn("git.pathTraversal " + workflowForm, ex);
480485
errors.rejectValue("url", "git.pathTraversal", "The path given did not resolve to a location within the repository");
481486
} catch (IOException ex) {
487+
logger.warn("git.parsingError " + workflowForm, ex);
482488
errors.rejectValue("url", "url.parsingError", "The workflow could not be parsed from the given URL");
483489
}
484490
}

‎src/main/java/org/commonwl/view/workflow/WorkflowForm.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,4 +75,11 @@ public void setPath(String path) {
7575
private String trimTrailingSlashes(String url) {
7676
return url.replaceAll("\\/+$", "");
7777
}
78+
79+
@Override
80+
public String toString() {
81+
return "WorkflowForm [" + (url != null ? "url=" + url + ", " : "")
82+
+ (branch != null ? "branch=" + branch + ", " : "") + (path != null ? "path=" + path : "") + "]";
83+
}
84+
7885
}

‎src/main/java/org/commonwl/view/workflow/WorkflowFormValidator.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ public GitDetails validateAndParse(WorkflowForm form, Errors e) {
9999

100100
// Github Dir URL
101101
m = githubDirPattern.matcher(form.getUrl());
102-
if (m.find()) {
102+
if (m.find() && ! m.group(2).endsWith(".git")) {
103103
String repoUrl = "https://github.com/" + m.group(1) + "/" + m.group(2) + ".git";
104104
if (branch == null) branch = m.group(3);
105105
if (path == null) path = m.group(4);
@@ -108,7 +108,7 @@ public GitDetails validateAndParse(WorkflowForm form, Errors e) {
108108

109109
// Gitlab Dir URL
110110
m = gitlabDirPattern.matcher(form.getUrl());
111-
if (m.find()) {
111+
if (m.find() && ! m.group(2).endsWith(".git")) {
112112
String repoUrl = "https://gitlab.com/" + m.group(1) + "/" + m.group(2) + ".git";
113113
if (branch == null) branch = m.group(3);
114114
if (path == null) path = m.group(4);

‎src/main/java/org/commonwl/view/workflow/WorkflowService.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@
4141

4242
import java.io.File;
4343
import java.io.IOException;
44+
import java.nio.file.Files;
4445
import java.nio.file.Path;
46+
import java.nio.file.Paths;
4547
import java.util.ArrayList;
4648
import java.util.Calendar;
4749
import java.util.Date;
@@ -212,7 +214,8 @@ public List<WorkflowOverview> getWorkflowsFromDirectory(GitDetails gitInfo) thro
212214

213215
Path localPath = repo.getRepository().getWorkTree().toPath();
214216
Path pathToDirectory = localPath.resolve(gitInfo.getPath()).normalize().toAbsolutePath();
215-
if (pathToDirectory.toString().equals("/")) {
217+
Path root = Paths.get("/").toAbsolutePath();
218+
if (pathToDirectory.equals(root)) {
216219
pathToDirectory = localPath;
217220
} else if (!pathToDirectory.startsWith(localPath.normalize().toAbsolutePath())) {
218221
// Prevent path traversal attacks
@@ -307,6 +310,9 @@ public QueuedWorkflow createQueuedWorkflow(GitDetails gitInfo)
307310
}
308311

309312
File workflowFile = new File(pathToWorkflowFile.toString());
313+
if (! Files.isReadable(workflowFile.toPath())) {
314+
throw new WorkflowNotFoundException();
315+
}
310316
Workflow basicModel = cwlService.parseWorkflowNative(workflowFile);
311317

312318
// Set origin details

‎src/main/resources/application.properties

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,14 @@
66
applicationName = Common Workflow Language Viewer
77
applicationURL = https://view.commonwl.org
88

9-
# Path to a directory in which the RO Bundles will be stored
10-
bundleStorage = /tmp
9+
# Path to a directory in which the RO Bundles will be stored, e.g. /tmp
10+
bundleStorage = ${java.io.tmpdir}
1111

12-
# Path to a directory in which graphviz images will be stored
13-
graphvizStorage = /tmp
12+
# Path to a directory in which graphviz images will be stored, e.g. /tmp
13+
graphvizStorage = ${java.io.tmpdir}
1414

15-
# Path to a directory in which git repositories will be checked out into
16-
gitStorage = /tmp
15+
# Path to a directory in which git repositories will be checked out into, e.g. /tmp
16+
gitStorage = ${java.io.tmpdir}
1717

1818
# How long to cache workflows in days before checking for changes via Github
1919
cacheDays = 1

‎src/test/java/org/commonwl/view/cwl/CWLServiceTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,8 @@ public void workflowOverviewOverSingleFileSizeLimitThrowsIOException() throws Ex
197197

198198
// Should throw IOException due to oversized file
199199
thrown.expect(IOException.class);
200-
thrown.expectMessage("File 'hello.cwl' is over singleFileSizeLimit - 672 bytes/0 bytes");
200+
thrown.expectMessage(String.format("File 'hello.cwl' is over singleFileSizeLimit - %s bytes/0 bytes",
201+
helloWorkflow.length()));
201202
cwlService.getWorkflowOverview(helloWorkflow);
202203

203204
}

‎src/test/java/org/commonwl/view/researchobject/ROBundleFactoryTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,8 @@ public void bundleForValidWorkflow() throws Exception {
6565
// Attempt to add RO to workflow
6666
factory.createWorkflowRO(validWorkflow);
6767

68-
assertEquals("test/path/to/check/for.zip", validWorkflow.getRoBundlePath());
68+
assertEquals(Paths.get("test/path/to/check/for.zip"),
69+
Paths.get(validWorkflow.getRoBundlePath()));
6970

7071
}
7172

‎src/test/java/org/commonwl/view/workflow/WorkflowServiceTest.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,8 @@ public void getWorkflowsFromDirectory() throws Exception {
107107
@Test
108108
public void getWorkflowCacheHasExpired() throws Exception {
109109

110-
GitDetails githubInfo = new GitDetails("https://github.com/owner/repoName.git", "sha", "path");
110+
GitDetails githubInfo = new GitDetails("https://github.com/common-workflow-language/workflows.git",
111+
"master", "dna.cwl");
111112

112113
Workflow oldWorkflow = new Workflow("old", "This is the expired workflow",
113114
new HashMap<>(), new HashMap<>(), new HashMap<>(), null);
@@ -128,7 +129,7 @@ public void getWorkflowCacheHasExpired() throws Exception {
128129
when(mockCWLService.parseWorkflowNative(anyObject())).thenReturn(updatedWorkflow);
129130

130131
Repository mockRepo = Mockito.mock(Repository.class);
131-
when(mockRepo.getWorkTree()).thenReturn(new File("src/test/resources/cwl/make_to_cwl/dna.cwl"));
132+
when(mockRepo.getWorkTree()).thenReturn(new File("src/test/resources/cwl/make_to_cwl"));
132133

133134
Git mockGitRepo = Mockito.mock(Git.class);
134135
when(mockGitRepo.getRepository()).thenReturn(mockRepo);

0 commit comments

Comments
 (0)