Clarify SAS token terminology in Azure upload documentation - Because we are using shared key not short lived token#315
Open
kishor-gupta wants to merge 31 commits intomainfrom
Open
Clarify SAS token terminology in Azure upload documentation - Because we are using shared key not short lived token#315kishor-gupta wants to merge 31 commits intomainfrom
kishor-gupta wants to merge 31 commits intomainfrom
Conversation
… we are using shared key not short lived token
Contributor
Reviewer's GuideUpdates the Azure upload ADR to clarify that the system uses the Valet Key architectural pattern implemented via Shared Key–based server-signed request headers rather than SAS tokens, expanding the rationale, tradeoffs, and security implications of this choice. Sequence diagram for Azure Valet Key upload with Shared Key server-signed headerssequenceDiagram
actor User
participant Frontend
participant Backend
participant KeyVault
participant Blob
participant Defender
User->>Frontend: Select file and click upload
Frontend->>Frontend: Validate file (type, size, dimensions)
Frontend->>Backend: Request_upload_auth(name, type, size)
Backend->>Backend: Build blob path, tags, metadata
Backend->>KeyVault: Get_storage_account_key
KeyVault-->>Backend: Storage_account_key
Backend->>Backend: Generate server_signed_request_headers using Shared Key
Backend-->>Frontend: AuthResult(blob_url, server_signed_headers, x_ms_date, tags, metadata)
Frontend->>Blob: PUT file bytes with server_signed_headers, tags, metadata
Blob-->>Frontend: 201 Created with x_ms_version_id
Frontend->>Backend: Persist_blob_reference(version_id, blob_path, metadata)
Backend->>Backend: Schedule malware scan status polling
Backend->>Blob: Poll Defender scan tag
Blob-->>Backend: Scan_result_tag(No_threats_found or Malicious)
alt No_threats_found
Backend->>Backend: Mark upload as clean and ready
Backend-->>Frontend: Upload_status(clean)
Frontend-->>User: Show success and preview
else Malicious
Backend->>Blob: Delete current version, restore previous version
Backend-->>Frontend: Upload_status(malicious_rolled_back)
Frontend-->>User: Show error and remediation message
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The updated wording suggests a switch from short-lived to Shared Key SAS tokens, but these are orthogonal concepts; consider clarifying that you are using account-key–signed SAS tokens which are still short-lived, to avoid implying they are not time-bound anymore.
- In the backend description, the phrase "with Shared Key and carefully permissioned SAS tokens" is a bit ambiguous; you might rephrase to something like "with carefully permissioned SAS tokens signed with the storage account key" to be clearer about what is shared and how.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The updated wording suggests a switch from short-lived to Shared Key SAS tokens, but these are orthogonal concepts; consider clarifying that you are using account-key–signed SAS tokens which are still short-lived, to avoid implying they are not time-bound anymore.
- In the backend description, the phrase "with Shared Key and carefully permissioned SAS tokens" is a bit ambiguous; you might rephrase to something like "with carefully permissioned SAS tokens signed with the storage account key" to be clearer about what is shared and how.
## Individual Comments
### Comment 1
<location> `apps/docs/docs/decisions/0022-existing-azure-upload.md:38` </location>
<code_context>
## Decision Outcome
-Chosen option: **Option 2: Direct client uploads using Azure Blob SAS tokens (current approach)**, because it offloads upload traffic from the backend, reduces latency, lower cost, maintains security via short-lived tokens.
+Chosen option: **Option 2: Direct client uploads using Azure Blob SAS tokens (current approach)**, because it offloads upload traffic from the backend, reduces latency, lower cost, maintains security via Shared Key tokens.
## Consequences
</code_context>
<issue_to_address>
**suggestion (typo):** Align verb forms in the list for smoother grammar and readability.
To keep the verbs parallel, you could write: “because it offloads upload traffic from the backend, reduces latency, lowers cost, and maintains security via Shared Key tokens.”
```suggestion
Chosen option: **Option 2: Direct client uploads using Azure Blob SAS tokens (current approach)**, because it offloads upload traffic from the backend, reduces latency, lowers cost, and maintains security via Shared Key tokens.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The updated terminology conflates the authentication mechanism and the token itself; consider wording like "SAS tokens signed with the storage account shared key" or "account key–based SAS" instead of "Shared Key SAS token" / "Shared Key tokens" to more accurately reflect how SAS works.
- In the "Security" and "Backend Services" sections, it may be worth explicitly stating that the SAS tokens remain short-lived/time-bound even though they are generated with the shared key, so readers don’t infer that token lifetimes have changed.
- The phrase "with Shared Key and carefully permissioned SAS tokens" is slightly awkward; consider rephrasing to something like "with carefully permissioned SAS tokens generated using the storage account shared key" for clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The updated terminology conflates the authentication mechanism and the token itself; consider wording like "SAS tokens signed with the storage account shared key" or "account key–based SAS" instead of "Shared Key SAS token" / "Shared Key tokens" to more accurately reflect how SAS works.
- In the "Security" and "Backend Services" sections, it may be worth explicitly stating that the SAS tokens remain short-lived/time-bound even though they are generated with the shared key, so readers don’t infer that token lifetimes have changed.
- The phrase "with Shared Key and carefully permissioned SAS tokens" is slightly awkward; consider rephrasing to something like "with carefully permissioned SAS tokens generated using the storage account shared key" for clarity.
## Individual Comments
### Comment 1
<location> `apps/docs/docs/decisions/0022-existing-azure-upload.md:38` </location>
<code_context>
## Decision Outcome
-Chosen option: **Option 2: Direct client uploads using Azure Blob SAS tokens (current approach)**, because it offloads upload traffic from the backend, reduces latency, lower cost, maintains security via short-lived tokens.
+Chosen option: **Option 2: Direct client uploads using Azure Blob SAS tokens (current approach)**, because it offloads upload traffic from the backend, reduces latency, lower cost, maintains security via Shared Key tokens.
## Consequences
</code_context>
<issue_to_address>
**suggestion (typo):** Align the verb form in the list for better grammatical consistency.
In the phrase "offloads upload traffic from the backend, reduces latency, lower cost, maintains security", the third item should use a parallel verb form (e.g., "lowers cost" or rephrase to "reduces latency, lowers cost, and maintains security").
```suggestion
Chosen option: **Option 2: Direct client uploads using Azure Blob SAS tokens (current approach)**, because it offloads upload traffic from the backend, reduces latency, lowers cost, and maintains security via Shared Key tokens.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
… the use of Shared Key SAS for enhanced security
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new section contrasting 'Short-lived SAS' and 'Shared Key SAS' is conceptually confusing: short‑lived vs shared‑key are orthogonal concerns in Azure (most SAS are shared‑key signed and can still be short‑lived), so consider rephrasing to distinguish token lifetime, permissions, and signing mechanism more accurately.
- There are several grammatical and style issues in the added text (e.g. 'we considered' should be capitalized, 'We Chosen' → 'We chose', 'can be breach' → 'can be breached'); tightening this up will make the decision record clearer and more professional.
- The sentence about short‑lived SAS tokens being breachable and misusable is vague and could apply equally to Shared Key SAS; consider clarifying the specific threat model and why the chosen approach meaningfully mitigates it, or removing the implication that short‑lived SAS are inherently less secure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new section contrasting 'Short-lived SAS' and 'Shared Key SAS' is conceptually confusing: short‑lived vs shared‑key are orthogonal concerns in Azure (most SAS are shared‑key signed and can still be short‑lived), so consider rephrasing to distinguish token lifetime, permissions, and signing mechanism more accurately.
- There are several grammatical and style issues in the added text (e.g. 'we considered' should be capitalized, 'We Chosen' → 'We chose', 'can be breach' → 'can be breached'); tightening this up will make the decision record clearer and more professional.
- The sentence about short‑lived SAS tokens being breachable and misusable is vague and could apply equally to Shared Key SAS; consider clarifying the specific threat model and why the chosen approach meaningfully mitigates it, or removing the implication that short‑lived SAS are inherently less secure.
## Individual Comments
### Comment 1
<location> `apps/docs/docs/decisions/0022-existing-azure-upload.md:20` </location>
<code_context>
+we considered two authorization strategies:
</code_context>
<issue_to_address>
**nitpick (typo):** Capitalize the beginning of the sentence.
Change `we considered two authorization strategies:` to `We considered two authorization strategies:`.
```suggestion
We considered two authorization strategies:
```
</issue_to_address>
### Comment 2
<location> `apps/docs/docs/decisions/0022-existing-azure-upload.md:21` </location>
<code_context>
+Users need to upload various file types (PDFs, images) along with metadata and tags for tracking. The system implements a direct upload flow where the client requests authorization from the backend, which issues a SAS (Shared Access Signature) token allowing the file to be uploaded directly to Azure Blob Storage.
+
+we considered two authorization strategies:
+- Short-lived SAS : A short-lived SAS (Shared Access Signature) in Azure Blob Storage is a token with a limited time validity that grants specific permissions to access a blob or container. And this can be breach by someone and can misuse this short lived token.
+- Shared Key SAS : A Shared Key–signed SAS (Shared Access Signature) is a time‑limited and permission‑scoped token generated using the storage account’s shared key. It allows clients temporary access to upload or modify blobs without exposing the actual account key.
+
</code_context>
<issue_to_address>
**suggestion (typo):** Fix grammar, hyphenation, and spacing in the Short-lived SAS description.
Consider rephrasing to:
`- Short-lived SAS: A short-lived SAS (Shared Access Signature) in Azure Blob Storage is a token with limited validity that grants specific permissions to access a blob or container. It can be breached by someone, who could then misuse this short-lived token.`
This corrects the colon spacing, verb tense, sentence flow, and hyphenation.
```suggestion
- Short-lived SAS: A short-lived SAS (Shared Access Signature) in Azure Blob Storage is a token with limited validity that grants specific permissions to access a blob or container. It can be breached by someone, who could then misuse this short-lived token.
```
</issue_to_address>
### Comment 3
<location> `apps/docs/docs/decisions/0022-existing-azure-upload.md:24` </location>
<code_context>
+- Short-lived SAS : A short-lived SAS (Shared Access Signature) in Azure Blob Storage is a token with a limited time validity that grants specific permissions to access a blob or container. And this can be breach by someone and can misuse this short lived token.
+- Shared Key SAS : A Shared Key–signed SAS (Shared Access Signature) is a time‑limited and permission‑scoped token generated using the storage account’s shared key. It allows clients temporary access to upload or modify blobs without exposing the actual account key.
+
+We Chosen **Shared Key SAS**
+Because:
+- The backend generates SAS tokens along with all relevant metadata for each specific upload request, ensuring contextual and controlled access.
</code_context>
<issue_to_address>
**issue (typo):** Correct the verb tense in "We Chosen".
Change this to `We chose **Shared Key SAS**` for correct tense, and remove the trailing space at the end of the line.
```suggestion
We chose **Shared Key SAS**
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…hared Key SAS tokens and clarifying decision outcomes for enhanced security and management.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the 'Considered Authorization Options' section, the option labels are inconsistent (Option 1, Option B, then 'Chosen option: Option 2: Option B')—consider standardizing these to a single numeric or alphabetic scheme and fixing the chosen-option text accordingly.
- The phrase 'Shared Key and carefully permissioned SAS tokens' in the backend services bullet reads as though 'Shared Key' and 'carefully permissioned SAS tokens' are separate mechanisms; consider rephrasing to something like 'Shared Key–signed, carefully permissioned SAS tokens' to avoid ambiguity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the 'Considered Authorization Options' section, the option labels are inconsistent (Option 1, Option B, then 'Chosen option: Option 2: Option B')—consider standardizing these to a single numeric or alphabetic scheme and fixing the chosen-option text accordingly.
- The phrase 'Shared Key and carefully permissioned SAS tokens' in the backend services bullet reads as though 'Shared Key' and 'carefully permissioned SAS tokens' are separate mechanisms; consider rephrasing to something like 'Shared Key–signed, carefully permissioned SAS tokens' to avoid ambiguity.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…umentation and clarify chosen option in decision outcome.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The distinction between "short‑lived SAS tokens" and "Shared Key SAS tokens" is a bit misleading, since Shared Key–signed SAS tokens can also be short‑lived; consider explicitly framing Option 2 as "Shared Key–signed SAS tokens (instead of user‑delegation/other SAS types)" or similar to align with Azure terminology.
- In the backend services bullet, the phrase "with Shared Key and carefully permissioned SAS tokens" reads awkwardly; consider rephrasing to something like "with Shared Key–signed, carefully permissioned SAS tokens" for clarity and consistency with earlier sections.
- For consistency, you might want to explicitly mention "Shared Key–signed" in the "Direct client uploads using Azure Blob SAS tokens" option title or description, so readers can easily connect the upload option with the chosen authorization mechanism.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The distinction between "short‑lived SAS tokens" and "Shared Key SAS tokens" is a bit misleading, since Shared Key–signed SAS tokens can also be short‑lived; consider explicitly framing Option 2 as "Shared Key–signed SAS tokens (instead of user‑delegation/other SAS types)" or similar to align with Azure terminology.
- In the backend services bullet, the phrase "with Shared Key and carefully permissioned SAS tokens" reads awkwardly; consider rephrasing to something like "with Shared Key–signed, carefully permissioned SAS tokens" for clarity and consistency with earlier sections.
- For consistency, you might want to explicitly mention "Shared Key–signed" in the "Direct client uploads using Azure Blob SAS tokens" option title or description, so readers can easily connect the upload option with the chosen authorization mechanism.
## Individual Comments
### Comment 1
<location> `apps/docs/docs/decisions/0022-existing-azure-upload.md:68` </location>
<code_context>
+ - The backend handles SAS token generation and validation for Azure Blob Storage uploads, ensuring secure and controlled access for file uploads. There are different mutations for PDF and image files. The backend service encapsulates all business logic enforcing file upload restrictions and security requirements before enabling clients to upload files directly to Azure Blob Storage with Shared Key and carefully permissioned SAS tokens.
</code_context>
<issue_to_address>
**suggestion (typo):** Clarify the phrase "with Shared Key and carefully permissioned SAS tokens" to avoid leaving "Shared Key" hanging.
Consider rephrasing to something like “with Shared Key–signed, carefully permissioned SAS tokens” or “with Shared Key SAS tokens that are carefully permissioned” so it’s clear that “Shared Key” modifies the SAS tokens rather than standing alone.
Suggested implementation:
```
- The backend handles SAS token generation and validation for Azure Blob Storage uploads, ensuring secure and controlled access for file uploads. There are different mutations for PDF and image files. The backend service encapsulates all business logic enforcing file upload restrictions and security requirements before enabling clients to upload files directly to Azure Blob Storage using carefully permissioned, Shared Key–signed SAS tokens.
```
```
- Future improvements will focus on implementing domain-driven permission checks before SAS tokens are issued and exploring pre-upload scanning alternatives to further reduce risk.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new "Considered Authorization Options" section risks conflating token lifetime with signing mechanism (short‑lived vs Shared Key); consider explicitly distinguishing token duration (short‑lived vs long‑lived) from the SAS type (e.g., account/service SAS signed with Shared Key vs user‑delegation SAS) to avoid confusion.
- Azure docs typically refer to "account SAS" or "service SAS" signed with the storage account key rather than "Shared Key SAS tokens"; consider aligning your terminology with Azure’s naming to make it clearer which SAS type is being used.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new "Considered Authorization Options" section risks conflating token lifetime with signing mechanism (short‑lived vs Shared Key); consider explicitly distinguishing token duration (short‑lived vs long‑lived) from the SAS type (e.g., account/service SAS signed with Shared Key vs user‑delegation SAS) to avoid confusion.
- Azure docs typically refer to "account SAS" or "service SAS" signed with the storage account key rather than "Shared Key SAS tokens"; consider aligning your terminology with Azure’s naming to make it clearer which SAS type is being used.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…Key Authorization) for Azure Blob uploads
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new terminology around "Shared Key Authorization token" and "Valet Key pattern" may confuse readers familiar with Azure docs, where the primitive is still a SAS signed with the account key; consider explicitly stating that the Valet Key implementation uses account-key–signed SAS tokens to align with Azure’s naming and avoid implying a distinct token type.
- In several places the phrase "Shared Key Authorization token" is used where previously "SAS token" was used; to improve clarity and searchability, consider keeping "SAS" in the phrase (e.g. "Shared Key–signed SAS token") so that readers can easily relate this to Azure documentation and SDK concepts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new terminology around "Shared Key Authorization token" and "Valet Key pattern" may confuse readers familiar with Azure docs, where the primitive is still a SAS signed with the account key; consider explicitly stating that the Valet Key implementation uses account-key–signed SAS tokens to align with Azure’s naming and avoid implying a distinct token type.
- In several places the phrase "Shared Key Authorization token" is used where previously "SAS token" was used; to improve clarity and searchability, consider keeping "SAS" in the phrase (e.g. "Shared Key–signed SAS token") so that readers can easily relate this to Azure documentation and SDK concepts.
## Individual Comments
### Comment 1
<location> `apps/docs/docs/decisions/0022-existing-azure-upload.md:25` </location>
<code_context>
- **Performance**: Direct client-to-Azure Blob uploads reduce backend latency and improve user upload speed.
- **Cost Optimization**: Offloading upload bandwidth from backend servers to Azure Blob Storage minimizes infrastructure and data transfer costs.
-- **Security**: Uploads must be secure and authenticated. Short-lived SAS tokens (valet keys) provide controlled, time-bound access to the storage account.
+- **Security**: Uploads must be secure and authenticated. Valet Key pattern provides controlled, time-bound access to the storage account.
- **Malware Scanning**: Uploaded files must undergo malware scanning. Any malicious files must be identified, quarantined, and deleted immediately to maintain data integrity and user safety.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider adding an article before "Valet Key pattern" for grammatical correctness.
You could rephrase this as: "The Valet Key pattern provides controlled, time-bound access to the storage account."
```suggestion
- **Security**: Uploads must be secure and authenticated. The Valet Key pattern provides controlled, time-bound access to the storage account.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ction of the Azure upload decision document.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The ADR now mixes architectural pattern rationale and very low-level auth-mechanism details in one file; consider splitting the detailed Shared Key vs SAS vs Entra ID analysis into a separate security-focused ADR or appendix to keep this document easier to scan for high-level decisions.
- You repeatedly explain the Valet Key pattern and server-signed headers in multiple sections; consider extracting a concise definition up front and then referencing it to reduce redundancy and make future edits less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The ADR now mixes architectural pattern rationale and very low-level auth-mechanism details in one file; consider splitting the detailed Shared Key vs SAS vs Entra ID analysis into a separate security-focused ADR or appendix to keep this document easier to scan for high-level decisions.
- You repeatedly explain the Valet Key pattern and server-signed headers in multiple sections; consider extracting a concise definition up front and then referencing it to reduce redundancy and make future edits less error-prone.
## Individual Comments
### Comment 1
<location> `apps/docs/docs/decisions/0022-existing-azure-upload.md:116-117` </location>
<code_context>
+- Server-signed request headers generation:
</code_context>
<issue_to_address>
**nitpick (typo):** Rephrase "Server-signed request headers generation" for more natural grammar.
Consider instead "Server-signed request header generation" or "Generation of server-signed request headers" so the heading reads more naturally.
```suggestion
- Server-signed request header generation:
- The backend handles generation and validation of server-signed request headers for Azure Blob Storage uploads, ensuring secure and controlled access for file uploads. There are different mutations for PDF and image files. The backend service encapsulates all business logic enforcing file upload restrictions and security requirements before enabling clients to upload files directly to Azure Blob Storage using carefully permissioned, server-signed request headers.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…zure Blob Storage uploads
…arethrift into dev-kishor/issue230-2
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new sections around Shared Key and Valet Key are quite dense and a bit repetitive; consider tightening wording (e.g., avoid re‑explaining the same pros/cons in multiple places) and pulling the key contrast with SAS into a short, skimmable summary table or bullets near the top of the decision rationale.
- In the "Why we have not switched to Entra ID" section, the statement that Managed Identity does not expose account keys could be misread as a hard limitation rather than a design choice; consider clarifying that you are intentionally not using MI to retrieve account keys via the control plane and briefly noting that alternative Entra ID patterns were evaluated and rejected for the header‑binding requirement.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new sections around Shared Key and Valet Key are quite dense and a bit repetitive; consider tightening wording (e.g., avoid re‑explaining the same pros/cons in multiple places) and pulling the key contrast with SAS into a short, skimmable summary table or bullets near the top of the decision rationale.
- In the "Why we have not switched to Entra ID" section, the statement that Managed Identity does not expose account keys could be misread as a hard limitation rather than a design choice; consider clarifying that you are intentionally not using MI to retrieve account keys via the control plane and briefly noting that alternative Entra ID patterns were evaluated and rejected for the header‑binding requirement.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…torage uploads, emphasizing header-binding requirements and limitations of Managed Identity and user delegation SAS.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The ADR has become quite long and detailed; consider adding a short “tl;dr” or condensed summary section near the top that clearly states the chosen approach and 2–3 key reasons, so readers don’t have to parse all sections to understand the decision.
- Terminology around ‘Valet Key’, ‘Shared Key authorization’, and ‘server-signed request headers’ is used in many places; it may help to add a brief definitions subsection early on and then use a single consistent term (with the others in parentheses) to reduce potential confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The ADR has become quite long and detailed; consider adding a short “tl;dr” or condensed summary section near the top that clearly states the chosen approach and 2–3 key reasons, so readers don’t have to parse all sections to understand the decision.
- Terminology around ‘Valet Key’, ‘Shared Key authorization’, and ‘server-signed request headers’ is used in many places; it may help to add a brief definitions subsection early on and then use a single consistent term (with the others in parentheses) to reduce potential confusion.
## Individual Comments
### Comment 1
<location> `apps/docs/docs/decisions/0022-existing-azure-upload.md:100` </location>
<code_context>
+- Future improvements will focus on implementing domain-driven permission checks before SAS tokens are issued and exploring pre-upload scanning alternatives to further reduce risk.
</code_context>
<issue_to_address>
**suggestion:** Update the future-improvements sentence to align with Shared Key / server-signed headers instead of SAS tokens.
This sentence still mentions "before SAS tokens are issued," but the ADR’s chosen approach is Shared Key with server-signed request headers (with SAS as a rejected alternative). Please update this to reflect the actual mechanism (e.g., "before server-signed request headers are issued" or a generic "before upload authorizations are issued").
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
… clarifying decision rationale and terminology for server-signed request headers.
Contributor
Author
|
@sourcery-ai review |
nnoce14
reviewed
Jan 6, 2026
nnoce14
reviewed
Jan 6, 2026
nnoce14
reviewed
Jan 6, 2026
…he description of server-signed request headers for Azure Blob Storage uploads.
… the Valet Key pattern's role in granting scoped upload permissions and controlling access.
… Key architectural pattern and its relationship with server-signed request headers and Shared Key authorization.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- There is still a lingering reference to SAS tokens in the final bullet point ('before SAS tokens are issued'); consider updating this to match the new Shared Key/server-signed headers terminology for consistency.
- The new architectural and security rationale sections are quite detailed; consider briefly summarizing the key differences between SAS and Shared Key near the top and moving some of the deeper rationale into a clearly labeled appendix-style section to keep the main ADR narrative easier to scan.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is still a lingering reference to SAS tokens in the final bullet point ('before SAS tokens are issued'); consider updating this to match the new Shared Key/server-signed headers terminology for consistency.
- The new architectural and security rationale sections are quite detailed; consider briefly summarizing the key differences between SAS and Shared Key near the top and moving some of the deeper rationale into a clearly labeled appendix-style section to keep the main ADR narrative easier to scan.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nnoce14
approved these changes
Jan 6, 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
2 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.
Update the Azure upload documentation to specify the use of Shared Key SAS tokens instead of short-lived SAS tokens, ensuring accurate terminology and understanding of the security mechanism in place.
Summary by Sourcery
Documentation: