Skip to content

fix: checkOrigin headers check - #12632

Merged
ematipico merged 2 commits into
4-legacyfrom
fix/check-origin
Dec 5, 2024
Merged

ematipico merged 2 commits into
4-legacyfrom
fix/check-origin

Conversation

@ematipico

Copy link
Copy Markdown
Member

Changes

This PR ports a fix of checkOrigin to v4

Testing

CI should pass

Docs

N/A

* fix: enforce check origin logic

* address feedback
@changeset-bot

changeset-bot Bot commented Dec 4, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 56597ff

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions Bot added the pkg: astro Related to the core `astro` package (scope) label Dec 4, 2024
@bluwy

bluwy commented Dec 5, 2024

Copy link
Copy Markdown
Member

Before merging this, we might want to first prepare the 4-legacy branch for changesets release, something like this change (see first 4 files): sveltejs/vite-plugin-svelte@main...v2

That way, changesets will create the correct PR for releasing the backport, and we don't clash the latest tag on npm once this backport is published. We want the latest tag to be the current v5 major.

We could name this backport tag as backport, legacy, or previous etc.

@ematipico

Copy link
Copy Markdown
Member Author

I don't think I understand what I have to do. I thought that adding 4-legacy to the workflow was enough.

request.method === 'PUT' ||
request.method === 'PATCH' ||
request.method === 'DELETE') &&
request.headers.get('origin') === url.origin;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we allow empty origins too?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought checking the origin was the whole point of CSRP protection 😅 Regardless, this port from v5 is a security fix. We shouldn't change the logic in v4, but we can enhance the v5.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's checking that the origin matches to prevent CSRF. A browser will always send the origin header for those requests. The only reason I ask is because of webhooks and APIs that might be called directly, and users might want to still have available while protecting forms from CSRF. However I realise the webhooks could just send a different content type, so would be fine

@bluwy

bluwy commented Dec 5, 2024

Copy link
Copy Markdown
Member

I don't think I understand what I have to do. I thought that adding 4-legacy to the workflow was enough.

I updated the 4-legacy branch to change the npm tag. We don't need it if we release the backport, and immediately release a patch for astro 5 again. But to be safe we can prevent sharing the tag by using a different tag for the backport

@ematipico
ematipico merged commit e7d14c3 into 4-legacy Dec 5, 2024
@ematipico
ematipico deleted the fix/check-origin branch December 5, 2024 14:29
@astrobot-houston astrobot-houston mentioned this pull request Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg: astro Related to the core `astro` package (scope)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants