Skip to content
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: Take csrf token from CMS config if possible #444

Merged
merged 8 commits into from
Jan 17, 2025
Merged

Conversation

fsbraun
Copy link
Member

@fsbraun fsbraun commented Jan 15, 2025

Description

Bug Fixes:

  • Fixed an issue where the CSRF token was not being fetched correctly.

Fixes #437

Related resources

Checklist

  • I have opened this pull request against master
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on
    Slack to find a “pr review buddy” who is going to review my pull request.

Copy link

sourcery-ai bot commented Jan 15, 2025

Reviewer's Guide by Sourcery

This pull request fixes an issue where the CSRF token was not being retrieved correctly. The change ensures that the CSRF token is taken from the CMS config if available, and falls back to the cookie if not.

Sequence diagram for CSRF token retrieval flow

sequenceDiagram
    participant Client
    participant CMS
    participant Cookie

    Client->>CMS: Check for CSRF token
    alt CMS config has token
        CMS-->>Client: Return CSRF token from config
    else No token in config
        Client->>Cookie: Check for CSRF token
        alt Cookie has token
            Cookie-->>Client: Return CSRF token from cookie
        else No token in cookie
            Cookie-->>Client: Return empty token
        end
    end
    Note over Client: Use token for POST request
Loading

Flow diagram for CSRF token retrieval logic

graph TD
    A[Start] --> B{CMS config has token?}
    B -->|Yes| C[Use CMS config token]
    B -->|No| D{Check cookie for token}
    D -->|Found| E[Use cookie token]
    D -->|Not found| F[Empty token]
    C --> G[Use token for POST request]
    E --> G
    F --> G
Loading

File-Level Changes

Change Details Files
Prioritize CSRF token from CMS config
  • Added logic to check for the CSRF token in the CMS config.
  • If the token is found in the config, it is used instead of the cookie value.
  • If the token is not found in the config, the code falls back to retrieving it from the cookie as before.
  • Added a check for the existence of the cookie token before attempting to access it, preventing potential errors if the cookie is not set.
  • Changed the parent window selection to always use window.top, ensuring consistency and preventing potential issues with nested iframes.
djangocms_versioning/static/djangocms_versioning/js/indicators.js

Assessment against linked issues

Issue Objective Addressed Explanation
#437 Fix CSRF token retrieval issue when CSRF_USE_SESSIONS = True
#437 Ensure state indicator actions work correctly for custom models with different CSRF settings
#437 Resolve JavaScript errors related to CSRF token retrieval

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @fsbraun - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

  • Add validation to ensure csrfToken is not empty before form submission (link)
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🔴 Security: 1 blocking issue
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@fsbraun
Copy link
Member Author

fsbraun commented Jan 15, 2025

@Will-Hoey Could you check if this PR solves your issue?

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.63%. Comparing base (09ec934) to head (206fba6).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #444   +/-   ##
=======================================
  Coverage   90.63%   90.63%           
=======================================
  Files          72       72           
  Lines        2702     2702           
  Branches      314      314           
=======================================
  Hits         2449     2449           
  Misses        179      179           
  Partials       74       74           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fsbraun
Copy link
Member Author

fsbraun commented Jan 17, 2025

@Will-Hoey Can you quickly check if this patch solves your issue?

@Will-Hoey
Copy link

Will-Hoey commented Jan 17, 2025

@Will-Hoey Can you quickly check if this patch solves your issue?

@fsbraun Apologies for the delay. Busy week!

With CSRF_USE_SESSIONS as either value - when the admin is in the modal it works as expected, however the same issue occurs when not in the modal, at admin/news/article/. The same console error of window.top.CMS is still there.

A side note, and this isn't really likely to be too much of a problem, if I switch that setting and then try any post request (logout, unpublish etc) it'll raise an incorrect token message unless I refresh the page. Kind of expected though!

@fsbraun
Copy link
Member Author

fsbraun commented Jan 17, 2025

@Will-Hoey Ah, right, that makes sense: In the admin (only) panel the CMS object is not available. In that case, the token needs to be pulled from a hidden input field. Do you think you can give it another try?

@Will-Hoey
Copy link

@fsbraun the new commit works when CSRF_USE_SESSIONS = False but not when it's True. Same js error as mentioned before

@fsbraun
Copy link
Member Author

fsbraun commented Jan 17, 2025

@Will-Hoey It works for me now. Maybe an outdated token still in the cookies? I've changed the order, so that the token from the form precedes the one in the cookies.

@Will-Hoey
Copy link

@fsbraun Resolved for me too using True/False values for the sessions setting, as well as in modal and in the admin. Thanks for the speedy responses this morning!

@fsbraun
Copy link
Member Author

fsbraun commented Jan 17, 2025

Thank you so much for the support! Sometimes the small things are more tricky than you expect!

@fsbraun fsbraun merged commit 8494eaf into master Jan 17, 2025
97 of 99 checks passed
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.

[BUG] StateIndicatorMixin raises CSRF error with CSRF_USE_SESSIONS = True
2 participants