Fix/codeql security findings

.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 }}

.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

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

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();
     }

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<String> 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<String, Object> body = new LinkedMultiValueMap<>();
-        body.add("file", new FileSystemResource(convFile));
+            MultiValueMap<String, Object> body = new LinkedMultiValueMap<>();
+            body.add("file", new FileSystemResource(convFile));
 
-        HttpEntity<MultiValueMap<String, Object>> requestEntity = new HttpEntity<>(body, headers);
+            HttpEntity<MultiValueMap<String, Object>> requestEntity = new HttpEntity<>(body, headers);
 
-        RestTemplate restTemplate = new RestTemplate();
-        ResponseEntity<String> response = restTemplate.postForEntity(dqtExcelToJsonUrl, requestEntity, String.class);
+            RestTemplate restTemplate = new RestTemplate();
+            ResponseEntity<String> 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;
     }
 
 

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;
+    }
+}

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('.'));
+    }
+}

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 @@
         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 @@
             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