fix: REVOKE-vs-GRANT bug + escape ' in CREATE LOGIN passwords#38
Merged
Merged
Conversation
RevokePermissionOnDatabase was building a GRANT...TO statement instead of REVOKE...FROM, so calling the revoke path re-granted the permission instead of removing it. The neighbouring GrantPermissionOnDatabase already builds the correct GRANT statement, so this was an inconsistency, not a missing helper. While here, drop the trailing positional args (fullPermission, db, user) from the ExecContext call -- the query string is built with fmt.Sprintf and contains no ? placeholders, so the args were inert. How found: occult-go-analysis c1 profile, sql_query_via_sprintf detector flagged the function for review. The detector finding itself is a false positive (input is validated against [\]"';]), but reading the function in detail surfaced the GRANT-vs-REVOKE swap. Origin: https://github.com/conductorone/occult-go-analysis
CreateLogin builds CREATE LOGIN ... WITH PASSWORD = '%s' via
fmt.Sprintf, interpolating the caller-supplied password into a
single-quoted SQL Server string literal without escaping. A password
containing ' breaks the literal -- syntactically invalid for benign
input, a SQL injection vector if any caller passes user-controlled
text through.
SQL Server escapes ' inside string literals by doubling it (''), so
this replaces single quotes with doubled single quotes before
interpolation. The stored password byte-string still contains ' as
authored.
How found:
occult-go-analysis c1 profile, sql_query_via_sprintf detector
flagged CreateLogin. Other sprintf sites in this package validate
inputs against ["';] before interpolation, but CreateLogin did
not, leaving the password path exposed.
Origin:
https://github.com/conductorone/occult-go-analysis
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| _, err := c.db.ExecContext(ctx, command, fullPermission, db, user) | ||
| _, err := c.db.ExecContext(ctx, command) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: The sibling GrantPermissionOnDatabase (line ~126) still passes inert positional args permission, db, user to ExecContext — the same pattern cleaned up here. Consider removing them there too for consistency.
Suggested change
| if err != nil { | |
| _, err := c.db.ExecContext(ctx, command) |
robert-chiniquy
changed the title
Contributor
Connector PR Review: fix: REVOKE-vs-GRANT bug + escape ' in CREATE LOGIN passwordsBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commits address both prior suggestions. Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
Comment on lines
400
to
+407
| loginName := fmt.Sprintf("[%s]", username) | ||
| l.Debug("creating SQL login", zap.String("login", loginName)) | ||
| query = fmt.Sprintf("CREATE LOGIN %s WITH PASSWORD = '%s';", loginName, password) | ||
| // SQL Server string literals escape ' by doubling it. Without this, | ||
| // a password containing ' breaks out of the literal -- a SQL | ||
| // injection vector and at minimum a syntax error for legitimate | ||
| // users whose passwords contain '. | ||
| escapedPassword := strings.ReplaceAll(password, "'", "''") | ||
| query = fmt.Sprintf("CREATE LOGIN %s WITH PASSWORD = '%s';", loginName, escapedPassword) |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: The password escaping is correct, but username is bracket-quoted via [%s] without escaping ] — a username containing ] can break out of the identifier quoting. Other functions in this file (lines 287, 334) validate inputs against []"'; before interpolation. Consider adding the same ContainsAny guard on username here, or escaping ] → ]] inside the brackets (SQL Server's bracket-identifier escape).
MarcusGoldschmidt
approved these changes
May 20, 2026
Two additional fixes flagged by the bot review: - pkg/mssqldb/databases.go: drop the inert positional args from GrantPermissionOnDatabase's ExecContext call. The command string is built via fmt.Sprintf with no ? placeholders, so the args were never used. Mirrors the cleanup the Revoke fix already applied to its sibling. - pkg/mssqldb/users.go: reject [\]"'; in the username passed to CreateLogin. The function interpolates username into bracket- quoted SQL Server identifiers; a username containing ] would escape the brackets the same way an unescaped ' in the password would. Matches the validation pattern used by the other identifier-interpolating functions in this package.
MarcusGoldschmidt
approved these changes
May 20, 2026
laurenleach
approved these changes
May 20, 2026
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
4 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.
Two sprintf-related bugs in
pkg/mssqldb/, both surfaced via the same static-analysis detector.1. RevokePermissionOnDatabase issues REVOKE, not GRANT
RevokePermissionOnDatabasewas building aGRANT ... TOstatement instead ofREVOKE ... FROM, so calling the revoke path re-granted the permission rather than removing it. The neighbouringGrantPermissionOnDatabasealready builds the correctGRANTstatement, so this was an inconsistency, not a missing helper.While here, the trailing positional args (
fullPermission, db, user) on theExecContextcall have been dropped -- the query string is built withfmt.Sprintfand contains no?placeholders, so the args were inert.A revoke operation against this connector would observe the permission staying granted (or being re-asserted), looking like a silent revoke that didn't take effect.
2. CreateLogin: escape ' in passwords
CreateLoginbuildsCREATE LOGIN ... WITH PASSWORD = '%s'viafmt.Sprintf, interpolating the caller-supplied password into a single-quoted SQL Server string literal without escaping. A password containing'breaks the literal -- syntactically invalid for benign input, a SQL injection vector if any caller passes user-controlled text through.SQL Server escapes
'inside string literals by doubling it (''), so this replaces single quotes with doubled single quotes before interpolation. The stored password byte-string still contains'as authored.how I found these
Static analysis with occult-go-analysis, c1 profile,
sql_query_via_sprintfdetector flagged a number of functions in this package. The other flagged functions validate inputs against[]"';characters before interpolation and are fine; the two above did not, leaving them exposed. Reading the Revoke function carefully also surfaced the GRANT-vs-REVOKE swap.