-
Notifications
You must be signed in to change notification settings - Fork 571
Fix CORS configuration handling in WebServer and StreamingWrapper #162
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
base: main
Are you sure you want to change the base?
Conversation
feat: improve security by enabling CORS with trusted origins git diff -U0 --stat code/webserver/WebServer.py config/config_webserver.yaml # Conflicts: # code/config/config_webserver.yaml # code/webserver/WebServer.py
Potential duplicate or synergies with #179 |
# Conflicts: # code/webserver/WebServer.py
fix fix: enhance CORS configuration to only allow trusted origins fix: enhance CORS configuration to only allow trusted origins
# If the wildcard is set we use the wildcard anyways | ||
if '*' in CONFIG.server.cors_trusted_origins: | ||
response_headers['Access-Control-Allow-Origin'] = '*' | ||
|
||
response_headers['Access-Control-Allow-Methods'] = 'GET, POST, OPTIONS' |
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.
as you are here maybe you'd like to all all the verbs?
GET, HEAD, POST, OPTIONS, PUT, PATCH, DELETE
@schmichri, Thanks again for your continued development in NLWeb. I tested this out and think we need more changes for the functionality to work. In my testing it looks like CORS headers are being returned regardless of if they match the allowed list in the config. Let me know if you have cycles to assist. Either way, can you please make sure your branch isn't behind main? Thanks! |
@trevoreberl, you r welcome. Thats strange. Would you mind to share your |
Sorry for the delay, just getting back from vacation. I have confirmed Access-Control-Allow-Origin is missing when passing an unapproved Origin. I'm assuming my last issue was PEBKAC. :) If you can pull the upstream changes, we can get this approved. |
@trevoreberl perfect. Would also be great to give contributors a bit recognition as valuation for the fix by approving this here as motivation: #250 |
@tim13337 Ack, I'll bring this up to the core team. @schmichri as a heads up it looks like there has been some refactoring of core and static folders. |
@trevoreberl I'm a bit disappointed. This PR is >1 month old and now again merge work is to do here. I do not have the feeling that PRs are welcome here. Please let me explain my feeling: Development Roadmap #261 clearly states Please understand me right. We want NLWeb running on the Web. These are our efforts so far:
#250 would really help my motivation to keep on going. |
@schmichri - Appreciate your contributions here. We have been trying to get some things in place to make some larger shifts (see the Wiki, and stay tuned), so while we are absolutely open to contributions, some of the things above you have added will be much easier shortly as we'll be able to structure a bit differently. |
@chelseacarter29 In the wiki is just the talk of the refactoring's and the k8s issue that is also in the development road map could have also been solved since a long time. Do not see how anything in the wiki (as there is not much) solves @schmichri's things. Maybe you can elaborate eon this. Imho, contributing to a project where one cannot influence what is merged or where ones company is not being mentioned does not make sense as it does not leverage any business value. I do not understand the problem why not to list the sites where NLWeb is deployed already? |
Fix CORS configuration handling in WebServer and StreamingWrapper
Problem
The server crashes with a
KeyboardInterrupt
when attempting to accessCONFIG.server.cors_trusted_origins
in thesend_response
method. This issue occurs during the SSE (Server-Sent Events) response setup process when handling client connections.Changes
WebServer.py
send_response
methodStreamingWrapper.py
Technical Details
The error occurred in line 133 when trying to access
CONFIG.server.cors_trusted_origins
. The code now properly checks if the CORS configuration exists before attempting to access it, preventing the crash during SSE streaming initialization.WebServer.py
Testing
Related Issues
Fixes #179
@microsoft-github-policy-service agree company="iunera"