WebPub parsing improvements

pkg/archive/archive.go

@@ -31,14 +31,15 @@ func (e DefaultArchiveFactory) Open(ctx context.Context, location url.URL, passw
 		return nil, errors.New("unsupported scheme " + u.Scheme().String())
 	}
 
-	st, err := os.Stat(u.Path())
+	path := u.ToFilepath()
+	st, err := os.Stat(path)
 	if err != nil {
 		return nil, err
 	}
 	if st.IsDir() {
-		return e.explodedFactory.Open(u.Path(), password)
+		return e.explodedFactory.Open(path, password)
 	} else {
-		return e.gozipFactory.Open(u.Path(), password)
+		return e.gozipFactory.Open(path, password)
 	}
 }
 

pkg/asset/asset_file.go

@@ -49,6 +49,10 @@ func (a *FileAsset) Name() string {
 }
 
 func (a *FileAsset) realPath() string {
+	// ToFilepath handles Windows drive paths in file URLs, e.g. file:///C:/dir.
+	if u, ok := a.uri.(url.AbsoluteURL); ok && u.IsFile() {
+		return filepath.ToSlash(u.ToFilepath())
+	}
 	return filepath.ToSlash(a.uri.Path())
 }
 
@@ -71,6 +75,33 @@ func (a *FileAsset) MediaType(ctx context.Context) mediatype.MediaType {
 	return *a.mediatype
 }
 
