diff --git a/internal/http/services/archiver/handler.go b/internal/http/services/archiver/handler.go index d665de7da1c..f1d89ed86d3 100644 --- a/internal/http/services/archiver/handler.go +++ b/internal/http/services/archiver/handler.go @@ -114,6 +114,10 @@ func (c *Config) init() { c.Prefix = "download_archive" } + // Sanitize before the default so s.config.Name is always safe to put in a + // Content-Disposition header: net.ContentDispositionAttachment escapes the filename* form + // but emits filename="..." verbatim, and a configured name is not otherwise validated. + c.Name = sanitizeArchiveName(c.Name) if c.Name == "" { c.Name = "download" } @@ -204,8 +208,9 @@ func (s *svc) allAllowed(paths []string) error { */ // resourceName resolves the name of a single resource so the archive can be named after it instead -// of the generic "download". It returns an empty string on any failure, so the caller keeps the -// default name. The name is sanitized via sanitizeArchiveName. +// of the configured default. It returns an error when the lookup fails, and an empty name when the +// resource yields nothing usable: neither a name nor a path, or a name that sanitizes away. +// Callers fall back to their own default on either. The name is sanitized via sanitizeArchiveName. func (s *svc) resourceName(ctx context.Context, id *provider.ResourceId) (string, error) { gatewayClient, err := s.gatewaySelector.Next() if err != nil { @@ -232,6 +237,20 @@ func (s *svc) resourceName(ctx context.Context, id *provider.ResourceId) (string return sanitizeArchiveName(name), nil } +// archiveName returns the name for the archive: the resource's own name when a single resource +// was requested and could be resolved, and the configured default otherwise. +func (s *svc) archiveName(ctx context.Context, resources []*provider.ResourceId) string { + if len(resources) != 1 { + return s.config.Name + } + name, err := s.resourceName(ctx, resources[0]) + if err != nil || name == "" { + s.log.Debug().Err(err).Msg("could not resolve the archive name, using the default") + return s.config.Name + } + return name +} + // sanitizeArchiveName removes characters that would break the Content-Disposition header (CR, LF, // double quote) or let the name act as a path (slash, backslash), plus all control characters // (C0, DEL and C1). It returns an empty string if nothing usable is left. @@ -305,19 +324,11 @@ func (s *svc) Handler() http.Handler { } // Name the archive after the resource when a single one was requested, instead of the - // generic "download". The name must be resolved here, before the body is streamed: the + // configured default. The name must be resolved here, before the body is streamed: the // Content-Disposition header below is written before CreateZip/CreateTar run, so the name // the walker resolves while building the archive would come too late. // See https://github.com/opencloud-eu/reva/issues/308 - archName := s.config.Name - if len(resources) == 1 { - if name, err := s.resourceName(ctx, resources[0]); name != "" && err == nil { - archName = name - } else { - s.log.Debug().Err(err).Msg("could not resolve the archive name, using the default") - archName = "download" - } - } + archName := s.archiveName(ctx, resources) if format == "tar" { archName += ".tar" } else { diff --git a/internal/http/services/archiver/handler_internal_test.go b/internal/http/services/archiver/handler_internal_test.go index 002645ae2a9..097a7469105 100644 --- a/internal/http/services/archiver/handler_internal_test.go +++ b/internal/http/services/archiver/handler_internal_test.go @@ -104,6 +104,89 @@ func TestResourceName(t *testing.T) { } } +// TestArchiveName covers what archiveName does with resourceName's result, which TestResourceName +// above does not reach. archiveName maps several distinct failures onto one fallback, so a +// regression in that fallback stays invisible to the callee's own tests. +// +// resourceName returns ("", nil) when the resource has no usable name; the caller must treat that +// like an error and keep s.config.Name. A deployment that configured a name must never silently +// receive a different one. +func TestArchiveName(t *testing.T) { + const configured = "meinarchiv" + ok := func(info *provider.ResourceInfo) *provider.StatResponse { + return &provider.StatResponse{Status: &rpc.Status{Code: rpc.Code_CODE_OK}, Info: info} + } + + cases := []struct { + name string + single bool + resp *provider.StatResponse + statErr error + selErr error + want string + }{ + {name: "resolved name wins", single: true, resp: ok(&provider.ResourceInfo{Name: "Documents"}), want: "Documents"}, + {name: "stat error keeps the configured name", single: true, statErr: errors.New("boom"), want: configured}, + {name: "non-OK status keeps the configured name", single: true, resp: &provider.StatResponse{Status: &rpc.Status{Code: rpc.Code_CODE_NOT_FOUND}}, want: configured}, + {name: "selector error keeps the configured name", single: true, selErr: errors.New("no gateway"), want: configured}, + {name: "name sanitizing to empty keeps the configured name", single: true, resp: ok(&provider.ResourceInfo{Name: "/"}), want: configured}, + {name: "empty name and path keep the configured name", single: true, resp: ok(&provider.ResourceInfo{}), want: configured}, + {name: "several resources keep the configured name", single: false, want: configured}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + gw := cs3mocks.NewGatewayAPIClient(t) + if tc.single && tc.selErr == nil { + gw.EXPECT().Stat(mock.Anything, mock.Anything).Return(tc.resp, tc.statErr).Once() + } + log := zerolog.Nop() + s := &svc{ + gatewaySelector: fakeSelector{client: gw, err: tc.selErr}, + log: &log, + config: &Config{Name: configured}, + } + + resources := []*provider.ResourceId{{OpaqueId: "x"}} + if !tc.single { + resources = append(resources, &provider.ResourceId{OpaqueId: "y"}) + } + + if got := s.archiveName(context.Background(), resources); got != tc.want { + t.Errorf("archiveName() = %q, want %q", got, tc.want) + } + }) + } +} + +// TestConfigNameSanitized pins that a configured archive name cannot break the +// Content-Disposition header. archiveName returns s.config.Name on every fallback path, so the +// value reaches the header unmodified unless it is sanitized once at config time. +func TestConfigNameSanitized(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"plain name kept", "meinarchiv", "meinarchiv"}, + {"double quote removed", `my"archive`, "myarchive"}, + {"crlf removed", "arch\r\nive", "archive"}, + {"slashes removed", "a/b", "ab"}, + {"unset falls back", "", "download"}, + {"sanitizing to empty falls back", "/", "download"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := &Config{Name: tc.in} + c.init() + if c.Name != tc.want { + t.Errorf("Config.init() Name = %q, want %q", c.Name, tc.want) + } + }) + } +} + func TestArchiveNameContentDisposition(t *testing.T) { // A name with umlauts survives sanitization and still encodes correctly: // net.ContentDispositionAttachment emits both the RFC 6266 filename* form and the raw filename.