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

Origin returns undefined #236

Closed
2 tasks done
joshmeads opened this issue Dec 14, 2022 · 14 comments
Closed
2 tasks done

Origin returns undefined #236

joshmeads opened this issue Dec 14, 2022 · 14 comments

Comments

@joshmeads
Copy link
Contributor

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.x.x

Plugin version

8.2.x

Node.js version

18.x.x

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

13.0.1

Description

Previously referenced in issue #154 and recently commented by @damey2011 with a later request to open a new ticket with an example.

origin always appears to be undefined.

I've created a simple example repo with both examples (sync & async) from the README provided.

  • The sync example will crash as the new URL constructor will throw.
  • The async example will pass but the localhost.test(req.headers.origin) will never execute. The way the example is provided will also cause the TypeScript compiler to throw (though this is understandable given the examples aren't TS examples).

I'm currently unsure if this is related to TypeScript or not as I haven't had a chance to duplicate it over into just JS, however given the very simple implementation example I wouldn't be surprised if it affects both.

Any assistance resolving this issue would be greatly appreciated. I've tried to debug it myself but I don't understand how req.headers.origin is ever populated.

I have seen another implementation use req.headers.host though i've also read that's also a poor way to resolve the issue.

Steps to Reproduce

Example Codebase

  • npm install
  • npm run dev
  • Send a request to localhost:3000/test

Expected Behavior

According to the examples provided - req.headers.origin should exist.

@climba03003
Copy link
Member

climba03003 commented Dec 14, 2022

We cannot guarantee a header to be exist, since it is controlled by the browser or client.
You should double check if the origin header is sent from your client, or provide a reproducible step that show when the origin header is sent and we still provide an undefined header.

If you cannot show in this case, I believe the types and plugin should be correct. The header can be undefined.

@joshmeads
Copy link
Contributor Author

joshmeads commented Dec 14, 2022

@climba03003 Understood, thank you! I think that's a little unclear in the documentation. Especially when the primary example causes an error to get thrown, it leads people to think something isn't right.

I appreciate the fast response and I'll close the issue for now based on the info you've provided. Hopefully it can serve as a quick reference for anyone else facing a similar blocker.

joshmeads added a commit to joshmeads/fastify-cors that referenced this issue Dec 14, 2022
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 added a commit that referenced this issue Dec 20, 2022
* Callback of origin should be potentially undefined

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 can't work with undefined.

* Update index.d.ts

Co-authored-by: KaKa <climba03003@gmail.com>

* restructure, add typings test

Co-authored-by: KaKa <climba03003@gmail.com>
Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>
@samuelkilada
Copy link

What's strange is this seems to happen when running a node fastify server locally (with the cors plugin added) and navigating to a localhost server route in my browser (Edge). Origin is undefined (though only sometimes). Is this normal behavior?

@Eomm
Copy link
Member

Eomm commented Jan 9, 2023

Please open a new detailed issue adding a Minimal, Reproducible Example.

Is this normal behavior?

No*, but there are edge cases: https://stackoverflow.com/questions/42239643/when-do-browsers-send-the-origin-header-when-do-browsers-set-the-origin-to-null

*if the browser send it

@climba03003
Copy link
Member

Origin is undefined (though only sometimes). Is this normal behavior?

Yes, as I said the origin exist or not depends on your client which means browser.
I would only consider it as bug when the client sent Origin header but we provide undefined. Otherwise, it is expected behavior.

@j0hnm4r5
Copy link

I totally understand this is expected behavior, and fastify-cors is doing exactly what it's supposed to do here.

However, this middleware is very often someone's first interaction with CORS, and not mentioning in the README that there may be an error thrown because the Origin header isn't always present is both doing the community a disservice, and also leaving opportunity for issues like this where people think the package is malfunctioning.

@Uzlopak
Copy link
Contributor

Uzlopak commented May 30, 2023

Were does it throw an error?

@j0hnm4r5
Copy link

When running the origin: (origin, cb) => example in the README exactly as written, Node will throw TypeError [ERR_INVALID_URL]: Invalid URL if the Origin header is missing.

Which again, is exactly what you designed it to do. But IMO the documentation should mention that the Origin header isn't present in all requests, why it isn't present in all requests, that this error will be thrown if it isn't, and what you should do if it is thrown.

@climba03003
Copy link
Member

It's better for you to send a PR for this changes rather than explaining what and why should it be change.

@wesley3295
Copy link

What should you do if you get "undefined" for the origin? I can't access the api through the browser or thunder client (Lightweight Rest API Client). What is the correct way to handle this? @climba03003 @j0hnm4r5 @Eomm @joshmeads

@Uzlopak Uzlopak reopened this Jan 26, 2024
@Uzlopak
Copy link
Contributor

Uzlopak commented Jan 26, 2024

Can you please provide a simple repository which shows the bug?

@JaoodxD
Copy link

JaoodxD commented Jan 26, 2024

Can you please provide a simple repository which shows the bug?

My bad. Just read answers once again and found that it wasn't a fault of fastify-cors.
Double checked what my curl sent localy and there was no origin header. That's why readme example failed.
When I explicitely set curl -H 'origin: http://localhost' localhost:3000 everything become fine.

@JaoodxD
Copy link

JaoodxD commented Jan 26, 2024

Seems like @samuelkilada faced similar behavior, but with browser

@climba03003
Copy link
Member

If anyone face the similar problem, please open a new issue with reproducible code.

Please be remindered that origin header is NOT always provided by Browser, using any tools like cURL need to explicitly set the origin header because it only provide minimal header by default.

When security is your highest concern, you should reject the request when NO origin header.
When high availability is your concern, you should accept the request in this case and provide proper check inside the other route.

@climba03003 climba03003 closed this as not planned Won't fix, can't repro, duplicate, stale Jan 26, 2024
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

No branches or pull requests

8 participants