Skip to content

server: fix OK session-track suffix and validate SetCapability#1167

Merged
lance6716 merged 3 commits into
go-mysql-org:masterfrom
nzin-alayacare:fix/server-ok-packet-and-capability-validation
Jul 10, 2026
Merged

server: fix OK session-track suffix and validate SetCapability#1167
lance6716 merged 3 commits into
go-mysql-org:masterfrom
nzin-alayacare:fix/server-ok-packet-and-capability-validation

Conversation

@nzin-alayacare

Copy link
Copy Markdown
Contributor

Summary

  • P1 — OK packet session track: When CLIENT_SESSION_TRACK is negotiated but SERVER_SESSION_STATE_CHANGED is not set, writeOK no longer appends a zero-length session-state block after the info string. The packet now ends after the length-encoded info string, matching the MySQL OK-packet format expected by strict clients (auth OK, normal OK).
  • P2 — SetCapability safety: Server.SetCapability / UnsetCapability now return an error unless the flag is one of CLIENT_LOCAL_FILES, CLIENT_MULTI_RESULTS, or CLIENT_PS_MULTI_RESULTS. Flags such as CLIENT_SSL remain tied to server construction (tlsConfig) and cannot be enabled independently.

Motivation

Follow-up to #1163: the new capability setter API and session-track OK encoding had two correctness issues reported in review.

Changes

  • mysql/result.goAppendOKSessionTrackSuffix only appends the session-state block when SERVER_SESSION_STATE_CHANGED is set in r.Status.
  • server/resp.go — simplified writeOK to delegate to AppendOKSessionTrackSuffix.
  • server/server_conf.go — whitelist validation for SetCapability / UnsetCapability; methods now return error.

Test plan

  • TestAppendOKSessionTrackSuffixWithoutStateChange — info string only, no session block
  • TestConnWriteOKStatusMessageWithoutStateChange — updated expected packet (no trailing 0x00 session block)
  • TestConnWriteOKSessionTrackWithoutStateChangeOrInfo — auth-style OK with empty info
  • TestSetCapabilityRejectsUnsafeFlagsCLIENT_SSL and CLIENT_PROTOCOL_41 rejected; whitelisted flags work

Made with Cursor

Omit the session-state block from OK packets unless SERVER_SESSION_STATE_CHANGED
is set, even when CLIENT_SESSION_TRACK is negotiated. An empty session-state
length byte after the info string breaks strict clients on auth and normal OKs.

Restrict Server.SetCapability and UnsetCapability to a whitelist of
CLIENT_LOCAL_FILES, CLIENT_MULTI_RESULTS, and CLIENT_PS_MULTI_RESULTS so flags
like CLIENT_SSL cannot be enabled without a matching tlsConfig.

Co-authored-by: Cursor <cursoragent@cursor.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors OK-packet session tracking by conditionally appending the session track block based on the SERVER_SESSION_STATE_CHANGED status flag. It also restricts SetCapability and UnsetCapability to only allow toggling of user-configurable capabilities (CLIENT_LOCAL_FILES, CLIENT_MULTI_RESULTS, and CLIENT_PS_MULTI_RESULTS), returning an error for unsafe flags. The feedback suggests improving the capability validation error message to isolate and report the specific invalid flags when a bitmask with multiple flags is provided.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread server/server_conf.go Outdated
nzin-alayacare and others added 2 commits July 3, 2026 05:32
…rror

When a bitmask mixes user-configurable and unsafe flags, include the
isolated invalid mask in the error message for easier debugging.

Co-authored-by: Cursor <cursoragent@cursor.com>
@nzin-alayacare

Copy link
Copy Markdown
Contributor Author

Hi @lance6716 ,

I dont have other channel to interact with you, but thanks for your responsiveness, and your follow ups.
When this one will be resolved, do you have an ETA for a new release of go-mysql? ;-)

@lance6716

Copy link
Copy Markdown
Collaborator

You can contact me through email. But GitHub activities also notify me by email, so a manual email won't change much :P
I'm a Chinese developer, so I guess we don't have common daily IM apps.

Releases are mostly done by @dveeden. If he's busy these days, I can do the release as well. There's no fixed schedule. We just release new versions according to requests.

For this PR I will take a look later.

@dveeden

dveeden commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

I'll try to prepare a new release by early next week

@lance6716 lance6716 merged commit f6b4b9c into go-mysql-org:master Jul 10, 2026
20 checks passed
dbnski pushed a commit to dbnski/go-mysql that referenced this pull request Jul 10, 2026
…sql-org#1167)

* server: fix OK session-track suffix and validate SetCapability

Omit the session-state block from OK packets unless SERVER_SESSION_STATE_CHANGED
is set, even when CLIENT_SESSION_TRACK is negotiated. An empty session-state
length byte after the info string breaks strict clients on auth and normal OKs.

Restrict Server.SetCapability and UnsetCapability to a whitelist of
CLIENT_LOCAL_FILES, CLIENT_MULTI_RESULTS, and CLIENT_PS_MULTI_RESULTS so flags
like CLIENT_SSL cannot be enabled without a matching tlsConfig.

Co-authored-by: Cursor <cursoragent@cursor.com>

* server: report invalid capability flags separately in SetCapability error

When a bitmask mixes user-configurable and unsafe flags, include the
isolated invalid mask in the error message for easier debugging.

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: lance6716 <lance6716@gmail.com>
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.

3 participants