Replace canvas share SAS links with an authenticated viewer - #179
Merged
Conversation
Add the CanvasShareViewer ASP.NET Core F# project with exact-name Blob reads via DefaultAzureCredential, strict path validation, metadata expiry enforcement, and shell/content GET routes returning a uniform 404. Cover the pipeline with CanvasShareViewerTests.
Render the shell as a sandboxed iframe pointing at the content route and apply the spec's exact shell/content response policies (CSP, nosniff, Referrer-Policy, Cache-Control) on every matched route response. Add viewer route tests plus a self-contained active-HTML fixture covering routing, headers, indistinguishable not-found outcomes, and byte-identical document streaming.
Replace user-delegation SAS minting with private-Blob uploads that carry expiresOn metadata, add a validated HTTPS viewerBaseUrl setting, raise the expiry ceiling to 30 days, and return clean /c/<prefix>/<filename> viewer URLs.
Record the user's decisions on the review's six Needs Your Decision findings: a tenant-wide authenticated audience with no enterprise-application assignment, an identical not-found response with no elapsed-time guarantee (malformed paths may fail before storage access), server-independent deployment config writes, and duplicated publisher/viewer contract constants pinned by compatibility tests.
Add a fail-closed audit of the viewer identity's effective Blob-read RBAC. It enumerates direct, group-derived, and parent-scope-inherited assignments, resolves each role definition's dataActions/notDataActions, and rejects any Blob-read grant whose scope is not the share container or a descendant. The audit runs before any Azure mutation when the identity already exists and again during deployed-state verification; it reports but never deletes the offending assignment. Adds mocked regression tests wired into CI and updates the spec and operator docs.
Limit the viewer's 404-to-not-found path to a genuine BlobNotFound and add an exception boundary before routing that turns non-404 storage and DefaultAzureCredential failures into one fixed, empty 503 with restrictive policy headers in every environment, logging only exception type and Azure status/error code. Pin and assert DOTNET_ENVIRONMENT and ASPNETCORE_ENVIRONMENT to Production in the deployment script, and record the behaviour in the spec and deployment doc.
Look up the expiresOn blob metadata key case-insensitively so a casing variation from Azure Blob metadata no longer makes a live share resolve as malformed/expired. Adds regression coverage over ShareExpiry.isLive and ShareLookup.resolve with a mixed-case key, and records the case-insensitive match in the spec wire contract. Fixes focused-review finding F3.
Mirror one filename predicate on both sides of the share wire contract: non-empty, case-insensitive .html suffix, no path separators, consecutive dots allowed. Validate in WorktreeApi before path/file I/O and again at the CanvasShare.publish boundary, replacing the leafName rewrite so the original casing survives into the Blob name and viewer URL. Adds cross-contract and route tests plus spec updates.
Append frame-ancestors 'none' to the shell route CSP so the viewer shell cannot be framed cross-origin, matching focused-review finding F7. Update the exact-header test expectation and the shell wire-contract row in the spec.
Split the viewer storage boundary so a page load no longer downloads each shared document twice. BlobReader gains a properties-only exact lookup backed by GetPropertiesAsync; the shell route resolves against it while the content route keeps the single body-bearing read. Both routes still re-validate the path and re-check expiry independently. Malformed paths now short-circuit to not-found without any storage call, removing the InvalidPathProbeBlobName probe under the recorded timing decision the spec already documents.
Reject canvasShare.viewerBaseUrl values whose path is not exactly "/", so published links can never point at routes the root-mounted viewer does not serve (focused-review finding F9 / A-07). Spec and tests updated to match.
Serve active canvas HTML only for a fail-closed same-origin iframe navigation identified by exact single-value Fetch Metadata (Sec-Fetch-Site: same-origin, Sec-Fetch-Mode: navigate, Sec-Fetch-Dest: iframe). Direct, top-level, cross-site, partial, duplicated, and metadata-missing /content requests now receive the normal non-executable shell after their own independent path and expiry check, closing the containment escape where a top-level /content load could self-navigate to an external probe. Add Playwright containment coverage plus a hostile fixture that attempts cookie/storage reads, viewer and external fetches, remote image and form exfiltration, popups, frame self-navigation, and parent navigation; the external probe receives zero requests.
…ndows Fix the four failures verification tm-canvas-safe-share-szi found on PowerShell 7.6 / Azure CLI 2.84: pass the pipe-bearing Linux runtime through the CLI JSON-file configuration input so az.cmd cannot reinterpret it, read resource fields that the CLI nests under properties or renames to appServicePlanId, retry restricted-tenant registration mutations with the one unambiguous publisher-owned serviceManagementReference, and keep dotnet publish output out of the package path value. Add Deployment.Tests.ps1 regressions and run them in CI.
Easy Auth's browser callback requests response_type=code id_token with response_mode=form_post, but the dedicated app registration disabled ID-token issuance, so the callback returned 401 for tenant users. Enable ID-token issuance while keeping browser access-token issuance disabled, re-read the registration during deployed-state validation to reject audience, redirect, or issuance drift, and cover the CLI update shape and both negative cases in the deployment regression tests.
…ection flow Add CrossSubscriptionCorrection.ps1 with mutually exclusive -PrepareCrossSubscriptionMove and -ReconcileCrossSubscriptionMove parameter sets on deploy-canvas-share-viewer.ps1. Preparation returns before ordinary app discovery, so it no longer aborts on the global App Service name-availability check while the source subscription still owns the canonical name. It reconciles only the destination resource group, replacement identity, and its container-scoped reader grant, then prints a redacted portal checklist. Reconciliation requires -ConfirmPortalMoveCompleted before any local tool or Azure call, resolves the moved app and plan in the approved destination only, and never checks global name availability. Ordinary creation and -ValidateOnly keep the unchanged global-name guard. Deployment.Tests.ps1 covers phase boundaries, the confirmation gate, subscription scoping, and checklist redaction.
…uth diagnostics Extract subscription resolution and Azure-context approval into SubscriptionGuard.ps1 dot-sourced after Common.ps1, resolve the selected Azure CLI account before either private subscription lookup so authentication failures keep their real diagnostic, and cover both with deployment regressions.
…ead of appending to it az webapp identity assign appends rather than replaces, so post-move reconciliation could leave a stale source-subscription identity attached. Removing it via CLI would require naming its source resource ID, which automation may never carry. Reconciliation now reads a sanitized destination-side summary (identity type, attachment count, prepared-identity match) before any App Service mutation and fails closed with portal guidance on a system-assigned identity or any foreign user-assigned attachment. The portal checklist detaches the attachment before the move, and the spec and operator doc record the detachment window and manual reattach on rollback.
Document the narrow response-text-only exception for redacted reports under gitignored .agents/verify/ written by bd-verify-executor and read by bd-verify-reviewer.
… move Record the browser-navigation health-probe contract: the post-move Entra redirect check sends browser navigation headers (Accept: text/html) so the probe exercises the same path a recipient's browser takes rather than an API-style client. Reconciliation itself was an Azure-side operation in the approved personal subscription only; post-move IDs, etags, timestamps, rollback snapshot, and PASS evidence stay in the ignored .agents/azure-migration/ manifest.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- remove checked-in Azure deployment reconciliation and consolidate its specs - src/CanvasShareViewer/BlobStorage.fs:99 - stream content instead of buffering - src/Server/CanvasShare.fs:143 - make share prefixes a distinct type - src/Tests/CanvasShareTests.fs:103 - remove redundant sharing tests - src/Tests/CanvasShareViewerTests.fs:163 - share viewer host lifecycle Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The user-assigned identity configuration is incomplete, and navigation probes can mask containment regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
docs/spec/canvas-sharing.md — The provisioning contract chooses a user-assigned managed identity, but this exhaustive App Service… |
 |
