Fix four latent bugs found by audit, and make the shared-secret check reachableg fixes - #128
Open
ispielma wants to merge 5 commits into
Open
Fix four latent bugs found by audit, and make the shared-secret check reachableg fixes#128ispielma wants to merge 5 commits into
ispielma wants to merge 5 commits into
Conversation
get_config() falls back to zprocess.zlock.DEFAULT_PORT, an int, whenever
the labconfig has no [ports] zlock entry. That value was placed directly
into the argv list passed to subprocess, which accepts only str, bytes or
os.PathLike:
TypeError: expected str, bytes or os.PathLike object, not int
So labscript-zlock could not start from a labconfig that omits the port.
The sibling launchers zlog.py and remote.py already wrap their ports with
str(); do the same here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
set_logger() stashed the currently-installed handler into
warnings._showwarning before installing logwarning. On a second
set_logger() call the stash therefore captured logwarning itself, so
logwarning recursed into itself and the next warning raised:
RecursionError: maximum recursion depth exceeded
This is reachable whenever a process installs the labscript excepthook
more than once, such as a subprocess that re-runs application startup or
an app that reconfigures logging.
Capture the real handler once at module import into a module-level name
instead, which is also idempotent and stops writing to a private-looking
attribute of the warnings module.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_default() is json.dumps' fallback for objects it cannot encode, and only
handled np.integer. np.float64 survives by accident because it subclasses
Python float, which masked the gap for the other numpy scalar types:
int64 ok
float64 ok
float32 TypeError
bool_ TypeError
Any device or plugin writing an np.float32 or np.bool_ into shot
properties therefore failed in serialise(). Add the two missing branches.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comment directly above states the intent: only insert the
security-related kwargs when the socket is going to be a SecureSocket.
The test used SecureContext, which is a Context class and can never be a
socket class, so the branch was unreachable for any explicit
socket_class:
issubclass(zmq.Socket, SecureContext) = False SecureSocket = False
issubclass(SecureSocket, SecureContext) = False SecureSocket = True
The common pyzmq 25 path, where ThreadAuthenticator passes zmq.Socket,
gives the intended result either way, which is why this went unnoticed.
A caller that explicitly passes SecureSocket, however, silently lost its
allow_insecure configuration.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard that raises _ERR_NO_SHARED_SECRET was nested inside the except
clause, so it only ran when [security] allow_insecure was absent from the
labconfig. A labconfig that explicitly sets
[security]
allow_insecure = False
with no shared_secret skipped the check entirely. Inside the except the
condition was also dead weight, since allow_insecure had just been set to
False on the line above.
_ERR_NO_SHARED_SECRET states the intended contract: configure a
shared_secret, or opt out with allow_insecure = True. Explicitly writing
allow_insecure = False without a secret is precisely the misconfiguration
the message exists to catch, so run the guard on both paths.
BEHAVIOUR CHANGE: an installation that explicitly sets allow_insecure =
False with no shared_secret now fails at startup with the message above
instead of continuing. Such a setup previously worked as long as all
communication stayed on loopback, because zprocess only enforces this at
send time (zprocess/security.py). Those users must now either supply a
shared secret or set allow_insecure = True. This commit is deliberately
kept separate so it can be dropped if that trade-off is unwanted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five independent bug fixes turned up while Claude audited this repository. Each is a
separate commit so any one can be dropped.
Four are unambiguous defects. The fifth (the last) is a real bug whose fix
changes behaviour for one configuration, and I have flagged it clearly so you
can decide.
1.
zlock.pypasses anintport tosubprocessget_config()falls back tozprocess.zlock.DEFAULT_PORT, anint, when thelabconfig has no
[ports] zlockentry, and that value goes straight into anargv list:
So
labscript-zlockcannot start from a labconfig that omits the port.zlog.pyandremote.pyalready wrap their ports withstr().2.
excepthook.set_logger()recurses if called twiceset_logger()stashed the currently-installed handler intowarnings._showwarningbefore installinglogwarning. On a second call thestash captures
logwarningitself, so it recurses into itself and the nextwarning raises
RecursionError. Reachable whenever a process installs theexcepthook more than once. Capturing the real handler once at import is also
idempotent, and stops writing to a private-looking name in
warnings.3.
properties._default()cannot serialisenp.floatingornp.bool__default()only handlednp.integer.np.float64survives by accidentbecause it subclasses Python
float, which masked the gap:Any device or plugin writing an
np.float32ornp.bool_into shotproperties fails in
serialise().4.
Context.socket()tests againstSecureContext, notSecureSocketThe comment directly above says the intent is to insert the security kwargs
only when the socket will be a
SecureSocket. The test usedSecureContext,a Context class that can never be a socket class:
The common pyzmq 25 path, where
ThreadAuthenticatorpasseszmq.Socket,gives the intended answer either way — which is why this went unnoticed. A
caller explicitly passing
SecureSocketsilently lost itsallow_insecureconfiguration.
5. Shared-secret guard is unreachable when
allow_insecureis configured — please readThe guard raising
_ERR_NO_SHARED_SECRETsits inside theexceptclause, soit only runs when
[security] allow_insecureis absent. A labconfig thatexplicitly sets
with no
shared_secretskips it entirely. Inside theexceptthe conditionwas also dead weight, since
allow_insecurehad just been set toFalseonthe line above.
_ERR_NO_SHARED_SECRETstates the intended contract — configure ashared_secret, or opt out withallow_insecure = True— so explicitlywriting
allow_insecure = Falsewithout a secret is exactly themisconfiguration the message exists to catch.
This changes behaviour. An installation with
allow_insecure = Falseandno shared secret now fails at startup with that message instead of continuing.
Such a setup previously worked as long as all traffic stayed on loopback,
because zprocess only enforces this at send time
(
zprocess/security.py). Those users would need to supply a shared secret orset
allow_insecure = True.It is the last commit on the branch, so drop
635aea9if you would rather nottake that trade-off; the other four are independent of it.