internal/v5: Support symlink for files in archive. #817
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
internal/charmstore/archive.go
Outdated
| if err != nil { | ||
| return nil, 0, errgo.Notef(err, "cannot read archive data for symlink %s", file.Name) | ||
| } | ||
| return nil, 0, ErrRedirect{URL: string(url)} |
There was a problem hiding this comment.
It's conventional to return pointers for errors, so that it's easy to remember.
| @@ -135,11 +137,27 @@ func (s *Store) OpenBlobFile(blob *Blob, filePath string) (io.ReadCloser, int64, | |||
| if err != nil { | |||
| return nil, 0, errgo.Notef(err, "unable to read file %q", filePath) | |||
| } | |||
| if file.Mode()&os.ModeSymlink != 0 { | |||
There was a problem hiding this comment.
Please document this behaviour in the doc comment for OpenBlobFile.
internal/charmstore/archive.go
Outdated
| return content, fileInfo.Size(), nil | ||
| } | ||
| return nil, 0, errgo.WithCausef(nil, params.ErrNotFound, "file %q not found in the archive", filePath) | ||
| } | ||
|
|
||
| type ErrRedirect struct { |
There was a problem hiding this comment.
// ErrRedirect holds the error returned by OpenBlobFile when a symbolic link
// is opened. The Path field holds the name of the file that the symbolic link
// refers to.
internal/charmstore/archive.go
Outdated
| return content, fileInfo.Size(), nil | ||
| } | ||
| return nil, 0, errgo.WithCausef(nil, params.ErrNotFound, "file %q not found in the archive", filePath) | ||
| } | ||
|
|
||
| type ErrRedirect struct { | ||
| URL string |
There was a problem hiding this comment.
This isn't a URL. Please call it "Path" not "URL".
internal/charmstore/archive.go
Outdated
| } | ||
|
|
||
| func (e ErrRedirect) Error() string { | ||
| return fmt.Sprintf("redirect to %v", e.URL) |
There was a problem hiding this comment.
Let's make this look a bit more like an error. How about: fmt.Sprintf("file is a symbolic link to %q", e.Path) ?
internal/v5/archive.go
Outdated
| @@ -334,6 +335,17 @@ func (h *ReqHandler) serveArchiveFile(id *router.ResolvedURL, w http.ResponseWri | |||
| func (h *ReqHandler) ServeBlobFile(w http.ResponseWriter, req *http.Request, id *router.ResolvedURL, blob *charmstore.Blob) error { | |||
| r, size, err := h.Store.OpenBlobFile(blob, req.URL.Path) | |||
| if err != nil { | |||
| if redirect, ok := err.(charmstore.ErrRedirect); ok { | |||
There was a problem hiding this comment.
How about checking whether it's not a symlink and returning early if so, to save a level of indirection?
internal/v5/archive.go
Outdated
| @@ -334,6 +335,17 @@ func (h *ReqHandler) serveArchiveFile(id *router.ResolvedURL, w http.ResponseWri | |||
| func (h *ReqHandler) ServeBlobFile(w http.ResponseWriter, req *http.Request, id *router.ResolvedURL, blob *charmstore.Blob) error { | |||
| r, size, err := h.Store.OpenBlobFile(blob, req.URL.Path) | |||
| if err != nil { | |||
| if redirect, ok := err.(charmstore.ErrRedirect); ok { | |||
| p, err := router.RelativeURLPath(req.URL.Path, path.Join(path.Dir(req.URL.Path), redirect.URL)) | |||
There was a problem hiding this comment.
I think it might be worth calling path.Clean on redirect and checking that the result doesn't begin with ".." (so it's not possible to have links that point outside the charm itself.
Also, I've just thought of another thing which is... not trivial to fix: what about symlinks to directories?
For example, say there's a charm archive that contains:
foo -> hooks
hooks/install
What do we see if we try to open $charmid/archive/foo/install ?
There was a problem hiding this comment.
it's not possible to have links that point outside the charm itself as we only read in the zip file.
| headers := rec.Header() | ||
| c.Assert(headers.Get("Content-Length"), gc.Equals, strconv.Itoa(len(expectBytes))) | ||
| // We only have text files in the charm repository used for tests. | ||
| c.Assert(headers.Get("Content-Type"), gc.Equals, "text/plain; charset=utf-8") | ||
| assertCacheControl(c, rec.Header(), true) | ||
| } | ||
|
|
||
| func retrieveExpectedBytes(c *gc.C, zipFile *zip.ReadCloser, filePath string) (expectBytes []byte) { |
There was a problem hiding this comment.
This might be better named readZipFilePath, but honestly I think we could just leave things as they were and just test the symlink behaviour directly in the symlink test rather than calling assertArchiveFileContents.
| @@ -1178,25 +1179,24 @@ func (s *ArchiveSuite) TestArchiveFileGet(c *gc.C) { | |||
| s.assertArchiveFileContents(c, zipFile, "~charmers/utopic/all-hooks-0/archive/hooks/install") | |||
| } | |||
|
|
|||
| func (s *ArchiveSuite) TestSymLinkArchiveFileGet(c *gc.C) { | |||
There was a problem hiding this comment.
I'd like to see a few more tests of this behaviour. For example, does it work when the symlink is absolute, contains multiple slashes, trailing slashes? Does it work when the URL path contains a trailing slash or isn't clean?
There was a problem hiding this comment.
I'd say if a zipfile contains an absolute symlink then it is broken.
|
Refer to this link for build results (access rights to CI server needed): |
fabricematrat
force-pushed
the
support-symlink-archive
branch
from
6dc8592
to
6a001cf
Compare
|
Refer to this link for build results (access rights to CI server needed): |
| @@ -23,6 +23,7 @@ import ( | |||
| "gopkg.in/juju/charmstore.v5/internal/charmstore" | |||
| "gopkg.in/juju/charmstore.v5/internal/mongodoc" | |||
| "gopkg.in/juju/charmstore.v5/internal/router" | |||
| "path" | |||
There was a problem hiding this comment.
this seems in the wrong place
| // We can't use the http.redirect function because it tries to build an absolute url but the path in the | ||
| // request URL has been changed. | ||
| w.Header().Set("Location", p) | ||
| w.WriteHeader(http.StatusMovedPermanently) |
There was a problem hiding this comment.
I think StatusFound would be better. We cannot say it is moved perminently only until a new zip file is uploaded.
| @@ -1178,25 +1179,24 @@ func (s *ArchiveSuite) TestArchiveFileGet(c *gc.C) { | |||
| s.assertArchiveFileContents(c, zipFile, "~charmers/utopic/all-hooks-0/archive/hooks/install") | |||
| } | |||
|
|
|||
| func (s *ArchiveSuite) TestSymLinkArchiveFileGet(c *gc.C) { | |||
There was a problem hiding this comment.
I'd say if a zipfile contains an absolute symlink then it is broken.
No description provided.