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(frontend): Include CSRF token value in beacon call #30718

Closed
wants to merge 14 commits into from

Conversation

Ralkion
Copy link
Contributor

@Ralkion Ralkion commented Oct 25, 2024

SUMMARY

Before sending the log beacon call, include the CSRF token if it exists.

TESTING INSTRUCTIONS

  1. Be sure that CSRF is enabled
  2. Navigate the site and watch logs
  3. Verify that the POST /superset/log messages are 200s and not 302s.

ADDITIONAL INFORMATION

  • Has associated issue: Error logs full of "CSRF token missing" on POST /superset/log #30717
  • 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

@dosubot dosubot bot added the change:frontend Requires changing the frontend label Oct 25, 2024
@Ralkion
Copy link
Contributor Author

Ralkion commented Oct 25, 2024

I see the failed frontend test. I'll fix the existing and create a new one specific to this case a little later today.

@Ralkion
Copy link
Contributor Author

Ralkion commented Oct 26, 2024

@rusackas unit tests passed. Ready to go.

@Ralkion
Copy link
Contributor Author

Ralkion commented Oct 28, 2024

Hold off on this, I'm just discovering that it could actually be an issue in our configuration.

Seems by default the WTF_CSRF_EXEMPT_LIST is

[
    "superset.views.core.log",
    "superset.views.core.explore_json",
    "superset.charts.data.api.data",
    "superset.dashboards.api.cache_dashboard_screenshot",
]

But for some reason ours was defined to be []. I may have been looking in the wrong place.

@Ralkion
Copy link
Contributor Author

Ralkion commented Oct 28, 2024

Confirming that the issue was actually a misconfiguration on our end. I'll close this PR.

@Ralkion Ralkion closed this Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:frontend Requires changing the frontend size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant