diff --git a/pkg/modules/api/context.go b/pkg/modules/api/context.go index 9172e0af..1e1cc476 100644 --- a/pkg/modules/api/context.go +++ b/pkg/modules/api/context.go @@ -396,6 +396,17 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys dlSpan.RecordError(err) dlSpan.SetStatus(codes.Error, err.Error()) dlSpan.End() + + // A redirect target is filtered inside the client, so the + // policy verdict surfaces here rather than from the + // pre-flight above. Keep it out of the response: the first + // hop answers a filtered URL with a generic 403, and a + // later hop must not describe the allow-list, the deny-list + // or the IP policy instead. + if errors.Is(err, gotenberg.ErrFiltered) { + return fmt.Errorf("download file from '%s': %w", dl.Url, err) + } + return WrapError( fmt.Errorf("download file from to '%s': %w", dl.Url, err), NewSentinelHttpError(http.StatusBadRequest, fmt.Sprintf("Unable to download file from '%s': %s", dl.Url, err)), diff --git a/pkg/modules/api/context_test.go b/pkg/modules/api/context_test.go index ed7f5691..8857f657 100644 --- a/pkg/modules/api/context_test.go +++ b/pkg/modules/api/context_test.go @@ -11,6 +11,7 @@ import ( "net/http" "net/http/httptest" "os" + "regexp" "runtime" "strings" "sync" @@ -18,6 +19,7 @@ import ( "testing" "time" + "github.com/dlclark/regexp2" "github.com/labstack/echo/v4" "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" @@ -832,3 +834,71 @@ func TestNewContext_DuplicateFilenamesAreBothKept(t *testing.T) { t.Fatalf("distinct disk paths = %d, want 2", len(seen)) } } + +// A redirect target is filtered inside the HTTP client, so the policy verdict +// surfaces from client.Do rather than from the pre-flight check. It used to be +// interpolated into the response body, so a redirect described the allow-list, +// the deny-list or the IP policy where the first hop returns a generic 403. +func TestNewContext_DownloadFromRedirectVerdictStaysGeneric(t *testing.T) { + private := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Disposition", `attachment; filename="secret.txt"`) + _, _ = w.Write([]byte("internal")) + })) + defer private.Close() + + redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, private.URL+"/secret", http.StatusFound) + })) + defer redirector.Close() + + payload, err := json.Marshal([]downloadFrom{{Url: redirector.URL + "/start"}}) + if err != nil { + t.Fatalf("marshal downloadFrom payload: %v", err) + } + + body := new(bytes.Buffer) + writer := multipart.NewWriter(body) + err = writer.WriteField("downloadFrom", string(payload)) + if err != nil { + t.Fatalf("write downloadFrom field: %v", err) + } + err = writer.Close() + if err != nil { + t.Fatalf("close multipart writer: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/forms/libreoffice/convert", body) + req.Header.Set("Content-Type", writer.FormDataContentType()) + + echoCtx := echo.New().NewContext(req, httptest.NewRecorder()) + logger := slog.New(slog.DiscardHandler) + fs := gotenberg.NewFileSystem(new(gotenberg.OsMkdirAll)) + + // The first hop is allowed, the redirect target is denied by the deny-list. + denyList := []*regexp2.Regexp{regexp2.MustCompile("^"+regexp.QuoteMeta(private.URL), 0)} + + _, cancel, err := newContext(echoCtx, logger, fs, 10*time.Second, 0, downloadFromConfig{ + denyList: denyList, + maxRetry: 0, + }) + if cancel != nil { + defer cancel() + } + if err == nil { + t.Fatal("expected the redirect to a denied host to fail") + } + + status, message := ParseError(err) + if status != http.StatusForbidden { + t.Fatalf("status = %d, want %d: a filtered redirect must answer like a filtered first hop", status, http.StatusForbidden) + } + if message != http.StatusText(http.StatusForbidden) { + t.Fatalf("message = %q, want the generic %q", message, http.StatusText(http.StatusForbidden)) + } + // The response must not name the policy, the pattern, or the blocked host. + for _, leak := range []string{"denied list", "allowed list", "non-public", "expression", private.URL} { + if strings.Contains(message, leak) { + t.Fatalf("response message %q leaks %q", message, leak) + } + } +}