+// Location implements RelativePublicationAsset
+func (a *FileAsset) Location() url.URL {
+	return a.uri
+}
+
+// CreateRelativeFetcher implements RelativePublicationAsset
+func (a *FileAsset) CreateRelativeFetcher(ctx context.Context, root url.URL) (fetcher.Fetcher, error) {
+	var rootPath string
+	if u, ok := root.(url.AbsoluteURL); ok {
+		if !u.IsFile() {
+			return nil, errors.New("root of a file asset must be a file URL")
+		}
+		rootPath = u.ToFilepath()
+	} else {
+		rootPath = filepath.FromSlash(root.Path())
+	}
+
+	rootPath, err := filepath.Abs(rootPath)
+	if err != nil {
+		return nil, err
+	}
+	// The fetcher is sandboxed to the publication root: the parser normalizes the
+	// manifest's HREFs so that none escapes it, and HREFs handed to the publication
+	// later (e.g. from an incoming server request) must not reach outside it either.
+	return fetcher.NewFileFetcher("", rootPath), nil
+}
+
 // CreateFetcher implements PublicationAsset
 func (a *FileAsset) CreateFetcher(ctx context.Context, dependencies Dependencies, credentials string) (fetcher.Fetcher, error) {
 	if u, ok := a.uri.(url.AbsoluteURL); ok && !u.IsFile() {

pkg/asset/asset_gcs.go

@@ -86,6 +86,24 @@ func (a *GCSAsset) MediaType(ctx context.Context) mediatype.MediaType {
 	return *a.mediatype
 }
 
+// Location implements RelativePublicationAsset
+func (a *GCSAsset) Location() url.URL {
+	return a.uri
+}
+
+// CreateRelativeFetcher implements RelativePublicationAsset
+func (a *GCSAsset) CreateRelativeFetcher(ctx context.Context, root url.URL) (fetcher.Fetcher, error) {
+	u, ok := root.(url.AbsoluteURL)
+	if !ok {
+		return nil, errors.New("root of a GCS asset must be an absolute GS URL")
+	}
+	handle, err := u.ToGSObject(a.client)
+	if err != nil {
+		return nil, err
+	}
+	return fetcher.NewGCSFetcher("", a.client, handle), nil
+}
+
 // CreateFetcher implements PublicationAsset
 func (a *GCSAsset) CreateFetcher(ctx context.Context, dependencies Dependencies, credentials string) (fetcher.Fetcher, error) {
 	handle, err := a.handle()

pkg/asset/asset_http.go

@@ -111,6 +111,20 @@ func (a *HTTPAsset) MediaType(ctx context.Context) mediatype.MediaType {
 	return *a.mediatype
 }
 
+// Location implements RelativePublicationAsset
+func (a *HTTPAsset) Location() url.URL {
+	return a.url
+}
+
+// CreateRelativeFetcher implements RelativePublicationAsset
+func (a *HTTPAsset) CreateRelativeFetcher(ctx context.Context, root url.URL) (fetcher.Fetcher, error) {
+	u, ok := root.(url.AbsoluteURL)
+	if !ok || !u.IsHTTP() {
+		return nil, errors.New("root of an HTTP asset must be an absolute HTTP(S) URL")
+	}
+	return fetcher.NewHTTPFetcher("", a.client, u), nil
+}
+
 // CreateFetcher implements PublicationAsset
 func (a *HTTPAsset) CreateFetcher(ctx context.Context, dependencies Dependencies, credentials string) (fetcher.Fetcher, error) {
 	// We can't determine if the provided path is a directory or not unless it ends in a "/"

pkg/asset/asset_publication.go

@@ -6,6 +6,7 @@ import (
 	"github.com/readium/go-toolkit/pkg/archive"
 	"github.com/readium/go-toolkit/pkg/fetcher"
 	"github.com/readium/go-toolkit/pkg/mediatype"
+	"github.com/readium/go-toolkit/pkg/util/url"
 )
 
 type Dependencies struct {
@@ -18,3 +19,21 @@ type PublicationAsset interface {
 	MediaType(ctx context.Context) mediatype.MediaType                                                         // Media type of the asset. If unknown, fallback on `MediaType.Binary`.
 	CreateFetcher(ctx context.Context, dependencies Dependencies, credentials string) (fetcher.Fetcher, error) // Creates a fetcher used to access the asset's content.
 }
+
+// A [PublicationAsset] which can also provide access to resources located relative to itself,
+// e.g. the resources referenced by a bare (exploded) Readium Web Publication Manifest.
+type RelativePublicationAsset interface {
+	PublicationAsset
+
+	// The URL of the asset within its medium, e.g. a file path URL, or an
+	// HTTP(S), S3 or GS URL. HREFs relative to the asset resolve against it.
+	Location() url.URL
+
+	// Creates a fetcher serving the resources surrounding this asset, rooted at [root]:
+	// a link with HREF `a/file.xhtml` is fetched from `a/file.xhtml` under the root
+	// directory, whether the asset lives on a local file system or on a remote server.
+	//
+	// [root] must be the URL of a directory containing the asset, typically derived
+	// from [Location] by resolving a `./` or `../` reference against it.
+	CreateRelativeFetcher(ctx context.Context, root url.URL) (fetcher.Fetcher, error)
+}

pkg/asset/asset_s3.go

@@ -92,6 +92,24 @@ func (a *S3Asset) MediaType(ctx context.Context) mediatype.MediaType {
 	return *a.mediatype
 }
 
+// Location implements RelativePublicationAsset
+func (a *S3Asset) Location() url.URL {
+	return a.uri
+}
+
+// CreateRelativeFetcher implements RelativePublicationAsset
+func (a *S3Asset) CreateRelativeFetcher(ctx context.Context, root url.URL) (fetcher.Fetcher, error) {
+	u, ok := root.(url.AbsoluteURL)
+	if !ok {
+		return nil, errors.New("root of an S3 asset must be an absolute S3 URL")
+	}
+	obj, err := u.ToS3Object()
+	if err != nil {
+		return nil, err
+	}
+	return fetcher.NewS3Fetcher("", a.client, *obj.Bucket, *obj.Key), nil
+}
+
 // CreateFetcher implements PublicationAsset
 func (a *S3Asset) CreateFetcher(ctx context.Context, dependencies Dependencies, credentials string) (fetcher.Fetcher, error) {
 	obj, err := a.object()

pkg/fetcher/fetcher.go

@@ -2,10 +2,26 @@ package fetcher
 
 import (
 	"context"
+	"path"
+	"strings"
 
 	"github.com/readium/go-toolkit/pkg/manifest"
 )
 
+// Reports whether the slash-separated object key [key] stays within [root]: it is
+// [root] itself or sits below it. Used by the object-store fetchers to keep a `..`
+// in an HREF from addressing objects outside the fetcher's prefix, the way
+// [FileFetcher] contains resources within its directory. An empty [root] is the
+// bucket root, which only forbids upward (`..`) escapes.
+func keyWithinRoot(root, key string) bool {
+	root = path.Clean(root)
+	key = path.Clean(key)
+	if root == "." || root == "/" {
+		return key != ".." && !strings.HasPrefix(key, "../")
+	}
+	return key == root || strings.HasPrefix(key, root+"/")
+}
+
 // Fetcher provides access to a Resource from a Link.
 type Fetcher interface {
 

pkg/fetcher/fetcher_cloud_test.go

@@ -0,0 +1,90 @@
+package fetcher
+
+import (
+	"testing"
+
+	"cloud.google.com/go/storage"
+	"github.com/aws/aws-sdk-go-v2/service/s3"
+	"github.com/readium/go-toolkit/pkg/manifest"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Object keys are raw strings on S3 and GCS: HREFs must be decoded before the lookup,
+// and queries/fragments dropped.
+func TestS3FetcherGetKey(t *testing.T) {
+	f := NewS3Fetcher("", &s3.Client{}, "bucket", "pub")
+	for _, tt := range []struct {
+		href string
+		key  string
+	}{
+		{"track.mp3", "pub/track.mp3"},
+		{"my%20track.mp3", "pub/my track.mp3"},
+		{"my track.mp3", "pub/my track.mp3"},
+		{"caf%C3%A9.mp3", "pub/café.mp3"},
+		{"track.mp3#t=30", "pub/track.mp3"},
+		{"track.mp3?token=abc", "pub/track.mp3"},
+		{"audio/track.mp3", "pub/audio/track.mp3"},
+	} {
+		res := f.Get(t.Context(), manifest.Link{Href: manifest.MustNewHREFFromString(tt.href, false)})
+		s3res, ok := res.(*s3Resource)
+		require.Truef(t, ok, "expected an s3Resource for %q", tt.href)
+		assert.Equalf(t, tt.key, s3res.key, "key mismatch for href %q", tt.href)
+	}
+}
+
+// A `..` in an HREF must not let a request reach objects outside the fetcher's
+// key prefix, the way [FileFetcher] contains resources within its directory.
+func TestS3FetcherContainsKey(t *testing.T) {
+	f := NewS3Fetcher("", &s3.Client{}, "bucket", "pub")
+	for _, href := range []string{
+		"../secret.txt",
+		"../../secret.txt",
+		"audio/../../secret.txt",
+		"../pub-other/x.txt",
+	} {
+		res := f.Get(t.Context(), manifest.Link{Href: manifest.MustNewHREFFromString(href, false)})
+		_, ok := res.(*s3Resource)
+		assert.Falsef(t, ok, "escaping href %q should not resolve to an object", href)
+	}
+
+	// Resources within the prefix still resolve, including a `..` that stays inside.
+	for _, tt := range []struct{ href, key string }{
+		{"audio/track.mp3", "pub/audio/track.mp3"},
+		{"audio/../cover.jpg", "pub/cover.jpg"},
+	} {
+		res := f.Get(t.Context(), manifest.Link{Href: manifest.MustNewHREFFromString(tt.href, false)})
+		s3res, ok := res.(*s3Resource)
+		require.Truef(t, ok, "href %q should resolve", tt.href)
+		assert.Equal(t, tt.key, s3res.key)
+	}
+}
+
+func TestGCSFetcherContainsObjectName(t *testing.T) {
+	client := &storage.Client{}
+	f := NewGCSFetcher("", client, client.Bucket("bucket").Object("pub"))
+	for _, href := range []string{"../secret.txt", "../../secret.txt", "audio/../../secret.txt"} {
+		res := f.Get(t.Context(), manifest.Link{Href: manifest.MustNewHREFFromString(href, false)})
+		_, ok := res.(*gcsResource)
+		assert.Falsef(t, ok, "escaping href %q should not resolve to an object", href)
+	}
+}
+
+func TestGCSFetcherGetObjectName(t *testing.T) {
+	client := &storage.Client{}
+	f := NewGCSFetcher("", client, client.Bucket("bucket").Object("pub"))
+	for _, tt := range []struct {
+		href string
+		name string
+	}{
+		{"track.mp3", "pub/track.mp3"},
+		{"my%20track.mp3", "pub/my track.mp3"},
+		{"caf%C3%A9.mp3", "pub/café.mp3"},
+		{"track.mp3#t=30", "pub/track.mp3"},
+	} {
+		res := f.Get(t.Context(), manifest.Link{Href: manifest.MustNewHREFFromString(tt.href, false)})
+		gcsres, ok := res.(*gcsResource)
+		require.Truef(t, ok, "expected a gcsResource for %q", tt.href)
+		assert.Equalf(t, tt.name, gcsres.handle.ObjectName(), "object name mismatch for href %q", tt.href)
+	}
+}

pkg/fetcher/fetcher_file.go

@@ -99,7 +99,10 @@ func (f *FileFetcher) Get(ctx context.Context, link manifest.Link) Resource {
 			if err != nil {
 				continue // TODO somehow get this error out?
 			}
-			if strings.HasPrefix(rapath, iapath) {
+			// The path must be [itemFile] itself, or sit below it beyond a separator:
+			// a plain prefix check would let "dir-other" pass as a descendant of "dir".
+			sep := string(filepath.Separator)
+			if rapath == iapath || strings.HasPrefix(rapath, strings.TrimSuffix(iapath, sep)+sep) {
 				resource := NewFileResource(link, resourceFile)
 				f.resources = append(f.resources, weak.Make(resource))
 				return resource

pkg/fetcher/fetcher_file_test.go

@@ -2,6 +2,8 @@ package fetcher
 
 import (
 	"bytes"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/readium/go-toolkit/pkg/manifest"
@@ -83,6 +85,27 @@ func TestFileFetcherDirectoryTraversalNotFound(t *testing.T) {
 	assert.Equal(t, NotFound(err.Cause), err, "cannot traverse up a directory using '..'")
 }
 
+func TestFileFetcherSiblingDirectoryNotFound(t *testing.T) {
+	// A directory sharing a name prefix with the fetcher's root must not be reachable.
+	dir := t.TempDir()
+	require.NoError(t, os.MkdirAll(filepath.Join(dir, "pub"), 0o755))
+	require.NoError(t, os.MkdirAll(filepath.Join(dir, "pub-secret"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "pub", "inside.txt"), []byte("inside"), 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "pub-secret", "leak.txt"), []byte("leak"), 0o644))
+
+	fetcher := NewFileFetcher("", filepath.Join(dir, "pub"))
+	defer fetcher.Close()
+
+	resource := fetcher.Get(t.Context(), manifest.Link{Href: manifest.MustNewHREFFromString("inside.txt", false)})
+	bin, err := resource.Read(t.Context(), 0, 0)
+	require.Nil(t, err)
+	assert.Equal(t, "inside", string(bin))
+
+	resource = fetcher.Get(t.Context(), manifest.Link{Href: manifest.MustNewHREFFromString("../pub-secret/leak.txt", false)})
+	_, err = resource.Read(t.Context(), 0, 0)
+	assert.Equal(t, NotFound(err.Cause), err, "cannot escape into a prefix-sharing sibling directory")
+}
+
 func TestFileFetcherReadRange(t *testing.T) {
 	resource := testFileFetcher.Get(t.Context(), manifest.Link{Href: manifest.MustNewHREFFromString("file_href", false)})
 	bin, err := resource.Read(t.Context(), 0, 2)

pkg/fetcher/fetcher_gcs.go

@@ -122,12 +122,23 @@ func (f *GCSFetcher) Links(ctx context.Context) (manifest.LinkList, error) {
 
 // Get implements Fetcher
 func (f *GCSFetcher) Get(ctx context.Context, link manifest.Link) Resource {
-	linkHref := link.Href.String()
+	// Use the decoded path for the object lookup: GCS object names are raw strings, so a
+	// percent-encoded HREF would never match, and queries/fragments don't belong in names.
+	var linkHref string
+	if hrefURL := link.Href.Resolve(nil, nil); hrefURL != nil {
+		linkHref = hrefURL.Path()
+	} else {
+		linkHref = link.Href.String()
+	}
 	if strings.HasPrefix(linkHref, f.href) {
 		resourceFile := path.Join(f.handle.ObjectName(), strings.TrimPrefix(linkHref, f.href))
-		return &gcsResource{
-			handle: f.client.Bucket(f.handle.BucketName()).Object(resourceFile),
-			link:   link,
+		// Keep the resource within the fetcher's object-name prefix: a `..` in the HREF
+		// must not let an incoming request reach objects elsewhere in the bucket.
+		if keyWithinRoot(f.handle.ObjectName(), resourceFile) {
+			return &gcsResource{
+				handle: f.client.Bucket(f.handle.BucketName()).Object(resourceFile),
+				link:   link,
+			}
 		}
 	}
 

pkg/fetcher/fetcher_http.go

@@ -66,10 +66,16 @@ func (f *HTTPFetcher) Get(ctx context.Context, link manifest.Link) Resource {
 	if strings.HasPrefix(linkHref, f.href) {
 		rurl, err := url.RelativeURLFromString(strings.TrimPrefix(linkHref, f.href))
 		if err == nil {
-			return &httpResource{
-				link:   link,
-				client: f.client,
-				url:    f.url.Resolve(rurl).(url.AbsoluteURL),
+			resolved := f.url.Resolve(rurl).(url.AbsoluteURL)
+			// Keep the resource within the base URL: a `..` or root-absolute HREF must
+			// not let an incoming request reach a path outside the fetcher's location.
+			// Relativize returns a relative URL only when [resolved] is under [f.url].
+			if _, ok := f.url.Relativize(resolved).(url.RelativeURL); ok {
+				return &httpResource{
+					link:   link,
+					client: f.client,
+					url:    resolved,
+				}
 			}
 		}
 	}
@@ -81,6 +87,18 @@ func (f *HTTPFetcher) Close() {
 	// No-op for HTTP
 }
 
+// Creates a [Resource] serving the contents of the given HTTP(S) URL.
+func NewHTTPResource(link manifest.Link, client *http.Client, url url.AbsoluteURL) Resource {
+	if client == nil {
+		panic("NewHTTPResource requires a non-nil client")
+	}
+	return &httpResource{
+		link:   link,
+		client: client,
+		url:    url,
+	}
+}
+
 // Resource from HTTP
 type httpResource struct {
 	link   manifest.Link