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(types): Origin within OriginFunction can be undefined #237

Merged
merged 3 commits into from
Dec 20, 2022

Conversation

joshmeads
Copy link
Contributor

In reference to #236 origin cannot be assumed to always be a string. Propose making it string | undefined to ensure people handle it correctly. This would flag quickly in the sync example on the README that new URL constructor can't work with undefined and should be tested to ensure it's a string first.

This also matches an existing test setup:

const corsDelegate: OriginFunction = (origin, cb) => {
  if (typeof origin === 'undefined' || /localhost/.test(origin)) {
    cb(null, true)
    return
  }
  cb(new Error(), false)
}

Checklist

In reference to fastify#236 origin cannot be assumed to always be a string. Propose making it `string | undefined` to ensure people handle it correctly. This would flag quickly in the sync example on the README that new URL can't work with undefined.
@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 14, 2022

Can you add a typing test please?

index.d.ts Outdated Show resolved Hide resolved
Co-authored-by: KaKa <climba03003@gmail.com>
@joshmeads
Copy link
Contributor Author

Can you add a typing test please?

@Uzlopak apologies for the slow reply, busy time of year. What do you mean by typing test exactly? The altered code matches the test within tests/index.test-d.ts linked in the description. I can't see any type specific tests within the codebase unless I'm missing something obvious. Happy to do it though - just need a pointer on where to look :)

Thanks guys!

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 19, 2022

Well, what issue does your PR fix. As you didnt change any test, we have to assume nothing was wrong in the first place...

@jeanvalentin
Copy link

This PR would prevent users from blindly calling new URL(origin) and throwing, like I just did. I assumed origin was always a valid URL; it is in fact not even guaranteed to be a string. Good call. You might want to update the example in the readme as well, by adding an instruction such as if (!origin) return cb(null, true); on top.

Another option is to prevent CORS checks from being performed when origin is empty, since that would not be cross-origin.

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 20, 2022

How about patching tests/index.test-d.ts to reflect that?

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 20, 2022

patched accordingly

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

