Skip to content

Comments

Add prompt format string for SSL/TLS version#1601

Open
rolandwalker wants to merge 1 commit intomainfrom
RW/add-prompt-escape-for-tls-version
Open

Add prompt format string for SSL/TLS version#1601
rolandwalker wants to merge 1 commit intomainfrom
RW/add-prompt-escape-for-tls-version

Conversation

@rolandwalker
Copy link
Contributor

Description

\T will show the TLS version for the connection, or (none) when appropriate.

The negotiated version seems to require a trip to the server, but get_ssl_version() has been cached per thread_id so that we don't need to make that trip for every prompt refresh.

xref #1592

Checklist

  • I added this contribution to the changelog.md file.
  • I added my name to the AUTHORS file (or it's already there).
  • To lint and format the code, I ran
    uv run ruff check && uv run ruff format && uv run mypy --install-types .

@rolandwalker rolandwalker self-assigned this Feb 21, 2026
@github-actions
Copy link

Findings

  • High: Cache key is only thread_id, which is not globally unique across servers or across reconnects. If a user connects to multiple servers or reconnects later and the new connection gets a reused thread id, \T can show a stale TLS version from a different connection. Consider keying by connection identity (e.g., id(connection) or (host, port, thread_id)), and clearing on disconnect. mycli/packages/special/utils.py
  • Medium: Prompt rendering now depends on a server query when \T is used. If SHOW STATUS is not permitted, this will raise during prompt rendering and break the prompt flow, not just the status command. Consider catching DB errors in get_ssl_version and returning None so \T renders as (none). mycli/packages/special/utils.py, mycli/main.py

Missing tests / edge cases

  • Add a unit test for \T substitution to ensure get_prompt renders (none) when get_ssl_version returns None. mycli/main.py
  • Add a test (mocked) to ensure cache does not bleed across distinct connections with the same thread_id. mycli/packages/special/utils.py

Notes

  • No obvious security issues beyond the above error handling.

If you want, I can draft a small test using a mocked cursor/connection to cover the cache key behavior.

@rolandwalker rolandwalker force-pushed the RW/add-prompt-escape-for-tls-version branch from 797de0a to aa7e206 Compare February 21, 2026 12:13
@rolandwalker rolandwalker force-pushed the RW/add-prompt-escape-for-tls-version branch 2 times, most recently from b7608a2 to 66e47af Compare February 21, 2026 13:02
@rolandwalker rolandwalker added the codex request Codex review for an updated PR label Feb 21, 2026
@github-actions
Copy link

Findings

  1. Stale SSL version across reconnects/server restarts
    get_ssl_version() caches by thread_id only and is never cleared. If the server restarts or a new connection reuses a thread_id, \T can show the previous connection’s TLS version. Consider clearing the cache on reconnect/disconnect or keying by connection object identity in addition to thread_id.
    File: mycli/packages/special/utils.py

  2. Prompt can crash if connection is closed but sqlexecute.conn is still set
    get_prompt() unconditionally opens a cursor when \T is present and sqlexecute.conn is non-None. If the connection has dropped but the object remains, cursor()/execute() will raise and break prompt rendering. Consider checking connection liveness (e.g., conn.open, conn.ping(reconnect=False), or a try/except fallback to (none)).
    File: mycli/main.py

Missing tests / edge cases

  • Add a unit test for \T prompt rendering when TLS is present and when it’s not (expect (none)).
  • Add a test for prompt rendering when the connection object exists but is closed or invalid (should not raise; should fall back to (none)).

If you want, I can suggest concrete test additions in test/test_main.py.

@rolandwalker rolandwalker force-pushed the RW/add-prompt-escape-for-tls-version branch 2 times, most recently from 151f427 to 665e34d Compare February 21, 2026 13:30
"\T" will show the TLS version for the connection, or "(none)" when
appropriate.

The negotiated version seems to require a trip to the server, but
get_ssl_version() has been cached per thread_id so that we don't need
to make that trip for every prompt refresh.

We also make the SSL version cache resistant to collisions by caching
on the connection id as well as the thread id.

Since the get_ssl_version() might get called on any prompt, we also
wrap the query in a try block.
@rolandwalker rolandwalker force-pushed the RW/add-prompt-escape-for-tls-version branch from 665e34d to 86ef86a Compare February 21, 2026 13:32
@rolandwalker rolandwalker removed the codex request Codex review for an updated PR label Feb 21, 2026
Copy link
Contributor

@scottnemes scottnemes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified new \T prompt value works with and without SSL enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants