Added endpoint to generate csrf tokens #1281
Conversation
Summary by CodeRabbit
WalkthroughThis pull request implements extensive updates to the CSRF protection mechanism across the codebase. On the backend, CSRF decorators have been switched, and a new endpoint for retrieving CSRF tokens has been added. In the frontend, functions for obtaining the CSRF token now use asynchronous patterns and are supported by a new environment variable and constant. Additionally, numerous test suites have been updated to set a predefined CSRF cookie before page navigation, and various Makefiles, workflow configurations, and proxy settings have been adjusted accordingly. Changes
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
backend/settings/urls.py (1)
28-28: Add trailing slash to the CSRF endpoint for consistencyAll other API endpoints in this file include a trailing slash. For consistency in URL patterns, consider adding a trailing slash to the CSRF endpoint as well.
- path("csrf", get_csrf_token), + path("csrf/", get_csrf_token),frontend/src/api/getCsrfToken.ts (2)
13-15: Enhance cookie security settings and add data validation.The cookie implementation has several issues:
- Missing the
Secureflag (prevents sending over HTTP)- Missing the
HttpOnlyflag (prevents JavaScript access)- No validation that
data.csrftokenexists before using it- const data = await response.json() - document.cookie = `csrftoken=${data.csrftoken}; path=/; SameSite=Lax` - return data.csrftoken + const data = await response.json() + if (!data || !data.csrftoken) { + throw new AppError(500, 'Invalid CSRF token response') + } + const isSecure = window.location.protocol === 'https:'; + document.cookie = `csrftoken=${data.csrftoken}; path=/; SameSite=Lax${isSecure ? '; Secure' : ''}` + return data.csrftoken
16-21: Improve error logging for better debugging.The current error handling doesn't provide specific information about non-AppError exceptions. Consider adding more detailed error logging for different error types.
} catch (error) { if (error instanceof AppError) { throw error } - throw new AppError(500, 'Internal server error') + console.error('Failed to fetch CSRF token:', error) + throw new AppError( + 500, + `Error fetching CSRF token: ${error instanceof Error ? error.message : 'Unknown error'}` + ) }frontend/__tests__/unit/utils/utility.test.ts (1)
16-20: Consider testing the original cookie value's integrity.While the test verifies that the correct cookie value is returned, it doesn't explicitly verify that the function doesn't modify or access the cookie unnecessarily. This could be important for security.
test('returns CSRF token when it exists in cookies', async () => { document.cookie = 'csrftoken=abc123; otherkey=xyz789' const result = await getCsrfToken() expect(result).toBe('abc123') + // Verify getInitialCsrfToken was not called when cookie exists + const { getInitialCsrfToken } = require('api/getCsrfToken') + expect(getInitialCsrfToken).not.toHaveBeenCalled() })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/core/api/csrf_token.py(1 hunks)backend/settings/urls.py(2 hunks)frontend/__tests__/unit/utils/utility.test.ts(2 hunks)frontend/src/api/fetchAlgoliaData.ts(1 hunks)frontend/src/api/getCsrfToken.ts(1 hunks)frontend/src/utils/helpers/apolloClient.ts(1 hunks)frontend/src/utils/utility.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
frontend/src/api/fetchAlgoliaData.ts (1)
frontend/src/utils/utility.ts (1)
getCsrfToken(69-82)
backend/settings/urls.py (2)
backend/apps/core/api/algolia.py (1)
algolia_search(57-111)backend/apps/core/api/csrf_token.py (1)
get_csrf_token(4-6)
frontend/src/utils/helpers/apolloClient.ts (1)
frontend/src/utils/utility.ts (1)
getCsrfToken(69-82)
frontend/src/utils/utility.ts (1)
frontend/src/api/getCsrfToken.ts (1)
getInitialCsrfToken(3-22)
frontend/__tests__/unit/utils/utility.test.ts (1)
frontend/src/utils/utility.ts (1)
getCsrfToken(69-82)
🪛 Ruff (0.8.2)
backend/apps/core/api/csrf_token.py
1-1: Missing docstring in public module
(D100)
5-5: First line of docstring should be in imperative mood: "Returns a response with the CSRF token to set it in cookies."
(D401)
7-7: Blank line contains whitespace
Remove whitespace from blank line
(W293)
7-7: No newline at end of file
Add trailing newline
(W292)
🪛 GitHub Actions: Run CI/CD
backend/apps/core/api/csrf_token.py
[error] 1-1: D100 Missing docstring in public module
[error] 6-6: D401 First line of docstring should be in imperative mood: 'Returns a response with the CSRF token to set it in cookies.'
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (8)
backend/settings/urls.py (3)
11-11: LGTM: Importing CSRF protection decoratorThe import of the
csrf_protectdecorator is appropriate for enhancing security by enforcing CSRF protection.
16-16: LGTM: Importing the CSRF token functionImporting the newly created function from the CSRF token module is the correct approach.
26-27: Security improvement: Enforcing CSRF protection on endpointsApplying the
csrf_protectdecorator to these endpoints enhances security by ensuring that CSRF protection is enforced, which is a good security practice especially for endpoints that modify data.frontend/src/api/fetchAlgoliaData.ts (1)
23-23: LGTM: Properly handling the asynchronous CSRF token retrievalThe code correctly awaits the CSRF token before including it in the request headers, which is necessary due to the updated asynchronous implementation of
getCsrfToken.frontend/src/utils/utility.ts (1)
1-1: LGTM: Importing the initial CSRF token functionThe import statement correctly brings in the
getInitialCsrfTokenfunction that will be used to fetch the CSRF token when it's not available in cookies.frontend/src/utils/helpers/apolloClient.ts (1)
19-26: LGTM - Appropriate use of async/await with CSRF token retrieval.The conversion to async/await is correctly implemented, ensuring that the CSRF token is properly retrieved before adding it to the headers.
frontend/__tests__/unit/utils/utility.test.ts (2)
3-5: LGTM - Good mocking approach for isolated testing.The mock for
getInitialCsrfTokenis well-structured and consistent with test best practices.
16-50: LGTM - Tests appropriately updated for asynchronous behavior.The test cases have been correctly updated to be asynchronous and properly test the new behavior of the
getCsrfTokenfunction. Test descriptions are clear and expectations are accurate.
frontend/src/utils/utility.ts
Outdated
| export const getCsrfToken = async () => { | ||
| const csrfToken = document.cookie | ||
| .split(';') | ||
| .map((cookie) => cookie.split('=')) | ||
| .find(([key]) => key.trim() === 'csrftoken')?.[1] | ||
| ?.trim() | ||
|
|
||
| if (csrfToken) { | ||
| return csrfToken | ||
| } | ||
|
|
||
| const res = await getInitialCsrfToken() | ||
| return res | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the hardcoded URL used in getInitialCsrfToken
While the asynchronous implementation is sound, the function ultimately calls getInitialCsrfToken() which uses a hardcoded URL (http://localhost:8000/csrf). This won't work in production environments.
Please check the implementation of getInitialCsrfToken to ensure it's using a configurable base URL rather than hardcoding localhost:
🏁 Script executed:
#!/bin/bash
# Check if the URL is hardcoded in getInitialCsrfToken
cat frontend/src/api/getCsrfToken.tsLength of output: 621
Attention: Refactor URL Configuration in getInitialCsrfToken
The implementation in frontend/src/api/getCsrfToken.ts still uses a hardcoded URL (http://localhost:8000/csrf). This approach will fail in production. Please update the function so that it retrieves the base URL from an environment variable or a configuration file rather than being fixed.
- File:
frontend/src/api/getCsrfToken.ts - Issue: Hardcoded URL in the
fetchcall. - Recommendation: Use a dynamic, configurable URL to ensure proper endpoint configuration across environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/api/getCsrfToken.ts (1)
1-23: CSRF token retrieval function looks good, but could use some enhancements.The implementation correctly fetches the CSRF token from the server and sets it as a cookie. However, there are a few improvements that could make the function more robust:
Consider these enhancements:
- Add cookie security options for production environments
- document.cookie = `csrftoken=${data.csrftoken}; path=/; SameSite=Lax` + document.cookie = `csrftoken=${data.csrftoken}; path=/; SameSite=Lax${ + window.location.protocol === 'https:' ? '; Secure' : '' + }`
- Add validation for the response data structure
const data = await response.json() + if (!data.csrftoken) { + throw new AppError(500, 'Invalid response: Missing CSRF token') + } document.cookie = `csrftoken=${data.csrftoken}; path=/; SameSite=Lax`
- Consider adding an expiration time for the cookie if needed, otherwise it will be a session cookie by default.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/core/api/csrf_token.py(1 hunks)backend/settings/urls.py(2 hunks)frontend/.env.example(1 hunks)frontend/src/api/getCsrfToken.ts(1 hunks)frontend/src/utils/credentials.ts(1 hunks)frontend/vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/core/api/csrf_token.py
- backend/settings/urls.py
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/api/getCsrfToken.ts (1)
frontend/src/utils/credentials.ts (1)
CSRF_URL(2-2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
frontend/.env.example (1)
2-2: LGTM: New CSRF URL environment variable is properly defined.The new environment variable follows the established pattern of the file and provides a clear endpoint for CSRF token generation.
frontend/src/utils/credentials.ts (1)
2-2: LGTM: New CSRF URL constant exports the environment variable correctly.The constant is properly defined and matches the pattern of other credential exports.
frontend/vite.config.ts (1)
18-18: LGTM: Environment variable properly added to Vite configuration.The CSRF URL is correctly included in the EnvironmentPlugin configuration, making it accessible to the application.
frontend/src/api/getCsrfToken.ts (1)
6-6: LGTM: Using the environment variable instead of hardcoded URL.The function correctly uses the CSRF_URL constant from environment variables, addressing a previous comment about hardcoded URLs.
|
@coderabbitai could you review this pr for any improvements and vulnurability! Thank you |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8381 Security Considerations
// Add conditional for production environment
document.cookie = `csrftoken=${data.csrftoken}; path=/; SameSite=Lax${window.location.protocol === 'https:' ? '; Secure' : ''}`
// Add max-age or expires attribute
const expiryMinutes = 60; // 1 hour
document.cookie = `csrftoken=${data.csrftoken}; path=/; SameSite=Lax; max-age=${expiryMinutes * 60}`
try {
// ...existing code
} catch (error) {
if (error instanceof AppError) {
throw error;
}
if (error instanceof TypeError && error.message.includes('fetch')) {
throw new AppError(503, 'Network error - unable to connect to server');
}
throw new AppError(500, 'Internal server error');
}Other Improvements
export const getCsrfToken = async () => {
const cookieValue = document.cookie
.split(';')
.map(cookie => cookie.trim().split('='))
.find(([key]) => key === 'csrftoken');
if (cookieValue && cookieValue[1] && cookieValue[1].trim()) {
return cookieValue[1].trim();
}
return await getInitialCsrfToken();
}
Positive Aspects
I hope these suggestions help improve your implementation. Let me know if you need any clarification! ( __/) Code Analysis ToolsTool Analysis ResultsCodeRabbit ran static analysis tools on your files and found the following: No specific issues were found in your files. You can proceed with your changes. |
|
i think We can do that but if we not specify expiry minutes then it will be treated as session key by default. So it depends on weather we want to specify expiry time or use it as a default session key |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/utils/utility.ts (3)
70-74: Improve cookie parsing robustnessThe current cookie parsing logic could be more robust to handle edge cases. If cookies are malformed or if there's an empty cookie string, this approach might not handle it gracefully.
- const csrfToken = document.cookie - .split(';') - .map((cookie) => cookie.split('=')) - .find(([key]) => key.trim() === 'csrftoken')?.[1] - ?.trim() + // Get CSRF token from cookies using regex for more robust parsing + const cookieValue = document.cookie.replace(/(?:(?:^|.*;\s*)csrftoken\s*=\s*([^;]*).*$)|^.*$/, "$1") + const csrfToken = cookieValue || undefined
69-69: Add explicit return typeThe function currently has an implicit return type. Adding an explicit return type would improve type safety and make it clearer to consumers what they can expect.
-export const getCsrfToken = async () => { +export const getCsrfToken = async (): Promise<string> => {
69-81: Add token expiration handlingThe function does not check if the CSRF token in the cookie has expired. If the token expires, the application might continue trying to use an invalid token until the page is refreshed.
Consider adding token expiration validation and refreshing expired tokens:
export const getCsrfToken = async (): Promise<string> => { + // Check if token exists and is not expired + const tokenExpirationKey = 'csrfTokenExpiration' const csrfToken = document.cookie .split(';') .map((cookie) => cookie.split('=')) .find(([key]) => key.trim() === 'csrftoken')?.[1] ?.trim() + const expirationTime = localStorage.getItem(tokenExpirationKey) + const isExpired = expirationTime && parseInt(expirationTime, 10) < Date.now() - if (csrfToken) { + if (csrfToken && !isExpired) { return csrfToken } + // If token is expired, clear it from storage + if (isExpired) { + document.cookie = 'csrftoken=; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT' + } try { const token = await getInitialCsrfToken() + // Store expiration time (e.g., 1 hour from now) + localStorage.setItem(tokenExpirationKey, (Date.now() + 3600000).toString()) return token } catch (error) { console.error('Failed to retrieve CSRF token:', error) throw error } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/api/getCsrfToken.ts(1 hunks)frontend/src/utils/utility.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/api/getCsrfToken.ts
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/utils/utility.ts (1)
frontend/src/api/getCsrfToken.ts (1)
getInitialCsrfToken(4-23)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
frontend/src/utils/utility.ts (1)
1-1: Verify URL configuration in dependent functionThis function now depends on
getInitialCsrfToken, which from past reviews appears to use a hardcoded URL. Ensure that the URL configuration ingetInitialCsrfTokenis properly configured for different environments.#!/bin/bash # Check if there's any configuration for the CSRF URL grep -r "CSRF_URL" --include="*.ts" --include="*.js" frontend/src/
frontend/src/utils/utility.ts
Outdated
| export const getCsrfToken = async () => { | ||
| const csrfToken = document.cookie | ||
| .split(';') | ||
| .map((cookie) => cookie.split('=')) | ||
| .find(([key]) => key.trim() === 'csrftoken')?.[1] | ||
| ?.trim() | ||
|
|
||
| if (csrfToken) { | ||
| return csrfToken | ||
| } | ||
|
|
||
| return await getInitialCsrfToken() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for token retrieval
While converting the function to async is a good improvement for handling token retrieval, there's no error handling around the getInitialCsrfToken() call. If the token fetching fails, the error will propagate to the caller without any context or handling.
export const getCsrfToken = async () => {
const csrfToken = document.cookie
.split(';')
.map((cookie) => cookie.split('='))
.find(([key]) => key.trim() === 'csrftoken')?.[1]
?.trim()
if (csrfToken) {
return csrfToken
}
- return await getInitialCsrfToken()
+ try {
+ return await getInitialCsrfToken()
+ } catch (error) {
+ console.error('Failed to retrieve CSRF token:', error)
+ throw error
+ }
}📝 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.
| export const getCsrfToken = async () => { | |
| const csrfToken = document.cookie | |
| .split(';') | |
| .map((cookie) => cookie.split('=')) | |
| .find(([key]) => key.trim() === 'csrftoken')?.[1] | |
| ?.trim() | |
| if (csrfToken) { | |
| return csrfToken | |
| } | |
| return await getInitialCsrfToken() | |
| } | |
| export const getCsrfToken = async () => { | |
| const csrfToken = document.cookie | |
| .split(';') | |
| .map((cookie) => cookie.split('=')) | |
| .find(([key]) => key.trim() === 'csrftoken')?.[1] | |
| ?.trim() | |
| if (csrfToken) { | |
| return csrfToken | |
| } | |
| try { | |
| return await getInitialCsrfToken() | |
| } catch (error) { | |
| console.error('Failed to retrieve CSRF token:', error) | |
| throw error | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/__tests__/e2e/pages/Projects.spec.ts (1)
15-22: CSRF token setup looks good.The addition of the CSRF token cookie in the
beforeEachhook is appropriate for ensuring tests run correctly with the new CSRF protection mechanisms. The implementation correctly uses Playwright's API to set up the necessary test state.Consider adding a helper function across test files to avoid duplicating this cookie setup code, which would improve maintainability if token requirements change in the future.
// Could be extracted to a shared helper function in a utils file + // import { setupCsrfToken } from '@e2e/utils/testHelpers' test.beforeEach(async ({ page }) => { await page.route('**/idx/', async (route) => { await route.fulfill({ status: 200, body: JSON.stringify({ hits: mockProjectData.projects, nbPages: 2, }), }) }) - await page.context().addCookies([ - { - name: 'csrftoken', - value: 'abc123', - domain: 'localhost', - path: '/', - }, - ]) + // await setupCsrfToken(page) await page.goto('/projects') })frontend/__tests__/e2e/pages/UserDetails.spec.ts (1)
12-19: CSRF token implementation maintains consistency across test files.This implementation follows the same pattern as other test files, ensuring that all pages have the necessary CSRF token available for testing. The test will now correctly simulate the environment where CSRF protection is enabled.
While the current implementation works well for testing, consider enhancing it in the future by:
- Creating a shared helper function to reduce duplication across test files
- Potentially using a more realistic token value (though 'abc123' is adequate for testing)
frontend/__tests__/e2e/pages/Contribute.spec.ts (2)
15-22: Consider adding security attributes to match production environment.While this works for testing, consider adding security attributes like
SecureandSameSiteto the test cookie to better mimic the production environment:await page.context().addCookies([ { name: 'csrftoken', value: 'abc123', domain: 'localhost', path: '/', + secure: true, + sameSite: 'Lax', }, ])
15-22: Consider adding an expiration time to the CSRF token cookie.As mentioned in the PR comments, it might be beneficial to add an expiration time to the cookie, even in tests, to fully simulate the intended production behavior:
await page.context().addCookies([ { name: 'csrftoken', value: 'abc123', domain: 'localhost', path: '/', + expires: Math.floor(Date.now() / 1000) + 3600, // 1 hour from now }, ])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
frontend/__tests__/e2e/pages/About.spec.ts(1 hunks)frontend/__tests__/e2e/pages/ChapterDetails.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Chapters.spec.tsx(1 hunks)frontend/__tests__/e2e/pages/CommitteeDetails.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Committees.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Contribute.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Home.spec.ts(1 hunks)frontend/__tests__/e2e/pages/ProjectDetails.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Projects.spec.ts(1 hunks)frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts(1 hunks)frontend/__tests__/e2e/pages/UserDetails.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Users.spec.tsx(1 hunks)frontend/__tests__/e2e/pages/footerIcons.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (11)
frontend/__tests__/e2e/pages/Home.spec.ts (1)
12-19: CSRF token setup is consistent.The implementation matches the pattern used in other test files, ensuring all pages have the necessary CSRF token cookie available. This is essential for the new asynchronous CSRF token mechanism introduced in this PR.
frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts (1)
12-19: CSRF token setup is properly implemented.The cookie setup follows the same pattern as other test files, maintaining consistency across the test suite. The static value 'abc123' is sufficient for testing purposes.
frontend/__tests__/e2e/pages/Users.spec.tsx (1)
15-22: CSRF token cookie setup is appropriateThe addition of the CSRF token cookie in the test setup is consistent with the PR's objective of enhancing CSRF protection. This ensures the tests will work correctly with the new CSRF token validation mechanisms.
frontend/__tests__/e2e/pages/ProjectDetails.spec.ts (1)
12-19: CSRF token cookie setup looks goodThe addition of the CSRF token cookie in the test setup aligns with the new CSRF protection mechanisms implemented in this PR. The cookie configuration is properly set with the appropriate domain and path.
frontend/__tests__/e2e/pages/Committees.spec.ts (1)
15-22: CSRF token cookie setup is correctly implementedThe addition of the CSRF token cookie before page navigation ensures that the tests will function correctly with the new CSRF protection. The implementation is consistent with other test files in this PR.
frontend/__tests__/e2e/pages/About.spec.ts (1)
19-26: CSRF token cookie setup is well integratedThe addition of the CSRF token cookie in the test setup is consistent with the rest of the changes in this PR. This ensures that tests will continue to work with the new CSRF protection mechanisms.
frontend/__tests__/e2e/pages/CommitteeDetails.spec.ts (1)
12-19: CSRF token setup looks good.The addition of a CSRF token cookie to the test setup is appropriate and follows best practices for E2E testing with protected routes. Setting this up in the
beforeEachblock ensures all tests have the necessary security context.frontend/__tests__/e2e/pages/Chapters.spec.tsx (1)
15-22: CSRF token implementation is consistent.The CSRF token setup matches the pattern implemented across other test files, establishing good consistency in your testing approach. This ensures that tests can interact with endpoints that may enforce CSRF protection.
frontend/__tests__/e2e/pages/ChapterDetails.spec.ts (1)
12-19: CSRF token cookie setup properly implemented.The addition of the CSRF token cookie before navigating to the chapter details page ensures that your tests will continue to function correctly with the new CSRF protection in place. This implementation is consistent with other test files.
frontend/__tests__/e2e/pages/footerIcons.spec.ts (1)
5-12: CSRF token setup for site-wide protection.Adding the CSRF token cookie setup for the homepage tests suggests that CSRF protection is being implemented application-wide. This is a good security practice that ensures all forms and state-changing operations across the site are protected.
frontend/__tests__/e2e/pages/Contribute.spec.ts (1)
15-22: LGTM! Good addition of CSRF token cookie for test environment.The implementation correctly adds a CSRF token cookie before navigating to the page, which is essential for testing the new CSRF protection mechanism introduced in this PR.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/api/getCsrfToken.ts (3)
15-16: Consider adding token format validation.The function assumes the response will contain a valid
csrftokenproperty but doesn't validate its format or presence.const data = await response.json() + if (!data.csrftoken || typeof data.csrftoken !== 'string') { + throw new AppError(500, 'Invalid CSRF token received') + } return data.csrftoken
17-22: Error handling could be more specific.The error handling is good at differentiating between AppError instances and other errors, but using a generic 500 error for all other error types might hide useful debugging information.
} catch (error) { if (error instanceof AppError) { throw error } - throw new AppError(500, 'Internal server error') + // Provide more context about the error type + const errorMessage = error instanceof Error ? error.message : 'Unknown error' + throw new AppError(500, `Failed to fetch CSRF token: ${errorMessage}`) }
6-9: Consider adding timeout handling for the fetch request.Network requests without timeouts can hang indefinitely in certain scenarios, potentially affecting user experience.
- const response = await fetch(CSRF_URL, { + // Add timeout handling with AbortController + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); // 10 second timeout + + try { + const response = await fetch(CSRF_URL, { method: 'GET', credentials: 'include', + signal: controller.signal, }) + clearTimeout(timeoutId);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/api/getCsrfToken.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/api/getCsrfToken.ts (1)
frontend/src/utils/credentials.ts (1)
CSRF_URL(2-2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
frontend/src/api/getCsrfToken.ts (3)
1-3: Good use of environment variables for the CSRF URL.The imports are clean and appropriate, using environment configuration via
CSRF_URLfrom credentials rather than hardcoding the endpoint URL. This ensures the code can work across different environments.
4-9: CSRF token fetch implementation looks good.The function is correctly implemented as async and uses the fetch API with appropriate parameters. Including
credentials: 'include'is essential for cookie-based CSRF protection to work properly.
11-13: Good response validation with specific error handling.The response status check is implemented correctly, throwing an appropriate error with the status code when the request fails.
github-actions
bot
added
docs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
backend/apps/core/api/csrf.py (2)
7-9: Implement recommended security enhancementsThe basic implementation looks good, but consider enhancing it with:
- Add the
Secureflag to CSRF cookies in production environments to prevent transmission over non-HTTPS connections- Consider adding an expiration time to the CSRF token cookie
- Restrict this endpoint to GET requests only using Django's method decorators
+from django.views.decorators.http import require_GET +from django.conf import settings +@require_GET def get_csrf_token(request): """Return a response with the CSRF token.""" - return JsonResponse({"csrftoken": get_token(request)}) + response = JsonResponse({"csrftoken": get_token(request)}) + # In production, ensure CSRF cookie is only sent over HTTPS + if settings.CSRF_COOKIE_SECURE: + response.cookies[settings.CSRF_COOKIE_NAME]['secure'] = True + return response
1-9: Consider adding error handlingWhile Django's
get_tokenfunction is generally reliable, it would be good practice to add error handling for robustness.def get_csrf_token(request): """Return a response with the CSRF token.""" - return JsonResponse({"csrftoken": get_token(request)}) + try: + token = get_token(request) + return JsonResponse({"csrftoken": token}) + except Exception as e: + return JsonResponse({"error": "Failed to generate CSRF token"}, status=500)backend/Makefile (1)
1-4: Rename target to reflect its actual purposeThe
clean-backendtarget is removing directories from thefrontend/path, which is counterintuitive. Consider renaming this target toclean-frontendor clarify its purpose in a comment to avoid developer confusion.-clean-backend: +clean-frontend: # Cleans frontend cache and environment directories @rm -rf frontend/.cache @rm -rf frontend/.local @rm -rf frontend/.venvfrontend/src/utils/utility.ts (1)
69-74: Consider making cookie parsing more robustThe current implementation for parsing cookies could be made more robust by handling edge cases such as empty cookies or malformed cookie strings.
export const getCsrfToken = async (): Promise<string> => { - const csrfToken = document.cookie - .split(';') - .map((cookie) => cookie.split('=')) - .find(([key]) => key.trim() === 'csrftoken')?.[1] - ?.trim() + const csrfToken = document.cookie + ? document.cookie + .split(';') + .map((cookie) => cookie.trim().split('=')) + .find(([key]) => key.trim() === 'csrftoken')?.[1] + ?.trim() + : undefinedfrontend/src/api/fetchCsrfToken.ts (3)
4-14: Error handling could be more specific.The error handling for the fetch operation is good, but could be improved by differentiating between network failures and server errors for better clarity in error reporting.
if (!response.ok) { - const message = `Failed to fetch CSRF token: ${response.status} ${response.statusText}` + let message = `Failed to fetch CSRF token: ${response.status} ${response.statusText}` + if (response.status >= 500) { + message = `Server error while fetching CSRF token: ${response.status} ${response.statusText}` + } else if (response.status >= 400) { + message = `Client error while fetching CSRF token: ${response.status} ${response.statusText}` + } else if (response.status === 0) { + message = 'Network error: Could not connect to the server to fetch CSRF token' + } throw new AppError(response.status, message) }
1-31: Consider adding token caching mechanism.The current implementation fetches a new CSRF token each time the function is called, which might lead to unnecessary API calls. Consider adding a simple caching mechanism to reuse tokens when appropriate.
+ let cachedToken: string | null = null; + let tokenExpiryTime: number | null = null; + const TOKEN_CACHE_DURATION = 5 * 60 * 1000; // 5 minutes in milliseconds export const fetchCsrfToken = async (): Promise<string> => { + // Return cached token if it's still valid + if (cachedToken && tokenExpiryTime && Date.now() < tokenExpiryTime) { + return cachedToken; + } try { const response = await fetch(CSRF_URL, { credentials: 'include', method: 'GET', }) if (!response.ok) { const message = `Failed to fetch CSRF token: ${response.status} ${response.statusText}` throw new AppError(response.status, message) } const data = await response.json() if (!data?.csrftoken) { throw new AppError(500, 'CSRF token missing in response') } + // Cache the token and set expiry time + cachedToken = data.csrftoken; + tokenExpiryTime = Date.now() + TOKEN_CACHE_DURATION; + return data.csrftoken } catch (error) { if (error instanceof AppError) { throw error } const message = error?.message || 'Unexpected error while fetching CSRF token' throw new AppError(500, message) } } + // Add a function to clear the token cache when needed (e.g., after logout) + export const clearCsrfTokenCache = (): void => { + cachedToken = null; + tokenExpiryTime = null; + }
6-8: Consider adding security headers for the CSRF request.For enhanced security, consider adding appropriate headers to the CSRF token request.
const response = await fetch(CSRF_URL, { credentials: 'include', method: 'GET', + headers: { + 'X-Requested-With': 'XMLHttpRequest', + 'Accept': 'application/json', + }, })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/run-ci-cd.yaml(3 hunks).github/workflows/test-schema.yaml(1 hunks)CONTRIBUTING.md(3 hunks)Makefile(2 hunks)backend/Makefile(1 hunks)backend/apps/core/api/csrf.py(1 hunks)backend/settings/urls.py(2 hunks)frontend/Makefile(2 hunks)frontend/__tests__/unit/utils/utility.test.ts(2 hunks)frontend/src/api/fetchCsrfToken.ts(1 hunks)frontend/src/utils/utility.ts(2 hunks)proxy/production.conf(1 hunks)proxy/staging.conf(1 hunks)schema/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/settings/urls.py
- frontend/tests/unit/utils/utility.test.ts
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/utils/utility.ts (1)
frontend/src/api/fetchCsrfToken.ts (1)
fetchCsrfToken(4-31)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (20)
schema/Makefile (1)
1-2: LGTM: Good addition to the build systemAdding a
clean-schematarget is a helpful maintenance utility that makes it easier to reset the virtual environment for testing and development..github/workflows/test-schema.yaml (1)
52-52: LGTM: Consistent command referenceGood update to maintain consistency with other files in the project that now use
make checkinstead ofmake check-all.frontend/Makefile (2)
6-8: LGTM: Clean target is well implementedThe new
clean-frontendtarget provides a useful utility for removing package manager-related directories, which is essential for clean builds and troubleshooting dependency issues.
25-27: LGTM: Logical reordering of test targetsReordering to run unit tests before e2e tests is a good practice as unit tests are typically faster and can catch issues before running the more time-consuming e2e tests.
proxy/staging.conf (1)
28-28: LGTM: Appropriate routing for the new CSRF endpointAdding the
csrfpath to the proxy routing pattern correctly ensures that CSRF token requests are properly forwarded to the backend service.proxy/production.conf (1)
28-28: LGTM: Consistent routing configuration for productionThe production configuration is correctly updated to match the staging configuration, ensuring that CSRF token requests are properly handled in the production environment.
frontend/src/utils/utility.ts (2)
1-1: LGTM: Appropriate import of the fetchCsrfToken functionThe import statement correctly references the new fetchCsrfToken function that will be used for CSRF token retrieval.
69-81: Add error handling for token retrievalWhile converting the function to async is a good improvement for handling token retrieval, there's no error handling around the
fetchCsrfToken()call. If the token fetching fails, the error will propagate to the caller without any context or handling.export const getCsrfToken = async (): Promise<string> => { const csrfToken = document.cookie .split(';') .map((cookie) => cookie.split('=')) .find(([key]) => key.trim() === 'csrftoken')?.[1] ?.trim() if (csrfToken) { return csrfToken } - return await fetchCsrfToken() + try { + return await fetchCsrfToken() + } catch (error) { + console.error('Failed to retrieve CSRF token:', error) + throw error + } }CONTRIBUTING.md (3)
237-237: Command update looks good.The change from
make check-alltomake checkaligns with the updated target names in the Makefile, providing a more concise command for contributors.
248-248: Command update looks good.The change from
make test-alltomake testaligns with the updated target names in the Makefile, providing a more concise command for contributors.
289-289: Command update looks good.The change from
make check-test-alltomake check-testaligns with the updated target names in the Makefile, providing a more concise command for contributors.Makefile (4)
10-13: Clean target implementation looks good.The new
cleantarget that depends on other clean operations provides a convenient way to clean all project components at once.
15-18: Check target implementation looks good.The new
checktarget replaces the previouscheck-alltarget with a more concise name while maintaining the same functionality of checking both backend and frontend code.
23-25: Check-test target update looks good.The updated
check-testtarget now depends on the newchecktarget and thetesttarget, maintaining the same functionality with a simpler naming convention.
41-43: Test target update looks good.The renamed
testtarget (previouslytest-all) maintains the same dependencies while using a more concise name..github/workflows/run-ci-cd.yaml (2)
81-81: CI/CD message update looks good.The error message correctly instructs users to run
make checkinstead ofmake check-all, aligning with the updated Makefile target names.
244-244: CSRF URL environment variable addition looks good.Adding the
VITE_CSRF_URLenvironment variable to both staging and production frontend environments is necessary to support the new CSRF token fetching functionality.Also applies to: 408-408
frontend/src/api/fetchCsrfToken.ts (3)
1-3: Imports look good.The imports for
CSRF_URLandAppErrorare appropriate for the functionality being implemented.
15-22: Token extraction looks good.The function correctly extracts the CSRF token from the response and validates its presence before returning it.
23-31: Comprehensive error handling.The catch block properly handles different error types, preserving AppError instances and providing a helpful default message for unexpected errors.
|
Resolves #(put the issue number here)
/csrffor the genration of csrfAdd the PR description here.