-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
fix: cache-warmup fails #31173
fix: cache-warmup fails #31173
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #31173 +/- ##
===========================================
+ Coverage 60.48% 83.81% +23.32%
===========================================
Files 1931 537 -1394
Lines 76236 38998 -37238
Branches 8568 0 -8568
===========================================
- Hits 46114 32685 -13429
+ Misses 28017 6313 -21704
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Co-authored-by: Pat Heard <patrick.heard@cds-snc.ca>
@nsivarajan I have tried it. But when I access the chart, it doesn't use the warmed up cache, but creates a different cache. |
@AnTapTanhCode, thanks for checking this. I have verified the behavior in my test environment, and the cache-warmup process successfully caches, and the charts/dashboards retrieve data as expected. It seems that your case, where a different cache is fetched and cached instead of the warm-up-cached data, might require further investigation. However, I believe this issue is unrelated to this PR. If needed, please open a new issue with reproducible steps, and we can debug it further later. |
Looks to me like this ticks all the boxes of test coverage, proper cookie formatting, and HTTPS validation, but since this is so close to security concerns, I'm hopeful that someone more qualified (perhaps @dpgaspar and/or @mistercrunch) can take a look. |
This reverts commit 949a147.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the contribution!
Thanks for your guidance and support! |
SUMMARY
This PR addresses #30900, which reports a "400 Bad Request: The CSRF session token is missing" error during cache warmup. The issue was due to missing header updates with the CSRF token and cookie (expecting session='session_cookie'). This PR ensures the token is correctly fetched and included in headers during the warmup process, resolving the error.
Fixes #30900
Fixes #31283
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Success:
TESTING INSTRUCTIONS
Since we are now explicitly fetching the CSRF token, we can enable
WTF_CSRF_ENABLED = True
or safely removeWTF_CSRF_EXEMPT_LIST
for any cache warmup-related endpoints. This ensures that all requests, including those for cache warmup, are properly validated with CSRF protection, thereby enhancing application security.If CSRF protection is disabled or the endpoint is exempted, no CSRF checks will be performed. However, the
ChartRestApi.warm_up_cache
endpoint still requires a session cookie (session=session_cookie
) to determine the user's context and permissions. Without this, the request will fail with a401 Unauthorized error
.This PR ensures the session cookie is correctly updated in the request headers under the Cookie field, enabling the user context to be accurately resolved for authorization.
ADDITIONAL INFORMATION