Skip to content

feat: add basic CORS support#392

Closed
Cali0707 wants to merge 2 commits intocontainers:mainfrom
Cali0707:add-basic-cors-support
Closed

feat: add basic CORS support#392
Cali0707 wants to merge 2 commits intocontainers:mainfrom
Cali0707:add-basic-cors-support

Conversation

@Cali0707
Copy link
Collaborator

This PR adds basic support for CORS.

This is needed for:

  1. Testing with auth setups (e.g. through mcp inspector), we don't want to require CORS disabled browsers
  2. Any clients that build a browser application that want to communicate to the MCP server

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707 Cali0707 requested review from manusa and matzew October 22, 2025 20:50
Signed-off-by: Calum Murray <cmurray@redhat.com>
@manusa
Copy link
Member

manusa commented Oct 23, 2025

I'm not sure about this one. I'll better check once we merge the others.

Note that all of the CORS issues are caused by a bad implementation of the modelcontextprotocol inspector. The main problem is that it's performing operations that should be handled by the backend from the browser (maybe due to a limitation on how it was implemented in the first place).

}
}

func setCORSHeaders(w http.ResponseWriter, origin string, corsConfig *config.CORSConfig) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not really sure we want that all the time.

Is it perhaps worth also following up on the cors issue, w/ the inspector tool?

@matzew matzew mentioned this pull request Oct 23, 2025
@Cali0707
Copy link
Collaborator Author

@manusa @matzew from modelcontextprotocol/inspector#817 (comment) it looks like we should at least be setting CORS headers to allow all origins for the .well-known/oauth-protected-resource endpoint

@manusa
Copy link
Member

manusa commented Oct 23, 2025

@manusa @matzew from modelcontextprotocol/inspector#817 (comment) it looks like we should at least be setting CORS headers to allow all origins for the .well-known/oauth-protected-resource endpoint

In any case, this should be handled by the proxied keycloak instance.
i.e. the header should be set by keycloak and not overridden by us which are only acting as a man in the middle.

@manusa
Copy link
Member

manusa commented Oct 23, 2025

Just to clarify:
We're proxying the .well-known endoints to be able to override certain values.
This is all implemented in https://github.com/containers/kubernetes-mcp-server/blob/49afbad502e5d97248f3c0d4b7b324311a91d021/pkg/http/wellknown.go

In this case, the implementation would be as simple as adding the header somewhere within:

func (w WellKnown) ServeHTTP(writer http.ResponseWriter, request *http.Request) {

But still, this should be responsibility of the proxied auth provider.

@Cali0707
Copy link
Collaborator Author

In any case, this should be handled by the proxied keycloak instance.

Ah, I had missed that - I am good with closing this then, but ideally I don't think we should require CORS disabled browsers for testing

@manusa
Copy link
Member

manusa commented Oct 23, 2025

In any case, this should be handled by the proxied keycloak instance.

Ah, I had missed that - I am good with closing this then, but ideally I don't think we should require CORS disabled browsers for testing

Agreed. AFAIU the changes in #393 should be enough (if not there might be some other underlying issue).

For me #393 is fine regardless if what it's stated in modelcontextprotocol/inspector#817 (comment) is a common practice or not (I'd really like to see the sources that back what's stated in that screen shot).
In the end, it's just a keycloak environment for development purposes.

@Cali0707
Copy link
Collaborator Author

Closing this as per discussion above - let's go with just the changes in #393 for now

@Cali0707 Cali0707 closed this Oct 23, 2025
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.

3 participants