From 963b4ee69d5d5d84c620821f7e0997eeae01fe5b Mon Sep 17 00:00:00 2001 From: Ludy Date: Thu, 4 Sep 2025 13:39:37 +0200 Subject: [PATCH] refactor(ssrf): default enum MEDIUM prevents OFF=false (#4280) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description of Changes - **What was changed** - **URL to PDF flow** - Changed `ConvertWebsiteToPDF#urlToPdf` to return `ResponseEntity` and perform a redirect (`303 SEE_OTHER`) back to `/url-to-pdf` with an `error` query param instead of throwing exceptions. - Added alert rendering in `url-to-pdf.html` using `param.error` for localized error display. - Introduced new translation key `error.invalidUrlFormat` in `messages_en_GB.properties`. - **Security / SSRF** - Migrated `ApplicationProperties.System.UrlSecurity.level` from `String` to `SsrfProtectionLevel` enum. - Default now set to `SsrfProtectionLevel.MEDIUM` (`// MAX, MEDIUM, OFF`). - This avoids the issue where setting `OFF` returned `false` in configuration parsing. - Updated `SsrfProtectionService#parseProtectionLevel` accordingly (using `level.name()`). - **Repo hygiene** - Added `**/LOCAL_APPDATA_FONTCONFIG_CACHE/**` to `.gitignore`. - **Why the change was made** - Provide user-friendly, localized error messages instead of exposing internal exceptions on URL-to-PDF conversions. - Ensure SSRF protection level parsing is type-safe and consistent—`OFF` can now be set without yielding a misleading `false` state. - Prevent unwanted fontconfig cache files from being tracked in Git. --- ## Checklist ### General - [x] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [x] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md) (if applicable) - [x] I have performed a self-review of my own code - [x] My changes generate no new warnings ### Documentation - [ ] I have updated relevant docs on [Stirling-PDF's doc repo](https://github.com/Stirling-Tools/Stirling-Tools.github.io/blob/main/docs/) (if functionality has heavily changed) - [ ] I have read the section [Add New Translation Tags](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md#add-new-translation-tags) (for new translation tags only) ### UI Changes (if applicable) - [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR) ### Testing (if applicable) - [ ] I have tested my changes locally. Refer to the [Testing Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/DeveloperGuide.md#6-testing) for more details. --- .gitignore | 3 + .../common/model/ApplicationProperties.java | 3 +- .../common/service/SsrfProtectionService.java | 7 +- .../api/converters/ConvertWebsiteToPDF.java | 42 ++- .../main/resources/messages_en_GB.properties | 1 + .../templates/convert/url-to-pdf.html | 1 + .../converters/ConvertWebsiteToPdfTest.java | 263 +++++++++++++++--- 7 files changed, 271 insertions(+), 49 deletions(-) diff --git a/.gitignore b/.gitignore index 6ebd87c35..6a5fe9b05 100644 --- a/.gitignore +++ b/.gitignore @@ -200,3 +200,6 @@ id_ed25519.pub # node_modules node_modules/ + +# weasyPrint +**/LOCAL_APPDATA_FONTCONFIG_CACHE/** diff --git a/app/common/src/main/java/stirling/software/common/model/ApplicationProperties.java b/app/common/src/main/java/stirling/software/common/model/ApplicationProperties.java index 5845c6d16..ece91b269 100644 --- a/app/common/src/main/java/stirling/software/common/model/ApplicationProperties.java +++ b/app/common/src/main/java/stirling/software/common/model/ApplicationProperties.java @@ -41,6 +41,7 @@ import stirling.software.common.model.oauth2.GitHubProvider; import stirling.software.common.model.oauth2.GoogleProvider; import stirling.software.common.model.oauth2.KeycloakProvider; import stirling.software.common.model.oauth2.Provider; +import stirling.software.common.service.SsrfProtectionService.SsrfProtectionLevel; import stirling.software.common.util.ValidationUtils; @Data @@ -390,7 +391,7 @@ public class ApplicationProperties { @Data public static class UrlSecurity { private boolean enabled = true; - private String level = "MEDIUM"; // MAX, MEDIUM, OFF + private SsrfProtectionLevel level = SsrfProtectionLevel.MEDIUM; // MAX, MEDIUM, OFF private List allowedDomains = new ArrayList<>(); private List blockedDomains = new ArrayList<>(); private List internalTlds = diff --git a/app/common/src/main/java/stirling/software/common/service/SsrfProtectionService.java b/app/common/src/main/java/stirling/software/common/service/SsrfProtectionService.java index b58e0d516..c34575cbb 100644 --- a/app/common/src/main/java/stirling/software/common/service/SsrfProtectionService.java +++ b/app/common/src/main/java/stirling/software/common/service/SsrfProtectionService.java @@ -61,9 +61,9 @@ public class SsrfProtectionService { }; } - private SsrfProtectionLevel parseProtectionLevel(String level) { + private SsrfProtectionLevel parseProtectionLevel(SsrfProtectionLevel level) { try { - return SsrfProtectionLevel.valueOf(level.toUpperCase()); + return SsrfProtectionLevel.valueOf(level.name()); } catch (IllegalArgumentException e) { log.warn("Invalid SSRF protection level '{}', defaulting to MEDIUM", level); return SsrfProtectionLevel.MEDIUM; @@ -215,7 +215,8 @@ public class SsrfProtectionService { return false; } } - // For IPv4-mapped IPv6 addresses, bytes 10 and 11 must be 0xff (i.e., address is ::ffff:w.x.y.z) + // For IPv4-mapped IPv6 addresses, bytes 10 and 11 must be 0xff (i.e., address is + // ::ffff:w.x.y.z) return addr[10] == (byte) 0xff && addr[11] == (byte) 0xff; } diff --git a/app/core/src/main/java/stirling/software/SPDF/controller/api/converters/ConvertWebsiteToPDF.java b/app/core/src/main/java/stirling/software/SPDF/controller/api/converters/ConvertWebsiteToPDF.java index a7e194d4f..1cf3c16b2 100644 --- a/app/core/src/main/java/stirling/software/SPDF/controller/api/converters/ConvertWebsiteToPDF.java +++ b/app/core/src/main/java/stirling/software/SPDF/controller/api/converters/ConvertWebsiteToPDF.java @@ -1,17 +1,21 @@ package stirling.software.SPDF.controller.api.converters; import java.io.IOException; +import java.net.URI; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; import java.util.List; import org.apache.pdfbox.pdmodel.PDDocument; +import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.ModelAttribute; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.servlet.support.ServletUriComponentsBuilder; +import org.springframework.web.util.UriComponentsBuilder; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.tags.Tag; @@ -23,7 +27,6 @@ import stirling.software.SPDF.model.api.converters.UrlToPdfRequest; import stirling.software.common.configuration.RuntimePathConfig; import stirling.software.common.model.ApplicationProperties; import stirling.software.common.service.CustomPDFDocumentFactory; -import stirling.software.common.util.ExceptionUtils; import stirling.software.common.util.GeneralUtils; import stirling.software.common.util.ProcessExecutor; import stirling.software.common.util.ProcessExecutor.ProcessExecutorResult; @@ -46,24 +49,43 @@ public class ConvertWebsiteToPDF { description = "This endpoint fetches content from a URL and converts it to a PDF format." + " Input:N/A Output:PDF Type:SISO") - public ResponseEntity urlToPdf(@ModelAttribute UrlToPdfRequest request) + public ResponseEntity urlToPdf(@ModelAttribute UrlToPdfRequest request) throws IOException, InterruptedException { String URL = request.getUrlInput(); + UriComponentsBuilder uriComponentsBuilder = + ServletUriComponentsBuilder.fromCurrentContextPath().path("/url-to-pdf"); + URI location = null; + HttpStatus status = HttpStatus.SEE_OTHER; if (!applicationProperties.getSystem().getEnableUrlToPDF()) { - throw ExceptionUtils.createIllegalArgumentException( - "error.endpointDisabled", "This endpoint has been disabled by the admin"); - } + location = + uriComponentsBuilder + .queryParam("error", "error.endpointDisabled") + .build() + .toUri(); + } else + // Validate the URL format if (!URL.matches("^https?://.*") || !GeneralUtils.isValidURL(URL)) { - throw ExceptionUtils.createInvalidArgumentException( - "URL", "provided format is invalid"); - } + location = + uriComponentsBuilder + .queryParam("error", "error.invalidUrlFormat") + .build() + .toUri(); + } else // validate the URL is reachable if (!GeneralUtils.isURLReachable(URL)) { - throw ExceptionUtils.createIllegalArgumentException( - "error.urlNotReachable", "URL is not reachable, please provide a valid URL"); + location = + uriComponentsBuilder + .queryParam("error", "error.urlNotReachable") + .build() + .toUri(); + } + + if (location != null) { + log.info("Redirecting to: {}", location.toString()); + return ResponseEntity.status(status).location(location).build(); } Path tempOutputFile = null; diff --git a/app/core/src/main/resources/messages_en_GB.properties b/app/core/src/main/resources/messages_en_GB.properties index e529defe1..89cb1d891 100644 --- a/app/core/src/main/resources/messages_en_GB.properties +++ b/app/core/src/main/resources/messages_en_GB.properties @@ -193,6 +193,7 @@ error.fileFormatRequired=File must be in {0} format error.invalidFormat=Invalid {0} format: {1} error.endpointDisabled=This endpoint has been disabled by the admin error.urlNotReachable=URL is not reachable, please provide a valid URL +error.invalidUrlFormat=Invalid URL format provided. The provided format is invalid. # DPI and image rendering messages - used by frontend for dynamic translation # Backend sends: [TRANSLATE:messageKey:arg1,arg2] English message diff --git a/app/core/src/main/resources/templates/convert/url-to-pdf.html b/app/core/src/main/resources/templates/convert/url-to-pdf.html index 67e89c101..231ad8c3c 100644 --- a/app/core/src/main/resources/templates/convert/url-to-pdf.html +++ b/app/core/src/main/resources/templates/convert/url-to-pdf.html @@ -19,6 +19,7 @@ link +


diff --git a/app/core/src/test/java/stirling/software/SPDF/controller/api/converters/ConvertWebsiteToPdfTest.java b/app/core/src/test/java/stirling/software/SPDF/controller/api/converters/ConvertWebsiteToPdfTest.java index 29ad90bbe..a51dad6a7 100644 --- a/app/core/src/test/java/stirling/software/SPDF/controller/api/converters/ConvertWebsiteToPdfTest.java +++ b/app/core/src/test/java/stirling/software/SPDF/controller/api/converters/ConvertWebsiteToPdfTest.java @@ -1,71 +1,264 @@ package stirling.software.SPDF.controller.api.converters; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.when; +import java.io.File; +import java.io.IOException; +import java.lang.reflect.Method; +import java.net.URI; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import org.apache.pdfbox.pdmodel.PDDocument; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import org.mockito.MockedStatic; +import org.mockito.Mockito; import org.mockito.MockitoAnnotations; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.web.context.request.RequestContextHolder; +import org.springframework.web.context.request.ServletRequestAttributes; import stirling.software.SPDF.model.api.converters.UrlToPdfRequest; import stirling.software.common.configuration.RuntimePathConfig; import stirling.software.common.model.ApplicationProperties; import stirling.software.common.service.CustomPDFDocumentFactory; +import stirling.software.common.util.GeneralUtils; +import stirling.software.common.util.ProcessExecutor; +import stirling.software.common.util.ProcessExecutor.ProcessExecutorResult; +import stirling.software.common.util.ProcessExecutor.Processes; +import stirling.software.common.util.WebResponseUtils; public class ConvertWebsiteToPdfTest { - @Mock private CustomPDFDocumentFactory mockPdfDocumentFactory; - + @Mock private CustomPDFDocumentFactory pdfDocumentFactory; @Mock private RuntimePathConfig runtimePathConfig; private ApplicationProperties applicationProperties; - - private ConvertWebsiteToPDF convertWebsiteToPDF; + private ConvertWebsiteToPDF sut; + private AutoCloseable mocks; @BeforeEach - void setUp() { - MockitoAnnotations.openMocks(this); + void setUp() throws Exception { + mocks = MockitoAnnotations.openMocks(this); + + // Feature einschalten (ggf. Struktur an dein Projekt anpassen) applicationProperties = new ApplicationProperties(); applicationProperties.getSystem().setEnableUrlToPDF(true); - convertWebsiteToPDF = - new ConvertWebsiteToPDF( - mockPdfDocumentFactory, runtimePathConfig, applicationProperties); + + // Stubs, falls der Code weiterlaufen sollte + when(runtimePathConfig.getWeasyPrintPath()).thenReturn("/usr/bin/weasyprint"); + when(pdfDocumentFactory.load(any(File.class))).thenReturn(new PDDocument()); + + // SUT bauen + sut = new ConvertWebsiteToPDF(pdfDocumentFactory, runtimePathConfig, applicationProperties); + + // RequestContext für ServletUriComponentsBuilder bereitstellen + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("http"); + req.setServerName("localhost"); + req.setServerPort(8080); + RequestContextHolder.setRequestAttributes(new ServletRequestAttributes(req)); + } + + @AfterEach + void tearDown() throws Exception { + RequestContextHolder.resetRequestAttributes(); + if (mocks != null) mocks.close(); } @Test - public void test_exemption_is_thrown_when_invalid_url_format_provided() { + void redirect_with_error_when_invalid_url_format_provided() throws Exception { + UrlToPdfRequest request = new UrlToPdfRequest(); + request.setUrlInput("not-a-url"); - String invalid_format_Url = "invalid-url"; + ResponseEntity resp = sut.urlToPdf(request); + + assertEquals(HttpStatus.SEE_OTHER, resp.getStatusCode()); + URI location = resp.getHeaders().getLocation(); + assertNotNull(location, "Location header expected"); + assertTrue( + location.getQuery() != null + && location.getQuery().contains("error=error.invalidUrlFormat")); + } + + @Test + void redirect_with_error_when_url_is_not_reachable() throws Exception { + UrlToPdfRequest request = new UrlToPdfRequest(); + // .invalid ist per RFC reserviert und nicht auflösbar + request.setUrlInput("https://nonexistent.invalid/"); + + ResponseEntity resp = sut.urlToPdf(request); + + assertEquals(HttpStatus.SEE_OTHER, resp.getStatusCode()); + URI location = resp.getHeaders().getLocation(); + assertNotNull(location, "Location header expected"); + assertTrue( + location.getQuery() != null + && location.getQuery().contains("error=error.urlNotReachable")); + } + + @Test + void redirect_with_error_when_endpoint_disabled() throws Exception { + // Feature deaktivieren + applicationProperties.getSystem().setEnableUrlToPDF(false); UrlToPdfRequest request = new UrlToPdfRequest(); - request.setUrlInput(invalid_format_Url); - // Act - IllegalArgumentException thrown = - assertThrows( - IllegalArgumentException.class, - () -> { - convertWebsiteToPDF.urlToPdf(request); - }); - // Assert - assertEquals("Invalid URL format: provided format is invalid", thrown.getMessage()); + request.setUrlInput("https://example.com/"); + + ResponseEntity resp = sut.urlToPdf(request); + + assertEquals(HttpStatus.SEE_OTHER, resp.getStatusCode()); + URI location = resp.getHeaders().getLocation(); + assertNotNull(location, "Location header expected"); + assertTrue( + location.getQuery() != null + && location.getQuery().contains("error=error.endpointDisabled")); } @Test - public void test_exemption_is_thrown_when_url_is_not_reachable() { + void convertURLToFileName_sanitizes_and_appends_pdf() throws Exception { + Method m = + ConvertWebsiteToPDF.class.getDeclaredMethod("convertURLToFileName", String.class); + m.setAccessible(true); - String unreachable_Url = "https://www.googleeeexyz.com"; + String in = "https://ex-ample.com/path?q=1&x=y#frag"; + String out = (String) m.invoke(sut, in); + + assertTrue(out.endsWith(".pdf")); + // Nur A–Z, a–z, 0–9, Unterstrich und Punkt erlaubt + assertTrue(out.matches("[A-Za-z0-9_]+\\.pdf")); + // keine Truncation hier (Quelle ist nicht so lang) + assertTrue(out.length() <= 54); + } + + @Test + void convertURLToFileName_truncates_to_50_chars_before_pdf_suffix() throws Exception { + Method m = + ConvertWebsiteToPDF.class.getDeclaredMethod("convertURLToFileName", String.class); + m.setAccessible(true); + + // Sehr lange URL → löst Truncation aus + String longUrl = + "https://very-very-long-domain.example.com/some/really/long/path/with?many=params&and=chars"; + String out = (String) m.invoke(sut, longUrl); + + assertTrue(out.endsWith(".pdf")); + assertTrue(out.matches("[A-Za-z0-9_]+\\.pdf")); + // safeName ist auf 50 begrenzt → total max 54 inkl. ".pdf" + assertTrue(out.length() <= 54, "Filename should be truncated to 50 + '.pdf'"); + } + + @Test + void happy_path_executes_weasyprint_loads_pdf_and_returns_response() throws Exception { + UrlToPdfRequest request = new UrlToPdfRequest(); + request.setUrlInput("https://example.com"); + + try (MockedStatic pe = Mockito.mockStatic(ProcessExecutor.class); + MockedStatic wr = Mockito.mockStatic(WebResponseUtils.class); + MockedStatic gu = Mockito.mockStatic(GeneralUtils.class)) { + + // URL-Checks positiv erzwingen + gu.when(() -> GeneralUtils.isValidURL("https://example.com")).thenReturn(true); + gu.when(() -> GeneralUtils.isURLReachable("https://example.com")).thenReturn(true); + + // richtiger ProcessExecutor! + ProcessExecutor mockExec = Mockito.mock(ProcessExecutor.class); + pe.when(() -> ProcessExecutor.getInstance(Processes.WEASYPRINT)).thenReturn(mockExec); + + @SuppressWarnings("unchecked") + ArgumentCaptor> cmdCaptor = ArgumentCaptor.forClass(List.class); + + // Rückgabewert typgerecht + ProcessExecutorResult dummyResult = Mockito.mock(ProcessExecutorResult.class); + when(mockExec.runCommandWithOutputHandling(cmdCaptor.capture())) + .thenReturn(dummyResult); + + // WebResponseUtils mocken + ResponseEntity fakeResponse = ResponseEntity.ok(new byte[0]); + wr.when(() -> WebResponseUtils.pdfDocToWebResponse(any(PDDocument.class), anyString())) + .thenReturn(fakeResponse); + + // Act + ResponseEntity resp = sut.urlToPdf(request); + + // Assert – Response OK + assertEquals(HttpStatus.OK, resp.getStatusCode()); + + // Assert – WeasyPrint-Kommando korrekt + List cmd = cmdCaptor.getValue(); + assertNotNull(cmd); + assertEquals("/usr/bin/weasyprint", cmd.get(0)); + assertEquals("https://example.com", cmd.get(1)); + assertEquals("--pdf-forms", cmd.get(2)); + assertTrue(cmd.size() >= 4, "WeasyPrint sollte einen Output-Pfad erhalten"); + String outPathStr = cmd.get(3); + assertNotNull(outPathStr); + + // Temp-Datei muss im finally gelöscht sein + Path outPath = Path.of(outPathStr); + assertFalse( + Files.exists(outPath), "Temp-Output-Datei sollte nach dem Call gelöscht sein"); + } + } + + @Test + void finally_block_logs_and_swallows_ioexception_on_delete() throws Exception { // Arrange UrlToPdfRequest request = new UrlToPdfRequest(); - request.setUrlInput(unreachable_Url); - // Act - IllegalArgumentException thrown = - assertThrows( - IllegalArgumentException.class, - () -> { - convertWebsiteToPDF.urlToPdf(request); - }); - // Assert - assertEquals("URL is not reachable, please provide a valid URL", thrown.getMessage()); + request.setUrlInput("https://example.com"); + + Path preCreatedTemp = java.nio.file.Files.createTempFile("test_output_", ".pdf"); + + try (MockedStatic gu = Mockito.mockStatic(GeneralUtils.class); + MockedStatic pe = Mockito.mockStatic(ProcessExecutor.class); + MockedStatic wr = Mockito.mockStatic(WebResponseUtils.class); + MockedStatic files = Mockito.mockStatic(Files.class)) { + + // URL-Checks positiv + gu.when(() -> GeneralUtils.isValidURL("https://example.com")).thenReturn(true); + gu.when(() -> GeneralUtils.isURLReachable("https://example.com")).thenReturn(true); + + // Temp-Datei erzwingen + Delete-Fehler provozieren + files.when(() -> Files.createTempFile("output_", ".pdf")).thenReturn(preCreatedTemp); + files.when(() -> Files.deleteIfExists(preCreatedTemp)) + .thenThrow(new IOException("fail delete")); + files.when(() -> Files.exists(preCreatedTemp)).thenReturn(true); // für den Assert + + // ProcessExecutor + ProcessExecutor mockExec = Mockito.mock(ProcessExecutor.class); + pe.when(() -> ProcessExecutor.getInstance(Processes.WEASYPRINT)).thenReturn(mockExec); + ProcessExecutorResult dummy = Mockito.mock(ProcessExecutorResult.class); + when(mockExec.runCommandWithOutputHandling(Mockito.any())).thenReturn(dummy); + + // WebResponseUtils + ResponseEntity fakeResponse = ResponseEntity.ok(new byte[0]); + wr.when(() -> WebResponseUtils.pdfDocToWebResponse(any(PDDocument.class), anyString())) + .thenReturn(fakeResponse); + + // Act: darf keine Exception werfen und soll eine Response liefern + ResponseEntity resp = assertDoesNotThrow(() -> sut.urlToPdf(request)); + + // Assert + assertNotNull(resp, "Response should not be null"); + assertEquals(HttpStatus.OK, resp.getStatusCode()); + assertTrue( + java.nio.file.Files.exists(preCreatedTemp), + "Temp-Datei sollte trotz Lösch-IOException noch existieren"); + } finally { + try { + java.nio.file.Files.deleteIfExists(preCreatedTemp); + } catch (IOException ignore) { + } + } } }