[azwebpubsub] Fix GenerateClientAccessURL on the TokenCredential path - #27422
Open
Liangying.Wei (vicancy) wants to merge 1 commit into
Open
[azwebpubsub] Fix GenerateClientAccessURL on the TokenCredential path#27422Liangying.Wei (vicancy) wants to merge 1 commit into
Liangying.Wei (vicancy) wants to merge 1 commit into
Conversation
`GenerateClientAccessURL` had three defects that made it unusable for clients created with `NewClient` (Entra ID): - It panicked with a nil-pointer dereference when `options` was nil, because `options.UserID` was read without a nil check. The key-based path handled nil options correctly. - It always forwarded `ExpirationTimeInMinutes` to the service, so the default zero value was sent as `minutesToExpire=0`. The service rejects that with `400 Error.BadRequest: MinutesToExpire must be greater than 0`. It now falls back to the same 60 minute default the key-based path uses. - It appended `client/hubs/<hub>` directly to the endpoint without normalizing a trailing slash, producing a malformed audience and client URL such as `wss://<host>client/hubs/<hub>`. `ParseConnectionString` adds the trailing slash, but an endpoint passed to `NewClient` may not have one. The other Azure SDK languages normalize this inside the token method, and this change does the same. A negative `ExpirationTimeInMinutes` is now also rejected on both credential types rather than only the key-based one. Note that the audience scheme is intentionally left as `http(s)`. The service normalizes the scheme and validates the host and path, and every other Azure SDK language builds the audience the same way. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf666f99-8d30-4763-878d-b566c0ac2f2d
Liangying.Wei (vicancy)
requested review from
Ken Chen (chenkennt) and
Joel Hendrix (jhendrixMSFT)
as code owners
August 20, 2026 04:58
|
Azure Pipelines: Successfully started running 1 pipeline(s). 6 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes GenerateClientAccessURL behavior for Entra ID clients and normalizes generated WebSocket URLs.
Changes:
- Handles nil options, default expiration, and negative values consistently.
- Normalizes endpoint trailing slashes.
- Adds regression tests and changelog entries.
Show a summary per file
| File | Description |
|---|---|
client_custom.go |
Corrects URL generation and option handling. |
client_custom_test.go |
Adds regression coverage for corrected behavior. |
CHANGELOG.md |
Documents the fixes. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
Ken Chen (chenkennt)
approved these changes
Aug 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
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.
Fixes #27414.
Context
Issue #27414 reports that
GenerateClientAccessURLproduces tokens the service rejects, and attributes it to theaudclaim using thehttpsscheme instead ofwss.That diagnosis is not correct — the service normalizes the scheme and validates the host and path of the audience. Verified against a live resource (Premium P1) with raw WebSocket upgrade requests, varying only the
audclaim:audclaimhttps://<host>/client/hubs/chatwss://<host>/client/hubs/chathttp://<host>/client/hubs/chathttps://<host>client/hubs/chat(missing/)https://<host>/client/hubs/otherhubhttps://other-resource.webpubsub.azure.com/client/hubs/chatEvery other Azure SDK language builds the audience the same way (.NET, JS, Python, Java all put the HTTP(S) endpoint in
audand only switch the returned URL tows(s)), so the scheme is left unchanged here.Investigating the report did, however, surface three real defects — all of which make
GenerateClientAccessURLunusable for clients created withNewClient(Entra ID).Fixes
1. Nil-pointer dereference on nil options
GenerateClientAccessURL(ctx, hub, nil)panicked, becauseoptions.UserIDwas read without a nil check on theTokenCredentialpath. The key-based path handled nil options correctly.2.
minutesToExpire=0sent by defaultExpirationTimeInMinuteswas always forwarded, so leaving it unset sentminutesToExpire=0, which the service rejects:It now falls back to the same 60 minute default the key-based path already used.
3. Missing trailing-slash normalization
client/hubs/<hub>was appended straight onto the endpoint.ParseConnectionStringappends a trailing/, but an endpoint passed toNewClientmay not have one — which is the form the portal andazsurface. The result was malformed:Normalization now happens inside the method, matching .NET/JS/Java/Python. This affects the audience of key-signed tokens too, and per the table above a missing separator is exactly the case the service rejects with a 401.
Additionally, a negative
ExpirationTimeInMinutesis now rejected on both credential types instead of only the key-based one.Testing
New
client_custom_test.gocovers all of the above using a stub transport, so theTokenCredentialpath is exercised without recordings:minutesToExpiredefaulting, and explicit values being honoreduserId/role/groupforwarded to the serviceAll tests fail against the current code (assertion failures plus the panic) and pass with the fix. The existing
TestClient_GenerateClientAccessURLFromConnectionStringcontinues to pass unchanged, and no recordings needed updating.