From 6587f967feef178b40f5041ec57aa40eb0ff73ee Mon Sep 17 00:00:00 2001 From: KFilippopolitis Date: Tue, 15 Sep 2026 16:15:55 +0300 Subject: [PATCH 1/4] fix(backend): keep CSRF protection on for every profile `authentication.enabled=0` disabled CSRF entirely, leaving state-changing requests unprotected whenever the API is reachable without a login. The double-submit cookie repository is now shared by both branches of the filter chain and documented in the README for curl/Postman users. Co-authored-by: ChatGPT --- README.md | 4 + .../configurations/SecurityConfiguration.java | 50 ++++++----- .../SecurityConfigurationCsrfTest.java | 87 +++++++++++++++++++ 3 files changed, 118 insertions(+), 23 deletions(-) create mode 100644 backend/src/test/java/ebrainsv2/mip/datacatalog/configurations/SecurityConfigurationCsrfTest.java diff --git a/README.md b/README.md index 6b75f95..3f32ecb 100644 --- a/README.md +++ b/README.md @@ -85,6 +85,10 @@ mvn spring-boot:run Use `mvn test` to run backend tests. +State-changing requests are CSRF-protected in every profile, including `AUTHENTICATION=0`. The Angular +client copies the `MIP-XSRF-TOKEN` cookie into the `X-MIP-XSRF-TOKEN` header on its own; send that pair +by hand when calling `POST`, `PUT`, or `DELETE` endpoints with `curl` or Postman. + ### Data Quality Tool ```bash diff --git a/backend/src/main/java/ebrainsv2/mip/datacatalog/configurations/SecurityConfiguration.java b/backend/src/main/java/ebrainsv2/mip/datacatalog/configurations/SecurityConfiguration.java index 9e6f7a3..7a0d918 100644 --- a/backend/src/main/java/ebrainsv2/mip/datacatalog/configurations/SecurityConfiguration.java +++ b/backend/src/main/java/ebrainsv2/mip/datacatalog/configurations/SecurityConfiguration.java @@ -7,7 +7,6 @@ import org.springframework.security.config.annotation.method.configuration.EnableMethodSecurity; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; -import org.springframework.security.config.annotation.web.configurers.AbstractHttpConfigurer; import org.springframework.security.config.http.SessionCreationPolicy; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.SimpleGrantedAuthority; @@ -75,21 +74,37 @@ public ClientRegistrationRepository clientRegistrationRepository() { return new InMemoryClientRegistrationRepository(dummyRegistration); } + /** + * The double-submit cookie consumed by the Angular XSRF interceptor (see + * {@code frontend/src/main.ts}). CSRF protection stays on for every profile: disabling it + * for {@code authentication.enabled=0} would leave state-changing requests unprotected + * whenever the API is reachable without a login. + */ + static CookieCsrfTokenRepository csrfTokenRepository() { + CookieCsrfTokenRepository csrfTokenRepository = CookieCsrfTokenRepository.withHttpOnlyFalse(); + csrfTokenRepository.setCookiePath("/"); + csrfTokenRepository.setCookieName("MIP-XSRF-TOKEN"); + csrfTokenRepository.setHeaderName("X-MIP-XSRF-TOKEN"); + return csrfTokenRepository; + } + @Bean public SecurityFilterChain clientSecurityFilterChain(HttpSecurity http, ClientRegistrationRepository clientRegistrationRepo, OAuth2AuthorizedClientService authorizedClientService) throws Exception { - if (authenticationEnabled) { - CookieCsrfTokenRepository csrfTokenRepository = CookieCsrfTokenRepository.withHttpOnlyFalse(); - csrfTokenRepository.setCookiePath("/"); - csrfTokenRepository.setCookieName("MIP-XSRF-TOKEN"); - csrfTokenRepository.setHeaderName("X-MIP-XSRF-TOKEN"); + http + .sessionManagement(session -> session.sessionCreationPolicy(SessionCreationPolicy.IF_REQUIRED)) + .authorizeHttpRequests(auth -> auth + .anyRequest().permitAll() // Allow access to any endpoint unless restricted by @PreAuthorize + ) + .csrf(csrf -> csrf + .csrfTokenRepository(csrfTokenRepository()) + .csrfTokenRequestHandler(new CsrfTokenRequestAttributeHandler()) + ) + .addFilterAfter(new CsrfCookieFilter(), BasicAuthenticationFilter.class); + if (authenticationEnabled) { http - .sessionManagement(session -> session.sessionCreationPolicy(SessionCreationPolicy.IF_REQUIRED)) - .authorizeHttpRequests(auth -> auth - .anyRequest().permitAll() // Allow access to any endpoint unless restricted by @PreAuthorize - ) .oauth2Login(oauth -> oauth .userInfoEndpoint(userInfo -> userInfo.oidcUserService(oidcUserService())) .defaultSuccessUrl(this.authCallbackUrl, true) @@ -102,7 +117,7 @@ public SecurityFilterChain clientSecurityFilterChain(HttpSecurity http, ClientRe ); String token = client.getAccessToken().getTokenValue(); - System.out.println("Authentication successful. Redirecting to Angular auth-callback with token: " + token); + System.out.println("Authentication successful. Redirecting to Angular auth-callback."); response.sendRedirect(this.authCallbackUrl + "?token=" + token); }) @@ -111,18 +126,7 @@ public SecurityFilterChain clientSecurityFilterChain(HttpSecurity http, ClientRe OidcClientInitiatedLogoutSuccessHandler successHandler = new OidcClientInitiatedLogoutSuccessHandler(clientRegistrationRepo); successHandler.setPostLogoutRedirectUri(this.frontendBaseUrl); logout.logoutSuccessHandler(successHandler); - }) - .csrf(csrf -> csrf - .csrfTokenRepository(csrfTokenRepository) - .csrfTokenRequestHandler(new CsrfTokenRequestAttributeHandler()) - ) - .addFilterAfter(new CsrfCookieFilter(), BasicAuthenticationFilter.class); - } else { - http - .authorizeHttpRequests(auth -> auth - .anyRequest().permitAll() - ) - .csrf(AbstractHttpConfigurer::disable); + }); } return http.build(); } diff --git a/backend/src/test/java/ebrainsv2/mip/datacatalog/configurations/SecurityConfigurationCsrfTest.java b/backend/src/test/java/ebrainsv2/mip/datacatalog/configurations/SecurityConfigurationCsrfTest.java new file mode 100644 index 0000000..0ddd9c4 --- /dev/null +++ b/backend/src/test/java/ebrainsv2/mip/datacatalog/configurations/SecurityConfigurationCsrfTest.java @@ -0,0 +1,87 @@ +package ebrainsv2.mip.datacatalog.configurations; + +import jakarta.servlet.http.Cookie; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.web.csrf.CookieCsrfTokenRepository; +import org.springframework.security.web.csrf.CsrfToken; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +public class SecurityConfigurationCsrfTest { + + private static final String COOKIE_NAME = "MIP-XSRF-TOKEN"; + private static final String HEADER_NAME = "X-MIP-XSRF-TOKEN"; + + @Test + void tokenIsStoredInAJavascriptReadableCookieOnTheRootPath() { + CookieCsrfTokenRepository csrfTokenRepository = SecurityConfiguration.csrfTokenRepository(); + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + + csrfTokenRepository.saveToken(csrfTokenRepository.generateToken(request), request, response); + + String serialized = serializedCookies(response); + assertTrue(serialized.contains(COOKIE_NAME + "="), "CSRF cookie missing: " + serialized); + assertTrue(serialized.contains("Path=/"), "cookie must be sent for every path: " + serialized); + assertFalse(serialized.contains("HttpOnly"), "the Angular interceptor must be able to read it: " + serialized); + } + + @Test + void cookieIsReadBackIntoTheHeaderExpectedFromTheClient() { + CookieCsrfTokenRepository csrfTokenRepository = SecurityConfiguration.csrfTokenRepository(); + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setCookies(new Cookie(COOKIE_NAME, "csrf-token-value")); + + CsrfToken token = csrfTokenRepository.loadToken(request); + + assertNotNull(token, "the token must be loaded from the double-submit cookie"); + assertEquals(HEADER_NAME, token.getHeaderName()); + } + + @Test + void angularBootstrapIsConfiguredWithTheSameCookieAndHeader() throws IOException { + Path bootstrapScript = angularBootstrapScript(); + assumeTrue(bootstrapScript != null, "frontend/src/main.ts is not part of this checkout"); + + String bootstrap = Files.readString(bootstrapScript); + + assertTrue(bootstrap.contains("cookieName: '" + COOKIE_NAME + "'"), "unrelated XSRF cookie in " + bootstrapScript); + assertTrue(bootstrap.contains("headerName: '" + HEADER_NAME + "'"), "unrelated XSRF header in " + bootstrapScript); + } + + private static String serializedCookies(MockHttpServletResponse response) { + StringBuilder serialized = new StringBuilder(); + for (String header : response.getHeaders("Set-Cookie")) { + serialized.append(header).append('\n'); + } + for (Cookie cookie : response.getCookies()) { + serialized.append(cookie.getName()).append('=').append(cookie.getValue()) + .append("; Path=").append(cookie.getPath()); + if (cookie.isHttpOnly()) { + serialized.append("; HttpOnly"); + } + serialized.append('\n'); + } + return serialized.toString(); + } + + private static Path angularBootstrapScript() { + for (Path directory = Path.of("").toAbsolutePath(); directory != null; directory = directory.getParent()) { + Path candidate = directory.resolve(Path.of("frontend", "src", "main.ts")); + if (Files.isRegularFile(candidate)) { + return candidate; + } + } + return null; + } +} From 0309d42e62ff5fd50752cf6e3f83c2714b8a82de Mon Sep 17 00:00:00 2001 From: KFilippopolitis Date: Tue, 15 Sep 2026 16:15:55 +0300 Subject: [PATCH 2/4] fix(backend): store data model uploads at server-generated paths `DataModelConverter.convertExcelToDataModel` joined the client-supplied upload name onto `java.io.tmpdir`, so a crafted name could write outside the temporary directory and the leftover file was never removed. The upload now goes to a generated `Files.createTempFile` path that keeps only an allow-listed extension, and is deleted in a `finally` block. Co-authored-by: ChatGPT --- .../datamodel/DataModelConverter.java | 76 ++++++++++++++----- .../DataModelConverterUploadTest.java | 49 ++++++++++++ 2 files changed, 106 insertions(+), 19 deletions(-) create mode 100644 backend/src/test/java/ebrainsv2/mip/datacatalog/datamodel/DataModelConverterUploadTest.java diff --git a/backend/src/main/java/ebrainsv2/mip/datacatalog/datamodel/DataModelConverter.java b/backend/src/main/java/ebrainsv2/mip/datacatalog/datamodel/DataModelConverter.java index 6df55cf..d226af0 100644 --- a/backend/src/main/java/ebrainsv2/mip/datacatalog/datamodel/DataModelConverter.java +++ b/backend/src/main/java/ebrainsv2/mip/datacatalog/datamodel/DataModelConverter.java @@ -13,15 +13,25 @@ import org.springframework.web.client.RestTemplate; import org.springframework.web.multipart.MultipartFile; -import java.io.File; import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.util.List; +import java.util.Locale; import java.util.Map; public class DataModelConverter { private static final ObjectMapper objectMapper = new ObjectMapper(); + private static final String UPLOAD_PREFIX = "datacatalog-import-"; + private static final String DEFAULT_UPLOAD_SUFFIX = ".xlsx"; + // The suffix is selected from this list instead of being derived from the upload name, so no + // part of the temporary path can be influenced by the client. + private static final List ALLOWED_UPLOAD_SUFFIXES = List.of(".xlsx", ".xls"); + public static ByteArrayResource convertDataModelDTOToExcel(String dqtJsonToExcelUrl, DataModelDTO dataModel) throws IOException { ObjectMapper objectMapper = new ObjectMapper(); @@ -42,30 +52,58 @@ public static DataModelDTO convertExcelToDataModelDTO(String dqtExcelToJsonUrl, MultipartFile file, String version, boolean longitudinal) throws IOException { - // Convert MultipartFile to File - File convFile = new File(System.getProperty("java.io.tmpdir") + "/" + file.getOriginalFilename()); - file.transferTo(convFile); + // The upload name is supplied by the client, so the upload is stored under a generated + // temporary path and only its extension is reused. + Path convFile = createTempUpload(file.getOriginalFilename()); + try (InputStream upload = file.getInputStream()) { + Files.copy(upload, convFile, StandardCopyOption.REPLACE_EXISTING); + } - // Setup the request to Flask API - HttpHeaders headers = new HttpHeaders(); - headers.setContentType(MediaType.MULTIPART_FORM_DATA); + try { + // Setup the request to Flask API + HttpHeaders headers = new HttpHeaders(); + headers.setContentType(MediaType.MULTIPART_FORM_DATA); - MultiValueMap body = new LinkedMultiValueMap<>(); - body.add("file", new FileSystemResource(convFile)); + MultiValueMap body = new LinkedMultiValueMap<>(); + body.add("file", new FileSystemResource(convFile)); - HttpEntity> requestEntity = new HttpEntity<>(body, headers); + HttpEntity> requestEntity = new HttpEntity<>(body, headers); - RestTemplate restTemplate = new RestTemplate(); - ResponseEntity response = restTemplate.postForEntity(dqtExcelToJsonUrl, requestEntity, String.class); + RestTemplate restTemplate = new RestTemplate(); + ResponseEntity response = restTemplate.postForEntity(dqtExcelToJsonUrl, requestEntity, String.class); - // Convert JSON response to a Map and add "longitudinal" and "version" fields - ObjectMapper objectMapper = new ObjectMapper(); - Map dataMap = objectMapper.readValue(response.getBody(), Map.class); - dataMap.put("longitudinal", longitudinal); - dataMap.put("version", version); + // Convert JSON response to a Map and add "longitudinal" and "version" fields + ObjectMapper objectMapper = new ObjectMapper(); + Map dataMap = objectMapper.readValue(response.getBody(), Map.class); + dataMap.put("longitudinal", longitudinal); + dataMap.put("version", version); + + // Convert the modified Map back to JSON and map it to DataModelDTO + return objectMapper.convertValue(dataMap, DataModelDTO.class); + } finally { + Files.deleteIfExists(convFile); + } + } + + static Path createTempUpload(String originalFilename) throws IOException { + return Files.createTempFile(UPLOAD_PREFIX, safeUploadSuffix(originalFilename)); + } - // Convert the modified Map back to JSON and map it to DataModelDTO - return objectMapper.convertValue(dataMap, DataModelDTO.class); + static String safeUploadSuffix(String originalFilename) { + if (originalFilename == null) { + return DEFAULT_UPLOAD_SUFFIX; + } + int lastDot = originalFilename.lastIndexOf('.'); + if (lastDot < 0) { + return DEFAULT_UPLOAD_SUFFIX; + } + String candidate = originalFilename.substring(lastDot).toLowerCase(Locale.ROOT); + for (String allowed : ALLOWED_UPLOAD_SUFFIXES) { + if (allowed.equals(candidate)) { + return allowed; + } + } + return DEFAULT_UPLOAD_SUFFIX; } diff --git a/backend/src/test/java/ebrainsv2/mip/datacatalog/datamodel/DataModelConverterUploadTest.java b/backend/src/test/java/ebrainsv2/mip/datacatalog/datamodel/DataModelConverterUploadTest.java new file mode 100644 index 0000000..530ad3a --- /dev/null +++ b/backend/src/test/java/ebrainsv2/mip/datacatalog/datamodel/DataModelConverterUploadTest.java @@ -0,0 +1,49 @@ +package ebrainsv2.mip.datacatalog.datamodel; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class DataModelConverterUploadTest { + + private static final Path TEMP_DIR = Path.of(System.getProperty("java.io.tmpdir")); + + @Test + void uploadIsStoredInTheTemporaryDirectoryNamedByTheServer() throws IOException { + Path upload = DataModelConverter.createTempUpload("../../../../etc/passwd.xlsx"); + try { + assertEquals(TEMP_DIR.toRealPath(), upload.getParent().toRealPath(), + "The client supplied file name must not steer the destination directory."); + assertTrue(upload.getFileName().toString().startsWith("datacatalog-import-"), + "The client supplied file name must not steer the destination file name."); + assertEquals(".xlsx", suffix(upload)); + } finally { + Files.deleteIfExists(upload); + } + } + + @Test + void ordinaryExcelExtensionIsKept() { + assertEquals(".xlsx", DataModelConverter.safeUploadSuffix("Minimal Data Model.xlsx")); + assertEquals(".xls", DataModelConverter.safeUploadSuffix("model.xls")); + } + + @Test + void unsafeOrMissingExtensionFallsBackToExcel() { + assertEquals(".xlsx", DataModelConverter.safeUploadSuffix("../../etc/passwd")); + assertEquals(".xlsx", DataModelConverter.safeUploadSuffix("model.")); + assertEquals(".xlsx", DataModelConverter.safeUploadSuffix("no-extension")); + assertEquals(".xlsx", DataModelConverter.safeUploadSuffix("very.long.extensionlength")); + assertEquals(".xlsx", DataModelConverter.safeUploadSuffix(null)); + } + + private static String suffix(Path path) { + String name = path.getFileName().toString(); + return name.substring(name.lastIndexOf('.')); + } +} From b54d2c216d16c2f64ae5c26caccddb96219600c4 Mon Sep 17 00:00:00 2001 From: KFilippopolitis Date: Tue, 15 Sep 2026 16:15:59 +0300 Subject: [PATCH 3/4] fix(data_quality_tool): bound validation error responses Both validation endpoints echoed the raw exception text, which CodeQL flags as stack-trace exposure. `error_reporting.client_error_message` now keeps a single line of the validator report, drops anything that looks like a trace back, and truncates to 300 characters; the unhandled-exception branch logs the full stack and returns a generic 500. `validate_json` also caught the imported alias instead of the `json_validator` attribute the controller raises, so domain errors were reported as 500. Co-authored-by: ChatGPT --- data_quality_tool/controller.py | 21 ++++-- data_quality_tool/error_reporting.py | 33 +++++++++ data_quality_tool/tests/test_endpoint.py | 70 ++++++++++++++++++- .../tests/test_error_reporting.py | 64 +++++++++++++++++ 4 files changed, 180 insertions(+), 8 deletions(-) create mode 100644 data_quality_tool/error_reporting.py create mode 100644 data_quality_tool/tests/test_error_reporting.py diff --git a/data_quality_tool/controller.py b/data_quality_tool/controller.py index 3aaebdd..0867eb1 100644 --- a/data_quality_tool/controller.py +++ b/data_quality_tool/controller.py @@ -9,6 +9,7 @@ from werkzeug.exceptions import RequestEntityTooLarge from common_entities import InvalidDataModelError +from error_reporting import client_error_message from converter.excel_to_json import convert_excel_to_json from converter.json_to_excel import convert_json_to_excel from validator import json_validator, excel_validator @@ -127,11 +128,14 @@ def validate_json(): json_validator.validate_json(json_data) logger.info("JSON data is valid") return jsonify({"message": "Data model is valid."}) - except json_validator.InvalidDataModelError as e: - logger.error(f"JSON validation error: {str(e)}") - return jsonify({"error": str(e)}), 400 - except Exception as e: - logger.error(f"Unhandled error: {str(e)}") + except InvalidDataModelError as e: + logger.error("JSON validation error: %s", e) + # client_error_message keeps the report to a single bounded line and replaces anything + # that looks like a stack trace, so no runtime detail reaches the caller. + # codeql[py/stack-trace-exposure] Validator report for the caller's own payload. + return jsonify({"error": client_error_message(e)}), 400 + except Exception: + logger.exception("Unhandled error while validating JSON.") return jsonify({"error": "Internal server error"}), 500 @@ -155,8 +159,11 @@ def validate_excel(): logger.info("Excel file is valid") return jsonify({"message": "Data model is valid."}) except InvalidDataModelError as e: - logger.error(f"Excel validation error: {str(e)}") - return jsonify({"error": str(e)}), 400 + logger.error("Excel validation error: %s", e) + # Same as for /validate-json: a single bounded line describing the uploaded + # workbook, never a stack trace. + # codeql[py/stack-trace-exposure] Validator report for the uploaded workbook. + return jsonify({"error": client_error_message(e)}), 400 except (BadZipFile, ValueError): logger.error("Invalid Excel file format.") return jsonify({"error": "Invalid Excel file format."}), 400 diff --git a/data_quality_tool/error_reporting.py b/data_quality_tool/error_reporting.py new file mode 100644 index 0000000..64f8e41 --- /dev/null +++ b/data_quality_tool/error_reporting.py @@ -0,0 +1,33 @@ +"""Rendering of error details for API responses. + +The validators build detailed, actionable messages for domain experts (missing column, bad +enumeration format, ...). Those describe the caller's own payload and are shown in the UI, but the +text still travels through the exception handling of the service, so anything that looks like a +stack trace is dropped and the remaining report is length-bounded before it leaves the process. +""" + +import re + +GENERIC_VALIDATION_ERROR = "The data model could not be validated." +MAX_CLIENT_ERROR_LENGTH = 300 + +_TRACEBACK_MARKER = re.compile( + r"Traceback \(most recent call last\)" + r"|File \"[^\"]+\", line \d+" + r"|site-packages" +) +# Control characters have no business in a message shown to a user; line breaks and tabs are +# already handled by the whitespace collapse below. +_CONTROL_CHARS = re.compile(r"[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]") + + +def client_error_message(details: object) -> str: + """Return a single-line, bounded rendering of validation details for an API response.""" + message = str(details) + if _TRACEBACK_MARKER.search(message): + # A stack trace reached us instead of a validation report; keep it in the logs only. + return GENERIC_VALIDATION_ERROR + message = _CONTROL_CHARS.sub("", " ".join(message.splitlines()[0].split())) + if len(message) > MAX_CLIENT_ERROR_LENGTH: + message = f"{message[: MAX_CLIENT_ERROR_LENGTH - 1].rstrip()}…" + return message diff --git a/data_quality_tool/tests/test_endpoint.py b/data_quality_tool/tests/test_endpoint.py index b1d4884..345f113 100644 --- a/data_quality_tool/tests/test_endpoint.py +++ b/data_quality_tool/tests/test_endpoint.py @@ -2,11 +2,15 @@ import unittest from io import BytesIO from pathlib import Path +from unittest.mock import patch import pandas as pd from data_quality_tool.common_entities import EXCEL_COLUMNS -from controller import app + +# The controller catches the class from its own module, so the tests raise that one. +from controller import InvalidDataModelError, app +from error_reporting import GENERIC_VALIDATION_ERROR, MAX_CLIENT_ERROR_LENGTH FIXTURES_DIR = Path(__file__).resolve().parent @@ -324,6 +328,70 @@ def test_validate_excel_missing_column(self): response_data["error"], ) + def _excel_payload(self): + with open(FIXTURES_DIR / "MinimalDataModelExample.xlsx", "rb") as file: + return {"file": (BytesIO(file.read()), "MinimalDataModelExample.xlsx")} + + def test_validate_json_hides_stack_traces(self): + with patch( + "controller.json_validator.validate_json", + side_effect=InvalidDataModelError( + "Traceback (most recent call last):\n" + ' File "/app/controller.py", line 42, in validate_json\n' + "KeyError: 'csvFile'" + ), + ): + response = self.client.post( + "/validate-json", json={"code": "DM"}, content_type="application/json" + ) + + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json, {"error": GENERIC_VALIDATION_ERROR}) + + def test_validate_json_bounds_validation_detail(self): + with patch( + "controller.json_validator.validate_json", + side_effect=InvalidDataModelError("y" * (MAX_CLIENT_ERROR_LENGTH * 2)), + ): + response = self.client.post( + "/validate-json", json={"code": "DM"}, content_type="application/json" + ) + + self.assertEqual(response.status_code, 400) + self.assertEqual(len(response.json["error"]), MAX_CLIENT_ERROR_LENGTH) + + def test_validate_excel_hides_stack_traces(self): + with patch( + "controller.excel_validator.validate_excel", + side_effect=InvalidDataModelError( + "Traceback (most recent call last):\n" + ' File "/app/controller.py", line 42, in validate_excel\n' + "KeyError: 'csvFile'" + ), + ): + response = self.client.post( + "/validate-excel", + content_type="multipart/form-data", + data=self._excel_payload(), + ) + + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json, {"error": GENERIC_VALIDATION_ERROR}) + + def test_validate_excel_bounds_validation_detail(self): + with patch( + "controller.excel_validator.validate_excel", + side_effect=InvalidDataModelError("y" * (MAX_CLIENT_ERROR_LENGTH * 2)), + ): + response = self.client.post( + "/validate-excel", + content_type="multipart/form-data", + data=self._excel_payload(), + ) + + self.assertEqual(response.status_code, 400) + self.assertEqual(len(response.json["error"]), MAX_CLIENT_ERROR_LENGTH) + def test_excel_to_json_invalid_excel_format(self): response = self.client.post( "/excel-to-json", diff --git a/data_quality_tool/tests/test_error_reporting.py b/data_quality_tool/tests/test_error_reporting.py new file mode 100644 index 0000000..b801f93 --- /dev/null +++ b/data_quality_tool/tests/test_error_reporting.py @@ -0,0 +1,64 @@ +import unittest + +from error_reporting import ( + GENERIC_VALIDATION_ERROR, + MAX_CLIENT_ERROR_LENGTH, + client_error_message, +) + +STACK_TRACE = ( + "Traceback (most recent call last):\n" + ' File "/app/controller.py", line 42, in validate_json\n' + "KeyError: 'csvFile'" +) + + +class TestClientErrorMessage(unittest.TestCase): + def test_keeps_validation_report_unchanged(self): + details = "On :dataset got: Missing value for required column 'name'." + self.assertEqual(client_error_message(details), details) + + def test_keeps_data_model_path(self): + details = "Missing required field 'code' in CommonDataElement at path: '/G1/G2/dataset'." + self.assertEqual(client_error_message(details), details) + + def test_keeps_concept_path_hint(self): + details = "ConceptPath format error: 'characters/characters/...' expected." + self.assertEqual(client_error_message(details), details) + + def test_replaces_stack_trace_with_generic_message(self): + self.assertEqual(client_error_message(STACK_TRACE), GENERIC_VALIDATION_ERROR) + + def test_replaces_frame_from_site_packages(self): + details = ' File "/usr/lib/python3/site-packages/pandas/io/excel.py", line 1' + self.assertEqual(client_error_message(details), GENERIC_VALIDATION_ERROR) + + def test_keeps_only_the_first_line(self): + message = client_error_message( + "Invalid Excel file format.\nSecond internal line" + ) + self.assertEqual(message, "Invalid Excel file format.") + + def test_collapses_whitespace(self): + self.assertEqual( + client_error_message(" bad type:\t\tnominal "), "bad type: nominal" + ) + + def test_drops_control_characters(self): + self.assertEqual( + client_error_message("missing 'name' \x00\x1b[31m column"), + "missing 'name' [31m column", + ) + + def test_keeps_quoted_and_accented_column_names(self): + details = 'Column "Âge (années)" is missing the required meta sheet.' + self.assertEqual(client_error_message(details), details) + + def test_bounds_length(self): + message = client_error_message("x" * (MAX_CLIENT_ERROR_LENGTH * 2)) + self.assertEqual(len(message), MAX_CLIENT_ERROR_LENGTH) + self.assertTrue(message.endswith("…")) + + +if __name__ == "__main__": + unittest.main() From 031766fd8db391e522abc8b43f621868b874da93 Mon Sep 17 00:00:00 2001 From: KFilippopolitis Date: Tue, 15 Sep 2026 16:15:59 +0300 Subject: [PATCH 4/4] fix(ci): grant least-privilege workflow permissions The CodeQL security findings flagged the default write token: pin `contents: read` on the data quality tool and EBRAINS sync jobs. Co-authored-by: ChatGPT --- .github/workflows/data_quality_tool.yml | 2 ++ .github/workflows/ebrains.yml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/.github/workflows/data_quality_tool.yml b/.github/workflows/data_quality_tool.yml index c7fd62f..3159e30 100644 --- a/.github/workflows/data_quality_tool.yml +++ b/.github/workflows/data_quality_tool.yml @@ -5,6 +5,8 @@ on: [push, pull_request] jobs: build: runs-on: ubuntu-latest + permissions: + contents: read env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} diff --git a/.github/workflows/ebrains.yml b/.github/workflows/ebrains.yml index 9f11a83..f3306d9 100644 --- a/.github/workflows/ebrains.yml +++ b/.github/workflows/ebrains.yml @@ -9,6 +9,8 @@ on: jobs: to_ebrains: runs-on: ubuntu-latest + permissions: + contents: read steps: - name: syncmaster uses: wei/git-sync@v3