@Uzlopak Uzlopak merged commit 2e2f41a into fastify:master Dec 20, 2022
bodinsamuel referenced this pull request in specfy/specfy Jul 17, 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.2.0`
->
`8.3.0`](https://renovatebot.com/diffs/npm/@fastify%2fcors/8.2.0/8.3.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@fastify%2fcors/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@fastify%2fcors/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@fastify%2fcors/8.2.0/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@fastify%2fcors/8.2.0/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

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

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

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

##### What's Changed

- chore(deps-dev): bump typescript from 4.9.5 to 5.0.2 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/247](https://github.com/fastify/fastify-cors/pull/247)
- chore(deps-dev): bump tsd from 0.27.0 to 0.28.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/248](https://github.com/fastify/fastify-cors/pull/248)
- ci: only trigger on pushes to main branches by
[@&#8203;Fdawgs](https://github.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/249](https://github.com/fastify/fastify-cors/pull/249)
- chore(deps-dev): bump
[@&#8203;types/node](https://github.com/types/node) from 18.16.5 to
20.1.0 by [@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/250](https://github.com/fastify/fastify-cors/pull/250)
- Add `cacheControl` to control caching in CDN by
[@&#8203;brettwillis](https://github.com/brettwillis) in
[https://github.com/fastify/fastify-cors/pull/252](https://github.com/fastify/fastify-cors/pull/252)

##### New Contributors

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

**Full Changelog**:
fastify/fastify-cors@v8.2.1...v8.3.0

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

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

#### What's Changed

- chore(deps-dev): bump tsd from 0.24.1 to 0.25.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/235](https://github.com/fastify/fastify-cors/pull/235)
- fix(types): Origin within OriginFunction can be undefined by
[@&#8203;joshmeads](https://github.com/joshmeads) in
[https://github.com/fastify/fastify-cors/pull/237](https://github.com/fastify/fastify-cors/pull/237)
- chore(.gitignore): add clinic by
[@&#8203;Fdawgs](https://github.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/239](https://github.com/fastify/fastify-cors/pull/239)
- chore(.gitignore): add bun lockfile by
[@&#8203;Fdawgs](https://github.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/241](https://github.com/fastify/fastify-cors/pull/241)
- chore(deps-dev): bump tsd from 0.25.0 to 0.26.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/242](https://github.com/fastify/fastify-cors/pull/242)
- chore(deps-dev): bump tsd from 0.26.1 to 0.27.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/245](https://github.com/fastify/fastify-cors/pull/245)
- add normalizeCorsOptions and handle wildcards in origin parameter by
[@&#8203;Uzlopak](https://github.com/Uzlopak) in
[https://github.com/fastify/fastify-cors/pull/244](https://github.com/fastify/fastify-cors/pull/244)

#### New Contributors

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

**Full Changelog**:
fastify/fastify-cors@v8.2.0...v8.2.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 4pm on friday,before 9am on
monday,every weekend" in timezone Europe/Paris, 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/specfy/specfy).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi41LjMiLCJ1cGRhdGVkSW5WZXIiOiIzNi44LjExIiwidGFyZ2V0QnJhbmNoIjoiY2hvcmUvcmVub3ZhdGVCYXNlQnJhbmNoIn0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
bodinsamuel referenced this pull request in specfy/specfy Aug 8, 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.2.0`
->
`8.3.0`](https://renovatebot.com/diffs/npm/@fastify%2fcors/8.2.0/8.3.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@fastify%2fcors/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@fastify%2fcors/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@fastify%2fcors/8.2.0/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@fastify%2fcors/8.2.0/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

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

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

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

##### What's Changed

- chore(deps-dev): bump typescript from 4.9.5 to 5.0.2 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/247](https://github.com/fastify/fastify-cors/pull/247)
- chore(deps-dev): bump tsd from 0.27.0 to 0.28.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/248](https://github.com/fastify/fastify-cors/pull/248)
- ci: only trigger on pushes to main branches by
[@&#8203;Fdawgs](https://github.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/249](https://github.com/fastify/fastify-cors/pull/249)
- chore(deps-dev): bump
[@&#8203;types/node](https://github.com/types/node) from 18.16.5 to
20.1.0 by [@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/250](https://github.com/fastify/fastify-cors/pull/250)
- Add `cacheControl` to control caching in CDN by
[@&#8203;brettwillis](https://github.com/brettwillis) in
[https://github.com/fastify/fastify-cors/pull/252](https://github.com/fastify/fastify-cors/pull/252)

##### New Contributors

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

**Full Changelog**:
fastify/fastify-cors@v8.2.1...v8.3.0

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

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

##### What's Changed

- chore(deps-dev): bump tsd from 0.24.1 to 0.25.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/235](https://github.com/fastify/fastify-cors/pull/235)
- fix(types): Origin within OriginFunction can be undefined by
[@&#8203;joshmeads](https://github.com/joshmeads) in
[https://github.com/fastify/fastify-cors/pull/237](https://github.com/fastify/fastify-cors/pull/237)
- chore(.gitignore): add clinic by
[@&#8203;Fdawgs](https://github.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/239](https://github.com/fastify/fastify-cors/pull/239)
- chore(.gitignore): add bun lockfile by
[@&#8203;Fdawgs](https://github.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/241](https://github.com/fastify/fastify-cors/pull/241)
- chore(deps-dev): bump tsd from 0.25.0 to 0.26.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/242](https://github.com/fastify/fastify-cors/pull/242)
- chore(deps-dev): bump tsd from 0.26.1 to 0.27.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/245](https://github.com/fastify/fastify-cors/pull/245)
- add normalizeCorsOptions and handle wildcards in origin parameter by
[@&#8203;Uzlopak](https://github.com/Uzlopak) in
[https://github.com/fastify/fastify-cors/pull/244](https://github.com/fastify/fastify-cors/pull/244)

##### New Contributors

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

**Full Changelog**:
fastify/fastify-cors@v8.2.0...v8.2.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 4pm on friday,before 9am on
monday,every weekend" in timezone Europe/Paris, Automerge - At any time
(no schedule defined).

🚦 **Automerge**: Disabled because a matching PR was automerged
previously.

♻ **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/specfy/specfy).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMS4wIiwidXBkYXRlZEluVmVyIjoiMzYuMjcuMSIsInRhcmdldEJyYW5jaCI6ImNob3JlL3Jlbm92YXRlQmFzZUJyYW5jaCJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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