Add openHabTrigger node#5
Conversation
0a3be2b to
46eca56
Compare
Including tests against a real WebSocket server. Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
There was a problem hiding this comment.
Pull request overview
Adds an openHAB Trigger node to the n8n community package and introduces a small internal WebSocket client (plus Jest-based unit tests) to avoid runtime dependencies while supporting custom headers and optional insecure TLS for local setups.
Changes:
- Added
openHABTriggernode to listen to openHAB event bus via WebSocket and emit incoming events into workflows. - Introduced a custom WebSocket client (
util/ws.ts) with unit tests and Jest configuration. - Updated build/lint/TS configs and packaging metadata to include the new node and util code.
Reviewed changes
Copilot reviewed 10 out of 15 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
util/ws.ts |
New dependency-free WebSocket client used by the trigger node (handshake, framing, ping/pong). |
util/ws.test.ts |
Jest tests for the WebSocket client (plain + TLS, headers, protocols, ping/pong). |
util/openHABApi.ts |
Shared helper to validate credentials/build base URL/source used by trigger and future code. |
nodes/openHABTrigger/openHABTrigger.node.ts |
New trigger node implementation subscribing to openHAB events and applying filters/heartbeat. |
nodes/openHABTrigger/openhab.svg |
Icon asset for the new trigger node. |
package.json |
Registers the trigger node, adds Jest tooling deps, and updates scripts (including asset copying). |
jest.config.js |
Adds Jest/ts-jest configuration and coverage output directory. |
tsconfig.json / tsconfig.eslint.json |
Includes util/**, adjusts excludes for build vs lint typechecking. |
eslint.config.mjs |
Switches ESLint project file to tsconfig.eslint.json and ignores coverage output. |
index.ts |
Exports the new node from the package entrypoint. |
README.md |
Documents the new trigger node usage and output schema. |
.gitignore / .eslintrc.json |
Ignores Jest coverage output; removes legacy ESLint config in favor of flat config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Inject Sec-WebSocket-Protocol if requested | ||
| if (this.options.protocols) { | ||
| const protocolsStr = Array.isArray(this.options.protocols) | ||
| ? this.options.protocols.join(', ') | ||
| : this.options.protocols; | ||
| request += `Sec-WebSocket-Protocol: ${protocolsStr}\r\n`; | ||
| } | ||
|
|
||
| if (this.options.headers) { | ||
| for (const [headerName, headerValue] of Object.entries(this.options.headers)) { | ||
| request += `${headerName}: ${headerValue}\r\n`; | ||
| } | ||
| } |
There was a problem hiding this comment.
The handshake request is built by string-concatenating protocols and headers values directly into the HTTP header block. If any value contains \r/\n, it can inject additional headers or corrupt the request (request smuggling). Reject or sanitize header names/values and subprotocol strings to disallow CR/LF characters before writing the request.
There was a problem hiding this comment.
The WebSocketClient is only used by our own, trusted code. I don't think we have to handle this.
| const encodedAccessToken = accessToken ? Buffer.from(accessToken).toString('base64').replace(/=*$/, '') : null | ||
| const subProtocols = encodedAccessToken ? [`org.openhab.ws.accessToken.base64.${encodedAccessToken}`, 'org.openhab.ws.protocol.default'] : []; | ||
|
|
There was a problem hiding this comment.
Sec-WebSocket-Protocol values are defined as HTTP tokens (RFC6455), so the subprotocol string must not contain characters like / or =. Buffer.from(accessToken).toString('base64') can produce + and / (and you only strip =), which can make the handshake fail on stricter servers/proxies. Consider using base64url encoding for the token part (replace +→-, /→_, and strip =) to keep the subprotocol token RFC-compliant.
There was a problem hiding this comment.
This code works fine in Main UI for quite some time now:
Signed-off-by: Florian Hotze <dev@florianhotze.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 15 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('should emit error on connection failure', (done) => { | ||
| const client = new WebSocketClient(`ws://localhost:1`); | ||
| client.on('error', (err) => { | ||
| expect(err).toBeDefined(); | ||
| done(); | ||
| }); | ||
| client.connect(); | ||
| }); |
There was a problem hiding this comment.
This test assumes ws://localhost:1 will fail, but port 1 could be open/forwarded in some environments, making the test flaky. Prefer connecting to a guaranteed-unused port (e.g., bind a server to port 0 to obtain an ephemeral port, close it, then connect and assert ECONNREFUSED) or mock net.connect to deterministically trigger an error.
| } | ||
| } else { | ||
| const token = credentials.token as string; | ||
| if (!token || !token.trim()) { |
There was a problem hiding this comment.
Credential validation here treats a whitespace-only token as missing (if (!token || !token.trim())), but the existing openHAB node’s request helper only checks if (!token) (see nodes/openHAB/openHAB.node.ts), so the two nodes will behave differently for the same credential edge-case. Consider aligning the validation logic across nodes (or reusing a single shared helper) to avoid inconsistent user experience.
| if (!token || !token.trim()) { | |
| if (!token) { |
There was a problem hiding this comment.
Will be aligned in a follow-up PR.
Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
As n8n requires verified nodes to not have any runtime dependencies, I had to implement my own WebSocket client based on Node.js primitives. Node.js introduced a WebSocket client in Node.js 22, but this one doesn't support ignoring invalid TLS certs, sending additional headers etc.
The WebSocket client and its unit tests were generated with Gemini and reviewed and tested myself.