Skip to content

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

schmichri
Copy link
Contributor

@schmichri schmichri commented Jun 3, 2025

Fix CORS configuration handling in WebServer and StreamingWrapper

Problem

The server crashes with a KeyboardInterrupt when attempting to access CONFIG.server.cors_trusted_origins in the send_response method. This issue occurs during the SSE (Server-Sent Events) response setup process when handling client connections.

Changes

  • Fixed access to CORS configuration in WebServer.py
  • Ensured proper handling of configuration properties in send_response method
  • Added error handling for missing configuration values
  • Improved robustness of SSE response setup in StreamingWrapper.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

  • Tested with local server startup
  • Verified proper handling of client connections
  • Confirmed SSE responses work correctly with the fixed configuration handling

Related Issues

Fixes #179

@microsoft-github-policy-service agree company="iunera"

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
@schmichri schmichri marked this pull request as ready for review June 3, 2025 05:30
@schmichri schmichri changed the title Improve CORS configuration to only allow trusted origins Draft: Improve CORS configuration to only allow trusted origins Jun 6, 2025
@schmichri schmichri changed the title Draft: Improve CORS configuration to only allow trusted origins Improve CORS configuration to only allow trusted origins Jun 6, 2025
@schmichri schmichri marked this pull request as draft June 6, 2025 07:25
@tim13337
Copy link

tim13337 commented Jun 8, 2025

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
@schmichri schmichri marked this pull request as ready for review June 10, 2025 05:23
@schmichri schmichri changed the title Improve CORS configuration to only allow trusted origins Fix CORS configuration handling in WebServer and StreamingWrapper Jun 10, 2025
# 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'
Copy link

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

@trevoreberl trevoreberl self-requested a review June 23, 2025 20:04
@trevoreberl
Copy link
Collaborator

@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!

@schmichri
Copy link
Contributor Author

@trevoreberl, you r welcome. Thats strange. Would you mind to share your config_webserver.yaml and some information about your testsetup? Add the request and response header please.

@trevoreberl
Copy link
Collaborator

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.

@tim13337
Copy link

tim13337 commented Jul 9, 2025

@trevoreberl perfect. Would also be great to give contributors a bit recognition as valuation for the fix by approving this here as motivation: #250

@trevoreberl
Copy link
Collaborator

@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.

@schmichri
Copy link
Contributor Author

@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 Making it easier to standup an instance of NLWeb. The Docker/Kubernetes support that's in the was never recognized even that the open issue #183 and offered PR of an already running Implementation. More examples here
#165 / #191

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.

@chelseacarter29
Copy link
Collaborator

@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.

@tim13337
Copy link

@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?
What is the blocker there?

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.

6 participants