fix: verify the server certificate on websocket connections - #27
Merged
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
This was referenced Sep 12, 2026
cigamit
pushed a commit
that referenced
this pull request
Sep 13, 2026
Using this package as a library meant unverified TLS, silently. `Connection.__init__` took `verify=False`, and the value that actually governs the normal path, `config.assume_untrusted`, defaulted to `True`. So `Page(...)` built a connection with verification off, and the constructor then called `disable_warnings()` so nothing said as much. A library that talks to an Ascender over HTTPS was not checking who answered. Two defaults flip: - `Connection(server, verify=True)`. - `config.assume_untrusted` now defaults to `False`, and reads `ASCENDERKIT_ASSUME_UNTRUSTED` for the opt-out, following the same pattern as `ASCENDERKIT_SESSIONS` and `ASCENDERKIT_PREVENT_TEARDOWN`. There was no environment variable for it before, only the config dictionary. This is a behaviour change for library consumers and is in the changelog as one. Anyone pointed at an Ascender with a self-signed certificate sets the variable, or passes `verify=False` explicitly, and is back where they were. **The CLI is unaffected.** `CLI.connect()` already sets `config.assume_untrusted = False` and only sets it `True` for `-k` / `--conf.insecure`, so it has always verified by default. This brings the library in line with the CLI rather than changing the CLI. Out of scope, and worth its own change: `ascenderkit/cli/__init__.py:16` calls `urllib3.disable_warnings(InsecureRequestWarning)` unconditionally at import, so even a verifying connection has the warning suppressed. And `ws.py` hardcodes `ssl.CERT_NONE` regardless of any of this, which #27 covers. Note for whoever merges: #25 also touches this constructor, so whichever lands second needs a rebase. Verified with `black --check`, `flake8` and the unit suite, 355 passing.
cigamit
previously approved these changes
Sep 13, 2026
`WSClient.connect()` passed a fixed `{"cert_reqs": ssl.CERT_NONE}` to
`run_forever`, so the websocket never verified the server certificate and there
was no way to make it. It ignored `config.assume_untrusted`, which the HTTP
connection reads, so a caller that had deliberately turned verification on still
got an unverified socket for the event stream, carrying a session cookie or a
bearer token.
`WSClient` now takes `verify=None`, resolving to `not config.assume_untrusted`
when it is not passed, which is the same expression `Page` uses to build its
`Connection`. When verifying, `connect()` passes no `sslopt` at all, so
websocket-client applies its own defaults and checks both the chain and the
hostname. When not verifying, it sends `cert_reqs=ssl.CERT_NONE` with
`check_hostname=False`, which is what the old behaviour actually was.
Four tests added: the default follows the config in both directions, and an
explicit `verify=` overrides it in both directions.
Worth knowing while reviewing: the CLI never reaches this code. `--monitor` is
HTTP polling through `monitor()` and `monitor_workflow()`, so `WSClient` is a
library entry point only. That limits the blast radius, and it is also why this
went unnoticed.
Verified with `black --check`, `flake8` and the unit suite, 359 passing.
blaipr
added a commit
to blaipr/ascender-kit
that referenced
this pull request
Sep 13, 2026
`WSClient._should_subscribe_to_pending_job` holds `False` until a pending subscription is queued and a dict afterwards, and three places subscript it. The subscripts are safe, because the only path that reaches them tests the flag first, but the test was buried:
```python
if all([message.get('group_name') == 'jobs', message.get('status') == 'pending', message.get('unified_job_id'), self._should_subscribe_to_pending_job]):
if bool(message.get('project_id')) == (self._should_subscribe_to_pending_job['events'] == 'project_update_events'):
self._update_subscription(message['unified_job_id'])
```
Four unrelated conditions in an `all([...])`, one of them the guard for the line below it, and `_update_subscription` then reaching back for the attribute a second time rather than being handed it.
Three changes, none of which alters behaviour:
- The sentinel is `None` rather than `False`, annotated `dict | None`. Both are falsy and nothing compares it by identity or to `False`, so every existing check behaves the same. `None` is what "not set yet" means.
- The flag is bound to a local and tested first, with `and` instead of `all([...])`. Short-circuiting rather than eager evaluation, which is fine here since every element is a pure `.get()`.
- `_update_subscription` takes the dict as an argument instead of re-reading the attribute, so it cannot be called in a state where that attribute is unset.
Exercised the whole path directly, since the unit suite covers the callbacks but not this branch: queueing with `subscribe_to_pending_events('job_events')`, then feeding a pending-job message through `_on_message`, resubscribes with `{'jobs': ['status_changed'], 'job_events': [7]}` and clears the sentinel back to `None`.
Four diagnostics retired. Diffed the full list before and after: strict subset, nothing introduced.
Note for whoever merges: ctrliq#27 also edits this file, so whichever lands second needs a rebase.
Verified with `black --check`, `flake8` and the unit suite, 355 passing.
blaipr
force-pushed
the
fix/ws-tls-verification
branch
from
September 13, 2026 09:03
95c07e0 to
7c2a4df
Compare
cigamit
approved these changes
Sep 13, 2026
cigamit
pushed a commit
that referenced
this pull request
Sep 13, 2026
`WSClient._should_subscribe_to_pending_job` holds `False` until a pending subscription is queued and a dict afterwards, and three places subscript it. The subscripts are safe, because the only path that reaches them tests the flag first, but the test was buried:
```python
if all([message.get('group_name') == 'jobs', message.get('status') == 'pending', message.get('unified_job_id'), self._should_subscribe_to_pending_job]):
if bool(message.get('project_id')) == (self._should_subscribe_to_pending_job['events'] == 'project_update_events'):
self._update_subscription(message['unified_job_id'])
```
Four unrelated conditions in an `all([...])`, one of them the guard for the line below it, and `_update_subscription` then reaching back for the attribute a second time rather than being handed it.
Three changes, none of which alters behaviour:
- The sentinel is `None` rather than `False`, annotated `dict | None`. Both are falsy and nothing compares it by identity or to `False`, so every existing check behaves the same. `None` is what "not set yet" means.
- The flag is bound to a local and tested first, with `and` instead of `all([...])`. Short-circuiting rather than eager evaluation, which is fine here since every element is a pure `.get()`.
- `_update_subscription` takes the dict as an argument instead of re-reading the attribute, so it cannot be called in a state where that attribute is unset.
Exercised the whole path directly, since the unit suite covers the callbacks but not this branch: queueing with `subscribe_to_pending_events('job_events')`, then feeding a pending-job message through `_on_message`, resubscribes with `{'jobs': ['status_changed'], 'job_events': [7]}` and clears the sentinel back to `None`.
Four diagnostics retired. Diffed the full list before and after: strict subset, nothing introduced.
Note for whoever merges: #27 also edits this file, so whichever lands second needs a rebase.
Verified with `black --check`, `flake8` and the unit suite, 355 passing.
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
3 participants
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.
WSClient.connect()passed a fixed{"cert_reqs": ssl.CERT_NONE}torun_forever, so the websocket never verified the server certificate and there was no way to make it. It ignoredconfig.assume_untrusted, which the HTTP connection reads, so a caller that had deliberately turned verification on still got an unverified socket for the event stream, carrying a session cookie or a bearer token.WSClientnow takesverify=None, resolving tonot config.assume_untrustedwhen it is not passed, which is the same expressionPageuses to build itsConnection. When verifying,connect()passes nossloptat all, so websocket-client applies its own defaults and checks both the chain and the hostname. When not verifying, it sendscert_reqs=ssl.CERT_NONEwithcheck_hostname=False, which is what the old behaviour actually was.Four tests added: the default follows the config in both directions, and an explicit
verify=overrides it in both directions.Worth knowing while reviewing: the CLI never reaches this code.
--monitoris HTTP polling throughmonitor()andmonitor_workflow(), soWSClientis a library entry point only. That limits the blast radius, and it is also why this went unnoticed.Verified with
black --check,flake8and the unit suite, 359 passing.