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

chore: exempt cache-warmup endpoint from CSRF protection #25978

Closed
wants to merge 1 commit into from

Conversation

mapledan
Copy link
Contributor

The default configuration for WTF_CSRF_ENABLED is 'True,' and this was causing the cache-warmup task to fail.
The API request for the celery worker only includes session cookie.
Therefore, It should include superset.charts.api.warm_up_cache in the WTF_CSRF_EXEMPT_LIST.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

  1. Configure the cache-warmup task.
  2. The Celery task successfully executed the fetch_url action.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@mapledan mapledan changed the title Exempt cache-warmup endpoint from CSRF error chore: exempt cache-warmup endpoint from CSRF error Nov 14, 2023
@mapledan mapledan closed this Nov 14, 2023
@mapledan mapledan reopened this Nov 14, 2023
@mapledan mapledan changed the title chore: exempt cache-warmup endpoint from CSRF error chore: exempt cache-warmup endpoint from CSRF protection Nov 14, 2023
@john-bodley
Copy link
Member

@dpgaspar do you know why we have CSRF exemption in the first place? @michael-s-molina and myself are somewhat perplexed as to why a handful of API endpoints are exempt from requiring the CSRF token.

@michael-s-molina
Copy link
Member

@mapledan Although I understand why you would want to disable this in the Celery context, you need to consider that anyone can invoke the endpoint. @dpgaspar do we need to revisit this configuration?

@dpgaspar
Copy link
Member

@mapledan Although I understand why you would want to disable this in the Celery context, you need to consider that anyone can invoke the endpoint. @dpgaspar do we need to revisit this configuration?

By default only non HTTP GET endpoints are protected by CSRF, we have considered superset.views.core.explore_json and superset.charts.data.api.data to work as HTTP GET since no state is changed by these endpoints.

charts.data was exempted to make it easier for users to use this API #10397
explore_json probably required by embedded, yet it does work as an HTTP GET endpoint also #17530

no sure about the log endpoint

@dpgaspar
Copy link
Member

@mapledan is this endpoint called by celery on Superset?

@mapledan
Copy link
Contributor Author

mapledan commented Nov 16, 2023

Yes, the warm_up_cache endpoint is called by cache-warmup task. #23853
It's clear to me now that superset.charts.api.warm_up_cache does not work like a GET request, it has changed the cache resource.
I submitted another PR (#26003) that allows machine_auth to generate a CSRF token.
Possibly, this is the more correct approach.

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

probably unnecessary because of: #26003

@mapledan mapledan closed this Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants