Skip CSRF check for query-only GraphQL requests

changed milestone to %17.8

added Category:System Access SLOMissed automation:self-triage-encouraged bugfunctional devopssoftware supply chain security groupauthentication missed:17.6 missed:17.7 priority3 sectionsec sessions severity3 typebug workflowin dev labels

assigned to @engwan

added pipelinetier-1 label

added backend label

1 Message

CHANGELOG missing:

If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog.

If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

Reviewer roulette

Category	Reviewer	Maintainer
backend	`@charlieeekroon` (UTC+1, same timezone as author)	`@sgarg_gitlab` (UTC+5.5, 4.5 hours ahead of author)

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

removed workflowin dev label

changed the description

@atevans @grzesiek what do you think of this change?

I think to fully solve the CSRF problem, we'd have to store the token outside the session as described in https://github.com/demarches-simplifiees/demarches-simplifiees.fr/blob/5b4f7f9ae9eaf0ac94008b62f7047e4714626cf9/doc/adr-csrf-forgery.md#using-a-long-lived-cookie-for-csrf-tokens. This is a much bigger change that also requires Rails 7.1 which we're still in the process of upgrading to.

So to mitigate most of the problems, I think we can do this change where we skip CSRF check for query-only requests. This should allow pages restored from cache to load properly. Though the user would still encounter an error when they try to submit a comment or any other mutation. They would still need to refresh for that.

requested review from @grzesiek and @atevans

approved this merge request

added pipeline:mr-approved label

added pipelinetier-3 pipeline:run-e2e-omnibus-once labels and removed pipelinetier-1 label

Before you set this MR to auto-merge

This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3. We will trigger a new pipeline for each transition to a higher tier.

Before you set this MR to auto-merge, please check the following:

You are the last maintainer of this merge request
The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
This pipeline is recent enough (created in the last 8 hours)

If all the criteria above apply, please set auto-merge for this merge request.

See pipeline tiers and merging a merge request for more details.

I was able to reproduce the functionality locally. Approved! After reading the code, I agree we're not making a huge change here, security-wise. It should help further reduce cases where users get logged out due to an unexpected CSRF miss.

Agree that moving to cookie-based storage for CSRF tokens would be a big change. There may be alternate solutions we can try - maybe frontend can check for the newest CSRF token on page restoration before launching any further requests? Could be hard to implement. It's probably worth discussing further.

E2E Test Result Summary

allure-report-publisher generated test report!

e2e-test-on-cng: test report for f6b56b32

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Verify      | 51     | 0      | 15      | 0     | 66    | ✅     |
| Create      | 140    | 0      | 19      | 1     | 159   | ✅     |
| Govern      | 84     | 0      | 10      | 0     | 94    | ✅     |
| Monitor     | 8      | 0      | 12      | 0     | 20    | ✅     |
| Package     | 30     | 0      | 14      | 0     | 44    | ✅     |
| Plan        | 86     | 0      | 8       | 0     | 94    | ✅     |
| Secure      | 2      | 0      | 5       | 0     | 7     | ✅     |
| Data Stores | 33     | 0      | 10      | 0     | 43    | ✅     |
| Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
| Fulfillment | 2      | 0      | 7       | 0     | 9     | ✅     |
| Growth      | 0      | 0      | 2       | 0     | 2     | ➖     |
| Manage      | 1      | 0      | 9       | 0     | 10    | ✅     |
| Release     | 5      | 0      | 1       | 0     | 6     | ✅     |
| Ai-powered  | 0      | 0      | 2       | 0     | 2     | ➖     |
| ModelOps    | 0      | 0      | 1       | 0     | 1     | ➖     |
| Configure   | 0      | 0      | 3       | 0     | 3     | ➖     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 444    | 0      | 118     | 1     | 562   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

e2e-test-on-gdk: test report for f6b56b32

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Create      | 135    | 0      | 20      | 0     | 155   | ✅     |
| Govern      | 80     | 0      | 12      | 0     | 92    | ✅     |
| Data Stores | 33     | 0      | 10      | 0     | 43    | ✅     |
| Verify      | 50     | 0      | 16      | 0     | 66    | ✅     |
| Package     | 25     | 0      | 13      | 0     | 38    | ✅     |
| Growth      | 0      | 0      | 2       | 0     | 2     | ➖     |
| Manage      | 1      | 0      | 9       | 0     | 10    | ✅     |
| Ai-powered  | 0      | 0      | 2       | 0     | 2     | ➖     |
| Secure      | 4      | 0      | 3       | 0     | 7     | ✅     |
| Monitor     | 8      | 0      | 12      | 0     | 20    | ✅     |
| Plan        | 82     | 0      | 8       | 0     | 90    | ✅     |
| Configure   | 0      | 0      | 3       | 0     | 3     | ➖     |
| Fulfillment | 2      | 0      | 7       | 0     | 9     | ✅     |
| Release     | 5      | 0      | 1       | 0     | 6     | ✅     |
| ModelOps    | 0      | 0      | 1       | 0     | 1     | ➖     |
| Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 427    | 0      | 119     | 0     | 546   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+