diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index e84227bd6b5..468d7099588 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -3,7 +3,6 @@ - [ ] Read the [contribution guidelines](https://github.com/swagger-api/swagger-codegen/blob/master/CONTRIBUTING.md). - [ ] Ran the shell script under `./bin/` to update Petstore sample so that CIs can verify the change. (For instance, only need to run `./bin/{LANG}-petstore.sh` and `./bin/security/{LANG}-petstore.sh` if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in `.\bin\windows\`. - [ ] Filed the PR against the correct branch: `3.0.0` branch for changes related to OpenAPI spec 3.0. Default: `master`. -- [ ] Copied the [technical committee](https://github.com/swagger-api/swagger-codegen/#swagger-codegen-technical-committee) to review the pull request if your PR is targeting a particular programming language. ### Description of the PR diff --git a/modules/swagger-generator/pom.xml b/modules/swagger-generator/pom.xml index 498df49532a..f6bd9bfb3cd 100644 --- a/modules/swagger-generator/pom.xml +++ b/modules/swagger-generator/pom.xml @@ -146,7 +146,6 @@ https://github.com/swagger-api/swagger-ui/archive/master.tar.gz true - ${project.build.directory} @@ -327,6 +326,18 @@ ${junit-version} test + + org.mockito + mockito-core + 5.20.0 + test + + + org.mockito + mockito-inline + 5.2.0 + test + 2.5 diff --git a/modules/swagger-generator/src/main/java/io/swagger/generator/resource/SwaggerResource.java b/modules/swagger-generator/src/main/java/io/swagger/generator/resource/SwaggerResource.java index 8fdf10ecfb7..42de6987cd2 100644 --- a/modules/swagger-generator/src/main/java/io/swagger/generator/resource/SwaggerResource.java +++ b/modules/swagger-generator/src/main/java/io/swagger/generator/resource/SwaggerResource.java @@ -3,11 +3,14 @@ import io.swagger.annotations.Api; import io.swagger.annotations.ApiOperation; import io.swagger.annotations.ApiParam; +import io.swagger.annotations.ApiResponse; +import io.swagger.annotations.ApiResponses; import io.swagger.codegen.CliOption; import io.swagger.codegen.Codegen; import io.swagger.codegen.CodegenConfig; import io.swagger.codegen.CodegenType; import io.swagger.codegen.utils.SecureFileUtils; +import io.swagger.generator.exception.ApiException; import io.swagger.generator.exception.BadRequestException; import io.swagger.generator.model.Generated; import io.swagger.generator.model.GeneratorInput; @@ -15,22 +18,35 @@ import io.swagger.generator.online.Generator; import org.apache.commons.io.FileUtils; import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import javax.servlet.http.HttpServletRequest; -import javax.ws.rs.*; +import javax.ws.rs.GET; +import javax.ws.rs.POST; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; +import javax.ws.rs.Produces; import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import java.io.File; -import java.util.*; +import java.io.IOException; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.UUID; @Path("/gen") -@Api(value = "/gen", description = "Resource for generating swagger components") +@Api(value = "/gen") @SuppressWarnings("static-method") public class SwaggerResource { - static List clients = new ArrayList(); - static List servers = new ArrayList(); - private static Map fileMap = new HashMap(); + private static final Logger LOGGER = LoggerFactory.getLogger(SwaggerResource.class); + + static List clients = new ArrayList<>(); + static List servers = new ArrayList<>(); + private static Map fileMap = new HashMap<>(); static { List extensions = Codegen.getExtensions(); @@ -43,8 +59,8 @@ public class SwaggerResource { } } - Collections.sort(clients, String.CASE_INSENSITIVE_ORDER); - Collections.sort(servers, String.CASE_INSENSITIVE_ORDER); + clients.sort(String.CASE_INSENSITIVE_ORDER); + servers.sort(String.CASE_INSENSITIVE_ORDER); } @GET @@ -55,20 +71,21 @@ public class SwaggerResource { notes = "A valid `fileId` is generated by the `/clients/{language}` or `/servers/{language}` POST " + "operations. The fileId code can be used just once, after which a new `fileId` will need to " + "be requested.", response = String.class, tags = {"clients", "servers"}) - public Response downloadFile(@PathParam("fileId") String fileId) throws Exception { + @ApiResponses(value = { + @ApiResponse(code = 200, message = "File successfully downloaded. Response contains ZIP file bytes."), + @ApiResponse(code = 404, message = "File with the given fileId not found or already downloaded."), + @ApiResponse(code = 500, message = "Server error while reading or returning the file.") + }) + public Response downloadFile(@PathParam("fileId") String fileId) { Generated g = fileMap.get(fileId); - System.out.println("looking for fileId " + fileId); - System.out.println("got filename " + g.getFilename()); - if (g.getFilename() != null) { + LOGGER.info("Looking for fileId: {}", fileId); + if (g != null && g.getFilename() != null) { + LOGGER.info("Got filename: {}", g.getFilename()); SecureFileUtils.validatePath(g.getFilename()); - File file = new java.io.File(g.getFilename()); - byte[] bytes = org.apache.commons.io.FileUtils.readFileToByteArray(file); + final File file = new java.io.File(g.getFilename()); - try { - FileUtils.deleteDirectory(file.getParentFile()); - } catch (Exception e) { - System.out.println("failed to delete file " + file.getAbsolutePath()); - } + byte[] bytes = getFileBytes(file); + removeFile(fileId, file); return Response .ok(bytes, "application/zip") @@ -80,6 +97,23 @@ public Response downloadFile(@PathParam("fileId") String fileId) throws Exceptio } } + private static byte[] getFileBytes(File file) { + try { + return FileUtils.readFileToByteArray(file); + } catch (IOException e) { + throw new IllegalStateException("Cannot read the file: " + file.getAbsolutePath(), e); + } + } + + private static void removeFile(String fileId, File file) { + try { + FileUtils.deleteDirectory(file.getParentFile()); + fileMap.remove(fileId); + } catch (Exception e) { + LOGGER.error("Failed to delete file: {} ", file.getAbsolutePath()); + } + } + @POST @Path("/clients/{language}") @ApiOperation( @@ -90,7 +124,7 @@ public Response generateClient( @Context HttpServletRequest request, @ApiParam(value = "The target language for the client library", required = true) @PathParam("language") String language, @ApiParam(value = "Configuration for building the client library", required = true) GeneratorInput opts) - throws Exception { + throws ApiException { String filename = Generator.generateClient(language, opts); String host = getHost(request); @@ -101,7 +135,6 @@ public Response generateClient( g.setFilename(filename); g.setFriendlyName(language + "-client"); fileMap.put(code, g); - System.out.println(code + ", " + filename); String link = host + "/api/gen/download/" + code; return Response.ok().entity(new ResponseCode(code, link)).build(); } else { @@ -117,7 +150,7 @@ public Response generateClient( public Response getClientOptions( @SuppressWarnings("unused") @Context HttpServletRequest request, @ApiParam(value = "The target language for the client library", required = true) @PathParam("language") String language) - throws Exception { + throws ApiException { Map opts = Generator.getOptions(language); @@ -136,7 +169,7 @@ public Response getClientOptions( public Response getServerOptions( @SuppressWarnings("unused") @Context HttpServletRequest request, @ApiParam(value = "The target language for the server framework", required = true) @PathParam("framework") String framework) - throws Exception { + throws ApiException { Map opts = Generator.getOptions(framework); @@ -173,14 +206,13 @@ public Response serverOptions() { value = "Generates a server library", notes = "Accepts a `GeneratorInput` options map for spec location and generation options.", response = ResponseCode.class, tags = "servers") - public Response generateServerForLanguage(@Context HttpServletRequest request, @ApiParam( - value = "framework", required = true) @PathParam("framework") String framework, - @ApiParam(value = "parameters", required = true) GeneratorInput opts) throws Exception { + public Response generateServerForLanguage(@Context HttpServletRequest request, @ApiParam(value = "framework", required = true) @PathParam("framework") String framework, + @ApiParam(value = "parameters", required = true) GeneratorInput opts) throws ApiException { if (framework == null) { throw new BadRequestException("Framework is required"); } String filename = Generator.generateServer(framework, opts); - System.out.println("generated name: " + filename); + LOGGER.info("Generated filename: {}", filename); String host = getHost(request); @@ -190,7 +222,6 @@ public Response generateServerForLanguage(@Context HttpServletRequest request, @ g.setFilename(filename); g.setFriendlyName(framework + "-server"); fileMap.put(code, g); - System.out.println(code + ", " + filename); String link = host + "/api/gen/download/" + code; return Response.ok().entity(new ResponseCode(code, link)).build(); } else { diff --git a/modules/swagger-generator/src/test/java/io/swagger/generator/resource/SwaggerResourceTest.java b/modules/swagger-generator/src/test/java/io/swagger/generator/resource/SwaggerResourceTest.java index 590e6245302..c197e4df947 100644 --- a/modules/swagger-generator/src/test/java/io/swagger/generator/resource/SwaggerResourceTest.java +++ b/modules/swagger-generator/src/test/java/io/swagger/generator/resource/SwaggerResourceTest.java @@ -1,21 +1,114 @@ package io.swagger.generator.resource; +import io.swagger.codegen.utils.SecureFileUtils; +import io.swagger.generator.model.Generated; +import org.apache.commons.io.FileUtils; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; +import javax.ws.rs.core.Response; +import java.io.File; +import java.lang.reflect.Field; +import java.nio.charset.StandardCharsets; +import java.util.HashMap; +import java.util.Map; + +import static org.testng.Assert.assertFalse; +import static org.testng.AssertJUnit.assertEquals; + public class SwaggerResourceTest { + private SwaggerResource resource; + private Map fileMap; + + @BeforeMethod + public void setUp() throws Exception { + resource = new SwaggerResource(); + + fileMap = new HashMap<>(); + Field fm = SwaggerResource.class.getDeclaredField("fileMap"); + fm.setAccessible(true); + fm.set(null, fileMap); + } + + @AfterMethod + public void after() { + fileMap.clear(); + } + + @Test + public void shouldReturnSuccessWhenDownloadFileAndBadRequestAfterSecondTry() throws Exception { + File dir = new File("target/testng-gen1"); + File zip = new File(dir, "client.zip"); + FileUtils.write(zip, "TESTDATA", StandardCharsets.UTF_8); + + Generated g = new Generated(); + g.setFilename(zip.getAbsolutePath()); + g.setFriendlyName("clientX"); + + fileMap.put("123", g); + + Response response = resource.downloadFile("123"); + + assertEquals(200, response.getStatus()); + assertEquals("TESTDATA", new String((byte[]) response.getEntity())); + assertFalse(zip.exists(), "File should be removed after download."); + assertFalse(dir.exists(), "Directory should be removed after download."); + + Response response2 = resource.downloadFile("123"); + assertEquals(404, response2.getStatus()); + } + + @Test + public void shouldReturnNotFoundWhenFileDoesntExist() { + Response response = resource.downloadFile("nope"); + assertEquals(404, response.getStatus()); + } + + @Test(expectedExceptions = Exception.class) + public void testDownloadFile_missingPhysicalFile_causes500() { + Generated g = new Generated(); + g.setFilename("target/no_such_dir/file.zip"); + g.setFriendlyName("missing"); + + fileMap.put("777", g); + + resource.downloadFile("777"); + } + + @Test(expectedExceptions = Exception.class) + public void shouldPathValidationFailsWhenDownloadFile() throws Exception { + try (MockedStatic mocked = Mockito.mockStatic(SecureFileUtils.class)) { + + mocked.when(() -> SecureFileUtils.validatePath(Mockito.anyString())) + .thenThrow(new RuntimeException("Invalid path")); + + File dir = new File("target/testng-gen2"); + File zip = new File(dir, "client.zip"); + FileUtils.write(zip, "XYZ", StandardCharsets.UTF_8); + + Generated g = new Generated(); + g.setFilename(zip.getAbsolutePath()); + g.setFriendlyName("clientY"); + + fileMap.put("xyz", g); + + resource.downloadFile("xyz"); + } + } + @Test(expectedExceptions = SecurityException.class) public void testDownloadFileWithPathTraversal() throws Exception { - SwaggerResource resource = new SwaggerResource(); io.swagger.generator.model.Generated generated = new io.swagger.generator.model.Generated(); generated.setFilename("../../../etc/passwd"); java.lang.reflect.Field fileMapField = SwaggerResource.class.getDeclaredField("fileMap"); fileMapField.setAccessible(true); - @SuppressWarnings("unchecked") - java.util.Map fileMap = - (java.util.Map) fileMapField.get(null); + fileMap.put("test-file-id", generated); try {