[18.0][IMP] cetmix_tower_server_terminal: implement interactive SSH terminal sessions#485
[18.0][IMP] cetmix_tower_server_terminal: implement interactive SSH terminal sessions#485francoleyes wants to merge 16 commits intocetmix:18.0from
Conversation
|
|
WalkthroughA new interactive SSH terminal module for Cetmix Tower servers is introduced with a full backend implementation (transient session model, broker daemon, SSH utilities) and xterm.js-based frontend UI. Concurrently, backend asset bundle configuration in the main module is refined to explicitly enumerate assets instead of using broad glob patterns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server/models/cx_tower_server.py`:
- Around line 648-673: In _build_ssh_connection, avoid reassigning self by
calling self.ensure_one() on the original and then assigning a local variable
(e.g., sudoed = self.sudo()) and use that variable for subsequent
calls/attribute access (replace uses of self._get_secret_value,
self._get_ssh_password, self._get_ssh_key, and attributes ip_v4_address,
ip_v6_address, skip_host_key, ssh_port, ssh_username, ssh_auth_mode with
sudoed._get_secret_value, sudoed._get_ssh_password, sudoed._get_ssh_key,
sudoed.ip_v4_address, sudoed.ip_v6_address, sudoed.skip_host_key,
sudoed.ssh_port, sudoed.ssh_username, sudoed.ssh_auth_mode respectively) so
behavior is unchanged but code is clearer and avoids reassigning self.
In `@cetmix_tower_server/models/cx_tower_terminal_session.py`:
- Around line 152-173: The background thread _reader_loop currently calls the
translation function _() when setting state (calls inside set_state("closed",
_("The remote shell was closed.")) and set_state("error", _("Terminal read
error: %(err)s", err=err))) which may lack the user's locale; capture the user
language/translator at session creation (e.g., store self._locale or
self._translate = get_translator_for_user(...)) and use that stored translator
or pre-translated messages inside _reader_loop (or pass already-translated
strings into set_state) so no cross-thread calls to the global _() occur; update
code paths that create the session to populate the stored translator and replace
direct _() calls in _reader_loop with translation via the stored translator or
with precomputed strings.
- Around line 230-264: The _runtime_sessions dict is accessed concurrently and
needs protection: add a class-level threading.Lock named _runtime_sessions_lock
and use it to guard all accesses/modifications (reads, iterations, pops,
assignments) to type(self)._runtime_sessions in methods like
_cleanup_runtime_sessions and _get_runtime_session; wrap the iteration that
builds stale_tokens, the subsequent pop loop, and any other lookup or mutation
in with type(self)._runtime_sessions_lock: blocks so dictionary iteration and
modification are atomic and thread-safe.
In `@cetmix_tower_server/ssh/ssh.py`:
- Around line 340-348: In the receive method, wrap the loop that calls
channel.recv (guarded by channel.recv_ready()) in a try-except catching
socket.timeout so transient non-blocking timeouts don't raise; update the
receive function to import socket if needed, catch socket.timeout around the
recv calls (or the whole loop), silently break or continue on timeout and then
return the joined/decode result — this preserves the existing recv_ready() guard
while defensively handling rare socket.timeout from channel.recv.
- Around line 331-338: The send method in ssh.py currently passes a str to
Paramiko's channel.send; update send(self, payload: str) to ensure payload is
bytes before sending by encoding strings (e.g., if isinstance(payload, str):
payload = payload.encode('utf-8')) and then call channel.send(payload); keep the
early empty-check and preserve the return type (int) and use the existing
self.open() channel reference.
In `@cetmix_tower_server/static/src/terminal/tower_terminal_action.esm.js`:
- Around line 696-714: In scrollToBottom, clear any previously scheduled
timeouts before scheduling new ones to avoid accumulation: iterate over
this.scrollTimeoutHandles and call clearTimeout(handle), then reset
this.scrollTimeoutHandles = [] before pushing the new setTimeout handles;
reference the scrollToBottom method and the this.scrollTimeoutHandles array (and
optionally clear any prior animation frame via this.scrollHandle with
cancelAnimationFrame if you choose) so the component doesn't retain stale
timeout handles.
- Around line 566-594: The nested try block inside the method handling the send
queue (the loop using this.isConnected and this.queuedPayload, which calls
this.callSession("terminal_send") and this.applyResponse) is mis-indented;
reformat the inner try/catch so its opening "try {" and the corresponding "catch
(error) { ... }" are indented one level further inside the while loop (along
with the const payload assignment and this.renderQueuedLocalEcho()), keeping the
surrounding finally block (setting this.sendInFlight, this.state.isSending,
this.focusTerminal, this.pollOutput, and the recursive this.processSendQueue()
call) aligned as before to improve readability.
In `@cetmix_tower_server/static/src/terminal/tower_terminal_action.scss`:
- Line 79: Replace the deprecated CSS declaration "word-break: break-word" with
the supported "overflow-wrap: break-word" in tower_terminal_action.scss; locate
the rule that contains "word-break: break-word" and change it to "overflow-wrap:
break-word" and (for broader compatibility) set "word-break: normal" if you need
to retain default word-breaking behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 556e2afc-d3e1-4a15-af09-d3ec1d131fd4
⛔ Files ignored due to path filters (1)
cetmix_tower_server/security/ir.model.access.csvis excluded by!**/*.csv
📒 Files selected for processing (10)
cetmix_tower_server/__manifest__.pycetmix_tower_server/models/__init__.pycetmix_tower_server/models/cx_tower_server.pycetmix_tower_server/models/cx_tower_terminal_session.pycetmix_tower_server/security/cx_tower_terminal_session_security.xmlcetmix_tower_server/ssh/ssh.pycetmix_tower_server/static/src/terminal/tower_terminal_action.esm.jscetmix_tower_server/static/src/terminal/tower_terminal_action.scsscetmix_tower_server/static/src/terminal/tower_terminal_action.xmlcetmix_tower_server/views/cx_tower_server_view.xml
francoleyes
force-pushed
the
18.0-ssh_terminal
branch
from
366a9e1 to
537b68c
Compare
ivs-cetmix
left a comment
ivs-cetmix
left a comment
There was a problem hiding this comment.
Hello @francoleyes , thank you to your contribution! Looks really interesting.
To proceed further, please sign the CLA using the CLA bot in the PR.
Regarding the code, I did a preliminary review, overall looks nice, check my comments please.
Also please ensure that all the functions have proper docstrings, so the PR is aligned with the code in this project.
| host_key=host_key if host_key and not skip_host_key else None, | ||
| mode=self.ssh_auth_mode, | ||
| timeout=timeout, | ||
| ) |
There was a problem hiding this comment.
There is a helper that build an SSH connection already:
You should import it and use like this:
| try: | ||
| while not self.stop_event.is_set(): | ||
| if not self.shell.is_active(): | ||
| self.set_state("closed", _("The remote shell was closed.")) |
There was a problem hiding this comment.
Please use self.env._ instead, as this is the right way for variable translation since Odoo 18.0
odoo/odoo#174844
|
Should close #448 |
francoleyes
force-pushed
the
18.0-ssh_terminal
branch
from
537b68c to
197d621
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
cetmix_tower_server/static/src/terminal/tower_terminal_action.scss (1)
88-88:⚠️ Potential issue | 🟡 MinorReplace deprecated
word-break: break-wordvalue.The
word-break: break-wordvalue is deprecated. Useoverflow-wrap: break-wordinstead for the same behavior.Proposed fix
- word-break: break-word; + overflow-wrap: break-word;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server/static/src/terminal/tower_terminal_action.scss` at line 88, Replace the deprecated CSS declaration `word-break: break-word;` in tower_terminal_action.scss with the modern equivalent by removing that property and adding `overflow-wrap: break-word;` (or `word-wrap: break-word;` for broader support) in the same rule so text wrapping behavior remains the same.cetmix_tower_server/ssh/ssh.py (2)
340-348: 🧹 Nitpick | 🔵 TrivialConsider handling
socket.timeoutfor robustness.While the
recv_ready()guard is correct, wrapping the recv loop in a try-except forsocket.timeoutwould add defensive protection against edge cases in non-blocking mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server/ssh/ssh.py` around lines 340 - 348, The receive method should defensively handle socket.timeout when calling channel.recv: wrap the recv loop in a try/except that catches socket.timeout and breaks/continues safely so a timeout doesn't bubble up; ensure socket is imported if not already and keep using channel = self.open(), channel.recv_ready() and channel.recv(max_bytes) as the guarded calls inside the try block, returning the joined/decoded bytes as before.
331-338:⚠️ Potential issue | 🟡 MinorEnsure
payloadis encoded to bytes before sending.The
send()method passes a string directly to Paramiko'schannel.send(), which expects bytes per the documented API. While Paramiko internally coerces strings, this violates the API contract and may cause type checker failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server/ssh/ssh.py` around lines 331 - 338, The send method currently passes a str to Paramiko's channel.send which expects bytes; update the send(self, payload: str) implementation (and any callers if needed) to ensure payload is encoded to bytes (e.g., payload_bytes = payload.encode('utf-8') if isinstance(payload, str)) before calling channel = self.open() and channel.send(payload_bytes); preserve the empty payload short-circuit and ensure any existing type hints reflect the intended input if you want to accept bytes too.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server/models/cx_tower_terminal_session.py`:
- Around line 242-275: The cleanup currently overwrites all stale session
records with the generic "Terminal session expired." message, losing any
existing runtime.message (e.g., remote shell ended). Change
_cleanup_runtime_sessions to preserve each runtime's real close reason: when you
collect stale_tokens, also map token -> runtime.message (or
runtime.close_reason) and after popping tokens iterate the found stale_sessions
and set each record's message to the corresponding runtime message if present,
falling back to self.env._("Terminal session expired.") only when no
runtime-specific message exists; keep using type(self)._runtime_sessions,
_get_runtime_session, runtime.close and runtime.message to locate and use the
right values.
- Around line 177-183: The reader-loop exception handler should ignore errors
caused by intentional shutdown: add a check for a shutdown flag set by close()
(e.g., self._closing or self._closed) in the except block that wraps
receive()/reader loop (the handler using _logger.exception,
self._flush_bus_output and self.set_state), and only call
self.set_state("error", ...) and log the exception as an error if the session is
not closing; always call self._flush_bus_output(force=True) but suppress setting
error state and suppress/log at debug level when closing to avoid flipping a
reopened terminal into error (apply the same change to the other similar
handlers referenced around the reader loop and the blocks at 185-197 and
511-519).
In `@cetmix_tower_server/static/src/terminal/tower_terminal_action.esm.js`:
- Around line 300-311: When running in bus mode we must drain any bytes produced
between the initial load and the bus subscription: after calling
setupBusSubscriptions() in onMounted, invoke loadOutput() once (e.g. if
(this.busService) await this.loadOutput()) so any backlog is replayed; ensure
this is done only when polling is disabled (startPolling not used). Apply the
same change in the other similar lifecycle blocks that call
setupBusSubscriptions/startPolling (the other onMounted/onWillStart pairs around
the 407-414 and 684-691 regions) so each setupBusSubscriptions() is immediately
followed by a single loadOutput() when busService is active.
- Around line 596-606: The finally block leaves state.isSending latched after a
transport error because it sets state.isSending solely from queuedPayload even
when the connection is down; update the finally in the method that calls
callSession() so that after catching an error (where handleError() may set
connection state) you set sendInFlight = false and set state.isSending =
Boolean(this.queuedPayload && this.state.isConnected) (or explicitly clear
state.isSending when !this.state.isConnected) before calling focusTerminal();
reference callSession(), handleError(), sendInFlight, state.isSending,
queuedPayload, and focusTerminal to locate and change the logic.
---
Duplicate comments:
In `@cetmix_tower_server/ssh/ssh.py`:
- Around line 340-348: The receive method should defensively handle
socket.timeout when calling channel.recv: wrap the recv loop in a try/except
that catches socket.timeout and breaks/continues safely so a timeout doesn't
bubble up; ensure socket is imported if not already and keep using channel =
self.open(), channel.recv_ready() and channel.recv(max_bytes) as the guarded
calls inside the try block, returning the joined/decoded bytes as before.
- Around line 331-338: The send method currently passes a str to Paramiko's
channel.send which expects bytes; update the send(self, payload: str)
implementation (and any callers if needed) to ensure payload is encoded to bytes
(e.g., payload_bytes = payload.encode('utf-8') if isinstance(payload, str))
before calling channel = self.open() and channel.send(payload_bytes); preserve
the empty payload short-circuit and ensure any existing type hints reflect the
intended input if you want to accept bytes too.
In `@cetmix_tower_server/static/src/terminal/tower_terminal_action.scss`:
- Line 88: Replace the deprecated CSS declaration `word-break: break-word;` in
tower_terminal_action.scss with the modern equivalent by removing that property
and adding `overflow-wrap: break-word;` (or `word-wrap: break-word;` for broader
support) in the same rule so text wrapping behavior remains the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b35f751b-e654-4934-99d1-1dec4e4fd38f
⛔ Files ignored due to path filters (1)
cetmix_tower_server/security/ir.model.access.csvis excluded by!**/*.csv
📒 Files selected for processing (10)
cetmix_tower_server/__manifest__.pycetmix_tower_server/models/__init__.pycetmix_tower_server/models/cx_tower_server.pycetmix_tower_server/models/cx_tower_terminal_session.pycetmix_tower_server/security/cx_tower_terminal_session_security.xmlcetmix_tower_server/ssh/ssh.pycetmix_tower_server/static/src/terminal/tower_terminal_action.esm.jscetmix_tower_server/static/src/terminal/tower_terminal_action.scsscetmix_tower_server/static/src/terminal/tower_terminal_action.xmlcetmix_tower_server/views/cx_tower_server_view.xml
| } catch (error) { | ||
| this.handleError(error, _t("Failed to send data to terminal.")); | ||
| this.queuedPayload = `${payload}${this.queuedPayload}`; | ||
| this.renderQueuedLocalEcho(); | ||
| return; | ||
| } | ||
| } | ||
| } finally { | ||
| this.sendInFlight = false; | ||
| this.state.isSending = Boolean(this.queuedPayload); | ||
| this.focusTerminal(); |
There was a problem hiding this comment.
Do not leave isSending latched after a transport error.
If callSession() rejects here, the payload is re-queued, handleError() flips the terminal to "error", and finally still sets isSending from queuedPayload alone. Because isConnected is now false, that queue will never drain, but the UI remains stuck in a sending state until reconnect.
♻️ Suggested fix
} finally {
this.sendInFlight = false;
- this.state.isSending = Boolean(this.queuedPayload);
+ this.state.isSending = this.isConnected && Boolean(this.queuedPayload);
this.focusTerminal();
if (!this.busService) {
this.pollOutput();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cetmix_tower_server/static/src/terminal/tower_terminal_action.esm.js` around
lines 596 - 606, The finally block leaves state.isSending latched after a
transport error because it sets state.isSending solely from queuedPayload even
when the connection is down; update the finally in the method that calls
callSession() so that after catching an error (where handleError() may set
connection state) you set sendInFlight = false and set state.isSending =
Boolean(this.queuedPayload && this.state.isConnected) (or explicitly clear
state.isSending when !this.state.isConnected) before calling focusTerminal();
reference callSession(), handleError(), sendInFlight, state.isSending,
queuedPayload, and focusTerminal to locate and change the logic.
There was a problem hiding this comment.
@francoleyes
please use this comment; it's valid
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
francoleyes
force-pushed
the
18.0-ssh_terminal
branch
from
197d621 to
7c738d7
Compare
ivs-cetmix
changed the title
francoleyes
force-pushed
the
18.0-ssh_terminal
branch
2 times, most recently
from
10154bc to
4d52ac9
Compare
|
@ivs-cetmix I've already made the changes |
francoleyes
force-pushed
the
18.0-ssh_terminal
branch
2 times, most recently
from
a80715e to
b2f5c4d
Compare
|
@ivs-cetmix Do you think it's better to put it in a separate module? Or is it fine as is? |
|
@francoleyes I think this is a really good idea 👍 |
francoleyes
force-pushed
the
18.0-ssh_terminal
branch
from
9019e5a to
451ac02
Compare
|
@ivs-cetmix Done! |
francoleyes
changed the title
|
@francoleyes thank you very much! Will check asap. |
There was a problem hiding this comment.
thank you for your contribution!
- we need to write tests for the new logic;
- all methods must include signature typing, docstrings, and return values;
- the current PR title implies that you added a new module called
cetmix_tower_server, but that is not the case; - all documentation must be updated to reflect these changes.
| ("closed", "Closed"), | ||
| ("error", "Error"), | ||
| ], | ||
| default="open", |
There was a problem hiding this comment.
Default functions should be declared with a lambda call on self. The reason for this is so a default function can be inherited. Assigning a function pointer directly to the default parameter does not allow for inheritance.
a_field(..., default=lambda self: self._default_get())
francoleyes
changed the title
francoleyes
force-pushed
the
18.0-ssh_terminal
branch
5 times, most recently
from
a673ab7 to
f3ca84e
Compare
francoleyes
force-pushed
the
18.0-ssh_terminal
branch
2 times, most recently
from
7404460 to
ed23e9a
Compare
francoleyes
force-pushed
the
18.0-ssh_terminal
branch
from
ed23e9a to
9332ddc
Compare
|
@ivs-cetmix @tendil I think all the corrections have been made. Let me know if there's anything else. Thanks! |
@francoleyes thank you! Also important: |
…convention (<model>_view_<type>)
… naming convention (<model>_rule_<group>)
…__init__ and ssh files
…with real configuration instructions
|
@tendil @ivs-cetmix |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
cetmix_tower_server_terminal/static/src/terminal/tower_terminal_action.esm.js (1)
415-422:⚠️ Potential issue | 🟠 Major
isSendingstays latched after a transport error — UI gets stuck.When
callSession("terminal_send", payload)rejects, the catch block re-queues the payload and callshandleError()which flipsstate.statusto"error"(soisConnectedis now false). Thefinallythen setsstate.isSending = Boolean(this.queuedPayload)— stilltruebecause we just re-queued. The retry guardif (this.queuedPayload && this.isConnected)blocks any drain, so the spinner/sending indicator remains on indefinitely until the user manually reconnects.♻️ Proposed fix
} finally { this.sendInFlight = false; - this.state.isSending = Boolean(this.queuedPayload); + this.state.isSending = this.isConnected && Boolean(this.queuedPayload); this.focusTerminal(); if (this.queuedPayload && this.isConnected) { this.processSendQueue(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_terminal/static/src/terminal/tower_terminal_action.esm.js` around lines 415 - 422, The finally block leaves state.isSending true when a payload is re-queued after a transport error; change the logic so isSending only stays true when there is a queued payload AND the transport is connected. Update the finally in the send flow (where sendInFlight, state.isSending, queuedPayload, isConnected, processSendQueue are referenced) to set this.state.isSending = Boolean(this.queuedPayload && this.isConnected) and ensure this.sendInFlight is set false; keep the existing retry guard (if (this.queuedPayload && this.isConnected) this.processSendQueue()) so the spinner clears when the connection is lost and only shows while actually able to send.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server_terminal/__manifest__.py`:
- Around line 18-24: The manifest currently uses broad glob patterns in the
"assets" -> "web.assets_backend" entry
(cetmix_tower_server_terminal/static/src/terminal/**/*.xml, **/*.js, **/*.scss),
which can accidentally bundle unwanted files; replace these globs by explicitly
listing each asset file that should be included (each specific .xml, .js, and
.scss under static/src/terminal) so only intended files are packaged—update the
"assets" mapping in __manifest__.py to enumerate the exact filenames instead of
the glob patterns.
In `@cetmix_tower_server_terminal/models/cx_tower_terminal_session.py`:
- Around line 448-471: Replace the bare "except Exception:" logging that
currently calls _logger.warning in the broker-call failure handlers with
exception-logging so tracebacks are preserved: update the handlers in
terminal_reconnect (the try/except around _broker_call({"action": "close",
"token": self.session_token})), the corresponding block in terminal_close, and
the unlink exception block to use _logger.exception(...) (or
_logger.warning(..., exc_info=True)) instead of _logger.warning(...), keeping
the same message text and session id formatting.
- Around line 188-240: Replace the predictable bus channel that uses the
transient record id with the unguessable session token: in _run_output_pusher
change bus_channel = f"terminal_{session_id}" to use session_token (e.g.,
f"terminal_{session_token}") so notifications are published to a token-scoped
channel; then update _get_client_action to include params.session_token in the
action payload so the frontend can subscribe to that token-based channel, and
update the frontend (tower_terminal_action.esm.js) to call
addChannel(`terminal_${params.session_token}`) and/or filter payloads by
session_token instead of sessionId.
In `@cetmix_tower_server_terminal/ssh/ssh.py`:
- Around line 75-83: The method receive(self, max_bytes=65535) is misleading
because max_bytes currently limits only each recv() call while the loop with
channel.recv_ready() can return an unbounded total; update receive to treat
max_bytes as a total byte cap (or rename to chunk_size). Specifically, in
receive replace the loop so you track bytes_read and on each iteration call
channel.recv(min(max_bytes - bytes_read, chunk_limit)) (or if you prefer
renaming, change the parameter name to chunk_size and document it) and break
once bytes_read >= max_bytes or channel.recv_ready() is false; reference the
receive method, channel.recv_ready(), channel.recv(), and open() when making the
change.
- Line 4: The import path is wrong: replace the incorrect import of
SSHConnection from cetmix_tower_server_terminal.ssh.ssh with an import from the
actual defining module cetmix_tower_server.ssh.ssh so that SSHConnection is
resolved when the broker bootstraps sys.modules; update the import statement
referencing SSHConnection in cetmix_tower_server_terminal/ssh/ssh.py accordingly
to import from cetmix_tower_server.ssh.ssh.
In `@cetmix_tower_server_terminal/ssh/terminal_broker.py`:
- Around line 99-104: The log path _LOG_PATH and socket creation are insecure:
after logging.basicConfig creates the log file it may be world-readable, and the
socket is bound before permissions are tightened allowing a TOCTOU race; fix by
(1) preferring XDG_RUNTIME_DIR (mode 0o700) when available for the broker
directory and falling back to /tmp, (2) setting a restrictive process umask
(os.umask(0o077)) before creating or binding sockets and files, (3) explicitly
os.chmod(_LOG_PATH, 0o600) immediately after logging.basicConfig (or recreate
the file with correct mode if needed), and (4) set the umask prior to socket
bind() in the function that creates the UNIX socket (reference the socket
creation/bind and subsequent os.chmod(sock_path, 0o600) calls) so the socket is
never observable with permissive default modes.
In
`@cetmix_tower_server_terminal/static/src/terminal/tower_terminal_action.esm.js`:
- Around line 20-23: The current runtime CDN injection of XTERM_CSS_URL,
XTERM_JS_URL and XTERM_FIT_ADDON_URL (used by ensureXtermAssetsLoaded which
calls loadStylesheetOnce and loadScriptOnce) must be replaced by vendoring the
assets as module/static assets and declaring them in __manifest__.py asset
bundles; move `@xterm/xterm`@5.5.0, `@xterm/addon-fit`@0.10.0 and xterm.css into
static/lib/ (or upgrade to 6.0.0/0.11.0 first), remove ensureXtermAssetsLoaded
and any runtime dynamic injection, and reference the vendored files via the Odoo
asset system. If vendoring cannot be done immediately, modify loadScriptOnce and
loadStylesheetOnce to set integrity (SRI hashes) and crossorigin="anonymous" on
injected tags to mitigate CDN risk until assets are vendored.
In `@cetmix_tower_server/ssh/ssh.py`:
- Around line 89-93: The get_transport() method currently only checks if
self._ssh_client is None and can return a transport for a dead connection;
update get_transport() to call connect() (which contains the liveness check that
uses disconnect()) before returning the transport so callers always get a live
transport or a fresh client; reference the _ssh_client attribute, the connect()
method that contains the new liveness check and disconnect(), and ensure
get_transport() uses connect() unconditionally (or at least when _ssh_client
exists) to reuse that logic rather than directly accessing get_transport() on a
possibly-stale _ssh_client.
---
Duplicate comments:
In
`@cetmix_tower_server_terminal/static/src/terminal/tower_terminal_action.esm.js`:
- Around line 415-422: The finally block leaves state.isSending true when a
payload is re-queued after a transport error; change the logic so isSending only
stays true when there is a queued payload AND the transport is connected. Update
the finally in the send flow (where sendInFlight, state.isSending,
queuedPayload, isConnected, processSendQueue are referenced) to set
this.state.isSending = Boolean(this.queuedPayload && this.isConnected) and
ensure this.sendInFlight is set false; keep the existing retry guard (if
(this.queuedPayload && this.isConnected) this.processSendQueue()) so the spinner
clears when the connection is lost and only shows while actually able to send.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd60cebd-ad39-48cb-b639-321d429eefc8
⛔ Files ignored due to path filters (2)
cetmix_tower_server_terminal/security/ir.model.access.csvis excluded by!**/*.csvcetmix_tower_server_terminal/static/description/icon.pngis excluded by!**/*.png
📒 Files selected for processing (24)
cetmix_tower_server/__manifest__.pycetmix_tower_server/ssh/ssh.pycetmix_tower_server_terminal/README.rstcetmix_tower_server_terminal/__init__.pycetmix_tower_server_terminal/__manifest__.pycetmix_tower_server_terminal/models/__init__.pycetmix_tower_server_terminal/models/cx_tower_server.pycetmix_tower_server_terminal/models/cx_tower_terminal_session.pycetmix_tower_server_terminal/pyproject.tomlcetmix_tower_server_terminal/readme/CONFIGURE.mdcetmix_tower_server_terminal/readme/DESCRIPTION.mdcetmix_tower_server_terminal/readme/HISTORY.mdcetmix_tower_server_terminal/readme/USAGE.mdcetmix_tower_server_terminal/security/cx_tower_terminal_session_security.xmlcetmix_tower_server_terminal/ssh/__init__.pycetmix_tower_server_terminal/ssh/ssh.pycetmix_tower_server_terminal/ssh/terminal_broker.pycetmix_tower_server_terminal/static/description/index.htmlcetmix_tower_server_terminal/static/src/terminal/tower_terminal_action.esm.jscetmix_tower_server_terminal/static/src/terminal/tower_terminal_action.scsscetmix_tower_server_terminal/static/src/terminal/tower_terminal_action.xmlcetmix_tower_server_terminal/tests/__init__.pycetmix_tower_server_terminal/tests/test_terminal_session.pycetmix_tower_server_terminal/views/cx_tower_server_view.xml
|
|
||
| sock_path = cls._broker_socket_path() | ||
| bus_channel = f"terminal_{session_id}" | ||
|
|
||
| while not stop_event.is_set(): | ||
| try: | ||
| sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | ||
| sock.settimeout(5.0) | ||
| sock.connect(sock_path) | ||
| sock.settimeout(None) # switch to blocking for streaming | ||
| sock.sendall( | ||
| ( | ||
| json.dumps({"action": "subscribe", "token": session_token}) | ||
| + "\n" | ||
| ).encode() | ||
| ) | ||
| f = sock.makefile("r") | ||
| for line in f: | ||
| if stop_event.is_set(): | ||
| return | ||
| line = line.strip() | ||
| if not line: | ||
| continue | ||
| try: | ||
| msg = json.loads(line) | ||
| except json.JSONDecodeError: | ||
| continue | ||
| output = msg.get("output", "") | ||
| state = msg.get("state", "open") | ||
| # Broker rejected the subscribe (another pusher already active) | ||
| if msg.get("status") == "error": | ||
| _logger.debug( | ||
| "Output pusher: broker rejected subscribe for %s (%s)", | ||
| session_token[:8], | ||
| msg.get("message"), | ||
| ) | ||
| return | ||
| if not output and state == "open": | ||
| continue | ||
| try: | ||
| registry = Registry(db_name) | ||
| with registry.cursor() as cr: | ||
| env = odoo.api.Environment(cr, uid, {}) | ||
| env["bus.bus"]._sendone( | ||
| bus_channel, | ||
| "terminal.output", | ||
| { | ||
| "session_id": session_id, | ||
| "output": output, | ||
| "state": state, | ||
| "message": msg.get("message"), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Bus channel name uses a predictable record id — terminal output is exposed to other authenticated users.
bus_channel = f"terminal_{session_id}" uses the transient model's integer primary key. In Odoo, any authenticated user can call bus_service.addChannel("terminal_42") (no server-side ACL on string channels) and immediately receive every terminal.output notification published to that channel. The frontend at tower_terminal_action.esm.js does exactly that (line 123: \terminal_${this.sessionId}``).
Since terminal output may contain command output, environment variables, file contents, or secrets typed into the shell by an admin, this is a confidentiality leak across users on the same Odoo instance.
The session_token field is already a uuid4().hex (line 48) — unguessable and per-session. Use it for the bus channel name instead. The frontend will then need the token (not the id) to subscribe; pass it through _get_client_action's params.
🔒️ Sketch of the change
In _run_output_pusher:
- bus_channel = f"terminal_{session_id}"
+ bus_channel = f"terminal_{session_token}"In _get_client_action:
"params": {
"session_id": self.id,
+ "session_token": self.session_token,
"title": self.name,
},…and update the frontend to read params.session_token and use it for addChannel / payload filtering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cetmix_tower_server_terminal/models/cx_tower_terminal_session.py` around
lines 188 - 240, Replace the predictable bus channel that uses the transient
record id with the unguessable session token: in _run_output_pusher change
bus_channel = f"terminal_{session_id}" to use session_token (e.g.,
f"terminal_{session_token}") so notifications are published to a token-scoped
channel; then update _get_client_action to include params.session_token in the
action payload so the frontend can subscribe to that token-based channel, and
update the frontend (tower_terminal_action.esm.js) to call
addChannel(`terminal_${params.session_token}`) and/or filter payloads by
session_token instead of sessionId.
There was a problem hiding this comment.
@francoleyes
please use this comment; it's valid
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| while not stop_event.is_set(): | ||
| try: | ||
| sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | ||
| sock.settimeout(5.0) | ||
| sock.connect(sock_path) | ||
| sock.settimeout(None) # switch to blocking for streaming | ||
| sock.sendall( | ||
| ( | ||
| json.dumps({"action": "subscribe", "token": session_token}) | ||
| + "\n" | ||
| ).encode() | ||
| ) | ||
| f = sock.makefile("r") | ||
| for line in f: | ||
| if stop_event.is_set(): | ||
| return | ||
| line = line.strip() | ||
| if not line: | ||
| continue | ||
| try: | ||
| msg = json.loads(line) | ||
| except json.JSONDecodeError: | ||
| continue | ||
| output = msg.get("output", "") | ||
| state = msg.get("state", "open") | ||
| # Broker rejected the subscribe (another pusher already active) | ||
| if msg.get("status") == "error": | ||
| _logger.debug( | ||
| "Output pusher: broker rejected subscribe for %s (%s)", | ||
| session_token[:8], | ||
| msg.get("message"), | ||
| ) | ||
| return | ||
| if not output and state == "open": | ||
| continue | ||
| try: | ||
| registry = Registry(db_name) | ||
| with registry.cursor() as cr: | ||
| env = odoo.api.Environment(cr, uid, {}) | ||
| env["bus.bus"]._sendone( | ||
| bus_channel, | ||
| "terminal.output", | ||
| { | ||
| "session_id": session_id, | ||
| "output": output, | ||
| "state": state, | ||
| "message": msg.get("message"), | ||
| }, | ||
| ) | ||
| except Exception: | ||
| _logger.exception( | ||
| "Output pusher: bus publish failed for session %s", | ||
| session_id, | ||
| ) | ||
| if state != "open": | ||
| return | ||
| except OSError as exc: | ||
| if stop_event.is_set(): | ||
| return | ||
| _logger.warning( | ||
| "Output pusher: broker socket error for session %s: %s", | ||
| session_id, | ||
| exc, | ||
| ) | ||
| time.sleep(1.0) |
There was a problem hiding this comment.
Socket leak in _run_output_pusher.
sock = socket.socket(...) is created on every reconnect attempt without a with block or finally: sock.close(). Every return path inside the for line in f: loop (lines 207, 224, 247) leaves the socket open, and the outer while reconnects, so leaks accumulate across worker recycling and broker hiccups. Over time a busy worker exhausts its file descriptor table.
🐛 Proposed fix
while not stop_event.is_set():
+ sock = None
try:
sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
sock.settimeout(5.0)
sock.connect(sock_path)
sock.settimeout(None) # switch to blocking for streaming
sock.sendall(
(
json.dumps({"action": "subscribe", "token": session_token})
+ "\n"
).encode()
)
f = sock.makefile("r")
for line in f:
...
except OSError as exc:
if stop_event.is_set():
return
_logger.warning(
"Output pusher: broker socket error for session %s: %s",
session_id,
exc,
)
time.sleep(1.0)
+ finally:
+ if sock is not None:
+ try:
+ sock.close()
+ except OSError:
+ pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while not stop_event.is_set(): | |
| try: | |
| sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | |
| sock.settimeout(5.0) | |
| sock.connect(sock_path) | |
| sock.settimeout(None) # switch to blocking for streaming | |
| sock.sendall( | |
| ( | |
| json.dumps({"action": "subscribe", "token": session_token}) | |
| + "\n" | |
| ).encode() | |
| ) | |
| f = sock.makefile("r") | |
| for line in f: | |
| if stop_event.is_set(): | |
| return | |
| line = line.strip() | |
| if not line: | |
| continue | |
| try: | |
| msg = json.loads(line) | |
| except json.JSONDecodeError: | |
| continue | |
| output = msg.get("output", "") | |
| state = msg.get("state", "open") | |
| # Broker rejected the subscribe (another pusher already active) | |
| if msg.get("status") == "error": | |
| _logger.debug( | |
| "Output pusher: broker rejected subscribe for %s (%s)", | |
| session_token[:8], | |
| msg.get("message"), | |
| ) | |
| return | |
| if not output and state == "open": | |
| continue | |
| try: | |
| registry = Registry(db_name) | |
| with registry.cursor() as cr: | |
| env = odoo.api.Environment(cr, uid, {}) | |
| env["bus.bus"]._sendone( | |
| bus_channel, | |
| "terminal.output", | |
| { | |
| "session_id": session_id, | |
| "output": output, | |
| "state": state, | |
| "message": msg.get("message"), | |
| }, | |
| ) | |
| except Exception: | |
| _logger.exception( | |
| "Output pusher: bus publish failed for session %s", | |
| session_id, | |
| ) | |
| if state != "open": | |
| return | |
| except OSError as exc: | |
| if stop_event.is_set(): | |
| return | |
| _logger.warning( | |
| "Output pusher: broker socket error for session %s: %s", | |
| session_id, | |
| exc, | |
| ) | |
| time.sleep(1.0) | |
| while not stop_event.is_set(): | |
| sock = None | |
| try: | |
| sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | |
| sock.settimeout(5.0) | |
| sock.connect(sock_path) | |
| sock.settimeout(None) # switch to blocking for streaming | |
| sock.sendall( | |
| ( | |
| json.dumps({"action": "subscribe", "token": session_token}) | |
| "\n" | |
| ).encode() | |
| ) | |
| f = sock.makefile("r") | |
| for line in f: | |
| if stop_event.is_set(): | |
| return | |
| line = line.strip() | |
| if not line: | |
| continue | |
| try: | |
| msg = json.loads(line) | |
| except json.JSONDecodeError: | |
| continue | |
| output = msg.get("output", "") | |
| state = msg.get("state", "open") | |
| # Broker rejected the subscribe (another pusher already active) | |
| if msg.get("status") == "error": | |
| _logger.debug( | |
| "Output pusher: broker rejected subscribe for %s (%s)", | |
| session_token[:8], | |
| msg.get("message"), | |
| ) | |
| return | |
| if not output and state == "open": | |
| continue | |
| try: | |
| registry = Registry(db_name) | |
| with registry.cursor() as cr: | |
| env = odoo.api.Environment(cr, uid, {}) | |
| env["bus.bus"]._sendone( | |
| bus_channel, | |
| "terminal.output", | |
| { | |
| "session_id": session_id, | |
| "output": output, | |
| "state": state, | |
| "message": msg.get("message"), | |
| }, | |
| ) | |
| except Exception: | |
| _logger.exception( | |
| "Output pusher: bus publish failed for session %s", | |
| session_id, | |
| ) | |
| if state != "open": | |
| return | |
| except OSError as exc: | |
| if stop_event.is_set(): | |
| return | |
| _logger.warning( | |
| "Output pusher: broker socket error for session %s: %s", | |
| session_id, | |
| exc, | |
| ) | |
| time.sleep(1.0) | |
| finally: | |
| if sock is not None: | |
| try: | |
| sock.close() | |
| except OSError: | |
| pass |
There was a problem hiding this comment.
@francoleyes
please use this comment; it's valid
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| try: | ||
| self._broker_call({"action": "close", "token": self.session_token}) | ||
| except Exception: | ||
| _logger.warning( | ||
| "terminal_close: broker call failed for session %s", self.id | ||
| ) | ||
| self.write({"state": "closed", "message": self.env._("Terminal disconnected.")}) | ||
| return { | ||
| "output": "", | ||
| "state": "closed", | ||
| "message": self.env._("Terminal disconnected."), | ||
| } | ||
|
|
||
| def terminal_reconnect(self): | ||
| """Reconnect the terminal session and return the initial output.""" | ||
| self.ensure_one() | ||
| self.check_access("read") | ||
| self._stop_output_pusher() | ||
| try: | ||
| self._broker_call({"action": "close", "token": self.session_token}) | ||
| except Exception: | ||
| _logger.warning( | ||
| "terminal_reconnect: broker close failed for session %s", self.id | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Lost diagnostics on broker-call failures.
except Exception: followed by _logger.warning("…broker call failed for session %s", self.id) discards both the exception type and the traceback, which makes debugging broker outages or socket errors needlessly hard. Use _logger.exception(...) (or pass exc_info=True) so the traceback is captured. Same issue at lines 504-507 in unlink.
♻️ Proposed change
try:
self._broker_call({"action": "close", "token": self.session_token})
except Exception:
- _logger.warning(
- "terminal_close: broker call failed for session %s", self.id
- )
+ _logger.exception(
+ "terminal_close: broker call failed for session %s", self.id
+ )Apply the same change to the terminal_reconnect and unlink except blocks.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| self._broker_call({"action": "close", "token": self.session_token}) | |
| except Exception: | |
| _logger.warning( | |
| "terminal_close: broker call failed for session %s", self.id | |
| ) | |
| self.write({"state": "closed", "message": self.env._("Terminal disconnected.")}) | |
| return { | |
| "output": "", | |
| "state": "closed", | |
| "message": self.env._("Terminal disconnected."), | |
| } | |
| def terminal_reconnect(self): | |
| """Reconnect the terminal session and return the initial output.""" | |
| self.ensure_one() | |
| self.check_access("read") | |
| self._stop_output_pusher() | |
| try: | |
| self._broker_call({"action": "close", "token": self.session_token}) | |
| except Exception: | |
| _logger.warning( | |
| "terminal_reconnect: broker close failed for session %s", self.id | |
| ) | |
| try: | |
| self._broker_call({"action": "close", "token": self.session_token}) | |
| except Exception: | |
| _logger.exception( | |
| "terminal_close: broker call failed for session %s", self.id | |
| ) | |
| self.write({"state": "closed", "message": self.env._("Terminal disconnected.")}) | |
| return { | |
| "output": "", | |
| "state": "closed", | |
| "message": self.env._("Terminal disconnected."), | |
| } | |
| def terminal_reconnect(self): | |
| """Reconnect the terminal session and return the initial output.""" | |
| self.ensure_one() | |
| self.check_access("read") | |
| self._stop_output_pusher() | |
| try: | |
| self._broker_call({"action": "close", "token": self.session_token}) | |
| except Exception: | |
| _logger.warning( | |
| "terminal_reconnect: broker close failed for session %s", self.id | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cetmix_tower_server_terminal/models/cx_tower_terminal_session.py` around
lines 448 - 471, Replace the bare "except Exception:" logging that currently
calls _logger.warning in the broker-call failure handlers with exception-logging
so tracebacks are preserved: update the handlers in terminal_reconnect (the
try/except around _broker_call({"action": "close", "token":
self.session_token})), the corresponding block in terminal_close, and the unlink
exception block to use _logger.exception(...) (or _logger.warning(...,
exc_info=True)) instead of _logger.warning(...), keeping the same message text
and session id formatting.
There was a problem hiding this comment.
@francoleyes
please use this comment; it's valid
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| # Copyright Cetmix OÜ 2026 | ||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | ||
|
|
||
| from cetmix_tower_server_terminal.ssh.ssh import SSHConnection |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify SSHConnection is NOT defined in this file (only InteractiveShell is)
# and IS defined in the parent cetmix_tower_server/ssh/ssh.py.
echo "=== Symbols defined in cetmix_tower_server_terminal/ssh/ssh.py ==="
ast-grep --pattern 'class $NAME { $$$ }' cetmix_tower_server_terminal/ssh/ssh.py 2>/dev/null || \
rg -nP '^class\s+\w+' cetmix_tower_server_terminal/ssh/ssh.py
echo ""
echo "=== Where SSHConnection is actually defined ==="
rg -nP '^class\s+SSHConnection\b' --type=py
echo ""
echo "=== sys.modules keys staged by the broker bootstrap ==="
rg -nP -C1 'sys\.modules' cetmix_tower_server_terminal/ssh/terminal_broker.pyRepository: cetmix/cetmix-tower
Length of output: 678
Critical: wrong import path — SSHConnection is not defined in this file.
Line 4 imports SSHConnection from cetmix_tower_server_terminal.ssh.ssh, but SSHConnection is defined in the parent module cetmix_tower_server.ssh.ssh (at line 54) and is not present in the current file. The broker's sys.modules bootstrap (terminal_broker.py lines 84-86) explicitly stages only sys.modules["cetmix_tower_server.ssh.ssh"] — as its comment confirms: "resolve 'from cetmix_tower_server.ssh.ssh import SSHConnection'".
When the broker subprocess loads this file via importlib, this import will raise ImportError because cetmix_tower_server_terminal.ssh.ssh is not in sys.modules, causing the broker to crash and all terminal sessions to fail.
Fix
-from cetmix_tower_server_terminal.ssh.ssh import SSHConnection
+from cetmix_tower_server.ssh.ssh import SSHConnection🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cetmix_tower_server_terminal/ssh/ssh.py` at line 4, The import path is wrong:
replace the incorrect import of SSHConnection from
cetmix_tower_server_terminal.ssh.ssh with an import from the actual defining
module cetmix_tower_server.ssh.ssh so that SSHConnection is resolved when the
broker bootstraps sys.modules; update the import statement referencing
SSHConnection in cetmix_tower_server_terminal/ssh/ssh.py accordingly to import
from cetmix_tower_server.ssh.ssh.
| def receive(self, max_bytes: int = 65535) -> str: | ||
| """ | ||
| Read all currently available bytes from the shell channel. | ||
| """ | ||
| channel = self.open() | ||
| chunks = [] | ||
| while channel.recv_ready(): | ||
| chunks.append(channel.recv(max_bytes)) | ||
| return b"".join(chunks).decode("utf-8", errors="replace") |
There was a problem hiding this comment.
Misleading max_bytes — total returned size is unbounded.
max_bytes only caps each individual recv(); the loop continues while recv_ready() is true, so the returned string can be arbitrarily large for a chatty shell (e.g., find /, cat huge_log). Either bound the cumulative read, or rename the parameter to chunk_size to reflect actual semantics.
♻️ Proposed change (cap total bytes)
- def receive(self, max_bytes: int = 65535) -> str:
+ def receive(self, max_bytes: int = 65535) -> str:
"""
Read all currently available bytes from the shell channel.
"""
channel = self.open()
chunks = []
- while channel.recv_ready():
- chunks.append(channel.recv(max_bytes))
+ remaining = max_bytes
+ while remaining > 0 and channel.recv_ready():
+ data = channel.recv(remaining)
+ if not data:
+ break
+ chunks.append(data)
+ remaining -= len(data)
return b"".join(chunks).decode("utf-8", errors="replace")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cetmix_tower_server_terminal/ssh/ssh.py` around lines 75 - 83, The method
receive(self, max_bytes=65535) is misleading because max_bytes currently limits
only each recv() call while the loop with channel.recv_ready() can return an
unbounded total; update receive to treat max_bytes as a total byte cap (or
rename to chunk_size). Specifically, in receive replace the loop so you track
bytes_read and on each iteration call channel.recv(min(max_bytes - bytes_read,
chunk_limit)) (or if you prefer renaming, change the parameter name to
chunk_size and document it) and break once bytes_read >= max_bytes or
channel.recv_ready() is false; reference the receive method,
channel.recv_ready(), channel.recv(), and open() when making the change.
| _LOG_PATH = f"/tmp/tower_terminal_broker_{os.getuid()}.log" | ||
| logging.basicConfig( | ||
| level=logging.WARNING, | ||
| filename=_LOG_PATH, | ||
| format="%(asctime)s [broker-%(process)d] %(levelname)s %(message)s", | ||
| ) |
There was a problem hiding this comment.
Harden temp-file paths and the socket-permission TOCTOU.
Two related issues that affect a multi-user Linux host:
-
Log file at
/tmp/tower_terminal_broker_<uid>.logis created with the broker process's default umask (commonly0o022), so the file ends up world-readable. SSH error strings, host names, and broker traces (including SSH backtraces inside_logger.exception) end up readable by any local user. -
Socket creation does
bind()thenos.chmod(sock_path, 0o600). Between those calls the socket exists with default permissions; another local user canconnect()during that window and impersonate an Odoo worker. Set the umask beforebind()to close the race.
Consider using XDG_RUNTIME_DIR (mode 0o700) when available, or at minimum apply os.umask(0o077) for the broker process and os.chmod(_LOG_PATH, 0o600) after basicConfig.
🔒️ Suggested hardening
+# Restrict permissions for everything the broker creates in /tmp.
+os.umask(0o077)
+
_LOG_PATH = f"/tmp/tower_terminal_broker_{os.getuid()}.log"
logging.basicConfig(
level=logging.WARNING,
filename=_LOG_PATH,
format="%(asctime)s [broker-%(process)d] %(levelname)s %(message)s",
)
+try:
+ os.chmod(_LOG_PATH, 0o600)
+except OSError:
+ pass
_logger = logging.getLogger(__name__) server_sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+ # umask above already restricts the socket file to mode 0600
server_sock.bind(sock_path)
- os.chmod(sock_path, 0o600) # owner-only: same security level as process memory
server_sock.listen(100)Also applies to: 627-664
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cetmix_tower_server_terminal/ssh/terminal_broker.py` around lines 99 - 104,
The log path _LOG_PATH and socket creation are insecure: after
logging.basicConfig creates the log file it may be world-readable, and the
socket is bound before permissions are tightened allowing a TOCTOU race; fix by
(1) preferring XDG_RUNTIME_DIR (mode 0o700) when available for the broker
directory and falling back to /tmp, (2) setting a restrictive process umask
(os.umask(0o077)) before creating or binding sockets and files, (3) explicitly
os.chmod(_LOG_PATH, 0o600) immediately after logging.basicConfig (or recreate
the file with correct mode if needed), and (4) set the umask prior to socket
bind() in the function that creates the UNIX socket (reference the socket
creation/bind and subsequent os.chmod(sock_path, 0o600) calls) so the socket is
never observable with permissive default modes.
| const XTERM_CSS_URL = "https://cdn.jsdelivr.net/npm/@xterm/xterm@5.5.0/css/xterm.css"; | ||
| const XTERM_JS_URL = "https://cdn.jsdelivr.net/npm/@xterm/xterm@5.5.0/lib/xterm.js"; | ||
| const XTERM_FIT_ADDON_URL = | ||
| "https://cdn.jsdelivr.net/npm/@xterm/addon-fit@0.10.0/lib/addon-fit.js"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
xterm.js npm package latest stable version 5.x and addon-fit
💡 Result:
Latest stable xterm.js 5.x (npm): There is no “5.x latest stable” package you can install today, because the current stable release is 6.0.0. The project publishes under the @xterm scope, e.g. @xterm/xterm latest is 6.0.0, and the old unscoped/deprecated packages don’t reflect current 5.x stable. npm @xterm/xterm latest stable (as of 2026-04-25): 6.0.0. If you specifically need the 5.x line, the releases in the 5.x line include 5.5.0 (released 2024-04-05), which is the newest 5.x release shown on the official xterm.js releases. Addon @xterm/addon-fit latest stable: 0.11.0. Compatibility: @xterm/addon-fit requires xterm.js v4+. Install commands (current stable): - npm install @xterm/xterm - npm install @xterm/addon-fit Install commands (pinning to the newest 5.x release shown): - npm install @xterm/xterm@5.5.0 - npm install @xterm/addon-fit@0.11.0
Citations:
- 1: https://github.com/xtermjs/xterm.js/releases
- 2: https://www.npmjs.com/package/@xterm/xterm
- 3: https://www.npmjs.com/package/@xterm/addon-fit?activeTab=readme
- 4: https://github.com/xtermjs/xterm.js/releases/tag/5.4.0
- 5: https://github.com/xtermjs/xterm.js/releases/tag/5.5.0
🏁 Script executed:
# Find the file and check its location
fd -t f "tower_terminal_action.esm.js"Repository: cetmix/cetmix-tower
Length of output: 141
🏁 Script executed:
# Check for __manifest__.py in the module
fd -t f "__manifest__.py"Repository: cetmix/cetmix-tower
Length of output: 429
🏁 Script executed:
# Check for static/lib directory with potential vendored assets
find . -type d -name "lib" | head -20Repository: cetmix/cetmix-tower
Length of output: 45
🏁 Script executed:
cat -n cetmix_tower_server_terminal/__manifest__.pyRepository: cetmix/cetmix-tower
Length of output: 1120
🏁 Script executed:
head -n 150 cetmix_tower_server_terminal/static/src/terminal/tower_terminal_action.esm.js | cat -nRepository: cetmix/cetmix-tower
Length of output: 5950
Vendor xterm.js, addon-fit, and CSS as module assets instead of loading from CDN at runtime.
The code hardcodes and dynamically injects xterm.js, addon-fit, and the CSS stylesheet from cdn.jsdelivr.net at runtime (lines 20-23, injected via ensureXtermAssetsLoaded at lines 78-94). This approach:
- Violates CSP policies (
script-src 'self')—fails in any environment with restrictive Content Security Policy - Breaks offline and air-gapped deployments
- Lacks Subresource Integrity (SRI)—a CDN compromise can inject arbitrary code into authenticated admin sessions
- Creates external dependency on third-party uptime and adds page-load latency
- Does not follow standard Odoo patterns
The correct approach is to vendor @xterm/xterm@5.5.0, @xterm/addon-fit@0.10.0, and xterm.css in static/lib/ (or install via npm), declare them in __manifest__.py assets, and remove the runtime loading functions entirely. If immediate vendoring is not possible, at minimum add integrity hashes and crossorigin="anonymous" to the injected script/link tags in loadScriptOnce (line 58, 74) and loadStylesheetOnce (lines 31, 33).
Also consider upgrading xterm.js: current stable is 6.0.0 (5.5.0 released 2024-04-05); addon-fit latest is 0.11.0 (current is 0.10.0).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cetmix_tower_server_terminal/static/src/terminal/tower_terminal_action.esm.js`
around lines 20 - 23, The current runtime CDN injection of XTERM_CSS_URL,
XTERM_JS_URL and XTERM_FIT_ADDON_URL (used by ensureXtermAssetsLoaded which
calls loadStylesheetOnce and loadScriptOnce) must be replaced by vendoring the
assets as module/static assets and declaring them in __manifest__.py asset
bundles; move `@xterm/xterm`@5.5.0, `@xterm/addon-fit`@0.10.0 and xterm.css into
static/lib/ (or upgrade to 6.0.0/0.11.0 first), remove ensureXtermAssetsLoaded
and any runtime dynamic injection, and reference the vendored files via the Odoo
asset system. If vendoring cannot be done immediately, modify loadScriptOnce and
loadStylesheetOnce to set integrity (SRI hashes) and crossorigin="anonymous" on
injected tags to mitigate CDN risk until assets are vendored.
| if self._ssh_client is not None: | ||
| return self._ssh_client | ||
| transport = self._ssh_client.get_transport() | ||
| if transport and transport.is_active(): | ||
| return self._ssh_client | ||
| self.disconnect() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM — handles stale/dead transports correctly.
The new check ensures a cached _ssh_client whose transport is no longer active is properly torn down via disconnect() before a fresh SSHClient is created. This avoids returning an unusable cached client to callers like check_ssh_connection and the broker daemon.
Minor heads-up (out of scope of this hunk): get_transport() at lines 140-142 still only checks _ssh_client is None, so it can still return a transport on a dead connection without going through this new path. Consider making it call connect() unconditionally so it benefits from the same liveness check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cetmix_tower_server/ssh/ssh.py` around lines 89 - 93, The get_transport()
method currently only checks if self._ssh_client is None and can return a
transport for a dead connection; update get_transport() to call connect() (which
contains the liveness check that uses disconnect()) before returning the
transport so callers always get a live transport or a fresh client; reference
the _ssh_client attribute, the connect() method that contains the new liveness
check and disconnect(), and ensure get_transport() uses connect()
unconditionally (or at least when _ssh_client exists) to reuse that logic rather
than directly accessing get_transport() on a possibly-stale _ssh_client.
| "cetmix_tower_server/static/src/components/ace_variables/autocomplete_popup.scss", | ||
| "cetmix_tower_server/static/src/components/ace_variables/code_editor_tower.xml", | ||
| "cetmix_tower_server/static/src/components/ace_variables/code_editor_tower.esm.js", | ||
| "cetmix_tower_server/static/src/components/server_status/server_status_field.esm.js", |
There was a problem hiding this comment.
don't change what has already been done /**/*.
if you added data via a different path, simply extend your path with a new line
|
|
||
| state = fields.Selection( | ||
| selection=lambda self: self._selection_state(), | ||
| default="open", |
There was a problem hiding this comment.
the default value must be defined using a lambda function
| id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink | ||
| access_cx_tower_terminal_session_user,Terminal Session->User,model_cx_tower_terminal_session,cetmix_tower_server.group_user,1,1,1,1 | ||
| access_cx_tower_terminal_session_manager,Terminal Session->Manager,model_cx_tower_terminal_session,cetmix_tower_server.group_manager,1,1,1,1 | ||
| access_cx_tower_terminal_session_root,Terminal Session->Root,model_cx_tower_terminal_session,cetmix_tower_server.group_root,1,1,1,1 |
There was a problem hiding this comment.
if all permissions are indeed set for the three roles, then a single entry for group_user is sufficient, since manager and root will inherit these ACLs through implied groups
but you should check whether group_user actually needs all permissions; I don't think it needs them for deletion, at least
| @@ -0,0 +1,16 @@ | |||
| <?xml version="1.0" encoding="utf-8" ?> | |||
| <odoo> | |||
| <record id="cx_tower_terminal_session_rule_group_user_own" model="ir.rule"> | |||
There was a problem hiding this comment.
Correct answer:
<record id="cx_tower_terminal_session_rule_group_user_own" model="ir.rule">
<field name="name">Tower terminal session: own records only</field>
<field name="model_id" ref="model_cx_tower_terminal_session"/>
<field name="groups" eval="[(4, ref('cetmix_tower_server.group_user'))]"/>
<field name="domain_force">[("create_uid", "=", user.id)]</field>
</record>IMPORTANT QUESTION (architectural):
Right now rule - "create_uid", "=", user.id)
this means: users can only see their own entries! However, managers and root users will also be restricted because they are included in the "user" group via the "implied" rule
if this is NOT the intended behavior (for example, if root needs to see everything), then you need to separate them
| # Copyright Cetmix OÜ 2026 | ||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | ||
|
|
||
| from cetmix_tower_server_terminal.ssh.ssh import SSHConnection |
There was a problem hiding this comment.
the import is incorrect; coderabbitAI is correct
you need to change it to from cetmix_tower_server.ssh.ssh import SSHConnection
| from odoo.addons.cetmix_tower_server.tests.common import TestTowerCommon | ||
|
|
||
|
|
||
| class TestTowerTerminalSession(TestTowerCommon): |
There was a problem hiding this comment.
we have tests - great!
but we need to ensure maximum coverage of the core new logic added by the new module;
tests are an essential and indispensable part of modules, so please create a common.py file and a few more files organized by functionality, and cover the different branches of your module
| return; | ||
| } | ||
|
|
||
| const script = document.createElement("script"); |
There was a problem hiding this comment.
please avoid manually injecting third-party scripts/styles from a CDN here
odoo 18 already provides the standard asset loading helpers (loadAssets / useAssets),
and in the common case assets should be managed through the regular asset pipeline
| if (document.querySelector(`link[data-cetmix-terminal-lib="${url}"]`)) { | ||
| return; | ||
| } | ||
| const link = document.createElement("link"); |
| this._unsubscribeBus(); | ||
| this.unregisterResizeHandlers(); | ||
| this.disposeTerminal(); | ||
| this.closeTerminal(true); |
There was a problem hiding this comment.
closeTerminal(true) inside onWillUnmount() is only a best-effort cleanup
if the server-side session really must be closed, that lifecycle should not depend only on a fire-and-forget call from unmount
| this.processResizeQueue(); | ||
| } | ||
|
|
||
| async processResizeQueue() { |
There was a problem hiding this comment.
do not silently swallow all resize rpc errors here
at least preserve some debug signal or handle only the expected transient failure case explicitly
5 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation