-
Notifications
You must be signed in to change notification settings - Fork 11k
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
Enable CORS requests on all routes #5781
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets put this feature behind a command-line flag: --enable-cors
By default, it should not be enabled
Ohhh so that's how you apply middleware to routes, I couldn't find it at first. I read the discussion in the linked thread, and I'm honestly a bit unsure about what the right approach is. In my opinion CORS is not an amazing security mechanism because as was already stated, anyone can still approach the server using anything other than a browser that respects CORS. Still, you'd be preventing sites doing something similar to crypto mining on your pc by accessing any locally running LLMs if it's not disabled. I want to add two points The first is, if you do the flag thing, at the very least add a short log to the terminal that CORS is disabled when an endpoint is rejected due to CORS, informing the user they can enable it, but should understand the security risks (and recommend setting an API key) So when CORS is disabled and an OPTIONS endpoint is hit, log a message to the console
The alternative is to enable it by default, but work on a whitelist basis. No hosts except localhost are allowed by default. You can enable hosts in something like a cors-whitelist.txt. This is how CORS is supposed to be used by design. This is simpler and safer but requires people to interact with the whitelist file. It's possible to maybe add a Y/N prompt to the server when an unknown host is trying to access the server, or otherwise list a similar warning message as the one above
|
@Azeirah I love the log idea. I think that would substantially help cut down on confused users and new github issues if we disable CORS by default. @ggerganov @ngxson thoughts on using a cors-whitelist .txt file, or adding a --cors-whitelist flag? |
@ggerganov @Azeirah (Modified and moved to a comment below) |
Also what we're currently doing:
This is equivalent to
That's why I say that it's redundant. What is interesting for us is to checkthat |
No, they're not equivalent. CORS requires the domain to be mirrored back explicitly in the case of credentialed requests (a credentialed request is one with the Authorization header) |
@Azeirah Ah yeah sorry I missed that part. But even with what you say, then the fact that we currently mirror the In this case, I think we should:
Basically, what I suggest here is either use |
Yep! I think whitelisting is the best approach. The current implementation is a catchall |
Thanks, we finally have an agreement here. I modified my proposal, my idea is to keep the
By throwing an error instead of letting browser to block it, some frontends can show the error message Do you still prefer this approach (reflect Origin) or you prefer to have |
Only remark I have is to allow localhost regardless. I think it's a sensible default. Am not 100% certain whether cors even applies to localhost though? So maybe not necessary. But many chat clients are just electron wrappers around some js webapp and they run into CORS issues. |
* Disable CORS requests by default. * Add --public-domain flag to allow specifying a CORS allowed domain. * Warn about using "*" without an API key.
I've implemented the feature as specified. The origin is always reflected. I added a flag "--public-domain" which allows users to specify a domain which will allow CORS requests. Any requests which specify an origin other than the public domain will receive an error message and an http 403 status. Blocked requests are logged. The public-domain setting defaults to "http://localhost:8080". Setting the public-domain to "*" without also including an API key logs a warning. I used "public-domain" instead of "whitelist" because a CORS header only allows for setting a single origin (as far as I understand) and I implemented the origin check as a simple string comparison, so you can only specify a single CORS enabled origin. That seemed more robust, and if users want to enable multiple origins, then setting the public domain to "*" and using an API key is probably the best way to handle that. |
examples/server/README.md
Outdated
@@ -42,6 +42,7 @@ see https://github.com/ggerganov/llama.cpp/issues/1437 | |||
- `-to N`, `--timeout N`: Server read/write timeout in seconds. Default `600`. | |||
- `--host`: Set the hostname or ip address to listen. Default `127.0.0.1`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did not change here to the hostname. Anyway I dont understand why you switched from IP to hostname? is not IP valid URL in Origin ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's currently a mixture of "127.0.0.1" and "localhost" throughout the codebase. I started to do some normalization to a single value, and moved towards localhost, as that's been the domain suggested by Azeirah and Ngxson. But then I ran into trouble with the python tests not liking localhost, so I went back, but missed this instance.
There's probably a consideration to be made about which value is more likely to be intuitive for a typical user, but maybe that's it's own issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also prefer 127.0.0.1
because sometimes localhost
is not defined in docker environment
@@ -47,5 +47,5 @@ Feature: Security | |||
| localhost | Access-Control-Allow-Origin | localhost | | |||
| web.mydomain.fr | Access-Control-Allow-Origin | web.mydomain.fr | | |||
| origin | Access-Control-Allow-Credentials | true | | |||
| web.mydomain.fr | Access-Control-Allow-Methods | POST | | |||
| web.mydomain.fr | Access-Control-Allow-Methods | * | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually can we just restrict to GET and POST ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah you're right, I though it's a good idea to use *
, but turns out it shouldn't be. Can you change it to GET, POST
@StrangeBytesDev ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we want GET, POST, OPTIONS so that preflight requests are allowed.
examples/server/README.md
Outdated
@@ -42,6 +42,7 @@ see https://github.com/ggerganov/llama.cpp/issues/1437 | |||
- `-to N`, `--timeout N`: Server read/write timeout in seconds. Default `600`. | |||
- `--host`: Set the hostname or ip address to listen. Default `127.0.0.1`. | |||
- `--port`: Set the port to listen. Default: `8080`. | |||
- `--public-domain`: Set a public domain which will be allowed for Cross Origin Requests. If you are using the server as an API from a browser, this is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename it to reference CORS at the very least, public domain is not the right terminology, it might confuse people. I'd recommend something like --cors-allowed-origin
or --cors-origin
.
- `--cors-origin`: Set what origin (example.com) is allowed to access the API. Use * to allow all origins (insecure without --api-key). If you are using the server as an API from a browser, this parameter is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ and the user is sending cross origin requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, to be coherent with other parameters, I suggest --http-cors-origin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought was that a typical user probably wouldn't know what CORS is or why they need it. I've updated the flag to be "--http-cors-origin". There's potentially some consideration to avoiding "http" as the origin "https://example.org" is different from "http://example.org".
|
||
if (req.has_header("Origin") && sparams.public_domain != "*") { | ||
if (req.get_header_value("Origin") != sparams.public_domain) { | ||
LOG_WARNING("Request from origin not allowed.", {{"origin", req.get_header_value("Origin")}}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to note, I believe this also solves the issue that the server previously had, when a request was sent that was disallowed by CORS, the server would still process the request fully. So even disallowed origins could make your PC waste cycles by calling the completion API.
This is a better way to handle CORS :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Origin header actually have the protocol + port within it. I found a simple code snippet allow you to parse URI and extract the domain name here: https://gist.github.com/RedCarrottt/c7a056695e6951415a0368a87ad1e493
* Restrict HTTP requests to GET, POST, and OPTIONS. * rename cors flag from "--public-domain" to "--http-cors-origin"
examples/server/server.cpp
Outdated
invalid_param = true; | ||
break; | ||
} | ||
sparams.http_cors_origin = argv[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible here to input a list of domain names, for example example.com,mywebsite.com,example.net
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. It also made for a pretty convenient way to include both "localhost" and "127.0.0.1" by default.
* Allow setting multiple CORS enabled origins. * Add both "http://localhost:8080" and "http://127.0.0.1:8080" by default. * Move CORS logging below server startup to make it more visible.
Enable's CORS requests on all endpoints for all request types. Update's tests to reflect that change.