Skip to content

Reverse decisions for CORS and strict content type parsing. #902

Description

@SamMorrowDrums

#842 introduces to regressions to valid content types and applies erroneous CORS protections that are not the right security model for MCP servers, and just lock out web clients.

It would be great if you can confirm that the intention is to remove the CORS stuff, as we will use the temporary compatibility flag, but we expect that in hindsight once the flag is gone, it will be replaced with that being the default (whereas the release docs say the opposite).

Thanks so much, just want to ensure we are aligned on intention here 🙏

Activity

  1. maciej-kisiel commented on Apr 21, 2026

    @maciej-kisiel
    Contributor

    @jba What do you think about reverting the default CORS protection enablement? Admittedly, it may have been a bit overeager change from my side to address a security advisory and I didn't realize the impact it would have on web clients.

  2. jba commented on Apr 21, 2026

    @jba
    Contributor

    I'd like to understand this a little better.

    You say "any client can try to connect to an MCP and the CORS headers don't matter"
    (github/github-mcp-server#2359 (comment)). Are you saying what we have now (installing a zero http.CrossOriginProtection) is completely ineffective?

    The CVE says, "a remote attacker can exploit this Cross-Site Request Forgery (CSRF) vulnerability... [b]y sending browser-generated cross-site POST requests to a local server without proper validation of the Origin header". What is erroneous about this?

    You say you are using "the temporary compatibility flag," presumably the debug env var. But your PR is actually using the API as intended, by setting StreamableHTTPOptions.CrossOrigin.CrossOriginProtection.

    In short, why aren't we working as intended, locking down the server as much as possible while allowing fully flexible overrides?

  3. SamMorrowDrums commented on Apr 21, 2026

    @SamMorrowDrums
    ContributorAuthor

    Thanks for looking into this.

    I'd like to understand this a little better.

    You say "any client can try to connect to an MCP and the CORS headers don't matter"
    (github/github-mcp-server#2359 (comment)). Are you saying what we have now (installing a zero http.CrossOriginProtection) is completely ineffective?

    Sorry if I was confusing, I meant to say it only blocks web based connection attempts (and only ones that have a concept of CORS at that).

    It is effective at blocking web based attacks, it just shouldn't be auto-enabled in my opinion. It blocks valid web based MCP clients. Also web browsers do not distinguish between a local web page (like MCP Inspector) and a remote web page making cross origin requests.

    The CVE says, "a remote attacker can exploit this Cross-Site Request Forgery (CSRF) vulnerability... [b]y sending browser-generated cross-site POST requests to a local server without proper validation of the Origin header". What is erroneous about this?

    Erroneous might be overkill, it's possible. I just don't think it's a meaningful vulnerability. Browsers could already do this, and MCP isn't really special here.

    The same could be said for any local service running with open ports. If the security model is that nothing external should ever hit the port then it's already risky.

    The user also has to visit a malicious webpage for this to work too, while the server is running.

    You say you are using "the temporary compatibility flag," presumably the debug env var. But your PR is actually using the API as intended, by setting StreamableHTTPOptions.CrossOrigin.CrossOriginProtection.

    Apologies, we changed track after I posted this and decided it was impractical to inject env given the range of ways the server is run.

    In short, why aren't we working as intended, locking down the server as much as possible while allowing fully flexible overrides?

    I not certain this is a position the MCP SDK itself should be taking. I am fine with option to set it, but blocking all valid web clients still feels extreme to me.

  4. SamMorrowDrums commented on Apr 21, 2026

    @SamMorrowDrums
    ContributorAuthor

    I guess another way to put this is, I am not certain the SDK should be adding stuff outside the MCP spec to a transport it has been given without the caller asking it to do so.

  5. jba commented on Apr 21, 2026

    @jba
    Contributor

    Thanks, that was very useful. We should (and must) leave the option and use it when it is provided, but the default should be to do nothing. We will need another MCPGODEBUG option, enableoriginverification, which should last at least one release.

  6. maciej-kisiel commented on Apr 22, 2026

    @maciej-kisiel
    Contributor

    Default enforcement is what actually made me add it to the SDK API. If it's optional, the added benefit is just bringing more attention to the problem, because the users can equally easily do:

    handler := mcp.NewStreamableHTTPHandler(...)
    protection := http.NewCrossOriginProtection()
    // ... configure protection in a desired way.
    protectedHandler = protection.Handler(handler)
    http.ListenAndServe(url, protectedHandler)

    IMO that should be sufficient as long as the security concern is documented. Unfortunately, now the damage is done and we must support the option to pass the protection to the handler, but I will add appropriate paragraph to docs/rough_edges.md to recommend removing it in the next major version of the SDK.

    We should also partially roll back 15e93a2 which adds this protection to the SSE transport as well.

  7. jba commented on Apr 22, 2026

    @jba
    Contributor

    I think you should mark the field as deprecated, and mention the alternative you sketched above.

  8. added a commit that references this issue on Apr 24, 2026
    41e1f94
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions