Skip to content

Conversation

@simon-k
Copy link
Contributor

@simon-k simon-k commented Jun 6, 2025

Check that 'origin' header exists instead of 'Origin'.

When you parse headers you lowercase all the header keys. So there will never be a header with the key 'Origin'. It will be 'origin', So the CORS configuration will never work. This PR should fix that.

Fixes #178

Check that 'origin' header exists instead of 'Origin'
Copilot AI review requested due to automatic review settings June 6, 2025 21:24

This comment was marked as outdated.

@tim13337
Copy link

tim13337 commented Jun 8, 2025

Hi @simon-k great you found the same issue shows it is valid - can we somehow merge that with #162 or decide which is the better or more complete fix? I think that alignment will help to get it fixed in main.

@schmichri
Copy link
Contributor

schmichri commented Jun 8, 2025

#162 is still in draft. It's more than just fixing the case sensitivity. we need to be able to configure proper CORS origins.

@tim13337
Copy link

tim13337 commented Jun 8, 2025

I'd focus on low hanging fruits.
Let us keep that simple: First check if @schmichri or @simon-k 's fix is better or best
What I understood from you is that the CORs is not working at all, because of the "O" and not "o".
Hence,

  • merge your fixes if they are complementary in @simon-k or @schmichri's repo
  • or decide for the better one and surrender the other one
  • and then try to push that in a team effort for merge with the NLWeb team
  • separately, I'd discuss what is required for CORs in general in addition:

Question what is required in addition: I'd surrender this ticket then if one of you makes a new one with all requirements or you add the requirements to mine. Your choice.

@simon-k
Copy link
Contributor Author

simon-k commented Jun 8, 2025

Hi @tim13337. Thanks for replying to the PR. Yes, my experience is that CORS does not work because of the "O" and not "o". When the WebServer.py parse headers then it lowercase all the header keys (here). So looking for a header with a capital letter won't work.

To be honest, I'm just the random dude seeing NLWeb at MS Build and then going home to try it out. I found this "bug" and thought this PR would be the simplest and least intrusive change to the source code. And It would fix the issues I am having. So I was hoping that it would have the highest chance to get in. The PR can't get any smaller, and I really think it a blocker if you want to do something more than just running the "HelloWorld" example.

I'm working on a Medium article where I take the HelloWorld example in this repo a bit further and show how to integrate NLWeb to your own website and how you use the MCP feature. Nothing groundbreaking at all, but many of the other guides out there are just a replica og the HelloWorld. I wanted to take it a step further.

@jennifermarsman jennifermarsman requested a review from Copilot June 9, 2025 12:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the CORS header check by using the lowercased header key so that incoming requests with headers already normalized to lowercase are correctly detected.

  • Change the CORS-origin check from 'Origin' to 'origin'
  • Ensures CORS headers are applied when headers are parsed in lowercase
Comments suppressed due to low confidence (1)

code/webserver/WebServer.py:131

  • Add a unit or integration test to verify that CORS headers are correctly added when an incoming request includes a lowercase 'origin' header.
if CONFIG.server.enable_cors and 'origin' in headers:

Copy link
Collaborator

@jennifermarsman jennifermarsman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this fixes a mistake. We will also look at #162 when it is out of draft for a more complete solution.

@jennifermarsman jennifermarsman merged commit 419575f into nlweb-ai:main Jun 9, 2025
1 check passed
@tim13337
Copy link

Had the same problem here.
@simon-k oh that sounds like an interesting article. Please share when finished.

@simon-k
Copy link
Contributor Author

simon-k commented Jun 10, 2025

Thanks @jennifermarsman!

@tim13337 here’s the article. Again, nothing ground breaking https://medium.com/@simon.c.kofod/nlweb-how-to-get-an-ai-chatbot-on-your-website-fac9cc54b5a0

medhabelwadi pushed a commit to medhabelwadi/NLWeb that referenced this pull request Jul 1, 2025
rvguha pushed a commit that referenced this pull request Sep 23, 2025
Check for 'origin' header
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.

Checking "Origin" header is wrong.

4 participants