Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

General: Add csrf protection + sameSite none cookie #9404

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -54,6 +55,8 @@ public class SecurityConfiguration {

private final CorsFilter corsFilter;

private final CsrfArtemisFilter csrfArtemisFilter = new CsrfArtemisFilter();

Comment on lines +58 to +59
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using constructor injection for CsrfArtemisFilter.

While the field declaration is correct and follows naming conventions, consider using constructor injection for CsrfArtemisFilter instead of instantiating it inline. This approach would improve testability and align with the dependency injection principle used elsewhere in this class.

You could modify the code as follows:

-private final CsrfArtemisFilter csrfArtemisFilter = new CsrfArtemisFilter();
+private final CsrfArtemisFilter csrfArtemisFilter;

 public SecurityConfiguration(TokenProvider tokenProvider, PasswordService passwordService, CorsFilter corsFilter, ProfileService profileService,
-        Optional<CustomLti13Configurer> customLti13Configurer) {
+        Optional<CustomLti13Configurer> customLti13Configurer, CsrfArtemisFilter csrfArtemisFilter) {
     this.tokenProvider = tokenProvider;
     this.passwordService = passwordService;
     this.corsFilter = corsFilter;
     this.profileService = profileService;
     this.customLti13Configurer = customLti13Configurer;
+    this.csrfArtemisFilter = csrfArtemisFilter;
 }

Committable suggestion was skipped due to low confidence.

private final ProfileService profileService;

private final Optional<CustomLti13Configurer> customLti13Configurer;
Expand Down Expand Up @@ -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)
Comment on lines +177 to +179
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM: CSRF filter added correctly. Minor suggestion for comment improvement.

The addition of the CsrfArtemisFilter before the UsernamePasswordAuthenticationFilter is correct and aligns with the PR objective. The filter chain order (CORS -> CSRF -> UsernamePasswordAuthentication) follows security best practices.

A minor suggestion to improve the comment:

Consider updating the comment to be more explicit about the CSRF filter:

-// sets the filter chain: CorsFilter -> CsrfArtemisFilter -> UsernamePasswordAuthenticationFilter
+// Sets the filter chain: CorsFilter -> CsrfArtemisFilter (for CSRF protection) -> UsernamePasswordAuthenticationFilter

This change would provide more context about the purpose of the CsrfArtemisFilter.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// sets the filter chain: CorsFilter -> CsrfArtemisFilter -> UsernamePasswordAuthenticationFilter
.addFilterBefore(corsFilter, UsernamePasswordAuthenticationFilter.class)
.addFilterBefore(csrfArtemisFilter, UsernamePasswordAuthenticationFilter.class)
// Sets the filter chain: CorsFilter -> CsrfArtemisFilter (for CSRF protection) -> 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.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
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");
}
}
Comment on lines +18 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM with suggestions: Method implementation is correct but can be improved.

The doFilterInternal method correctly implements the CSRF protection logic. However, consider the following improvements as suggested in the previous review:

  1. Use a constant for the header name to improve maintainability.
  2. Add logging when rejecting requests without the CSRF header.
  3. Consider using Spring's HttpStatus enum for the status code.

Here's a suggested refactoring:

 public class CsrfArtemisFilter extends OncePerRequestFilter {
 
     private static final Logger log = LoggerFactory.getLogger(CsrfArtemisFilter.class);
+    private static final String CSRF_HEADER = "X-ARTEMIS-CSRF";
 
     @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) {
+        if (request.getHeader(CSRF_HEADER) != null) {
             filterChain.doFilter(request, response);
         }
         else {
             // Reject the request if the header is missing
-            response.sendError(HttpServletResponse.SC_FORBIDDEN, "Missing CSRF protection header");
+            log.warn("Rejected request due to missing CSRF header: {}", request.getRequestURI());
+            response.sendError(HttpStatus.FORBIDDEN.value(), "Missing CSRF protection header");
         }
     }
 }

These changes will improve code maintainability and provide better logging for security-related events.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@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");
}
}
public class CsrfArtemisFilter extends OncePerRequestFilter {
private static final Logger log = LoggerFactory.getLogger(CsrfArtemisFilter.class);
private static final String CSRF_HEADER = "X-ARTEMIS-CSRF";
@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(CSRF_HEADER) != null) {
filterChain.doFilter(request, response);
}
else {
// Reject the request if the header is missing
log.warn("Rejected request due to missing CSRF header: {}", request.getRequestURI());
response.sendError(HttpStatus.FORBIDDEN.value(), "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/");
}
Comment on lines +31 to +35
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM with a minor suggestion: Method implementation is correct.

The shouldNotFilter method correctly implements the logic to exclude non-API calls from filtering. However, to improve clarity, consider the following minor change:

 // exclude all non api calls such as git, ...
 @Override
 protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException {
-    return !request.getRequestURI().startsWith("/api/");
+    return !request.getRequestURI().startsWith("/api/"); // Only filter API calls
 }

This inline comment makes the method's purpose even more explicit at a glance.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// exclude all non api calls such as git, ...
@Override
protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException {
return !request.getRequestURI().startsWith("/api/");
}
// exclude all non api calls such as git, ...
@Override
protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException {
return !request.getRequestURI().startsWith("/api/"); // Only filter API calls
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}

/**
Expand Down Expand Up @@ -59,12 +51,9 @@ public ResponseCookie buildLogoutCookie() {
*/
private ResponseCookie buildJWTCookie(String jwt, Duration duration) {

Collection<String> 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
Comment on lines +55 to +56
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Approve cookie security changes with a suggestion for additional documentation.

The changes to set sameSite to "None" and secure to true are appropriate for supporting the VS Code plugin while maintaining security. These modifications align with the PR objectives and follow the REST API guideline of using HTTP-only cookies.

Consider adding a comment about the potential security implications of using sameSite=None and how the CSRF protection mechanism (mentioned in the PR description) mitigates these risks. This additional context would be valuable for future maintainers.

Example:

// Setting sameSite to "None" introduces potential CSRF vulnerabilities,
// which are mitigated by the custom X-ARTEMIS-CSRF header implementation.

.path("/") // Must be "/" to be sent in ALL request
.maxAge(duration) // Duration should match the duration of the jwt
.build(); // Build cookie
Expand Down
1 change: 1 addition & 0 deletions src/main/resources/config/application.yml
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ jhipster:
#exposed-headers: "Authorization,Link,X-Total-Count"
#allow-credentials: true
#max-age: 1800

mail:
from: artemis@localhost
registry:
Expand Down
3 changes: 3 additions & 0 deletions src/main/webapp/app/app.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -59,6 +61,7 @@ import { ScrollingModule } from '@angular/cdk/scrolling';
SystemNotificationComponent,
LoadingNotificationComponent,
],
providers: [{ provide: HTTP_INTERCEPTORS, useClass: CsrfInterceptor, multi: true }],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM: CSRF interceptor correctly added as a provider.

The CsrfInterceptor is properly registered as an HTTP interceptor, which aligns with the PR objective of implementing CSRF protection. This will ensure that the custom header X-ARTEMIS-CSRF is added to every request, enhancing security against CSRF attacks.

Consider moving the provider to a separate line for better readability:

providers: [
    { provide: HTTP_INTERCEPTORS, useClass: CsrfInterceptor, multi: true }
],

bootstrap: [JhiMainComponent],
})
export class ArtemisAppModule {
Expand Down
12 changes: 12 additions & 0 deletions src/main/webapp/app/core/csrf/csrf.interceptor.ts
Original file line number Diff line number Diff line change
@@ -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<any>, next: HttpHandler): Observable<HttpEvent<any>> {
// 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);
}
}
Loading