src/Tests/fixtures/canvas-share-viewer/hostile.html — These self-navigation probes run in one JavaScript task, so each later location.replace/_self… |
 |
src/Tests/CanvasShareViewerTests.fs — This new module is 1,598 lines and combines storage-failure, path, expiry, configuration, and route… |
What changed in this PR
Replaces Blob SAS sharing with a private-Blob, Entra-authenticated viewer that enforces expiry and sandboxes active canvas HTML.
Changes:
- Adds the standalone ASP.NET Core viewer with exact Blob lookup, expiry checks, CSP, and iframe containment.
- Updates publishing to validate filenames, write expiry metadata, and return clean viewer URLs.
- Expands tests and consolidates sharing/security documentation.
| File | Description |
|---|---|
treemon.slnx |
Adds the viewer project. |
src/Tests/Tests.fsproj |
Registers viewer tests and reference. |
src/Tests/fixtures/canvas-share-viewer/self-contained.html |
Adds benign interactive fixture. |
src/Tests/fixtures/canvas-share-viewer/hostile.html |
Adds containment attack fixture. |
src/Tests/CanvasShareViewerTests.fs |
Tests viewer contracts and routes. |
src/Tests/CanvasShareViewerContainmentTests.fs |
Adds Playwright containment coverage. |
src/Tests/CanvasShareViewerContainmentTestHelpers.fs |
Provides isolated viewer/probe harness. |
src/Tests/CanvasShareTests.fs |
Updates publisher and configuration tests. |
src/Tests/CanvasShareClientTests.fs |
Uses clean viewer URLs in clipboard tests. |
src/Tests/CanvasAwarenessTests.fs |
Updates share-result fixtures. |
src/Shared/Types.fs |
Documents the authenticated viewer URL. |
src/Server/WorktreeApi.fs |
Validates filenames before file access. |
src/Server/GlobalConfig.fs |
Adds viewer URL and 30-day expiry support. |
src/Server/CanvasShare.fs |
Replaces SAS signing with metadata and viewer URLs. |
src/Client/index.html |
Updates publishing-progress documentation. |
src/Client/CanvasUpdate.fs |
Updates viewer-link clipboard terminology. |
src/Client/AppTypes.fs |
Updates share-message documentation. |
src/CanvasShareViewer/ViewerConfiguration.fs |
Reads required viewer storage settings. |
src/CanvasShareViewer/ViewerApplication.fs |
Implements secured shell and content routes. |
src/CanvasShareViewer/SharePath.fs |
Validates share URL segments. |
src/CanvasShareViewer/ShareLookup.fs |
Performs exact, expiry-gated lookups. |
src/CanvasShareViewer/ShareExpiry.fs |
Parses and enforces expiry metadata. |
src/CanvasShareViewer/Program.fs |
Starts the viewer with Azure credentials. |
src/CanvasShareViewer/CanvasShareViewer.fsproj |
Defines the viewer web project. |
src/CanvasShareViewer/BlobStorage.fs |
Implements managed-identity Blob reads. |
src/CanvasShareViewer/appsettings.json |
Adds blank deployment placeholders. |
scripts/canvas-share-lifecycle-policy.json |
Removes obsolete eight-day policy. |
docs/spec/worktree-monitor.md |
Consolidates loopback CSRF guidance. |
docs/spec/session-status-push.md |
Updates security-spec reference. |
docs/spec/remoting-csrf-hardening.md |
Removes consolidated standalone spec. |
docs/spec/process-execution.md |
Updates security-spec reference. |
docs/spec/future/code-improvements.md |
Removes completed CSRF roadmap item. |
docs/spec/canvas-sharing.md |
Rewrites the sharing architecture and deployment contract. |
docs/spec/canvas-pane.md |
Documents authenticated viewer sharing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Summary
Deployment notes
Validation
dotnet build treemon.slnxdotnet test src/Tests/Tests.fsproj --filter "Category=Unit&FullyQualifiedName~CanvasShare"- 94 passedPR CI runs the full non-local suite.