x11 screen number silently ignored: opts.x11 compared against the string 'number' in exec() and shell()

[bernardartis]

Summary

In both exec() and shell(), the check that decides whether to send an x11-req compares opts.x11 against the string 'number' rather than using typeof. As a result, passing a numeric screen number — which the TypeScript declarations document as supported — silently does nothing: no x11-req is sent, no error is raised, and X11 forwarding simply never happens.

Version: 1.17.0 (latest published). Also present on master at the time of writing, at the same lines.

The code

lib/client.js, in exec() at 1235-1238 and identically in shell() at 1296-1299:

if ((typeof opts.x11 === 'object' && opts.x11 !== null)
    || opts.x11 === 'number'          // <-- compares against the string "number"
    || opts.x11 === true) {
  todo.push(() => reqX11(chan, opts.x11, reqCb));
}

opts.x11 === 'number' is true only when the caller literally passes the string 'number'. A real screen number such as 0 or 1 falls through all three branches, so reqX11 is never queued.

Why this looks supported

@types/ssh2 declares it as accepted, in both option types:

export interface ExecOptions {
    /** Set either to `true` to use defaults, a number to specify a specific screen number, or an object containing x11 settings. */
    x11?: X11Options | number | boolean;
}

export interface ShellOptions {
    /** Set either to `true` to use defaults, a number to specify a specific screen number, or an object containing x11 settings. */
    x11?: X11Options | number | boolean;
}

So a caller following the types passes a number, gets no error, and no forwarding.

Reproduce

Against any server with X11Forwarding yes:

// Silently no-ops: no x11-req is sent.
conn.shell({ x11: 0 }, (err, stream) => { /* remote DISPLAY is unset */ });

// Works.
conn.shell({ x11: true }, (err, stream) => { /* … */ });
conn.shell({ x11: { screen: 0 } }, (err, stream) => { /* … */ });

The difference is observable on the remote side: echo $DISPLAY is empty in the first case and set in the other two.

Suggested fix

if ((typeof opts.x11 === 'object' && opts.x11 !== null)
    || typeof opts.x11 === 'number'
    || opts.x11 === true) {

reqX11 already ignores a bare number — it only reads single, screen, protocol and cookie from an object — so a caller passing a number would get defaults rather than their requested screen. If the intent is that numbers were never meant to be supported, the fix would instead be to drop number from the two type declarations; either way the code and the types currently disagree.

Related, and possibly documentation rather than a bug

shell() reassigns its arguments at 1259-1262:

if (wndopts && (wndopts.x11 !== undefined || wndopts.env !== undefined)) {
  opts = wndopts;
  wndopts = undefined;
}

This is presumably how shell(opts, cb) is disambiguated from shell(wndopts, opts, cb). The consequence is that putting x11 (or env) into the window options object silently discards term, rows and cols, and the session gets a default PTY instead.

Measured against a real sshd:

shell({ term, cols: 203, rows: 47 }, cb)                      -> TERM=xterm-256color  47x203
shell({ term, cols: 203, rows: 47, x11: {...} }, cb)          -> TERM=vt100           24x80
shell({ term, cols: 203, rows: 47 }, { x11: {...} }, cb)      -> TERM=xterm-256color  47x203

The three-argument form is correct and works. This is easy to get wrong when adding X11 to an existing shell() call that already passes window options, and the silent downgrade of the PTY is hard to attribute. A note in the docs would probably be enough.

Happy to open the type-declarations side separately against DefinitelyTyped if that's preferred.