fix: support MySQL usernames containing @ - #27
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: fix: support MySQL usernames containing @Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit addresses all three prior findings: Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Manual verification against a live MySQL 8.0 containerSpun up SetupCREATE USER 'someone@orion.com'@'%' IDENTIFIED BY 'Passw0rd!';
GRANT SELECT, INSERT ON testdb.* TO 'someone@orion.com'@'%';
CREATE ROLE 'app_reader';
GRANT SELECT ON testdb.* TO 'app_reader';
GRANT 'app_reader' TO 'someone@orion.com'@'%';Read path (the originally reported bug)Ran a full
Reverting the fix (checking out Write path (provisioning)Wrote a throwaway test exercising the
All operations round-tripped correctly end-to-end for a username containing Automated checks |
All three blocking findings from this review were fixed in commit 1bc5821 (column grant/revoke escaping, backslash removed from validUserHost, TrimPrefix-based ID parsing in Delete) and verified against a live MySQL container. The bot re-reviewed at 2026-08-21T17:06:22Z and reported "No blocking issues found." Dismissing this stale CHANGES_REQUESTED review since it only supports COMMENTED reviews, not approvals.
All findings from this review round were fixed: idx<=0 -> idx<0 regression fix for the anonymous MySQL account (commit 8857601), escapeMySQLUserHost zero-or-more fix for the same account's empty username (commit c01af17), and the roles.go error-wrapping suggestion (commit 8857601). Verified against a live MySQL container including full Grant/Revoke/DropUser round-trip against a real ''@'localhost' account. The bot's own follow-up review at 2026-08-21T21:36:23Z reported "No blocking issues found." Dismissing this stale CHANGES_REQUESTED review since it only supports COMMENTED reviews, not approvals.
MySQL usernames can legally contain "@" (e.g. an account named after an email address), but the connector's user@host composite ID was built as "user@host" and then reassembled everywhere by naively splitting on every "@" and requiring exactly 2 parts. A username like "someone@orion.com" produced an ID like "someone@orion.com@%", which failed to parse and aborted sync during grant processing with "malformed principal ID". Add client.SplitUserHost, which splits on the *last* "@" instead (MySQL host specs never contain "@", so this is unambiguous), and use it at every user@host parsing site: grant/revoke for databases, tables, columns, routines, servers, roles, users, plus principal-ID parsing in grants listing and user deletion. Also widen the user/host identifier validation regex to allow "@". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- GrantColumnPrivilege/RevokeColumnPrivilege now validate user/host via escapeMySQLUserHost, matching every other converted call site. They previously interpolated the raw split values straight into the GRANT/ REVOKE statement, which was a SQL injection vector for a user/host containing a quote. Also wrap the SplitUserHost error with %w instead of dropping it. - validUserHost no longer matches a literal backslash. Combined with the '%s'@'%s' quoting used throughout, a name ending in "\" could escape the closing quote under MySQL's default (non-NO_BACKSLASH_ESCAPES) sql_mode. Backslash was already allowed before this PR; this was a good moment to drop it while rewriting the character class. - userSyncer.Delete now derives the composite ID via TrimPrefix on the resource type, matching grantsForUserOrRole, instead of strings.Split(...)[1], which panics if the ID has no ":" and silently truncates names containing ":". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rs.WithUserProfile and rs.WithStatus are deprecated (profile/status moved from UserTrait to a Resource-level attribute). CI's lint check flags these regardless of whether the resource-level mirroring happens, which only applies when going through WithUserTrait/NewUserResource — this connector builds *v2.Resource via a struct literal instead, so switch to setting the resource-level fields directly via rs.WithResourceProfile/rs.WithResourceStatus. Verified no SA1019 findings remain repo-wide (golangci-lint), and that the resulting resource has HasProfile()/HasStatus() populated correctly via a standalone check against the real SDK types. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address review feedback: List() and parseIntoUserResource() previously hand-built *v2.Resource via a struct literal, duplicating the same profile-map/status boilerplate in two places and skipping the SDK's NewUserResource/WithUserTrait helper entirely. Consolidated List() to just call parseIntoUserResource() per user, and rewrote parseIntoUserResource() to build through rs.NewUserResource with WithParentResourceID/WithResourceProfile/WithResourceStatus. Verified against a live MySQL container that resources, parent linkage, display name, and the @-in-username grants still round-trip identically after the refactor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
SplitUserHost rejected idx <= 0, which also rejected the empty-name
case (idx == 0) -- but MySQL's anonymous account is a real, valid
entity of the form ''@'host'. The old strings.Split-based check
accepted it (split on "@" yields ["", host]), so this was a regression:
default MySQL/MariaDB installs ship an anonymous account, and
grantsForUserOrRole would now fail the entire sync on it. Only reject
when there's no "@" at all (idx < 0) or the host half is empty.
Also wrap the SplitUserHost error with %w in GrantRolePrivilege/
RevokeRolePrivilege instead of discarding it, matching every other
converted call site.
Verified against a live MySQL 8.0 container with an actual anonymous
account (''@'localhost'): sync succeeds and its grant is correctly
attributed to principal "@localhost".
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
SplitUserHost now accepts the empty username of MySQL's anonymous
account (''@'host'), but escapeMySQLUserHost's regex still required
one-or-more characters, so Grant/Revoke/CreateUser/DropUser against
that account failed one step later with "invalid user/host: ".//
Every call site feeds escapeMySQLUserHost values derived from
SplitUserHost, which already guarantees the host half is non-empty, so
loosening the regex to zero-or-more only ever affects the anonymous
account's empty username -- it can't accidentally allow an empty host.
Verified against a live MySQL 8.0 container: GrantDatabasePrivilege,
RevokeDatabasePrivilege, and DropUser all now succeed against a real
''@'localhost' account.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
c01af17 to
8abb097
Compare
… hosts
Three findings from review:
- CreateAccount built client.User without UserType, so GetID() returned
":user@host" instead of "user:user@host". The old
strings.Split(id, ":")[1] in Delete tolerated that; TrimPrefix does
not, so the ID stayed ":user@host", SplitUserHost yielded ":user",
and escapeMySQLUserHost rejected the ":" -- deleting a freshly
provisioned account failed. The malformed ID was a pre-existing
problem in its own right; setting UserType fixes both.
- CreateAccount took username from the account profile with only a type
assertion. Now that escapeMySQLUserHost accepts the empty string, an
empty username would provision MySQL's anonymous account (''@'host').
Guard against it: empty names are legitimate to read and delete, never
to create.
- validUserHost rejected ":" and "/", which are legal in MySQL host
specs -- IPv6 literals (the stock root@::1) and netmask forms
(198.51.100.0/255.255.255.0). Such accounts synced but every
grant/revoke/drop against them failed with "invalid user/host". Both
characters are inert inside the single-quoted '%s'@'%s' the callers
build; "'" and "\" remain excluded.
Verified against a live MySQL 8.0 container: grant/revoke round-trips
for a v6user@::1 and a netuser@198.51.100.0/255.255.255.0 account, the
CreateAccount composite-ID round trip through to DropUser, and that
quote-injection and trailing-backslash inputs are still rejected. Full
sync over all four edge-case account shapes exits clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All three findings from this review were fixed in commit ea0c493 and verified against a live MySQL 8.0 container: (1) CreateAccount now sets UserType so GetID() yields a parseable 'user:name@host' composite ID -- this was a real bug, reproduced and confirmed; (2) CreateAccount now guards against an empty username so the anonymous account can never be provisioned; (3) validUserHost now permits ':' and '/' for IPv6 literals and netmask host specs, while still rejecting quote and backslash. Each thread has been replied to and resolved.
| // literals (the stock root@::1) and netmask forms (198.51.100.0/255.255.255.0); | ||
| // both are inert inside the single-quoted '%s'@'%s' the callers build. "'" and | ||
| // "\" stay excluded, as those are what could break out of that quoting. | ||
| var validUserHost = regexp.MustCompile(`^[a-zA-Z0-9_%.@:/\-]*$`) |
There was a problem hiding this comment.
🟡 Suggestion: Widening the regex unblocks IPv6 at the escape layer, but the connector-side principal-ID parsing still splits on :, so IPv6 hosts remain unreachable for grant/revoke. For user:root@::1, pkg/connector/server.go:77, database.go:85, and column.go:80 take Split(id, ":")[1] → "root@" (then SplitUserHost errors), while table.go:91, routine.go:106, role.go:96, and column.go:105 hit their len(parts) != 2 guard and return invalid principal ID. Netmask hosts are fine since / isn't a separator. Consider strings.TrimPrefix(id, resourceType+":") at those sites, as already done in grants.go:25 and user.go:179.
1 participant
Summary
@(e.g. accounts named after an email address likesomeone@orion.com). The connector built itsuser@hostcomposite resource ID viafmt.Sprintf("%s@%s", user, host), then re-parsed it everywhere by splitting on every@and requiring exactly 2 parts. For a username containing@, this produced ambiguous IDs (e.g.someone@orion.com@%) that failed the parts-count check, aborting sync withmalformed principal IDduring grant processing — even though role/entitlement listing had already succeeded.client.SplitUserHost, which splits on the last@instead — MySQL host specifications (hostnames, IPs, netmasks,%wildcards) never contain@, so this unambiguously recovers(user, host)even when the username itself has one or more@characters.pkg/connector/grants.go) and user deletion (pkg/connector/user.go), plus grant/revoke for databases, tables, columns, routines, servers, roles, and user create/drop (pkg/client/*.go).validUserHostidentifier-validation regex to permit@(previously any@-containing username would still be rejected here even if parsing succeeded).Test plan
go build ./...go vet ./...go test ./... -count=1(existing suite + newpkg/client/helper_test.gocoveringSplitUserHostandescapeMySQLUserHost, including usernames with one or multiple embedded@, comma-separated collapsed hosts, and malformed-input error cases)🤖 Generated with Claude Code