Skip to content

Commit a9f2bee

Browse files
aledbfclaude
andcommitted
feat(features): make direct-tarball download match the TS CLI
downloadFeatureTarball was a bare GET: no auth, no User-Agent, no integrity check, 200-only. It worked for public tarballs but 404'd on private GitHub release assets and never recorded a digest. Bring it to parity with the TS request/getRequestHeaders/fetchContentsAtTarballUri path: - User-Agent: devcontainer on every request. - Authorization: Bearer $GITHUB_TOKEN for github.com / api.github.com URLs (from the process env via osEnvMap), so private assets resolve. Go strips the header on the cross-host redirect to the presigned CDN URL, which is the correct behavior for release assets. - Accept any 2xx, not only 200 (TS treats 200-299 as success). - Warn when fetching over plaintext HTTP (http:// or a localhost host). - Compute and return the tarball's sha256; the direct-tarball feature Set now carries ComputedDigest so the lockfile pins it, and a fetch is rejected when its digest does not match an existing lockfile integrity. - Thread ctx/logger/env; keep the shared proxy- and custom-CA-aware transport. Header/auth/digest logic is factored into small pure helpers with unit tests (featureRequestHeaders, isGitHubTarballURI, isPlainHTTPURL) plus httptest coverage of the real download path. A sweep confirms every HTTP path in the CLI (httpx, oras/OCI, tarball) now goes through httpx.NewTransport. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent f05eaee commit a9f2bee

3 files changed

Lines changed: 223 additions & 11 deletions

File tree

internal/cli/feature_deps.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package cli
22

33
import (
4+
"context"
45
"encoding/json"
56
"fmt"
67
"os"
@@ -63,7 +64,7 @@ func newMetadataProcessFeature(client oci.Registry, logger log.Logger, basePath
6364
}, nil
6465

6566
case features.SourceDirectTarball:
66-
meta, err := readTarballFeatureMetadata(id)
67+
meta, err := readTarballFeatureMetadata(logger, id)
6768
if err != nil {
6869
return nil, fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, err)
6970
}
@@ -177,8 +178,8 @@ func readLocalFeatureMetadata(dir string) (features.Feature, error) {
177178
return meta, nil
178179
}
179180

