General: Add csrf protection + sameSite none cookie
#9404
Conversation
WalkthroughThe pull request introduces a new Changes
Possibly related issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
server
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
src/main/resources/config/application.ymlis excluded by!**/*.yml
📒 Files selected for processing (5)
- src/main/java/de/tum/cit/aet/artemis/core/config/SecurityConfiguration.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTCookieService.java (2 hunks)
- src/main/webapp/app/app.module.ts (2 hunks)
- src/main/webapp/app/core/csrf/csrf.interceptor.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/main/java/de/tum/cit/aet/artemis/core/config/SecurityConfiguration.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTCookieService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/webapp/app/app.module.ts (1)
src/main/webapp/app/core/csrf/csrf.interceptor.ts (1)
🔇 Additional comments (12)
src/main/webapp/app/core/csrf/csrf.interceptor.ts (3)
1-2: LGTM: Imports are correct and concise.The necessary imports for implementing the
HttpInterceptorare present, and there are no unused imports.
4-4: LGTM: Class declaration is correct and follows guidelines.The
CsrfInterceptorclass is properly declared, exported, and implements theHttpInterceptorinterface. The naming convention (PascalCase) adheres to the coding guidelines.
5-11:⚠️ Potential issueImprove CSRF token generation and handling.
The implementation of the
interceptmethod is generally correct and follows the coding guidelines. However, there are some concerns:
- The CSRF token is hardcoded ('Dennis ist schuld'), which is not secure for a production environment.
- There's no check for an existing CSRF token or generation of a unique token for each request.
- The method doesn't address the
sameSitecookie attribute mentioned in the PR objectives.Consider the following improvements:
- Generate a unique CSRF token for each request or user session.
- Store the CSRF token securely (e.g., in a secure cookie or local storage).
- Implement logic to handle the
sameSitecookie attribute as mentioned in the PR objectives.Example implementation:
import { Injectable } from '@angular/core'; import { HttpEvent, HttpHandler, HttpInterceptor, HttpRequest } from '@angular/common/http'; import { Observable } from 'rxjs'; @Injectable() export class CsrfInterceptor implements HttpInterceptor { private csrfToken: string; constructor() { this.csrfToken = this.generateCsrfToken(); } intercept(req: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> { // Clone the request and add the CSRF token header const csrfReq = req.clone({ headers: req.headers.set('X-ARTEMIS-CSRF', this.csrfToken), withCredentials: true // This ensures cookies are sent with the request }); return next.handle(csrfReq); } private generateCsrfToken(): string { // Implement a secure method to generate a unique token return 'generated-unique-token'; } }Also, ensure that the server-side implementation validates the CSRF token and sets the
sameSiteattribute of cookies to 'none' as required.To verify the implementation of CSRF protection and cookie handling on the server-side, you can run the following script:
This script will help identify if the server-side code properly validates the CSRF token and sets the
sameSiteattribute of cookies to 'none'.src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java (3)
1-12: LGTM: Package declaration and imports are well-organized.The package name follows the naming convention, and imports are properly organized without using wildcard imports, adhering to the coding guidelines. All imported classes are used in the code, indicating no unnecessary imports.
14-16: LGTM: Class declaration and logger initialization are correct.The class name
CsrfArtemisFilterfollows the CamelCase naming convention as per coding guidelines. ExtendingOncePerRequestFilteris appropriate for implementing a custom filter. The logger is correctly initialized as a private static final field using the class name.
1-30: LGTM: Class structure and adherence to coding guidelines are excellent.The
CsrfArtemisFilterclass demonstrates good adherence to coding guidelines and best practices:
- It follows the single responsibility principle, focusing solely on CSRF protection.
- The class is small, focused, and easy to understand (KISS principle).
- It uses appropriate access modifiers, following the principle of least access.
- The implementation is stateless, which is suitable for a filter.
- Naming conventions and import organization align with the provided guidelines.
Overall, the class structure and implementation show a good understanding of Java best practices and the specific requirements of the project.
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTCookieService.java (2)
Line range hint
1-61: Summary: Approve changes with minor suggestions for documentation.The modifications to
JWTCookieServicesuccessfully address the PR objectives by enabling support for the VS Code plugin through appropriate cookie settings. The changes adhere to coding guidelines, maintain security best practices, and improve code simplicity.Key points:
- Constructor simplification improves maintainability.
- Cookie security settings are appropriately configured for cross-site requests.
- Code comments provide context for the changes.
Suggestions for improvement:
- Verify the impact of removing the development mode check.
- Add a note about the security implications of using
sameSite=Noneand how they are mitigated.Overall, these changes effectively balance functionality and security requirements.
20-22: Approve constructor simplification with a note on environment determination.The constructor simplification adheres to the KISS principle and aligns with the coding guideline of constructor injection. This change improves code readability and maintainability.
However, we should verify that removing the
Environmentparameter doesn't impact the application's behavior in different environments. Please run the following script to check for any remaining references to development mode or environment-specific behavior:✅ Verification successful
To gather more information about the specific changes to the JWTCookieService and related classes, I'll need to examine the relevant files. Let's run some targeted searches.
Based on the analysis, it appears that the removal of the Environment parameter from the JWTCookieService constructor has indeed simplified the class and removed environment-specific logic. However, we need to verify if this change has any unintended consequences, particularly regarding security. Let's gather more information about the TokenProvider and any potential environment-specific configurations that might be relevant.
Based on the analysis of the TokenProvider class and JWT configurations, it appears that the removal of the Environment parameter from the JWTCookieService constructor does not have a significant impact on the overall JWT handling in the application. The environment-specific configurations are managed through different application property files, and the TokenProvider uses JHipsterProperties to load these configurations dynamically.
However, we should address the following points:
The removal of environment-specific checks in JWTCookieService has resulted in a consistent cookie configuration (always secure and SameSite=None) across all environments. This change appears to be intentional and may enhance security, but we should verify that it doesn't cause issues in development or testing environments.
The always-secure cookie configuration in JWTCookieService might be appropriate for production but could potentially cause issues in non-HTTPS development environments. We should confirm that this change is intentional and doesn't hinder local development or testing.
While the JWT token generation and validation seem to be handled appropriately by the TokenProvider, we should check if there are any other components that might have been relying on environment-specific behavior previously provided by JWTCookieService.
Given these considerations, I believe the changes to JWTCookieService are generally positive, simplifying the code and potentially improving security. However, we should verify that these changes don't introduce any unintended consequences in different deployment scenarios.
The removal of the Environment parameter from the JWTCookieService constructor appears to be a valid simplification. The JWT handling is now more consistent across environments, with configuration differences managed through application properties. However, it's recommended to manually verify the following:
- Ensure that the always-secure cookie configuration doesn't cause issues in development or testing environments, especially those not using HTTPS.
- Confirm that no other components were relying on environment-specific behavior previously provided by JWTCookieService.
- Verify that the consistent use of secure cookies aligns with the security requirements for all deployment scenarios.
If these points are confirmed to be non-issues, the changes can be considered an improvement in terms of code simplicity and potentially security.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for references to development mode or environment-specific behavior # Search for references to development mode or environment checks rg --type java -i '(development|environment|profile)'Length of output: 365375
Script:
#!/bin/bash # Search for the JWTCookieService class content echo "JWTCookieService class content:" cat src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTCookieService.java echo -e "\nReferences to JWTCookieService:" grep -r "JWTCookieService" src/ echo -e "\nReferences to ProfileService in security-related files:" grep -r "ProfileService" src/main/java/de/tum/cit/aet/artemis/core/security/Length of output: 4901
Script:
#!/bin/bash # Check TokenProvider for environment-specific configurations echo "TokenProvider class content:" cat src/main/java/de/tum/cit/aet/artemis/core/security/jwt/TokenProvider.java echo -e "\nReferences to Environment or ProfileService in TokenProvider:" grep -E "Environment|ProfileService" src/main/java/de/tum/cit/aet/artemis/core/security/jwt/TokenProvider.java echo -e "\nJWT-related configurations in application properties:" grep -E "jwt|token" src/main/resources/config/application.yml src/main/resources/config/application-dev.yml src/main/resources/config/application-prod.ymlLength of output: 8424
src/main/webapp/app/app.module.ts (2)
29-30: LGTM: Necessary imports for CSRF protection added.The import statements for
HTTP_INTERCEPTORSandCsrfInterceptorare correctly added and are essential for implementing the CSRF protection mechanism as outlined in the PR objectives.
Line range hint
29-64: Overall changes look good, but verify client-side implementation.The changes to
app.module.tssuccessfully implement the server-side CSRF protection mechanism as outlined in the PR objectives. The use of a custom header (X-ARTEMIS-CSRF) aligns with OWASP guidelines and should work well with thesameSite: nonecookie setting required for the VSCode plugin.To ensure full implementation, please verify that the client-side code (especially in the VSCode plugin) is updated to include the
X-ARTEMIS-CSRFheader in all requests. You can use the following script to check for the usage of this header in the codebase:This will help ensure that the CSRF protection is properly implemented on both the server and client sides.
✅ Verification successful
CSRF Protection Verified Successfully
The implementation of the
CsrfInterceptorinapp/core/csrf/csrf.interceptor.tscorrectly adds theX-ARTEMIS-CSRFheader to all outgoing HTTP requests. This aligns with the PR objectives and OWASP guidelines, ensuring effective CSRF protection on the client side.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for client-side usage of the X-ARTEMIS-CSRF header # Test: Search for the custom header in TypeScript files echo "Searching for X-ARTEMIS-CSRF header usage in TypeScript files:" rg --type typescript 'X-ARTEMIS-CSRF' # Test: Search for HTTP interceptors in TypeScript files echo "Searching for HTTP interceptors in TypeScript files:" rg --type typescript 'implements HttpInterceptor'Length of output: 409
Script:
#!/bin/bash # Description: Search for client-side usage of the X-ARTEMIS-CSRF header in TypeScript files # Test: Search for the custom header in .ts files echo "Searching for X-ARTEMIS-CSRF header usage in TypeScript (*.ts) files:" rg -g "*.ts" 'X-ARTEMIS-CSRF' # Test: Search for HTTP interceptors in .ts files echo "Searching for HTTP interceptors implementing HttpInterceptor in TypeScript (*.ts) files:" rg -g "*.ts" 'implements\s+HttpInterceptor'Length of output: 1660
src/main/java/de/tum/cit/aet/artemis/core/config/SecurityConfiguration.java (2)
38-38: LGTM: Import statement for CsrfArtemisFilter added correctly.The import statement for
CsrfArtemisFilteris appropriately placed and follows the coding guideline of avoiding star imports.
Line range hint
1-265: Overall assessment: CSRF protection successfully implemented with minor suggestions for improvement.The changes in this file successfully implement CSRF protection as per the PR objectives. The new
CsrfArtemisFilteris correctly integrated into the security filter chain, enhancing the application's defense against CSRF attacks. The implementation generally follows coding guidelines and security best practices.Key points:
- The CSRF filter is correctly positioned in the filter chain.
- The changes align with the requirement for
sameSiteattribute of cookies to be set tononefor the VSCode plugin.- The implementation follows the OWASP guidelines for CSRF protection.
Suggestions for improvement:
- Consider using constructor injection for
CsrfArtemisFilterto improve testability and flexibility.- The comment describing the filter chain could be more explicit about the purpose of the CSRF filter.
These changes enhance the security of the Artemis application, particularly for the VSCode plugin use case, without introducing any apparent vulnerabilities.
To ensure the CSRF protection is working as intended, consider running the following verification steps:
This script will help verify the consistent usage of CSRF protection across the codebase and identify any potential exclusions or custom implementations.
✅ Verification successful
CSRF Protection Verified Successfully
The verification confirms that CSRF protection is properly implemented across the codebase:
- Consistent Usage:
CsrfArtemisFilteris consistently used within the security configuration and its implementation is present.- No Exclusions: No endpoints exclude CSRF protection, ensuring comprehensive security coverage.
- Custom Header: The custom
X-ARTEMIS-CSRFheader is correctly implemented, adhering to security requirements.These findings indicate that the CSRF protection measures meet the intended security standards without introducing vulnerabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify CSRF protection implementation # Check if CsrfArtemisFilter is used in other parts of the codebase echo "Checking usage of CsrfArtemisFilter:" rg "CsrfArtemisFilter" --type java # Verify if there are any endpoints that might need to be excluded from CSRF protection echo "\nChecking for potential CSRF exclusions:" rg "@CrossOrigin|csrf\(\).disable\(\)" --type java # Look for custom headers related to CSRF protection echo "\nChecking for custom CSRF headers:" rg "X-ARTEMIS-CSRF" --type javaLength of output: 1317
| .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 |
There was a problem hiding this comment.
🧹 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.| @@ -59,6 +61,7 @@ import { ScrollingModule } from '@angular/cdk/scrolling'; | |||
| SystemNotificationComponent, | |||
| LoadingNotificationComponent, | |||
| ], | |||
| providers: [{ provide: HTTP_INTERCEPTORS, useClass: CsrfInterceptor, multi: true }], | |||
There was a problem hiding this comment.
🧹 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 }
],| // sets the filter chain: CorsFilter -> CsrfArtemisFilter -> UsernamePasswordAuthenticationFilter | ||
| .addFilterBefore(corsFilter, UsernamePasswordAuthenticationFilter.class) | ||
| .addFilterBefore(csrfArtemisFilter, UsernamePasswordAuthenticationFilter.class) |
There was a problem hiding this comment.
🧹 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) -> UsernamePasswordAuthenticationFilterThis 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.
| // 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) |
| private final CsrfArtemisFilter csrfArtemisFilter = new CsrfArtemisFilter(); | ||
|
|
There was a problem hiding this comment.
🛠️ 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.
janthoXO
changed the title
General Add csrf protection + sameSite none cookie
janthoXO
changed the title
General Add csrf protection + sameSite none cookieGeneral: Add csrf protection + sameSite none cookie
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/core/security/filter/CsrfArtemisFilter.java (3)
1-12: LGTM: Package declaration and imports are well-organized.The package declaration follows the correct naming convention, and imports are properly organized without using wildcard imports, adhering to the coding guidelines.
14-16: LGTM: Class declaration and logger initialization are correct.The class name follows the CamelCase naming convention, and the logger is properly initialized as a private static final field. The class correctly extends
OncePerRequestFilterfor implementing a custom filter.
1-36: Summary: CSRF protection implementation aligns with PR objectives.This new
CsrfArtemisFilterclass successfully implements the CSRF protection mechanism required for the VSCode plugin, as outlined in the PR objectives. The filter checks for the presence of the custom "X-ARTEMIS-CSRF" header, which enhances security against CSRF threats.Key points:
- The implementation follows coding guidelines, including naming conventions and principles like single responsibility.
- The filter correctly handles API and non-API requests separately.
- Suggestions for improvements have been provided to enhance maintainability and logging.
Overall, this implementation addresses the security concerns raised in the PR objectives while maintaining code quality standards.
| // exclude all non api calls such as git, ... | ||
| @Override | ||
| protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException { | ||
| return !request.getRequestURI().startsWith("/api/"); | ||
| } |
There was a problem hiding this comment.
🧹 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.
| // 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 | |
| } |
| @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"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 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:
- Use a constant for the header name to improve maintainability.
- Add logging when rejecting requests without the CSRF header.
- Consider using Spring's
HttpStatusenum 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.
| @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"); | |
| } | |
| } | |
| } |
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
THIS PR IS STALED BECAUSE OF CHANGED REQUIREMENTS IN THE VSCODE PLUGIN
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Description
For the VSCode plugin it is important that the
sameSiteattribute is set tonone, so that the cookie is send with request coming from an iframe. Because this would make artemis vulnerable to CSRF attacks a CSRF measure with custom header was implemented (refer to OWASP). The custom headerX-ARTEMIS-CSRFis needed now for every request, which triggers a CORS discovery and protects against CSRF attacks.Steps for Testing
This PR contains mostly config changes which are hard to test. So code reviews are appreciated as well as just general functionality checks in the client that everything works like before.
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
CsrfArtemisFilteron the server-side.CsrfInterceptor, to the client-side for enhanced CSRF protection by appending custom headers to requests.Improvements
JWTCookieServicefor better security and cross-site request compatibility.These enhancements improve the overall security of the application by ensuring that only authorized requests are processed.