Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
379 changes: 0 additions & 379 deletions NOTICE.md

Large diffs are not rendered by default.

15 changes: 1 addition & 14 deletions lib/server-init.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,9 @@
// app.handle mutates req.url before morgan reads req.originalUrl.

require('./core/log');
var crypto = require('crypto');
var fs = require('fs');
var url = require('url');
var compression = require('compression');
var client_sessions = require('client-sessions');
var express = require('express');
var morgan = require('morgan');
var Q = require('q');
Expand Down Expand Up @@ -109,10 +107,6 @@ function createServer_p(configFilePath, options) {
var compressionMiddleware = compression();
let useCompression = true;

var clientSessionMiddleware = client_sessions({
secret: crypto.randomBytes(16).toString('hex')
});

// Setup a placeholder middleware function until we can create one after
// parsing the config.
var sockjsServer = false;
Expand All @@ -132,7 +126,6 @@ function createServer_p(configFilePath, options) {
else
next();
});
app.use(clientSessionMiddleware);
app.use(function(req, res, next) {
if (!sockjsHandler(req, res))
next();
Expand Down Expand Up @@ -202,13 +195,7 @@ function createServer_p(configFilePath, options) {
res.end();
return;
}
// KNOWN DEFECT (characterized, not yet fixed): passing null for `res`
// makes client-sessions throw internally and call back on nextTick with an
// error this callback ignores, so req.session is never defined on the
// upgrade path. See memory-bank/requestLifecycle.md §7.
clientSessionMiddleware(request, null, function() {
sockjsHandler.upgrade(request, socket, head);
});
sockjsHandler.upgrade(request, socket, head);
});

