Skip to content

fix: remove credentials omit from agenda fetch call #8779

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

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

NGPixel
Copy link
Member

@NGPixel NGPixel commented Apr 3, 2025

No description provided.

@NGPixel NGPixel requested review from rjsparks and Copilot April 3, 2025 14:52
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request fixes the fetch call for agenda data by removing the unnecessary credentials option, streamlining the API request.

  • Removed "credentials: 'omit'" from the fetch call.
  • Simplified the request to better align with the API's expected behavior.

Copy link
Member

@jennifer-richards jennifer-richards left a comment

Choose a reason for hiding this comment

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

Wonder if it should be 'same-origin' to reduce risk of CSRF issues if the code gets reused?

https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#including_credentials

edit: never mind, I see that's the default if I actually read my own reference 😬

@rjsparks rjsparks marked this pull request as draft April 15, 2025 15:15
@rjsparks
Copy link
Member

From discussion - instead of this PR we probably want to either change the code to make production and staging behave differently, or change the CF rules around the api endpoint at staging instead.

If we want to do that in this code, please update this PR, otherwise close it.

@rjsparks rjsparks marked this pull request as ready for review April 15, 2025 15:44
@rjsparks
Copy link
Member

Subsequent discussion convinced me I was incorrect above.

@rjsparks rjsparks merged commit 86988eb into main Apr 15, 2025
8 checks passed
@NGPixel NGPixel deleted the fix-agenda-fetch-creds branch April 15, 2025 15:48
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants