Files
MachinePainting 09362f42c7 v1.1.0: refactor Setup Node, security hardening, auto-curl auth flow
Setup Node refactor (the secret-bearing fields no longer exist on the
node at all):
  • Removed app_key / app_secret / auth_code from INPUT_TYPES. The
    Setup Node now exposes only storage_method, encryption_key_method,
    and reconnect — none of which are credentials.
  • Added a "Set credentials..." button on the node (LiteGraph button
    widget installed via beforeRegisterNodeDef + onNodeCreated
    prototype hook so it survives reload, drag-and-drop workflow
    JSON, and PNG metadata restore).
  • Click opens a browser-only modal with three <input type=password>
    fields. Browser-native dot masking the whole time the user types
    or pastes — no canvas-rendered plaintext, no shoulder-surf.
  • Modal Save POSTs values to a new same-origin route
    /dropsend/setup/stash and closes. setup() consumes the stash
    entry on the next Queue (one-shot, keyed by client_id, 60-second
    TTL, 32-entry cap with oldest-eviction).
  • Architectural consequence: secrets never become widget values, so
    they cannot enter saved workflow JSON, ComfyUI's localStorage
    auto-save, PNG metadata, copy-pasted nodes, or PromptServer.history
    (and therefore the unauthenticated /history HTTP endpoint).

Auto-curl auth flow (one-click second paste):
  • After Run 1 prints the OAuth URL banner, setup() sends a
    sid-targeted dropsend_credentials_needed WebSocket event.
  • JS handler auto-opens the entry modal in the originating browser
    so the user can paste auth_code as soon as Dropbox shows it,
    without clicking the button between runs.
  • Defense in depth: client_id is echoed in the WS payload and the
    JS handler verifies it matches api.clientId before opening.

Security hardening on /dropsend/setup/stash:
  • Same-origin enforcement: Origin header must match request.host.
    Closes cross-origin CSRF from another browser tab even when an
    attacker has sniffed a victim's client_id.
  • Content-Type lockdown: rejects anything other than
    application/json with HTTP 415. Closes the confused-deputy class
    where form-encoded bodies could be parsed as JSON.
  • Live-session check: client_id must correspond to an active
    PromptServer.instance.sockets entry. Returns 403 otherwise.
    Closes the stash-poisoning vector where an attacker who guesses
    a victim's client_id could pre-populate credentials.
  • Per-IP rate limit: 30 POSTs / 10-second sliding window, returns
    429 over budget. Defense against stash-spam DoS attempting to
    evict a legitimate user's entry.
  • Body cap (32 KB), client_id regex validation, JSON parse errors
    return 400, empty payloads dropped.
  • Both rate-limit and live-session checks empirically verified via
    curl smoke tests.

Reconnect now revokes the Dropbox refresh token at the source:
  • reconnect=True populates DropboxAuthManager from os.environ and
    calls reset(revoke_token=True).
  • POST /2/auth/token/revoke with an 8-second timeout so a Dropbox
    outage cannot hang the operator mid-reconnect. Local cleanup
    proceeds regardless of revocation outcome.
  • Revocation outcome is now logged at logger.error level (not just
    print) and the failure message tells the operator to manually
    disconnect at https://www.dropbox.com/account/connected_apps so
    a silent revocation failure cannot leave a still-valid token at
    Dropbox.

Logging:
  • Single FileHandler attached to the root logger from __init__.py,
    pointed at a plugin-relative dropsend.log. Replaces the four
    duplicated logging.basicConfig() blocks in monitor_output.py /
    encrypt_file.py / dropbox_upload.py / dropsend_uploader_node.py
    that were dead code (basicConfig is a no-op once ComfyUI has
    already configured root). Log records from every DropSend module
    now actually persist to disk.
  • Race-free creation: pre-create with os.open(O_CREAT | O_WRONLY |
    O_APPEND, 0o600) before FileHandler attaches, follow-up chmod as
    belt-and-suspenders. Closes the brief default-umask window
    between FileHandler creating the file and post-hoc chmod.
  • Rotation via RotatingFileHandler (5 MB per file, 3 backups, ~20
    MB max). Bounds disk use on long-lived hosts.
  • dropbox_auth_manager.py now has its own logger so revocation
    successes/failures land in dropsend.log alongside stdout.

Tests (tests/ directory, unittest, no extra deps):
  • test_safe_paths.py — 23 tests covering symlink rejection, path-
    traversal rejection, allowlist behaviour, dropbox_dest validation
    (NUL bytes, missing leading slash, parent-traversal segments),
    and resolve_safe_watch_folder edge cases. safe_paths is the only
    barrier between remote workflow inputs and arbitrary-file-read
    on the host; this is regression coverage for that barrier.
  • test_setup_node_security.py — static AST analysis of
    dropsend_setup_node.py asserting that every send_sync call for a
    credential event passes a non-None sid AND sits inside an
    "if sid:" guard. Catches future regressions of the original
    WebSocket-broadcast vulnerability deterministically. Also asserts
    INPUT_TYPES never re-introduces app_key/app_secret/auth_code.

WebSocket-broadcast guard maintained on all three event sites
(dropsend_credentials_ready, dropbox_reconnect_complete,
dropsend_credentials_needed) — refuse to send when client_id is None
to prevent send_sync(..., None) from broadcasting credentials to
every connected browser tab.

README:
  • New Changelog section documenting the v1.1.0 security advisory
    (workflow-persistence + WS-broadcast leaks in earlier versions)
    and the fixes shipped in this release.
  • New "Bind ComfyUI to localhost or front it with auth" Best
    Practice item (item 4) explicitly calling out that ComfyUI's
    /prompt endpoint is unauthenticated.
  • New "Encryption key rotation" subsection covering symmetric-key
    implications, leaked-key recovery, and how to generate a new key.
  • New "Uninstalling / disconnecting" subsection with step-by-step
    instructions for revoking Dropbox app, resetting app secret,
    removing platform secrets, and handling existing .enc files.
  • Threat model reflects same-origin/Content-Type checks, log
    rotation, plugin-directory metadata-leak note for multi-tenant
    hosts, reconnect-revocation silent-failure note, and Dropbox
    SDK error-message log review.

.gitignore:
  • Expanded to cover *.bak, *.enc, *.key/.pem/.p12/.pfx, .idea/,
    .pytest_cache/, build artifacts, Thumbs.db/Desktop.ini.

Bundled in this minor release:
  • Single-Observer fix for the macOS _fsevents conflict that
    previously prevented .enc files from being picked up by the
    upload watcher.
  • folder_format dropdown for AutoUploader (project_name default
    plus five worldwide date formats), with two-base persistence so
    switching between project-name and dated modes preserves both
    patterns.
  • RUNNING banner with per-setting plain-English explanations.
  • Branded watching/encrypted/sent log emojis.
  • Per-shell copy-paste commands and echo verification for the
    COMFYUI_DROPSEND_ALLOW_SETUP gate.
2026-05-09 05:44:47 -07:00

222 lines
8.7 KiB
Python

"""Regression tests for security-critical patterns in
dropsend_setup_node.py.
These run static AST analysis against the source file rather than
invoking setup() at runtime. Reasons:
• setup() pulls in aiohttp, requests, dropbox, the Dropbox API,
and several global modules. A behavioral test would be heavy.
• The patterns we're guarding against are syntactic — "is there
a send_sync call without a sid argument" is exactly the kind
of regression an AST walk catches deterministically.
Catches:
• The original WebSocket-broadcast vulnerability (send_sync with
sid=None broadcasts to every connected client). Any future
change that drops the `if sid:` guard or hardcodes None as the
third argument fails this test loudly at CI time.
Run with:
python -m unittest tests.test_setup_node_security
or:
python tests/test_setup_node_security.py
"""
import ast
import os
import sys
import unittest
_SETUP_NODE_PATH = os.path.join(
os.path.dirname(os.path.dirname(os.path.abspath(__file__))),
"dropsend_setup_node.py",
)
# WebSocket events that carry credential data or trigger credential-
# adjacent UI changes. Any send_sync of one of these MUST NOT be a
# broadcast (sid=None).
CREDENTIAL_EVENTS = frozenset({
"dropsend_credentials_ready",
"dropsend_credentials_needed",
"dropbox_reconnect_complete",
})
def _iter_send_sync_calls(tree):
"""Yield every call to a `*.send_sync(...)` attribute in the tree."""
for node in ast.walk(tree):
if not isinstance(node, ast.Call):
continue
func = node.func
if isinstance(func, ast.Attribute) and func.attr == "send_sync":
yield node
def _first_arg_is_credential_event(call):
"""If the first positional arg is a string literal naming a
credential-class event, return that string; otherwise None."""
if not call.args:
return None
arg = call.args[0]
if isinstance(arg, ast.Constant) and isinstance(arg.value, str):
if arg.value in CREDENTIAL_EVENTS:
return arg.value
return None
class TestNoBroadcastForCredentialEvents(unittest.TestCase):
"""The dangerous pattern is `PromptServer.instance.send_sync(EVENT,
DATA)` (no third arg) or `send_sync(EVENT, DATA, None)`. Both
cause aiohttp's send_json to iterate every connected websocket
and deliver to all of them — i.e. a credential broadcast.
Every credential-class send_sync in the setup node must have a
third argument, and that third argument must not be a `None`
literal."""
@classmethod
def setUpClass(cls):
with open(_SETUP_NODE_PATH, "r", encoding="utf-8") as f:
cls.tree = ast.parse(f.read(), filename=_SETUP_NODE_PATH)
def test_every_credential_send_sync_supplies_a_sid(self):
offenders = []
for call in _iter_send_sync_calls(self.tree):
event = _first_arg_is_credential_event(call)
if event is None:
continue
# Inspect the third positional or `sid=` keyword argument.
sid_arg = None
if len(call.args) >= 3:
sid_arg = call.args[2]
else:
for kw in call.keywords:
if kw.arg == "sid":
sid_arg = kw.value
break
if sid_arg is None:
offenders.append(
f"send_sync({event!r}, ...) at line {call.lineno} "
"supplies no sid argument; this would broadcast to "
"every connected WebSocket client."
)
continue
if isinstance(sid_arg, ast.Constant) and sid_arg.value is None:
offenders.append(
f"send_sync({event!r}, ..., None) at line {call.lineno} "
"passes None as sid; this would broadcast to every "
"connected WebSocket client."
)
if offenders:
self.fail(
"Credential events must never be broadcast. Offenders:\n "
+ "\n ".join(offenders)
)
def test_every_credential_send_sync_is_inside_a_truthy_sid_check(self):
"""Static-analysis defense in depth: walk every credential
send_sync and confirm it sits inside an `if sid:` (or `if X:`)
block. Catches the case where someone supplies a truthy-looking
variable that's actually `None` at runtime — the surrounding
guard is what proves the variable is non-empty."""
offenders = []
# Build parent links so we can walk upward from each call.
parent_of = {}
for parent in ast.walk(self.tree):
for child in ast.iter_child_nodes(parent):
parent_of[id(child)] = parent
def is_inside_truthy_check(node):
cur = parent_of.get(id(node))
while cur is not None:
if isinstance(cur, ast.If):
test = cur.test
# Accept `if sid:` and `if some_var:` (Name node)
# and `if sid is not None:` style.
if isinstance(test, ast.Name):
return True
if (
isinstance(test, ast.Compare)
and len(test.ops) == 1
and isinstance(test.ops[0], (ast.IsNot, ast.NotEq))
and isinstance(test.comparators[0], ast.Constant)
and test.comparators[0].value is None
):
return True
if (
isinstance(test, ast.UnaryOp)
and isinstance(test.op, ast.Not)
):
# `if not sid: ... else: send_sync(...)` is
# also an acceptable guard.
return True
cur = parent_of.get(id(cur))
return False
for call in _iter_send_sync_calls(self.tree):
event = _first_arg_is_credential_event(call)
if event is None:
continue
if not is_inside_truthy_check(call):
offenders.append(
f"send_sync({event!r}, ...) at line {call.lineno} "
"is not inside an `if sid:` (or equivalent) guard. "
"Without the guard, a falsy/missing client_id "
"would silently broadcast credentials."
)
if offenders:
self.fail(
"Credential events must be inside a truthy-sid check. "
"Offenders:\n " + "\n ".join(offenders)
)
class TestSetupNodeHasNoCredentialInputs(unittest.TestCase):
"""The Setup Node's INPUT_TYPES must not declare app_key,
app_secret, or auth_code as workflow inputs. The whole point of
the modal architecture is that these never enter the prompt JSON;
if they reappear in INPUT_TYPES, every leak vector we closed
(workflow JSON, PNG metadata, /history, etc.) silently reopens."""
FORBIDDEN_INPUT_NAMES = frozenset({"app_key", "app_secret", "auth_code"})
@classmethod
def setUpClass(cls):
with open(_SETUP_NODE_PATH, "r", encoding="utf-8") as f:
cls.source = f.read()
cls.tree = ast.parse(cls.source, filename=_SETUP_NODE_PATH)
def test_input_types_does_not_declare_credential_fields(self):
# Find INPUT_TYPES in DropSendSetupNode. It returns a dict
# literal of dicts; we check the keys at the inner level.
offenders = []
for node in ast.walk(self.tree):
if not isinstance(node, ast.FunctionDef):
continue
if node.name != "INPUT_TYPES":
continue
for sub in ast.walk(node):
if not isinstance(sub, ast.Dict):
continue
for key in sub.keys:
if (
isinstance(key, ast.Constant)
and isinstance(key.value, str)
and key.value in self.FORBIDDEN_INPUT_NAMES
):
offenders.append(
f"{key.value!r} appears as a key in an INPUT_TYPES "
f"dict at line {key.lineno}. Setup-Node credentials "
"must NOT be workflow inputs — they go through the "
"modal + stash route, never the prompt body."
)
if offenders:
self.fail(
"Forbidden credential fields declared as workflow inputs:\n "
+ "\n ".join(offenders)
)
if __name__ == "__main__":
unittest.main(verbosity=2)