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: support for embedded cors for route params #278

Merged
merged 3 commits into from
Nov 20, 2023
Merged

fix: support for embedded cors for route params #278

merged 3 commits into from
Nov 20, 2023

Conversation

mohammedSlimani
Copy link
Contributor

@mohammedSlimani mohammedSlimani commented Nov 17, 2023

Checklist

Description of the bug:

When we try to set cors for a route that has route params, it seems that the cors plugin fails at registering the route and any calls to that route will return a 404 instead.

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

I would mark it as a fix

Under the hood, the route registered was:

/:id*

and find my way does not support path parameter + wildcard like this.

Note that the preflight out of the /:id prefix, will not work anymore, still I think it is safe as preflight happens on the same context of other routes

test/cors.test.js Outdated Show resolved Hide resolved
@Eomm Eomm changed the title add support for imbedded cors for route params fix: support for imbedded cors for route params Nov 18, 2023
@Uzlopak Uzlopak changed the title fix: support for imbedded cors for route params fix: support for embedded cors for route params Nov 18, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -72,7 +72,7 @@ function fastifyCors (fastify, opts, next) {
// remove most headers (such as the Authentication header).
//
// This route simply enables fastify to accept preflight requests.
fastify.options('*', { schema: { hide: hideOptionsRoute } }, (req, reply) => {
fastify.options('/*', { schema: { hide: hideOptionsRoute } }, (req, reply) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Member

Choose a reason for hiding this comment

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

when we register the url into find-my-way, we do:

prefix + url

so, in this case:

path param: "/:id" + "*" = "/:id*"
static prefix: "/foo" + "*" = "/foo*"

while the 2nd seems to work (but it can generate conflicts in any case), the first one does not work because it is a path parameter + a whilcard

Co-authored-by: Manuel Spigolon <[email protected]>
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

Read the explaination, understood it, checked the code, checked the tests, still a little bit confused but

LGTM

@Uzlopak Uzlopak merged commit 9acbafe into fastify:master Nov 20, 2023
19 checks passed
@krvajal
Copy link

krvajal commented Dec 4, 2023

Any plans of cutting a new release soon with this changes @Uzlopak ?

@Eomm
Copy link
Member

Eomm commented Dec 4, 2023

Done, thanks for the ping

renovate bot referenced this pull request in tomacheese/telcheck Dec 4, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@fastify/cors](https://github.com/fastify/fastify-cors) | [`8.4.1`
->
`8.4.2`](https://renovatebot.com/diffs/npm/@fastify%2fcors/8.4.1/8.4.2)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@fastify%2fcors/8.4.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@fastify%2fcors/8.4.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@fastify%2fcors/8.4.1/8.4.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@fastify%2fcors/8.4.1/8.4.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>fastify/fastify-cors (@&#8203;fastify/cors)</summary>

###
[`v8.4.2`](https://github.com/fastify/fastify-cors/releases/tag/v8.4.2)

[Compare
Source](https://github.com/fastify/fastify-cors/compare/v8.4.1...v8.4.2)

#### What's Changed

- build(dependabot): ignore tap major updates by
[@&#8203;Fdawgs](https://github.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/276](https://github.com/fastify/fastify-cors/pull/276)
- fix: support for embedded cors for route params by
[@&#8203;mohammedSlimani](https://github.com/mohammedSlimani) in
[https://github.com/fastify/fastify-cors/pull/278](https://github.com/fastify/fastify-cors/pull/278)

#### New Contributors

- [@&#8203;mohammedSlimani](https://github.com/mohammedSlimani) made
their first contribution in
[https://github.com/fastify/fastify-cors/pull/278](https://github.com/fastify/fastify-cors/pull/278)

**Full Changelog**:
fastify/fastify-cors@v8.4.1...v8.4.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/tomacheese/telcheck).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy44MS4zIiwidXBkYXRlZEluVmVyIjoiMzcuODEuMyIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
climba03003 added a commit that referenced this pull request Dec 17, 2023
mcollina pushed a commit that referenced this pull request Dec 18, 2023
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.

5 participants