Fixed a few SAS bugs in blob and file. Incorrectly appending a duplicate ? and not treating versioned blobs correctly - #27592
Open
Gauri Lamunion (gapra-msft) wants to merge 1 commit into
Open
Gauri Lamunion (gapra-msft) wants to merge 1 commit into
Gauri Lamunion (gapra-msft) wants to merge 1 commit into
Conversation
…ate ? and not treating versioned blobs correctly
Gauri Lamunion (gapra-msft)
requested review from
Joel Hendrix (jhendrixMSFT) and
tanyasethi-msft (tanyasethi-msft)
as code owners
September 21, 2026 20:00
|
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. |
Copilot started reviewing on behalf of
Gauri Lamunion (gapra-msft)
September 21, 2026 20:01
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The advertised container fix is missing, and user-delegation version signing lacks regression coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Fixes SAS generation for versioned blobs and snapshot-scoped Azure Files resources.
Changes:
- Correctly signs blob-version SAS tokens.
- Merges SAS parameters into existing query strings.
- Adds live regression tests and changelog entries.
| File | Description |
|---|---|
sdk/storage/azfile/share/client.go |
Merges share SAS query parameters. |
sdk/storage/azfile/share/client_test.go |
Tests snapshot share/file SAS URLs. |
sdk/storage/azfile/file/client.go |
Merges file SAS query parameters. |
sdk/storage/azfile/CHANGELOG.md |
Documents Azure Files fixes. |
sdk/storage/azblob/sas/service.go |
Includes version IDs in signatures. |
sdk/storage/azblob/CHANGELOG.md |
Documents blob SAS fixes. |
sdk/storage/azblob/blob/client.go |
Generates version-scoped SAS URLs. |
sdk/storage/azblob/blob/client_test.go |
Tests version isolation and authentication. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| endpoint := b.URL() + "?" + qps.Encode() | ||
| // b.URL() may already contain a query string (e.g. "versionid" or "snapshot" for |
Member
Author
There was a problem hiding this comment.
container URL does not need this fix. It does not have resources with query params
Member
Author
There was a problem hiding this comment.
Hmm actually we should modify all the methods to have this fix in case there are customer custom query params in the URL
Comment on lines
+251
to
+255
| if resource == "bv" { | ||
| // See the corresponding comment in SignWithSharedKey: the "signed snapshot time" | ||
| // slot is shared between blob snapshots and blob versions and must carry the | ||
| // version ID for a version SAS. | ||
| snapshotTime = v.BlobVersion |
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 a Shared Access Signature (SAS) vulnerability/bug class affecting versioned blobs and snapshotted share/file resources in azblob and azfile:
blob.Client.GetSASURL() called on a client returned by WithVersionID produced a SAS that was byte-for-byte identical to an ordinary base-blob (sr=b) SAS, silently granting broader access than intended. After fixing the resource-type binding, a second bug was found: the version identifier was never included in the signed string-to-sign (sas/service.go), so the corrected sr=bv SAS would fail to authenticate. Both are now fixed in SignWithSharedKey and SignWithUserDelegation.
GetSASURL() on blob.Client, container.Client, share.Client, and file.Client appended a second ? to the URL when the client's URL already contained a query string (e.g. clients returned by WithSnapshot/WithVersionID, or a file client derived from a share snapshot), producing a malformed SAS URL.
The purpose of this PR is explained in this or a referenced issue.
The PR does not update generated files.
Tests are included and/or updated for code changes.
Updates to module CHANGELOG.md are included.
MIT license headers are included in each file.