Skip to content

Conversation

@rlorenzo
Copy link
Contributor

@rlorenzo rlorenzo commented Dec 9, 2025

  • Validate antiforgery tokens on POST/PUT/PATCH/DELETE requests globally
  • Return user-friendly error when CSRF validation fails

- Validate antiforgery tokens on POST/PUT/PATCH/DELETE requests globally
- Return user-friendly error when CSRF validation fails
Copilot AI review requested due to automatic review settings December 9, 2025 09:39
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements global CSRF token validation to enhance security by protecting against Cross-Site Request Forgery attacks. It adds a custom antiforgery filter that validates CSRF tokens on all state-modifying HTTP requests (POST, PUT, PATCH, DELETE) and returns user-friendly error messages when validation fails.

Key Changes:

  • Global CSRF validation filter for POST/PUT/PATCH/DELETE requests
  • User-friendly error handling for expired/invalid CSRF tokens
  • Frontend integration to treat CSRF errors as authentication errors, prompting page refresh

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
web/Classes/CustomAntiforgeryFilter.cs Implements custom authorization filter that validates antiforgery tokens and returns user-friendly error responses when validation fails
web/Program.cs Registers CustomAntiforgeryFilter globally for all controller actions
VueApp/src/composables/ViperFetch.ts Adds BAD_REQUEST status code constant and detection logic to treat antiforgery validation failures as authentication errors
Comments suppressed due to low confidence (1)

VueApp/src/composables/ViperFetch.ts:88

  • The error message assignment on line 76 is not used. Line 88 throws AuthError(result.errorMessage ?? "Auth Error", ...) instead of using the message variable. This means the hardcoded message "Your session token has expired. Please refresh the page." is never actually passed to the AuthError.

The code currently works because the backend response includes the same message in errorMessage, but this makes the frontend assignment redundant. Consider either:

  1. Change line 88 to use message consistently: throw new AuthError(message || "Auth Error", response.status)
  2. Or remove the message assignment on line 76 if relying on the backend's errorMessage

Option 1 is more consistent with the error handling pattern for other error types (lines 77-82).

            } else if (response.status === HTTP_STATUS.BAD_REQUEST && result?.title?.includes("Antiforgery")) {
                // CSRF token expired or invalid - treat as auth error so user refreshes
                isAuthError = true
                message = "Your session token has expired. Please refresh the page."
            } else if (result.errorMessage !== null && result.errorMessage !== undefined) {
                message = result.errorMessage
            } else if (result.detail !== null && result.detail !== undefined) {
                message = result.detail
            } else if (result.statusText !== null && result.statusText !== undefined) {
                message = result.statusText
            }
        } catch {
            throw new Error("An error occurred")
        }
        if (isAuthError) {
            throw new AuthError(result.errorMessage ?? "Auth Error", response.status)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Change CustomAntiforgeryFilter to return ApiResponse instead of
  anonymous object for consistency with other error handlers
- Update frontend to detect CSRF errors via errorMessage field
- Use message variable consistently in AuthError constructor
- Add unit tests for CustomAntiforgeryFilter
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Add test verifying [IgnoreAntiforgeryToken] skips CSRF validation
- Make frontend CSRF detection case-insensitive and type-safe
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Include CorrelationId in antiforgery error responses for traceability
- Add test assertions verifying CorrelationId is present and non-empty
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@rlorenzo rlorenzo merged commit 032682f into main Dec 10, 2025
12 checks passed
@rlorenzo rlorenzo deleted the VPR-38-csrf-validation branch December 10, 2025 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants