From 91ecc4a8d947fcf8009077c7d5a43f69695d2be5 Mon Sep 17 00:00:00 2001 From: dennis Date: Tue, 1 Oct 2024 21:33:17 +0200 Subject: [PATCH 1/3] add csrf protection for sameSite none cookie --- .../core/config/SecurityConfiguration.java | 5 ++++ .../artemis/core/config/WebConfigurer.java | 3 +- .../security/filter/CsrfArtemisFilter.java | 30 +++++++++++++++++++ .../core/security/jwt/JWTCookieService.java | 17 ++--------- src/main/resources/config/application.yml | 17 ++++++----- src/main/webapp/app/app.module.ts | 3 ++ .../webapp/app/core/csrf/csrf.interceptor.ts | 12 ++++++++ 7 files changed, 65 insertions(+), 22 deletions(-) create mode 100644 src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java create mode 100644 src/main/webapp/app/core/csrf/csrf.interceptor.ts diff --git a/src/main/java/de/tum/cit/aet/artemis/core/config/SecurityConfiguration.java b/src/main/java/de/tum/cit/aet/artemis/core/config/SecurityConfiguration.java index baacbfd73966..281fd3fe4d58 100644 --- a/src/main/java/de/tum/cit/aet/artemis/core/config/SecurityConfiguration.java +++ b/src/main/java/de/tum/cit/aet/artemis/core/config/SecurityConfiguration.java @@ -35,6 +35,7 @@ import de.tum.cit.aet.artemis.core.security.DomainUserDetailsService; import de.tum.cit.aet.artemis.core.security.Role; +import de.tum.cit.aet.artemis.core.security.filter.CsrfArtemisFilter; import de.tum.cit.aet.artemis.core.security.filter.SpaWebFilter; import de.tum.cit.aet.artemis.core.security.jwt.JWTConfigurer; import de.tum.cit.aet.artemis.core.security.jwt.TokenProvider; @@ -54,6 +55,8 @@ public class SecurityConfiguration { private final CorsFilter corsFilter; + private final CsrfArtemisFilter csrfArtemisFilter = new CsrfArtemisFilter(); + private final ProfileService profileService; private final Optional customLti13Configurer; @@ -171,7 +174,9 @@ public SecurityFilterChain filterChain(HttpSecurity http, SecurityProblemSupport // Disables CSRF (Cross-Site Request Forgery) protection; useful in stateless APIs where the token management is unnecessary. .csrf(AbstractHttpConfigurer::disable) // Adds a CORS (Cross-Origin Resource Sharing) filter before the username/password authentication to handle cross-origin requests. + // sets the filter chain: CorsFilter -> CsrfArtemisFilter -> UsernamePasswordAuthenticationFilter .addFilterBefore(corsFilter, UsernamePasswordAuthenticationFilter.class) + .addFilterBefore(csrfArtemisFilter, UsernamePasswordAuthenticationFilter.class) // Configures exception handling with a custom entry point and access denied handler for authentication issues. .exceptionHandling(handler -> handler.authenticationEntryPoint(securityProblemSupport).accessDeniedHandler(securityProblemSupport)) // Adds a custom filter for Single Page Applications (SPA), i.e. the client, after the basic authentication filter. diff --git a/src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java b/src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java index 646fa942e7e0..f72cd2576c59 100644 --- a/src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java +++ b/src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java @@ -117,7 +117,8 @@ private String resolvePathPrefix() { public CorsFilter corsFilter() { UrlBasedCorsConfigurationSource source = new UrlBasedCorsConfigurationSource(); CorsConfiguration config = jHipsterProperties.getCors(); - if (config.getAllowedOrigins() != null && !config.getAllowedOrigins().isEmpty()) { + if ((config.getAllowedOrigins() != null && !config.getAllowedOrigins().isEmpty()) + || (config.getAllowedOriginPatterns() != null && !config.getAllowedOriginPatterns().isEmpty())) { log.debug("Registering CORS filter"); source.registerCorsConfiguration("/api/**", config); source.registerCorsConfiguration("/management/**", config); diff --git a/src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java b/src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java new file mode 100644 index 000000000000..01046ede50f3 --- /dev/null +++ b/src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java @@ -0,0 +1,30 @@ +package de.tum.cit.aet.artemis.core.security.filter; + +import java.io.IOException; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.web.filter.OncePerRequestFilter; + +public class CsrfArtemisFilter extends OncePerRequestFilter { + + private static final Logger log = LoggerFactory.getLogger(CsrfArtemisFilter.class); + + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { + + // Check if the custom CSRF header is present in the request + if (request.getHeader("X-ARTEMIS-CSRF") != null) { + filterChain.doFilter(request, response); + } + else { + // Reject the request if the header is missing + response.sendError(HttpServletResponse.SC_FORBIDDEN, "Missing CSRF protection header"); + } + } +} diff --git a/src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTCookieService.java b/src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTCookieService.java index 73320dee2813..d9c33a7c08e4 100644 --- a/src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTCookieService.java +++ b/src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTCookieService.java @@ -5,11 +5,8 @@ import java.time.Duration; import java.time.temporal.ChronoUnit; -import java.util.Arrays; -import java.util.Collection; import org.springframework.context.annotation.Profile; -import org.springframework.core.env.Environment; import org.springframework.http.ResponseCookie; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.stereotype.Service; @@ -18,15 +15,10 @@ @Service public class JWTCookieService { - private static final String DEVELOPMENT_PROFILE = "dev"; - private final TokenProvider tokenProvider; - private final Environment environment; - - public JWTCookieService(TokenProvider tokenProvider, Environment environment) { + public JWTCookieService(TokenProvider tokenProvider) { this.tokenProvider = tokenProvider; - this.environment = environment; } /** @@ -59,12 +51,9 @@ public ResponseCookie buildLogoutCookie() { */ private ResponseCookie buildJWTCookie(String jwt, Duration duration) { - Collection activeProfiles = Arrays.asList(environment.getActiveProfiles()); - boolean isSecure = !activeProfiles.contains(DEVELOPMENT_PROFILE); - return ResponseCookie.from(JWT_COOKIE_NAME, jwt).httpOnly(true) // Must be httpOnly - .sameSite("Lax") // Must be Lax to allow navigation links to Artemis to work - .secure(isSecure) // Must be secure + .sameSite("None") // Must be None to allow cross-site requests to Artemis from the VS Code plugin + .secure(true) // Must be secure to allow sameSite=None .path("/") // Must be "/" to be sent in ALL request .maxAge(duration) // Duration should match the duration of the jwt .build(); // Build cookie diff --git a/src/main/resources/config/application.yml b/src/main/resources/config/application.yml index 924d087ec8f2..0a11737e3ffe 100644 --- a/src/main/resources/config/application.yml +++ b/src/main/resources/config/application.yml @@ -284,13 +284,16 @@ jhipster: clientApp: name: 'artemisApp' # By default CORS is disabled. Uncomment to enable. - #cors: - #allowed-origin-patterns: "*" - #allowed-methods: "*" - #allowed-headers: "*" - #exposed-headers: "Authorization,Link,X-Total-Count" - #allow-credentials: true - #max-age: 1800 + cors: + allowed-methods: "*" + allowed-headers: "*" + exposed-headers: "Authorization,Link,X-Total-Count,Set-Cookie" + allow-credentials: true + max-age: 1800 + allowed-origins: + - "vscode-file://vscode-app" + - "vscode-webview://*" + mail: from: artemis@localhost registry: diff --git a/src/main/webapp/app/app.module.ts b/src/main/webapp/app/app.module.ts index 8f90b73a34cb..ceeb2a1578c1 100644 --- a/src/main/webapp/app/app.module.ts +++ b/src/main/webapp/app/app.module.ts @@ -26,6 +26,8 @@ import { ArtemisSharedComponentModule } from 'app/shared/components/shared-compo import { FaIconLibrary } from '@fortawesome/angular-fontawesome'; import { artemisIconPack } from 'src/main/webapp/content/icons/icons'; import { ScrollingModule } from '@angular/cdk/scrolling'; +import { HTTP_INTERCEPTORS } from '@angular/common/http'; +import { CsrfInterceptor } from 'app/core/csrf/csrf.interceptor'; // NOTE: this module should only include the most important modules for normal users, all course management, admin and account functionality should be lazy loaded if possible @NgModule({ @@ -59,6 +61,7 @@ import { ScrollingModule } from '@angular/cdk/scrolling'; SystemNotificationComponent, LoadingNotificationComponent, ], + providers: [{ provide: HTTP_INTERCEPTORS, useClass: CsrfInterceptor, multi: true }], bootstrap: [JhiMainComponent], }) export class ArtemisAppModule { diff --git a/src/main/webapp/app/core/csrf/csrf.interceptor.ts b/src/main/webapp/app/core/csrf/csrf.interceptor.ts new file mode 100644 index 000000000000..6b963e5de879 --- /dev/null +++ b/src/main/webapp/app/core/csrf/csrf.interceptor.ts @@ -0,0 +1,12 @@ +import { HttpEvent, HttpHandler, HttpInterceptor, HttpRequest } from '@angular/common/http'; +import { Observable } from 'rxjs'; + +export class CsrfInterceptor implements HttpInterceptor { + intercept(req: HttpRequest, next: HttpHandler): Observable> { + // https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#employing-custom-request-headers-for-ajaxapi + // Clone the request and add the CSRF token header + const csrfReq = req.clone({ headers: req.headers.set('X-ARTEMIS-CSRF', 'Dennis ist schuld') }); + + return next.handle(csrfReq); + } +} From 43aeecbd93e7c00b9046560000c4367524cffa9a Mon Sep 17 00:00:00 2001 From: dennis Date: Tue, 1 Oct 2024 21:43:50 +0200 Subject: [PATCH 2/3] move cors settings in extra PR --- .../aet/artemis/core/config/WebConfigurer.java | 3 +-- src/main/resources/config/application.yml | 16 +++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java b/src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java index f72cd2576c59..646fa942e7e0 100644 --- a/src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java +++ b/src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java @@ -117,8 +117,7 @@ private String resolvePathPrefix() { public CorsFilter corsFilter() { UrlBasedCorsConfigurationSource source = new UrlBasedCorsConfigurationSource(); CorsConfiguration config = jHipsterProperties.getCors(); - if ((config.getAllowedOrigins() != null && !config.getAllowedOrigins().isEmpty()) - || (config.getAllowedOriginPatterns() != null && !config.getAllowedOriginPatterns().isEmpty())) { + if (config.getAllowedOrigins() != null && !config.getAllowedOrigins().isEmpty()) { log.debug("Registering CORS filter"); source.registerCorsConfiguration("/api/**", config); source.registerCorsConfiguration("/management/**", config); diff --git a/src/main/resources/config/application.yml b/src/main/resources/config/application.yml index 0a11737e3ffe..82a52759be3d 100644 --- a/src/main/resources/config/application.yml +++ b/src/main/resources/config/application.yml @@ -284,15 +284,13 @@ jhipster: clientApp: name: 'artemisApp' # By default CORS is disabled. Uncomment to enable. - cors: - allowed-methods: "*" - allowed-headers: "*" - exposed-headers: "Authorization,Link,X-Total-Count,Set-Cookie" - allow-credentials: true - max-age: 1800 - allowed-origins: - - "vscode-file://vscode-app" - - "vscode-webview://*" + #cors: + #allowed-origin-patterns: "*" + #allowed-methods: "*" + #allowed-headers: "*" + #exposed-headers: "Authorization,Link,X-Total-Count" + #allow-credentials: true + #max-age: 1800 mail: from: artemis@localhost From 335aa9f190c55069cf8c8b2f9312982bba41be6f Mon Sep 17 00:00:00 2001 From: dennis Date: Mon, 7 Oct 2024 14:04:48 +0200 Subject: [PATCH 3/3] exclude non api calls from csrf --- .../aet/artemis/core/security/filter/CsrfArtemisFilter.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java b/src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java index 01046ede50f3..a0ccdfdce395 100644 --- a/src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java +++ b/src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java @@ -27,4 +27,10 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response.sendError(HttpServletResponse.SC_FORBIDDEN, "Missing CSRF protection header"); } } + + // exclude all non api calls such as git, ... + @Override + protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException { + return !request.getRequestURI().startsWith("/api/"); + } }