From b13474a895fa02d5f3bfa9d6a38b74f0613d5052 Mon Sep 17 00:00:00 2001 From: silverwind Date: Tue, 28 Apr 2026 01:41:21 +0200 Subject: [PATCH] actions: align iframe sandbox with CSP and avoid duplicate v4 zip parse - Drop `allow-scripts` from the iframe sandbox attribute. The response CSP `default-src 'none'; sandbox` already blocks scripts, so the attribute was contradictory. Now both layers express the same intent: opaque origin, no scripts, no forms. - Skip listPreviewPaths in ArtifactsPreviewRawView and parse the v4 zip exactly once per request via serveArtifactV4PreviewRaw. Previously a cache miss caused two open+parse cycles for the same zip. The page handler still uses the cache. Co-Authored-By: Claude (Opus 4.7) --- routers/web/repo/actions/view.go | 102 +++++++++--------- .../actions/artifact_preview_content.tmpl | 3 +- .../actions_artifact_preview_test.go | 4 +- 3 files changed, 54 insertions(+), 55 deletions(-) diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index a09cb70965..b29255f5f4 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -1279,6 +1279,48 @@ func ArtifactsPreviewView(ctx *context_module.Context) { ctx.HTML(http.StatusOK, tplArtifactPreviewAction) } +// serveArtifactV4PreviewRaw opens the v4 artifact zip once and serves a single file from it, +// avoiding the redundant parse that listPreviewPaths would do for raw fetches. +func serveArtifactV4PreviewRaw(ctx *context_module.Context, artifact *actions_model.ActionArtifact, requested string) { + obj, reader, err := openArtifactV4ZipReader(artifact) + if err != nil { + if !errors.Is(err, zip.ErrFormat) { + ctx.ServerError("openArtifactV4ZipReader", err) + return + } + fallbackPath := artifactPreviewFallbackPath(artifact) + selectedPath := ChoosePreviewPath([]string{fallbackPath}, requested) + if selectedPath == "" { + ctx.HTTPError(http.StatusNotFound, "artifact file not found") + return + } + f, err := storage.ActionsArtifacts.Open(artifact.StoragePath) + if err != nil { + ctx.ServerError("ActionsArtifacts.Open", err) + return + } + defer f.Close() + previewArtifactByReadSeeker(ctx, selectedPath, f) + return + } + defer obj.Close() + + paths, files := listArtifactV4ZipFiles(reader) + selectedPath := ChoosePreviewPath(paths, requested) + if selectedPath == "" { + ctx.HTTPError(http.StatusNotFound, "artifact file not found") + return + } + zf := files[selectedPath] + r, err := zf.Open() + if err != nil { + ctx.ServerError("zip.File.Open", err) + return + } + defer r.Close() + previewArtifactByReader(ctx, selectedPath, r) +} + func ArtifactsPreviewRawView(ctx *context_module.Context) { artifactName := ctx.PathParam("artifact_name") @@ -1286,61 +1328,17 @@ func ArtifactsPreviewRawView(ctx *context_module.Context) { if !ok { return } - - paths, err := listPreviewPaths(artifacts) - if err != nil { - ctx.ServerError("listPreviewPaths", err) - return - } - selectedPath := ChoosePreviewPath(paths, GetRequestedPreviewPath(ctx)) - if selectedPath == "" { - ctx.HTTPError(http.StatusNotFound, "artifact file not found") - return - } + requested := GetRequestedPreviewPath(ctx) if len(artifacts) == 1 && actions.IsArtifactV4(artifacts[0]) { - artifact := artifacts[0] + serveArtifactV4PreviewRaw(ctx, artifacts[0], requested) + return + } - obj, reader, err := openArtifactV4ZipReader(artifact) - if err != nil { - if !errors.Is(err, zip.ErrFormat) { - ctx.ServerError("openArtifactV4ZipReader", err) - return - } - - fallbackPath := artifactPreviewFallbackPath(artifact) - if selectedPath != fallbackPath { - ctx.HTTPError(http.StatusNotFound, "artifact file not found") - return - } - - f, err := storage.ActionsArtifacts.Open(artifact.StoragePath) - if err != nil { - ctx.ServerError("ActionsArtifacts.Open", err) - return - } - defer f.Close() - - previewArtifactByReadSeeker(ctx, selectedPath, f) - return - } - defer obj.Close() - - _, files := listArtifactV4ZipFiles(reader) - zf, ok := files[selectedPath] - if !ok { - ctx.HTTPError(http.StatusNotFound, "artifact file not found") - return - } - - r, err := zf.Open() - if err != nil { - ctx.ServerError("zip.File.Open", err) - return - } - defer r.Close() - - previewArtifactByReader(ctx, selectedPath, r) + paths := listPreviewPathsForLegacyArtifacts(artifacts) + selectedPath := ChoosePreviewPath(paths, requested) + if selectedPath == "" { + ctx.HTTPError(http.StatusNotFound, "artifact file not found") return } diff --git a/templates/shared/actions/artifact_preview_content.tmpl b/templates/shared/actions/artifact_preview_content.tmpl index 1fc13fca2e..41c85faa2d 100644 --- a/templates/shared/actions/artifact_preview_content.tmpl +++ b/templates/shared/actions/artifact_preview_content.tmpl @@ -49,7 +49,8 @@
{{if .SelectedPath}} - + {{/* iframe sandbox="" matches the response CSP `sandbox` directive: opaque origin, no scripts, no forms, no popups. */}} + {{else}}
{{ctx.Locale.Tr "none"}}
{{end}} diff --git a/tests/integration/actions_artifact_preview_test.go b/tests/integration/actions_artifact_preview_test.go index 9e9b018215..afe627a20c 100644 --- a/tests/integration/actions_artifact_preview_test.go +++ b/tests/integration/actions_artifact_preview_test.go @@ -51,8 +51,8 @@ func TestActionsArtifactPreviewSingleFile(t *testing.T) { resp := session.MakeRequest(t, req, http.StatusOK) assert.Contains(t, resp.Body.String(), "abc.txt") assert.Contains(t, resp.Body.String(), "/preview/raw/abc.txt") - assert.Contains(t, resp.Body.String(), "sandbox=\"allow-scripts\"") - assert.Contains(t, resp.Body.String(), "referrerpolicy=\"no-referrer\"") + assert.Contains(t, resp.Body.String(), `sandbox=""`) + assert.Contains(t, resp.Body.String(), `referrerpolicy="no-referrer"`) req = NewRequestf(t, "GET", "/%s/actions/runs/187/artifacts/artifact-download/preview/raw", repo.FullName()) resp = session.MakeRequest(t, req, http.StatusOK)