var requestLogger = null;
Expand Down
27 changes: 18 additions & 9 deletions memory-bank/proxyLayer.md
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ What actually crosses the proxy boundary, verified rather than assumed:
| `connection: close` | to worker | forced by http-proxy (agent=false) |
| `x-frame-options` | to browser | `lib/proxy/http.js:257`, from `appDefaults.frameOptions` |
| `X-Powered-By: <serverName>` | to browser | `lib/server-init.js` (express's own is disabled) |
| `session_state` cookie | to browser | `client-sessions` middleware, `lib/server-init.js,170` |

**There are no `X-Forwarded-*` headers.** `http-proxy` only adds them when
`options.xfwd` is set (`passes/web-incoming.js:72`), and Shiny Server never sets it.
Expand All @@ -146,14 +145,24 @@ spawned it. There is no `Shiny-Server-*` header — the equivalent information
to the worker once at spawn time as JSON on stdin (`lib/worker/app-worker.ts:560-592`),
and the R side injects it into the page for `shiny-server-client` to read.

**Surprise:** the `client-sessions` middleware is applied to upgrade requests as
`clientSessionMiddleware(request, null, cb)` (`lib/server-init.js`). With `res === null`
the `Session` constructor throws on `res.socket`
(`node_modules/client-sessions/lib/client-sessions.js:378`), the library catches it and
calls `next(err)` on `process.nextTick`, and `lib/main.js` ignores the error argument.
Verified empirically. So WebSocket upgrades work, but only by accident, and one tick
later than they look. Also, nothing in `lib/` ever *reads* `req.session_state`; the
middleware appears vestigial.
**Sets no cookies at all.** Shiny Server's own middleware stack never calls
`res.setHeader('set-cookie', ...)`, and there is no session middleware left (see
below). Client cookies are forwarded to the worker verbatim, but nothing is minted
here. Verified against a live `/r-hello/` response.

**Removed: `client-sessions`.** Until 2026-09 a `client-sessions` middleware sat in
the stack and was also applied to upgrade requests as
`clientSessionMiddleware(request, null, cb)`. It was fully vestigial and has been
deleted: nothing in `lib/`, `test/`, `R/`, or `assets/` ever read `req.session_state`,
and because the lazy `Session.content` getter was never touched the session stayed
`loaded === false` / `dirty === false`, so `updateCookie()` was a no-op and *no
`session_state` cookie was ever emitted* (verified empirically). On the upgrade path
`res === null` made the `Session` constructor throw on `res.socket`; the library caught
it and called `next(err)` on `process.nextTick`, and the callback ignored the argument
— so upgrades worked, one tick late, with no session. It was added in `280b075`
(Apr 2013) as infrastructure for Shiny Server Pro's auth layer, which never shipped in
open source. The upgrade handler now calls `sockjsHandler.upgrade()` directly. Do not
reintroduce it looking for a session cookie that never existed.

## Connection accounting (get this right)

Expand Down
36 changes: 18 additions & 18 deletions memory-bank/requestLifecycle.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ sockjsServer = proxy_sockjs.createServer(metarouter, schedulerRegistry, ...) (R

Server (facade) ──> N x http.Server, one per unique listen address
'request' -> app.handle (express) AND -> requestLogger (morgan)
'upgrade' -> clientSessionMiddleware -> sockjsHandler.upgrade
'upgrade' -> sockjsHandler.upgrade
```

Two seams make reload possible without rebuilding the world:
Expand Down Expand Up @@ -127,10 +127,14 @@ Installed in this exact order (`lib/server-init.js`):
|---|---|---|
| 1 | `X-Powered-By: Shiny Server` | Express's own header is disabled at `:159` first. |
| 2 | conditional `compression()` | Guarded by the mutable `useCompression` flag, so `http_allow_compression` is honored per-request after a reload. |
| 3 | `client-sessions` | Random per-process secret (`lib/server-init.js`). |
| 4 | `sockjsHandler` | `if (!sockjsHandler(req,res)) next()`. |
| 5 | `__assets__` filter | `connect_util.filterByRegex(/\b__assets__\/.+/, ...)`. |
| 6 | `shinyProxy.httpListener` | Terminal — never calls `next()`. |
| 3 | `sockjsHandler` | `if (!sockjsHandler(req,res)) next()`. |
| 4 | `__assets__` filter | `connect_util.filterByRegex(/\b__assets__\/.+/, ...)`. |
| 5 | `shinyProxy.httpListener` | Terminal — never calls `next()`. |

There is **no session middleware**. A `client-sessions` entry sat at position 3 until
2026-09; it was entirely vestigial and was removed. See
`memory-bank/proxyLayer.md` for the full autopsy — in particular, it never emitted a
cookie, so don't go looking for one.

Why the order matters:

Expand All @@ -142,9 +146,6 @@ Why the order matters:
middlewares must therefore run before the proxy, and the assets regex is
deliberately unanchored (`\b__assets__\/`) with everything up to and including
`__assets__/` stripped from `req.url` at `lib/server-init.js`.
- **`client-sessions` before SockJS.** The session cookie is established before
SockJS transport requests are handled. (In practice nothing in `lib/` ever
reads `req.session`; the middleware looks vestigial.)
- **The proxy is terminal.** `httpListener(req, res)` takes no `next`. Every
path through it either responds (404/500/503) or hands off to `http-proxy`.
There is no error-handling middleware anywhere in the app, so Express's
Expand Down Expand Up @@ -211,15 +212,15 @@ Shiny Server ⇄ worker (a plain WebSocket via `faye-websocket`).

**HTTP-based SockJS transports** (xhr-polling, xhr-streaming, jsonp, eventsource,
htmlfile, plus `/info` and the iframe/welcome pages) arrive as ordinary requests
and are claimed by middleware #4. The SockJS prefix is
and are claimed by middleware #3. The SockJS prefix is
`'.*/__sockjs__(/[no]=\\w+)?'` (`lib/proxy/sockjs.js:42`) — the leading `.*`
is what lets a single SockJS server serve every app prefix, and the optional
`/n=` / `/o=` path param is the robust-reconnect session id consumed by
`lib/proxy/robust-sockjs.js`.

**WebSocket upgrades** bypass Express entirely — Express only handles
`'request'`. `lib/server-init.js` handles `'upgrade'` on the `Server` facade,
manually running `clientSessionMiddleware` and then `sockjsHandler.upgrade`.
`'request'`. `lib/server-init.js` handles `'upgrade'` on the `Server` facade by
calling `sockjsHandler.upgrade` directly.

Once SockJS produces a connection (`lib/proxy/sockjs.js:49-62`) it goes through
two wrappers before routing:
Expand Down Expand Up @@ -300,7 +301,7 @@ logger; `socketTimeout`; `useCompression`; the set of bound listeners.
Preserved: the event bus; the entire router decorator chain and
`LocalConfigRouter`'s `AppConfig` cache; the `SchedulerRegistry` **and every
running worker process**; the transport; the Express app and its middleware
instances; the `client-sessions` secret; and any `http.Server` whose
instances; and any `http.Server` whose
address/port is unchanged (`Server.setAddresses` diffs by
`http://<host>:<port>` key and only opens/closes the delta,
`lib/server/server.js`). Existing connections on a *removed* listener are
Expand Down Expand Up @@ -355,12 +356,11 @@ the `128+signal` convention.
(`lib/server-init.js`, marked `KNOWN DEFECT` in place) — a `ReferenceError` if
an upgrade arrives before the config finishes loading. Characterized but
deliberately not fixed yet.
- **`clientSessionMiddleware(request, null, cb)`** on the upgrade path
(`lib/server-init.js`, also marked `KNOWN DEFECT`) passes `null` for `res`. `client-sessions` dereferences
`res.socket`, throws inside its `try`, and calls `next(err)` on `nextTick`.
The callback ignores its argument, so upgrades still work — one tick later,
with no `req.session` defined. Verified against
`node_modules/client-sessions/lib/client-sessions.js:355-383,600-630`.
- **FIXED (2026-09): the `client-sessions`-on-upgrade swallow.** The middleware
was removed outright rather than repaired, because it was vestigial; the
upgrade handler now calls `sockjsHandler.upgrade()` directly and synchronously.
See `memory-bank/proxyLayer.md`. The pre-config `res` defect above is
unrelated and still open.
- **`server.listening` is read-only.** `lib/server/server.js` used to assign
`server.listening = true/false`; that was a silent no-op, because
`net.Server.prototype.listening` is a getter with no setter, so in sloppy mode
Expand Down
2 changes: 1 addition & 1 deletion memory-bank/techContext.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ From `package.json` `dependencies`:

| Role | Packages |
| --- | --- |
| HTTP framework / middleware | `express` (v5), `compression`, `morgan`, `client-sessions`, `send`, `mime-types`, `qs`, `pause` |
| HTTP framework / middleware | `express` (v5), `compression`, `morgan`, `send`, `mime-types`, `qs`, `pause` |
| Proxying | `http-proxy-3` |
| WebSocket / SockJS | `faye-websocket`, `sockjs` (server), `sockjs-client` (served to browsers), `shiny-server-client` |
| CLI / config | `optimist` (argv parsing), `ip-address` (config validation) |
Expand Down
42 changes: 1 addition & 41 deletions npm-shrinkwrap.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
},
"dependencies": {
"bash": "0.0.1",
"client-sessions": "^0.8.0",
"compression": "^1.8.1",
"express": "^5.2.1",
"faye-websocket": "^0.11.4",
Expand Down
Loading