fix(console): stop a malformed cookie killing the console gateway - #96
Merged
ralyodio merged 1 commit intoJul 30, 2026
Merged
Conversation
parseCookies ran decodeURIComponent on the raw cookie value, which throws a URIError on a stray "%". It is called before any auth check, from two places whose throw is fatal: the async request handler (unhandled rejection) and the `upgrade` listener (uncaught exception). So an unauthenticated client could end the gateway process with one header. Fall back to the raw value when a cookie will not decode, the way the `cookie` package does. The gate itself is untouched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
moshcode consoleputs an authenticating gateway in front of a real shell. An unauthenticated client can stop that gateway answering with a single header.The bug
parseCookiesdecodes each cookie value withdecodeURIComponent, which throwsURIError: URI malformedon a stray%:A cookie value is whatever the client chose to send, and this runs before any auth check, from two places where a throw is fatal:
upgradelistener, where it is an uncaught exceptionNo token, no session, no valid cookie name needed.
Cookie: sid=%is enough.Reproducing it
Against unmodified
main(1b8af96), gateway in a child process so the exit is visible:The process is gone, so every other user of that gateway loses their terminal too, and
moshcode console servestays down until someone restarts it by hand.There is a milder version that bites without an attacker: a browser sends every cookie set for the host, so one unrelated cookie holding a literal
%costs you the gateway on an ordinary page load.The fix
Fall back to the raw value when a cookie will not decode. This is what the
cookiepackage does, so it matches what express would have given you if the upgrade path could have used it.The gate itself is untouched: a value that fails to decode still has to survive
readCookie's HMAC check to authenticate anything, and an undecodable value will not.test/console.test.mjsalready asserts "malformed cookies are rejected rather than throwing" forreadCookie. This extends the same rule to the layer that runs first.Tests
New
test/console-cookie-malformed.test.mjs, 10 tests.Five are the bug. They fail before and pass after:
%, a literal100%, and a truncated escape all parse instead of throwingsid=%gets a 401 and raises no process-level errorFive are controls that pass both ways, so the fix cannot buy crash safety by weakening the gate: well-formed values are still percent-decoded, no cookie is still 401 on both paths, a forged cookie is still 401, an expired cookie is still 401, and a valid session still reaches the proxy.
The tests record
unhandledRejectionanduncaughtExceptionrather than letting them fly, because otherwise the unpatched run takes the whole file down and you cannot tell which test failed.Suite: 365 -> 375, 0 failing.
Scoped out deliberately
Two things I noticed in the same file and did not put in here, since they are judgement calls rather than defects:
proxyUpgradedoes not checkOrigin. Cross-site WebSocket hijacking is the classic risk for a cookie-authenticated websocket, andSameSite=Laxhas historically not been applied to websocket handshakes in every browser. A fix means deciding which origins are allowed, which is your call, not mine.Secure. That is right for the documented loopback and tailnet setups and wrong behind a TLS reverse proxy, so it probably wants to follow a flag rather than be hardcoded.Happy to send either as its own PR if you want them.