Skip to content
Merged
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
57 changes: 53 additions & 4 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,27 @@ infinite-loops on connection errors against this server.
- No new npm dependencies without a good reason. Current set: `ws`, `commander`,
`chokidar`, `diff`.

### Commit and PR titles

Conventional commits, enforced on the **PR title** by `.github/workflows/pr-title.yml`
— the title becomes the squashed commit message and release-please derives the next
version from it, so a title that does not parse is a release that silently never
happens.

Type is one of `feat`, `fix`, `perf`, `refactor`, `docs`, `test`, `build`, `ci`,
`chore`, `revert`. A scope is optional, **but any scope used must be one of**:

```
sync auth watch logs json types cli deps docs release main
```

That list is closed on purpose, to keep the vocabulary small enough to mean something.
A new component does **not** earn a new scope — `feat(doctor)` fails the check, and a
new CLI command belongs under `cli`. If a scope genuinely has to be added, add it to
the workflow in the same PR. (`main` is not a component: it is the branch name in
release-please's own `chore(main): release x.y.z` title, and without it the release PR
cannot merge.) Subjects must not end with a full stop.

## Commands

```bash
Expand All @@ -134,7 +155,8 @@ Directly tested: `client/socket`, `client/objects`, `client/auth` (HTTP and HTTP
`config`, `credentials`, `sync/compare`, `sync/mapping`, `sync/safe-path` (path
traversal, symlink-file writes, symlinked-directory writes), the `--json` record shapes,
and the commands `pull`, `push`, `status`, `diff` (including `--against`), `watch`,
`logs`, `list`, `start`, `stop`, `restart`, `new`, `rename`, `move`, `remove`, `backup`.
`logs`, `list`, `start`, `stop`, `restart`, `new`, `rename`, `move`, `remove`, `backup`,
`doctor`.
`sync/manifest` and `sync/scan` are covered indirectly by every pull/push test.

`test/cli.test.ts` spawns the built `dist/cli.js` and asserts on real argv handling.
Expand All @@ -146,7 +168,8 @@ there, and it needs `dist/` built first.**
`test/fake-server.ts` serves HTTP and websocket on one port, as real Admin does on 8081.
Its `auth` field selects how the HTTP side answers the probes in `client/auth.ts`
(`disabled` → `GET /login` 404s, `oauth` → `POST /oauth/token`, `legacy` → `POST /login`);
`reset()` returns it to `disabled`, so tests that do not care are unaffected.
`reset()` returns it to `disabled`, so tests that do not care are unaffected. Its
`requireCookieOnSocket` flag reproduces the silent-auth failure below.

`pull` also skips-and-continues per script rather than aborting the whole run: one
unwritable script (a bad id, a symlink in the way) is reported and the rest still
Expand Down Expand Up @@ -207,6 +230,27 @@ always `!allowSelfSigned` — the value is the user's decision, not a constant,
writing it as one both misreports what the code does and trips CodeQL's
`js/disabling-certificate-validation`.

### Two failures that look like a broken instance and are not

Both cost a session an hour, and `commands/doctor.ts` exists because of them.

1. **An unauthenticated socket is silent, not angry.** Admin accepts the connection,
sends `___ready___`, and then ignores every command. There is no auth error and no
close — requests simply never come back, and the only thing the caller ever sees is
`Request "..." timed out`. The timeout in `client/socket.ts` therefore carries a
hint naming this cause; `test/fake-server.ts` reproduces it via
`requireCookieOnSocket`.
2. **An expired self-signed certificate is harmless here.** With `allowSelfSigned` the
identity check is the pinned fingerprint, not the chain, so `iob-sync` keeps working
after expiry while every other client on that port fails with
`certificate has expired`. `doctor` reports it as OK plus a note. Do not "fix" this
by tightening the TLS path — the pin is the check, and it is stricter than the chain.

**`iob-sync` is a CLI, not a library.** `package.json` exposes `bin` only; there is no
`exports` map and nothing under `src/` is a supported import. Anything constructing
`AdminSocketClient` directly must reproduce `withContext` in `cli.ts` — certificate
check, then cookie, then socket — and the failure mode when it does not is (1) above.

### Two bugs the watch tests caught

Both were live in working code, and both are the kind that only show up under a test
Expand All @@ -224,8 +268,6 @@ testable at all, which matters given a regression there means an infinite push l

## Other known gaps

- Self-signed certificates are honoured on the websocket path but not on the HTTP
auth path (would require an `undici` Agent).
- `init --types` writes the ioBroker type definitions, but the download of
`javascript.d.ts` from GitHub has only been exercised against a live network.

Expand Down Expand Up @@ -296,3 +338,10 @@ window so it measures the debounce rather than the disk.
If it recurs, that file is the suspect and the fix is more timing margin — **not**
loosening an assertion. The thing being tested is the guard against an infinite push
loop against someone's house.

One of these turned out not to be timing at all. The `--pull` case waited for the file
content to appear and then asserted on the log line, but `applyRemote` writes the file,
saves the manifest and logs **last** — so the assertions could run inside that window.
It now waits for the `pull` line, which is the operation's real completion signal, and
asserts on the file and manifest afterwards. Before assuming load, check what the code
under test does in what order.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ If `logs` shows nothing, that is usually the adapter's own log level rather than
| `types` | Set up editor intellisense (`log`, `schedule`, ...). `--force`, `--offline`. |
| `login` / `logout` | Save or remove the password for this instance. Never stored in the project. |
| `trust` | Accept the instance's current TLS certificate. Only needed after it changes. `--yes` skips the prompt. |
| `doctor` | Check config, certificate, login, connection and a live round-trip, and say which one is wrong. Read-only, never prompts. Run this first when something looks broken. |
| `pull [pattern]` | Download scripts to disk. Never deletes or overwrites local files. |
| `push [pattern]` | Upload locally modified scripts. Never deletes remote objects. |
| `status` | Show what changed, locally and remotely. |
Expand Down
37 changes: 37 additions & 0 deletions docs/TROUBLESHOOTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,43 @@

Things that look like bugs and are not. See the [README](../README.md) for the overview.

**Start with `iob-sync doctor`.** It checks the config, the certificate, the login, the
connection and a live round-trip, and names the one that is wrong — including the two
cases below, which are the ones that reliably send people down the wrong path. It is
read-only and never prompts, so it is safe to run at any time.

## Commands time out, but the connection "works"

Symptom: `___ready___` arrives, the socket reports connected, and then every request
fails with `Request "getObject" timed out after 20000ms.`

The session is not authenticated. ioBroker Admin does not answer an unauthenticated
command with an error — it accepts the connection, sends `___ready___`, and then
ignores the command entirely. There is nothing to find in the log, because nothing
went wrong at the transport layer.

Usually this means the stored password is stale (`iob-sync login` replaces it) or the
session expired during a long-running `watch`. It also happens to anyone driving
`AdminSocketClient` from their own code without an auth cookie: **iob-sync is a CLI,
not a library** — there is no supported import path, and the wiring the commands rely
on (certificate check, then cookie, then socket) lives in `withContext` in
`src/cli.ts`. A client constructed without `cookie`, `allowSelfSigned` and
`certFingerprint` connects perfectly and then does nothing.

## `certificate has expired` from other tools on the same port

`iob-sync` keeps working while every other client refuses to connect. Both are correct.

A home ioBroker signs its own certificate, typically for one year, and nothing renews
it. With `allowSelfSigned` the chain is not what establishes identity here — the pinned
SHA-256 fingerprint is (see [AUTHENTICATION.md](AUTHENTICATION.md)) — and an expired
certificate signs exactly as well as a fresh one. Anything validating the chain the
normal way rejects it.

`iob-sync doctor` reports this as OK with a note rather than as a fault. To make the
other tools happy, regenerate the certificate on the instance and then run
`iob-sync trust` to accept the new fingerprint.

## `logs` prints the banner and nothing else

It is streaming; there is simply nothing to show. Two things surprise people:
Expand Down
15 changes: 15 additions & 0 deletions src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { move } from './commands/move';
import { remove } from './commands/remove';
import { login, logout } from './commands/login';
import { trust } from './commands/trust';
import { doctor } from './commands/doctor';
import { setupTypes } from './commands/types';

/**
Expand Down Expand Up @@ -292,6 +293,20 @@ program
})();
});

program
.command('doctor')
.description('check that iob-sync can reach and authenticate to the instance')
.option('--timeout <ms>', 'budget for the connect and round-trip probes', '8000')
.action(function (this: Command) {
const opts = this.opts();
return action(async () => {
// Not withContext: that connects first and aborts on the first failure, which
// is precisely the information this command is here to report.
const { root, config } = await loadConfig(resolveCwd());
await doctor(root, config, { timeoutMs: Number(opts.timeout) || undefined }, logger);
})();
});

program
.command('logout')
.description('remove the stored password for this instance')
Expand Down
22 changes: 21 additions & 1 deletion src/client/socket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,21 @@ const DEFAULT_CONNECT_TIMEOUT_MS = 15000;
const DEFAULT_REQUEST_TIMEOUT_MS = 20000;
const CLIENT_NAME = 'iobroker-sync';

/**
* Attached to every request timeout, because this is the only thing an unauthenticated
* caller ever sees.
*
* Admin accepts the connection and sends `___ready___` whether or not a session cookie
* came with it, and then simply ignores commands — no auth error, no close. A bare
* "timed out" therefore reads as a slow or broken instance, which sends people
* debugging the wrong thing entirely. Naming the likely cause here costs one line.
*/
const TIMEOUT_HINT =
'A socket that is open but ignores commands is what an unauthenticated or expired ' +
'session looks like — Admin sends no auth error. Run `iob-sync doctor`. If you are ' +
'driving AdminSocketClient yourself, it needs `cookie`, `allowSelfSigned` and ' +
'`certFingerprint` — see `withContext` in src/cli.ts.';

type Frame = [number, number | null, string?, unknown?];

/**
Expand Down Expand Up @@ -289,7 +304,12 @@ export class AdminSocketClient implements SocketClient {
return new Promise<T>((resolve, reject) => {
const timer = setTimeout(() => {
this.pending.delete(id);
reject(new UserError(`Request "${command}" timed out after ${this.requestTimeoutMs}ms.`));
reject(
new UserError(
`Request "${command}" timed out after ${this.requestTimeoutMs}ms.`,
TIMEOUT_HINT,
),
);
}, this.requestTimeoutMs);

this.pending.set(id, {
Expand Down
55 changes: 51 additions & 4 deletions src/client/tls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,54 @@ function hostAndPort(url: string): { host: string; port: number } {
return { host: parsed.hostname, port: Number(parsed.port) || 443 };
}

/**
* What the server's certificate says about itself.
*
* The fingerprint is the only field the pin depends on. The rest exists for
* `iob-sync doctor`, which has to be able to say *why* a certificate is or is not a
* problem — "expired seven months ago, and that is fine here" is a sentence nobody
* can write from a fingerprint alone.
*/
export interface CertificateInfo {
/** SHA-256 fingerprint, colon-separated uppercase hex. */
fingerprint: string;
/** Distinguished name, flattened for display, e.g. `CN=iobroker`. */
subject: string;
issuer: string;
/** Undefined when the certificate carries a date Node could not parse. */
validFrom?: Date;
validTo?: Date;
}

/** `{ CN: 'iobroker', O: 'ioBroker' }` -> `CN=iobroker, O=ioBroker`. */
function formatDn(dn: tls.PeerCertificate['subject'] | undefined): string {
if (!dn || typeof dn !== 'object') return '';
return Object.entries(dn)
.map(([key, value]) => `${key}=${String(value)}`)
.join(', ');
}

function parseCertDate(raw: string | undefined): Date | undefined {
if (!raw) return undefined;
const parsed = new Date(raw);
return Number.isNaN(parsed.getTime()) ? undefined : parsed;
}

/**
* Opens a TLS connection purely to read the certificate, then closes it.
*
* Nothing is written to the socket. That is the whole point: this runs before the
* password is sent, so a certificate the user ends up rejecting never sees it.
*/
export function probeCertificate(url: string, allowSelfSigned: boolean): Promise<string> {
export async function probeCertificate(url: string, allowSelfSigned: boolean): Promise<string> {
return (await probeCertificateInfo(url, allowSelfSigned)).fingerprint;
}

/** As `probeCertificate`, but keeps the fields the fingerprint alone cannot explain. */
export function probeCertificateInfo(
url: string,
allowSelfSigned: boolean,
): Promise<CertificateInfo> {
return new Promise((resolve, reject) => {
let target: { host: string; port: number };
try {
Expand All @@ -104,11 +145,11 @@ export function probeCertificate(url: string, allowSelfSigned: boolean): Promise
// The handshake, the timeout and the error handler all race to end this; whichever
// gets there first closes the socket and the rest become no-ops.
let settled = false;
const succeed = (fingerprint: string): void => {
const succeed = (info: CertificateInfo): void => {
if (settled) return;
settled = true;
socket.destroy();
resolve(fingerprint);
resolve(info);
};
const fail = (err: Error): void => {
if (settled) return;
Expand Down Expand Up @@ -138,7 +179,13 @@ export function probeCertificate(url: string, allowSelfSigned: boolean): Promise
);
return;
}
succeed(fingerprint.toUpperCase());
succeed({
fingerprint: fingerprint.toUpperCase(),
subject: formatDn(cert.subject),
issuer: formatDn(cert.issuer),
validFrom: parseCertDate(cert.valid_from),
validTo: parseCertDate(cert.valid_to),
});
},
);

Expand Down
Loading