180-
func readTarballFeatureMetadata(url string) (features.Feature, error) {
181-
blobData, err := downloadFeatureTarball(url)
181+
func readTarballFeatureMetadata(logger log.Logger, tarballURL string) (features.Feature, error) {
182+
blobData, _, err := downloadFeatureTarball(context.Background(), logger, tarballURL, osEnvMap())
182183
if err != nil {
183184
return features.Feature{}, err
184185
}

internal/cli/feature_install.go

Lines changed: 85 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,13 @@ import (
44
"archive/tar"
55
"compress/gzip"
66
"context"
7+
"crypto/sha256"
8+
"encoding/hex"
79
"encoding/json"
810
"fmt"
911
"io"
1012
"net/http"
13+
"net/url"
1114
"os"
1215
"path/filepath"
1316
"sort"
@@ -255,10 +258,17 @@ func processInstallFeature(
255258
pfs.MkdirAll(featureDir)
256259

257260
logger.Write(fmt.Sprintf("Fetching feature tarball %s...", id), log.LevelInfo)
258-
blobData, dlErr := downloadFeatureTarball(id)
261+
blobData, computedDigest, dlErr := downloadFeatureTarball(context.Background(), logger, id, osEnvMap())
259262
if dlErr != nil {
260263
return nil, fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, dlErr)
261264
}
265+
// Verify integrity against the lockfile pin when present (TS parity: it
266+
// rejects a tarball whose digest does not match the expected one).
267+
if lockfile != nil {
268+
if entry, ok := lockfile.Features[id]; ok && entry.Integrity != "" && entry.Integrity != computedDigest {
269+
return nil, fmt.Errorf("ERR: Feature '%s' tarball digest %s did not match lockfile integrity %s", id, computedDigest, entry.Integrity)
270+
}
271+
}
262272
tgzPath := filepath.Join(featureDir, "feature.tgz")
263273
if err := pfs.WriteFile(tgzPath, blobData); err != nil {
264274
return nil, fmt.Errorf("write feature tarball: %w", err)
@@ -281,6 +291,9 @@ func processInstallFeature(
281291
SourceInfo: &features.TarballSource{TarballURI: id, UserID: id},
282292
Features: []features.Feature{feat},
283293
InternalVersion: "2",
294+
// Record the tarball digest so the lockfile pins it (TS parity: a
295+
// direct-tarball feature is locked by its computed sha256).
296+
ComputedDigest: computedDigest,
284297
}, nil
285298

286299
default: // OCI (and legacy shorthand resolved to OCI)
@@ -779,18 +792,82 @@ func extractUserOptions(v interface{}) map[string]interface{} {
779792
return nil
780793
}
781794

782-
// downloadFeatureTarball fetches a Feature published as a direct HTTP(S) tarball.
783-
func downloadFeatureTarball(url string) ([]byte, error) {
795+
// downloadFeatureTarball fetches a Feature published as a direct HTTP(S) tarball,
796+
// matching the TS CLI's request/getRequestHeaders behavior instead of a bare GET:
797+
// - sends a "devcontainer" User-Agent;
798+
// - attaches Authorization: Bearer $GITHUB_TOKEN for github.com / api.github.com
799+
// URLs, so private release assets resolve (the naive GET 404'd on them);
800+
// - accepts any 2xx (TS treats 200-299 as success), not only 200;
801+
// - warns when downloading over plaintext HTTP;
802+
// - goes through the shared proxy- and custom-CA-aware transport.
803+
//
804+
// It returns the tarball bytes and their computed "sha256:<hex>" digest so the
805+
// caller can pin/verify integrity (TS records and checks this digest).
806+
func downloadFeatureTarball(ctx context.Context, logger log.Logger, tarballURL string, env map[string]string) ([]byte, string, error) {
807+
if ctx == nil {
808+
ctx = context.Background()
809+
}
810+
req, err := http.NewRequestWithContext(ctx, http.MethodGet, tarballURL, nil)
811+
if err != nil {
812+
return nil, "", err
813+
}
814+
for k, v := range featureRequestHeaders(tarballURL, env, logger) {
815+
req.Header.Set(k, v)
816+
}
817+
if isPlainHTTPURL(tarballURL) && logger != nil {
818+
logger.Write("Sending as plain HTTP request", log.LevelWarning)
819+
}
820+
784821
client := &http.Client{Transport: httpx.NewTransport(), Timeout: 60 * time.Second}
785-
resp, err := client.Get(url)
822+
resp, err := client.Do(req)
786823
if err != nil {
787-
return nil, err
824+
return nil, "", err
788825
}
789826
defer resp.Body.Close()
790-
if resp.StatusCode != http.StatusOK {
791-
return nil, fmt.Errorf("download %s: HTTP %d", url, resp.StatusCode)
827+
if resp.StatusCode < 200 || resp.StatusCode > 299 {
828+
return nil, "", fmt.Errorf("download %s: HTTP %d", tarballURL, resp.StatusCode)
829+
}
830+
data, err := io.ReadAll(io.LimitReader(resp.Body, 512<<20)) // 512 MiB cap
831+
if err != nil {
832+
return nil, "", err
833+
}
834+
sum := sha256.Sum256(data)
835+
return data, "sha256:" + hex.EncodeToString(sum[:]), nil
836+
}
837+
838+
// featureRequestHeaders mirrors the TS getRequestHeaders for a direct-tarball
839+
// source: always a "devcontainer" User-Agent, plus a bearer GITHUB_TOKEN when the
840+
// tarball is hosted on GitHub. Redirects to a signed CDN URL drop the header —
841+
// Go's http.Client strips Authorization on a cross-host redirect, which is the
842+
// correct behavior for GitHub release assets (the redirect target is presigned).
843+
func featureRequestHeaders(tarballURL string, env map[string]string, logger log.Logger) map[string]string {
844+
headers := map[string]string{"User-Agent": "devcontainer"}
845+
if isGitHubTarballURI(tarballURL) {
846+
if token := env["GITHUB_TOKEN"]; token != "" {
847+
if logger != nil {
848+
logger.Write("Using environment GITHUB_TOKEN.", log.LevelInfo)
849+
}
850+
headers["Authorization"] = "Bearer " + token
851+
} else if logger != nil {
852+
logger.Write("No environment GITHUB_TOKEN available.", log.LevelInfo)
853+
}
854+
}
855+
return headers
856+
}
857+
858+
// isGitHubTarballURI matches the TS isGitHubUri check (raw prefix, no parsing).
859+
func isGitHubTarballURI(u string) bool {
860+
return strings.HasPrefix(u, "https://github.com") || strings.HasPrefix(u, "https://api.github.com")
861+
}
862+
863+
// isPlainHTTPURL reports whether the URL would travel unencrypted (TS warns on
864+
// this): a http:// scheme, or a localhost host regardless of scheme.
865+
func isPlainHTTPURL(u string) bool {
866+
parsed, err := url.Parse(u)
867+
if err != nil {
868+
return false
792869
}
793-
return io.ReadAll(io.LimitReader(resp.Body, 512<<20)) // 512 MiB cap
870+
return parsed.Scheme == "http" || parsed.Hostname() == "localhost"
794871
}
795872

796873
// addFeatureOption returns the feature options with key=true added, preserving
Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
package cli
2+
3+
import (
4+
"context"
5+
"crypto/sha256"
6+
"encoding/hex"
7+
"net/http"
8+
"net/http/httptest"
9+
"strings"
10+
"testing"
11+
)
12+
13+
func TestIsGitHubTarballURI(t *testing.T) {
14+
cases := map[string]bool{
15+
"https://github.com/owner/repo/releases/download/v1/feat.tgz": true,
16+
"https://api.github.com/repos/owner/repo/releases/assets/1": true,
17+
"https://raw.githubusercontent.com/owner/repo/main/feat.tgz": false, // not github.com host
18+
"https://example.com/feat.tgz": false,
19+
"http://github.com/owner/repo/feat.tgz": false, // http, not https
20+
}
21+
for u, want := range cases {
22+
if got := isGitHubTarballURI(u); got != want {
23+
t.Errorf("isGitHubTarballURI(%q) = %v, want %v", u, got, want)
24+
}
25+
}
26+
}
27+
28+
func TestIsPlainHTTPURL(t *testing.T) {
29+
cases := map[string]bool{
30+
"http://example.com/feat.tgz": true,
31+
"https://example.com/feat.tgz": false,
32+
"https://localhost:8080/x.tgz": true, // localhost even over https
33+
"http://localhost/x.tgz": true,
34+
"https://127.0.0.1:5000/x.tgz": false, // only the literal "localhost" host is special
35+
}
36+
for u, want := range cases {
37+
if got := isPlainHTTPURL(u); got != want {
38+
t.Errorf("isPlainHTTPURL(%q) = %v, want %v", u, got, want)
39+
}
40+
}
41+
}
42+
43+
func TestFeatureRequestHeaders(t *testing.T) {
44+
// Always a devcontainer User-Agent.
45+
h := featureRequestHeaders("https://example.com/x.tgz", nil, nil)
46+
if h["User-Agent"] != "devcontainer" {
47+
t.Errorf("User-Agent = %q, want devcontainer", h["User-Agent"])
48+
}
49+
if _, ok := h["Authorization"]; ok {
50+
t.Error("non-GitHub URL must not get an Authorization header")
51+
}
52+
53+
// GitHub URL + token → bearer auth.
54+
h = featureRequestHeaders("https://github.com/o/r/releases/download/v1/f.tgz",
55+
map[string]string{"GITHUB_TOKEN": "secret"}, nil)
56+
if h["Authorization"] != "Bearer secret" {
57+
t.Errorf("Authorization = %q, want 'Bearer secret'", h["Authorization"])
58+
}
59+
60+
// GitHub URL without a token → no auth header (not an empty bearer).
61+
h = featureRequestHeaders("https://github.com/o/r/f.tgz", map[string]string{}, nil)
62+
if _, ok := h["Authorization"]; ok {
63+
t.Error("GitHub URL without GITHUB_TOKEN must not set Authorization")
64+
}
65+
66+
// Non-GitHub URL never leaks the token.
67+
h = featureRequestHeaders("https://example.com/f.tgz",
68+
map[string]string{"GITHUB_TOKEN": "secret"}, nil)
69+
if _, ok := h["Authorization"]; ok {
70+
t.Error("non-GitHub URL must not receive the GITHUB_TOKEN")
71+
}
72+
}
73+
74+
// TestDownloadFeatureTarball_HeadersAndDigest exercises the real HTTP path
75+
// against a local server: it must send the devcontainer UA + bearer token and
76+
// return the correct sha256 digest.
77+
func TestDownloadFeatureTarball_HeadersAndDigest(t *testing.T) {
78+
payload := []byte("fake-tarball-bytes")
79+
var gotUA, gotAuth string
80+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
81+
gotUA = r.Header.Get("User-Agent")
82+
gotAuth = r.Header.Get("Authorization")
83+
w.WriteHeader(http.StatusOK)
84+
w.Write(payload)
85+
}))
86+
defer srv.Close()
87+
88+
// The test server is not a github.com host, so no auth is expected here; use
89+
// the header helper test above for the GitHub-auth branch. Verify UA + digest.
90+
data, digest, err := downloadFeatureTarball(context.Background(), nil, srv.URL, map[string]string{"GITHUB_TOKEN": "secret"})
91+
if err != nil {
92+
t.Fatal(err)
93+
}
94+
if string(data) != string(payload) {
95+
t.Errorf("body = %q, want %q", data, payload)
96+
}
97+
if gotUA != "devcontainer" {
98+
t.Errorf("server saw User-Agent %q, want devcontainer", gotUA)
99+
}
100+
if gotAuth != "" {
101+
t.Errorf("non-GitHub host must not receive Authorization, got %q", gotAuth)
102+
}
103+
sum := sha256.Sum256(payload)
104+
want := "sha256:" + hex.EncodeToString(sum[:])
105+
if digest != want {
106+
t.Errorf("digest = %q, want %q", digest, want)
107+
}
108+
}
109+
110+
// TestDownloadFeatureTarball_Non2xx verifies non-2xx responses surface an error.
111+
func TestDownloadFeatureTarball_Non2xx(t *testing.T) {
112+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
113+
w.WriteHeader(http.StatusNotFound)
114+
}))
115+
defer srv.Close()
116+
117+
_, _, err := downloadFeatureTarball(context.Background(), nil, srv.URL, nil)
118+
if err == nil || !strings.Contains(err.Error(), "404") {
119+
t.Errorf("want an HTTP 404 error, got %v", err)
120+
}
121+
}
122+
123+
// TestDownloadFeatureTarball_2xxAccepted verifies a 2xx that is not 200 (e.g. 206)
124+
// is accepted, matching the TS 200-299 range.
125+
func TestDownloadFeatureTarball_2xxAccepted(t *testing.T) {
126+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
127+
w.WriteHeader(http.StatusNoContent) // 204, still 2xx
128+
}))
129+
defer srv.Close()
130+
131+
if _, _, err := downloadFeatureTarball(context.Background(), nil, srv.URL, nil); err != nil {
132+
t.Errorf("204 should be accepted as success, got %v", err)
133+
}
134+
}

0 commit comments

Comments
 (0)