Skip to content

fix: remove traversals calling ubuntuIndex.fetch #257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
upils wants to merge 16 commits into
base: main
Choose a base branch
from
Open

fix: remove traversals calling ubuntuIndex.fetch #257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
638e4c8
fix: Resolve URL path before fetching content
upils Nov 21, 2025
24dc87f
fix: missing import
upils Nov 21, 2025
183f188
test: URL cleaning
upils Nov 21, 2025
0c2633d
fix: mark error as internal
upils Nov 24, 2025
7b84848
style: improve naming
upils Nov 24, 2025
1fbc902
fix(test): improve error message formatting
upils Nov 24, 2025
57cef9f
docs: clarify suffix handling
upils Nov 25, 2025
31a78dc
refactor: simplify fetching package
upils Nov 25, 2025
a8a06ee
refactor: revert some changes to minimize diff
upils Nov 25, 2025
c4e2c66
style: clearer test error message
upils Jan 7, 2026
f9f7a9e
style: remove redondant comment
upils Jan 12, 2026
9cd4437
refactor: do not assume the archive structure
upils Jan 13, 2026
073443c
style: fix formatting
upils Jan 13, 2026
09c9b10
style: improving name to get the dist path
upils Jan 13, 2026
938e05d
ci: retry
upils Jan 14, 2026
e9ccfda
fix: construct URL in a robust way
upils Jan 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 17 additions & 16 deletions internal/archive/archive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"io"
"net/http"
"net/url"
"slices"
"strings"
"time"
Expand Down Expand Up @@ -134,9 +135,9 @@ func (a *ubuntuArchive) Fetch(pkg string) (io.ReadSeekCloser, *PackageInfo, erro
if err != nil {
return nil, nil, err
}
suffix := section.Get("Filename")
logf("Fetching %s...", suffix)
reader, err := index.fetch("../../"+suffix, section.Get("SHA256"), fetchBulk)
path := section.Get("Filename")
logf("Fetching %s...", path)
reader, err := index.fetch(path, section.Get("SHA256"), fetchBulk)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -276,7 +277,7 @@ func openUbuntu(options *Options) (Archive, error) {

func (index *ubuntuIndex) fetchRelease() error {
logf("Fetching %s %s %s suite details...", index.displayName(), index.version, index.suite)
reader, err := index.fetch("InRelease", "", fetchDefault)
reader, err := index.fetch(index.distPath("InRelease"), "", fetchDefault)
if err != nil {
return err
}
Expand Down Expand Up @@ -333,7 +334,7 @@ func (index *ubuntuIndex) fetchIndex() error {
}

logf("Fetching index for %s %s %s %s component...", index.displayName(), index.version, index.suite, index.component)
reader, err := index.fetch(packagesPath+".gz", digest, fetchBulk)
reader, err := index.fetch(index.distPath(packagesPath+".gz"), digest, fetchBulk)
if err != nil {
return err
}
Expand Down Expand Up @@ -368,27 +369,27 @@ func (index *ubuntuIndex) checkComponents(components []string) error {
return nil
}

func (index *ubuntuIndex) fetch(suffix, digest string, flags fetchFlags) (io.ReadSeekCloser, error) {
func (index *ubuntuIndex) distPath(suffix string) string {
return "dists/" + index.suite + "/" + suffix
}

func (index *ubuntuIndex) fetch(path, digest string, flags fetchFlags) (io.ReadSeekCloser, error) {
reader, err := index.archive.cache.Open(digest)
if err == nil {
return reader, nil
} else if err != cache.MissErr {
return nil, err
}

baseURL, creds := index.archive.baseURL, index.archive.creds

var url string
if strings.HasPrefix(suffix, "pool/") {
url = baseURL + suffix
} else {
url = baseURL + "dists/" + index.suite + "/" + suffix
cleanURL, err := url.JoinPath(index.archive.baseURL, path)
Copy link
Collaborator Author

@upils upils Jan 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Note to reviewer]: I wanted to avoid doing that because it does more than what I needed. But other solutions I could come up with looked hacky and feeble.

This is also subtly changing the error returned in case of an invalid baseURL. Previously it was going to fail when calling http.NewRequest. However these URLs are defined in the code, so this should never happen unless we mess with these URLs.

Copy link
Collaborator

@letFunny letFunny Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid doing that because it does more than what I needed.

I think this is okay, it is only cleaning the URL so there are no double slashes and path traversals. This is less hacky and more tested as it comes from the standard library.

if err != nil {
return nil, fmt.Errorf("internal error: cannot construct URL: %v", err)
}

req, err := http.NewRequest("GET", url, nil)
req, err := http.NewRequest("GET", cleanURL, nil)
if err != nil {
return nil, fmt.Errorf("cannot create HTTP request: %v", err)
}
creds := index.archive.creds
if creds != nil && !creds.Empty() {
req.SetBasicAuth(creds.Username, creds.Password)
}
Expand All @@ -415,7 +416,7 @@ func (index *ubuntuIndex) fetch(suffix, digest string, flags fetchFlags) (io.Rea
}

body := resp.Body
if strings.HasSuffix(suffix, ".gz") {
if strings.HasSuffix(path, ".gz") {
reader, err := gzip.NewReader(body)
if err != nil {
return nil, fmt.Errorf("cannot decompress data: %v", err)
Expand Down
8 changes: 8 additions & 0 deletions internal/archive/archive_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,14 @@ func (s *httpSuite) Do(req *http.Request) (*http.Response, error) {
return nil, fmt.Errorf("test expected base %q, got %q", s.base, req.URL.String())
}

cleanURL, err := url.JoinPath(req.URL.String())
if err != nil {
return nil, fmt.Errorf("cannot clean requested URL: %v", err)
}
if cleanURL != req.URL.String() {
return nil, fmt.Errorf("test expected clean URL %q, got %q", cleanURL, req.URL.String())
}

s.request = req
s.requests = append(s.requests, req)
body := s.response
Expand Down
Loading