Skip to content

Harden CORS, handshake origin handling and polling input validation - #218

Open
devin-ai-integration[bot] wants to merge 5 commits into
mainfrom
devin/1784979916-security-hardening
Open

Harden CORS, handshake origin handling and polling input validation#218
devin-ai-integration[bot] wants to merge 5 commits into
mainfrom
devin/1784979916-security-hardening

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Jul 25, 2026

Copy link
Copy Markdown

Description

Security hardening found during a scan of the codebase. The main issue: EncoderHandler.addOriginHeaders reflected any request Origin back in Access-Control-Allow-Origin together with Access-Control-Allow-Credentials: true (default enableCors=true), so any website could read a victim's authenticated polling responses cross-origin. Credentials are now only granted to explicitly configured origins, and a new allowedOrigins allow list is also enforced at handshake time (covering websocket, which is not subject to CORS at all).

// before: any origin, with credentials
ACAO: <request Origin>; ACAC: true

// after
origin configured        -> ACAO: <configured>;      ACAC: true
allowedOrigins matches   -> ACAO: <request Origin>;  ACAC: true
allowedOrigins non-empty -> no CORS headers, handshake rejected with 403
otherwise (default)      -> ACAO: <request Origin>;  no ACAC
no Origin header         -> ACAO: *

allowedOrigins entries are matched against the full origin and may contain * wildcards:

config.setAllowedOrigins(new HashSet<>(Arrays.asList(
        "https://app.example.com",   // exact
        "https://*.example.com",     // any subdomain
        "http://localhost:*")));     // any port

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related Issue

N/A

Changes Made

  • BasicConfiguration: new allowedOrigins set + isOriginAllowed() with * wildcard patterns (a wildcard never spans /, so it cannot escape the authority), copied in the copy constructor (so the Spring Boot / Micronaut property binding gets allowed-origins for free)
  • EncoderHandler: credentials only for configured/allow-listed origins, no CORS headers for disallowed origins, Vary: Origin added
  • AuthorizeHandler: reject handshakes from origins outside a non-empty allow list with 403; regenerate the session id when a client supplies (via the io header/cookie) one already in use, preventing takeover of a live session; redact Cookie/Authorization/Proxy-Authorization/X-Api-Key from handshake debug logs
  • PollingTransport: validate j, b64 and sid query params (previously NumberFormatException/IllegalArgumentException escaped to the pipeline) and null-check the client on ?disconnect (previously an NPE for an unknown sid)

Also scanned for, and did not find: hardcoded credentials in main sources, SQL/command injection (no SQL or process execution), unsafe Jackson polymorphic typing (Kafka/NATS codecs use a restrictive PolymorphicTypeValidator), or outdated dependencies with known CVEs.

Testing

  • All existing tests pass
  • New tests added for new functionality (AllowedOriginsTest, CORS cases in EncoderHandlerTest)
  • Tests pass locally with mvn test (netty-socketio-core: 711 tests, 0 failures; 4 pre-existing flaky distributed tests passed on retry)
  • Integration tests pass (if applicable)
  • Runtime verification against real SocketIOServer instances — see the test results comment

Checklist

  • Code follows project coding standards
  • Self-review completed
  • Code is commented where necessary
  • Documentation updated (if needed)
  • No merge conflicts
  • All CI checks pass

Additional Notes

Behaviour change to be aware of: cookie-authenticated cross-origin clients (withCredentials) will stop working until their origin is added to origin or the new allowedOrigins — that is the point of the fix, since the previous default granted credentials to every origin. Clients that don't send an Origin header (server-to-server, mobile, CLI) are never rejected. One EncoderHandlerTest assertion was updated because it asserted the vulnerable behaviour.

Link to Devin session: https://app.devin.ai/sessions/8e4e71040eda4df18038140a44f22f06
Requested by: @sanjomo

@sanjomo sanjomo self-assigned this Jul 25, 2026
@devin-ai-integration

Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration

Copy link
Copy Markdown
Author

Runtime verification — CORS / handshake origin / polling input hardening

Tested by starting three real SocketIOServer instances on localhost (default config, setAllowedOrigins({"http://allowed.example"}), setOrigin("http://x")) and driving them with raw HTTP + raw websocket upgrades and the real socket.io-client. All 14 checks passed.

CORS / origin enforcement
--- default config, Origin: http://evil.example
HTTP/1.1 200 OK
access-control-allow-origin: http://evil.example
vary: origin
(no access-control-allow-credentials)

--- default config, no Origin header
access-control-allow-origin: *

--- allowedOrigins={http://allowed.example}, Origin: http://allowed.example
HTTP/1.1 200 OK
access-control-allow-origin: http://allowed.example
access-control-allow-credentials: true
vary: origin
0{"sid":"c3ef6c8c-…","upgrades":["websocket"],…}

--- allowedOrigins={…}, Origin: http://evil.example
HTTP/1.1 403 Forbidden        (no CORS headers at all)

--- websocket upgrade, Origin: http://evil.example    -> HTTP/1.1 403 Forbidden
--- websocket upgrade, Origin: http://allowed.example -> HTTP/1.1 101 Switching Protocols + 0{"sid":…}

--- setOrigin("http://x"), Origin: http://evil.example
access-control-allow-origin: http://x
access-control-allow-credentials: true
Polling input validation
&j=abc                        -> 400 Bad Request
&b64=xyz                      -> 400 Bad Request
&sid=notauuid                 -> 400 Bad Request
&sid=<random uuid>&disconnect -> 500 Internal Server Error (no NPE / no crash)
plain handshake afterwards    -> 200 OK + sid   (server still healthy)

Nit: the ?disconnect-with-unknown-sid case returns 500; 400/404 would be semantically more accurate.

Session-id reuse, log redaction, regression
handshake with Cookie: io=<sid of a live client>
  firstSid=c2014c1a-894d-4a00-9f22-2b3d753d6520
  secondSid=4b6a93b4-1f34-4038-bd55-6f00140da2ff   sidReused=false

AuthorizeHandler debug log: headers: {Cookie=[[redacted]], Connection=[close], …}

Regression (default config, real socket.io-client):
  connected=true echoReceived=true payload=hello-world

@sanjomo
sanjomo requested review from NeatGuyCoding and sanjomo and removed request for NeatGuyCoding July 25, 2026 17:37

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

Open in Devin Review

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.

1 participant