fix: using ActiveDirectoryServicePrincipalAccessToken does not set password in connection URL

[Linkgoron]

Problem:

I know it's technically undocumented, but I saw that there's a PR for adding it to the docs. I tried using it, and faced an issue "Must provide 'password' parameter when using ActiveDirectoryServicePrincipalAccessToken authentication".

When using --authentication-method ActiveDirectoryServicePrincipalAccessToken, the access token passed via -P or SQLCMDPASSWORD is read into connect.Password but never included in the connection URL. ConnectionString() in pkg/sqlcmd/connect.go:128 only sets connectionURL.User for SqlPassword, ActiveDirectoryPassword, ActiveDirectoryServicePrincipal, and ActiveDirectoryApplication.

Since ActiveDirectoryServicePrincipalAccessToken is missing from this condition, the password field in the URL is empty, and go-mssqldb rejects it with the above error.

The downstream GetTokenBasedConnection appears to add fedauth to the query string, and the driver gets params["password"] from the URL userinfo, so I believe that the only gap is that the password never reaches the URL.

Fix:
Add azuread.ActiveDirectoryServicePrincipalAccessToken to the existing condition on line 128 of pkg/sqlcmd/connect.go.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes a bug where using ActiveDirectoryServicePrincipalAccessToken authentication did not include the password in the connection URL, causing the driver to reject the connection.

Changes:

• Added azuread.ActiveDirectoryServicePrincipalAccessToken to the condition that sets the connection URL user/password in ConnectionString().

[dlevy-msft-sql]

@Linkgoron - Thanks for the catch and the clear writeup the diagnosis is spot on. The ActiveDirectoryServicePrincipalAccessToken branch in go-mssqldb''s azuread/configuration.go reads params["password"] and errors out if empty, so the access token has to land in the URL userinfo just like the other four auth methods already in this condition. The fix is minimal and correct.

Before merging, could you add a unit test in pkg/sqlcmd/connect_test.go that calls ConnectionString() with ActiveDirectoryServicePrincipalAccessToken and asserts the password makes it into the returned URL? The bug was a silent omission from a list, so a regression test would help prevent the next auth method from slipping through the same way.

[Linkgoron]

@dlevy-msft-sql added tests, and also fixed RequiresPassword (which I missed originally).

[Linkgoron]

@microsoft-github-policy-service agree company="Eon.io"

[dlevy-msft-sql]

Thanks for the thorough follow-up — and for catching the RequiresPassword gap I missed. The tests are well-scoped: I reverted both code changes locally and confirmed the targeted ActiveDirectoryServicePrincipalAccessToken subtests fail in each case, so they genuinely exercise the fix rather than just shadowing existing behavior. LGTM.