SEAB-6173: Fix miscellaneous "fuzzing bugs" #5862
Conversation
dockstore-webservice/src/main/java/io/dockstore/webservice/jdbi/EntryDAO.java
Show resolved
Hide resolved
| @@ -330,7 +331,7 @@ private String getRSS() { | |||
| @ApiOperation(value = "Returns the file containing runner dependencies.", response = String.class) | |||
| public Response getRunnerDependencies( | |||
| @Parameter(name = "client_version", description = "The Dockstore client version (e.g. 1.13.0)", schema = @Schema(pattern = PipHelper.OPENAPI_SEM_VER_STRING)) | |||
| @ApiParam(value = "The Dockstore client version (e.g. 1.13.0)") @QueryParam("client_version") String clientVersion, | |||
| @ApiParam(value = "The Dockstore client version (e.g. 1.13.0)") @NotNull @QueryParam("client_version") String clientVersion, | |||
There was a problem hiding this comment.
Require client_version
| List<Workflow> workflows = workflowDAO.findAllByPath(path, false); | ||
| workflows.forEach(this::checkNotNullEntry); |
There was a problem hiding this comment.
This check was not necessary, the returned list of workflows never contains null entries.
There was a problem hiding this comment.
Nice, this fixes https://ucsc-cgl.atlassian.net/browse/SEAB-6177
| List<Workflow> workflows = workflowDAO.findAllByPath(path, false); | ||
| workflows.forEach(this::checkNotNullEntry); | ||
| checkNotNull(workflows, "Invalid repository path"); |
There was a problem hiding this comment.
findAllByPath returns null if the path is invalid (not the expected format).
| workflows.forEach(this::checkNotNullEntry); | ||
| public List<Workflow> getAllPublishedWorkflowByPath(@Parameter(description = "repository path") @PathParam("repository") String path) { | ||
| List<Workflow> workflows = workflowDAO.findAllByPath(path, true); | ||
| checkNotNull(workflows, "Invalid repository path"); |
There was a problem hiding this comment.
Make this endpoint behave like the others when the path is invalid.
| @Parameter(description = "Workflow id") @PathParam("workflowId") Long workflowId, | ||
| @QueryParam("tag") String tag, | ||
| @NotNull @QueryParam("language") DescriptorLanguage language) { | ||
| final FileType fileType = language.getFileType(); |
There was a problem hiding this comment.
Modernize parameter annotations and require language parameter.
| registry = segments.get(0); | ||
| organization = segments.get(1); | ||
| name = segments.get(2); | ||
| toolName = segments.size() > SEGMENTS_IN_ID ? segments.get(SEGMENTS_IN_ID) : ""; | ||
| } |
There was a problem hiding this comment.
Check the tool id earlier, and refactor to improve clarity and remove a similar variable that could be accidentally used.
| @Parameter(hidden = true, name = "user") @Auth Optional<User> user, | ||
| @Parameter(description = "Workflow id") @PathParam("workflowId") Long workflowId, | ||
| @QueryParam("tag") String tag, | ||
| @NotNull @QueryParam("language") DescriptorLanguage language) { |
There was a problem hiding this comment.
Change this parameter to an enum so that it works like similar endpoints.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5862 +/- ##
==============================================
+ Coverage 28.88% 74.52% +45.63%
- Complexity 2087 5276 +3189
==============================================
Files 369 369
Lines 19060 19063 +3
Branches 2025 2026 +1
==============================================
+ Hits 5506 14207 +8701
+ Misses 13123 3893 -9230
- Partials 431 963 +532
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| results = include.stream().collect(Collectors.toMap(name -> name, name -> healthCheckRegistry.runHealthCheck(name))); | ||
| // Run each of the health checks, making sure that if a duplicate name is specified, the corresponding | ||
| // health check is only run once, to avoid the toMap Collector from throwing due to a duplicate key. | ||
| results = include.stream().distinct().collect(Collectors.toMap(name -> name, name -> healthCheckRegistry.runHealthCheck(name))); |
There was a problem hiding this comment.
See code comment.
|
Ooops. forgot to note that the |
|
| @@ -791,9 +794,9 @@ public Tool publish(@ApiParam(hidden = true) @Parameter(hidden = true, name = "u | |||
| "containers"}, notes = "NO authentication", response = Tool.class, responseContainer = "List") | |||
| public List<Tool> allPublishedContainers( | |||
| @ApiParam(value = "Start index of paging. If not specified in the request, this will start at the beginning of the results.", | |||
There was a problem hiding this comment.
Beyond the scope of this PR, but ApiParam is a Swagger annotation, and as such this description does not show up here: https://staging.dockstore.org/api/static/swagger-ui/index.html#/containers/allPublishedContainers
I'll create a ticket to more methodically go through this.
There was a problem hiding this comment.
Yeah, there's a ton of those. I fixed some, but not in the endpoints that I added the Max/Min to. Hopefully there's a quick way to automate it. It'd be nice to make the argument blocks more readable, too, maybe one argument per line?
Description
This PR fixes the bugs that were uncovered during fuzz testing by the requests listed in https://ucsc-cgl.atlassian.net/browse/SEAB-6173, with the exception of the
api/containers/schema/514/publishedrequest, which resulted in more extensive changes and is fixed in PR #5860.I added some inline PR comments that describe what was fixed, and how.
Note that the "NUL character" bug is not fixed: #5862 (comment)
Review Instructions
Try the requests that are described in the issue, and confirm that the responses make sense and are not 500 status/error codes.
Issue
https://ucsc-cgl.atlassian.net/browse/SEAB-6173
Security and Privacy
No concerns.
e.g. Does this change...
Please make sure that you've checked the following before submitting your pull request. Thanks!
mvn clean install@RolesAllowedannotation