Skip to content

Commit 2353da4

Browse files
aledbfclaude
andcommitted
fix(features)!: stop treating http:// Feature ids as tarball URLs
The reference's getFeatureIdType takes only https:// ids as a direct tarball. An http:// id falls through to the OCI classification, where getRef rejects it without any request; the GitHub Releases fallback then refuses an id that is not owner/repo/feature, logs "Could not resolve Feature '<id>'", and the dependency graph fails with "ERR: Feature '<id>' could not be processed. You may not have permission ...". Go downloaded the tarball over plain HTTP. Any reference the OCI parser rejects and that fallback refuses now leaves the Feature unprocessed the same way, after the reference's warning and error lines. A reference the fallback would fetch from GitHub Releases keeps failing with the parse error, since this CLI does not implement that fallback. BREAKING CHANGE: an http:// Feature id no longer downloads a tarball; build, up, read-configuration and upgrade exit 1 with "ERR: Feature '<id>' could not be processed. ...". Serve the tarball over https:// (NODE_EXTRA_CA_CERTS or SSL_CERT_FILE can name a private CA) or publish the Feature to an OCI registry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 0dda553 commit 2353da4

13 files changed

Lines changed: 141 additions & 15 deletions

‎docs/migration.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,13 @@ runs the official CLI is unaffected:
208208
- Only a registry whose host is `localhost` is reached over plain HTTP. Earlier
209209
releases also used plain HTTP for `127.0.0.1`, which is now addressed over HTTPS
210210
like any other host. Refer to a local plain-HTTP registry as `localhost:<port>`.
211+
- Only an `https://` Feature id is a tarball URL. An `http://` id is now handled
212+
as the official CLI handles it: as a registry reference it cannot parse, so
213+
`build`, `up`, `read-configuration` and `upgrade` exit 1 with `ERR: Feature
214+
'<id>' could not be processed. ...` without downloading anything. Earlier
215+
releases downloaded the tarball over plain HTTP. Serve the tarball over HTTPS
216+
(with `NODE_EXTRA_CA_CERTS` or `SSL_CERT_FILE` naming a private CA if needed),
217+
or publish the Feature to an OCI registry.
211218
- Features are resolved in the order the configuration declares them, as the
212219
official CLI resolves them: integer-like ids such as `"9"` first, ascending,
213220
then the others in declaration order, followed by the `--additional-features`

‎docs/parity/parity-matrix.yaml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1027,6 +1027,19 @@ initial_cases:
10271027
current_status: match
10281028
notes: "userFeaturesToArray appends the --additional-features ids after the configuration's, so '../zeta' is processed and refused first. Go used to sort the merged ids and reported '../alpha'."
10291029

1030+
- id: build.features-http-id-not-a-tarball-error
1031+
lane: runtime
1032+
command: build
1033+
priority: p1
1034+
docker_required: true
1035+
setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"http://127.0.0.1:9/devcontainer-feature-a.tgz\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE"
1036+
cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}"
1037+
ts_cmd: "build --workspace-folder ${WORKSPACE}"
1038+
asserts: [exit_code, stdout_normalized]
1039+
class: features
1040+
current_status: match
1041+
notes: "getFeatureIdType takes only https:// as a tarball URL; an http:// id goes to the OCI path, where getRef rejects it, then to the GitHub Releases fallback, which refuses an id that is not owner/repo/feature. buildDependencyGraph throws and build reports ERR: Feature 'http://...' could not be processed. You may not have permission ... without any request. Go used to download it as a tarball and reported the connection error."
1042+
10301043
- id: up.update-uid-remote-user-from-image-metadata-success
10311044
lane: runtime
10321045
command: up

‎internal/cli/compose_up_held_test.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99
"errors"
1010
"io"
1111
"net/http"
12-
"net/http/httptest"
1312
"os"
1413
"path/filepath"
1514
"regexp"
@@ -685,9 +684,7 @@ func TestComposeFeatureSets(t *testing.T) {
685684
})
686685
t.Run("lockfile pins a tarball", func(t *testing.T) {
687686
tgz := featureTarball(t, "tarred")
688-
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Write(tgz) }))
689-
defer srv.Close()
690-
uri := srv.URL + "/devcontainer-feature-tarred.tgz"
687+
uri := serveTrustedTLS(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Write(tgz) })) + "/devcontainer-feature-tarred.tgz"
691688
for _, noLockfile := range []bool{false, true} {
692689
h := newComposeHarness(t)
693690
h.r.opts.noLockfile = noLockfile

‎internal/cli/feature_deps.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ func newMetadataProcessFeature(client oci.Registry, logger log.Logger, local loc
7373
default:
7474
ref, err := oci.ParseRef(resolvedID)
7575
if err != nil {
76-
return nil, fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, err)
76+
return nil, unparseableFeatureRef(logger, id, resolvedID, err)
7777
}
7878
if lockfile != nil {
7979
if integrity := lockfile.Features[resolvedID].Integrity; integrity != "" {

‎internal/cli/feature_install.go‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ func processInstallFeature(
297297
default:
298298
ref, err := oci.ParseRef(resolvedID)
299299
if err != nil {
300-
return nil, fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, err)
300+
return nil, unparseableFeatureRef(logger, id, resolvedID, err)
301301
}
302302

303303
userRef := ref
@@ -986,6 +986,27 @@ func isPlainHTTPURL(u string) bool {
986986
return parsed.Scheme == "http" || parsed.Hostname() == "localhost"
987987
}
988988

989+
var githubReleaseFeatureName = regexp.MustCompile(`^[a-zA-Z0-9_-]*$`)
990+
991+
// unparseableFeatureRef returns nil, after logging why, for a reference that
992+
// TS's GitHub Releases fallback refuses, which leaves the Feature
993+
// unprocessed; this CLI lacks that fallback, so it fails on any other.
994+
func unparseableFeatureRef(logger log.Logger, id, resolvedID string, parseErr error) error {
995+
at := strings.Split(resolvedID, "@")
996+
var refusal string
997+
switch parts := strings.Split(at[0], "/"); {
998+
case len(at) > 2:
999+
refusal = "Parse error. Use the '@' symbol only to designate a version tag."
1000+
case len(parts) != 3 || slices.Contains(parts, "") || !githubReleaseFeatureName.MatchString(parts[2]):
1001+
refusal = fmt.Sprintf("Could not resolve Feature '%s'. Ensure the Feature is published and accessible from your current environment.", resolvedID)
1002+
default:
1003+
return fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, parseErr)
1004+
}
1005+
logger.Write(fmt.Sprintf("Could not resolve Feature manifest for '%s'. If necessary, provide registry credentials with 'docker login <registry>'.", resolvedID), log.LevelWarning)
1006+
logger.Write(refusal, log.LevelError)
1007+
return nil
1008+
}
1009+
9891010
func resolveFeatureID(logger log.Logger, id string) (string, features.SourceType, error) {
9901011
resolvedID, warning := features.BackwardCompatibleID(id)
9911012
if warning != "" {

‎internal/cli/feature_install_held_test.go‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"crypto/sha256"
66
"encoding/hex"
77
"encoding/json"
8+
"encoding/pem"
89
"errors"
910
"fmt"
1011
"io"
@@ -337,11 +338,23 @@ func TestProcessInstallFeatureOCIErrors(t *testing.T) {
337338

338339
func serveBytes(t *testing.T, body []byte, status int) string {
339340
t.Helper()
340-
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
341+
return serveTrustedTLS(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
341342
w.WriteHeader(status)
342343
w.Write(body)
343344
}))
345+
}
346+
347+
func serveTrustedTLS(t *testing.T, h http.Handler) string {
348+
t.Helper()
349+
srv := httptest.NewTLSServer(h)
344350
t.Cleanup(srv.Close)
351+
caFile := filepath.Join(t.TempDir(), "ca.pem")
352+
certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: srv.Certificate().Raw})
353+
if err := os.WriteFile(caFile, certPEM, 0o600); err != nil {
354+
t.Fatal(err)
355+
}
356+
t.Setenv("NODE_EXTRA_CA_CERTS", "")
357+
t.Setenv("SSL_CERT_FILE", caFile)
345358
return srv.URL
346359
}
347360

‎internal/cli/feature_order_test.go‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,16 @@
11
package cli
22

33
import (
4+
"bytes"
5+
"net/http"
6+
"net/http/httptest"
47
"path/filepath"
58
"strings"
9+
"sync/atomic"
610
"testing"
11+
12+
"github.com/devcontainers/cli/internal/features"
13+
"github.com/devcontainers/cli/internal/log"
714
)
815

916
func TestReadConfigurationReportsTheFirstFeatureInDeclarationOrder(t *testing.T) {
@@ -73,3 +80,63 @@ func TestFeaturesResolveDependenciesListsFeaturesInDeclarationOrder(t *testing.T
7380
prev = i
7481
}
7582
}
83+
84+
func TestPlainHTTPFeatureIDIsNotATarball(t *testing.T) {
85+
var requests atomic.Int32
86+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
87+
requests.Add(1)
88+
w.Write(featureTgzBytes(t, map[string]string{"devcontainer-feature.json": `{"id":"x"}`}))
89+
}))
90+
t.Cleanup(srv.Close)
91+
id := srv.URL + "/devcontainer-feature-x.tgz"
92+
wantLogs := []string{
93+
"Could not resolve Feature manifest for '" + id + "'. If necessary, provide registry credentials with 'docker login <registry>'.\n",
94+
"Could not resolve Feature '" + id + "'. Ensure the Feature is published and accessible from your current environment.\n",
95+
}
96+
97+
for name, process := range map[string]func(log.Logger) (*features.Set, error){
98+
"install": func(l log.Logger) (*features.Set, error) {
99+
return processInstallFeature(l, nil, &features.FNode{UserFeatureID: id}, localFeatureRoots{}, nil, stageDirs(t))
100+
},
101+
"metadata": func(l log.Logger) (*features.Set, error) {
102+
return newMetadataProcessFeature(nil, l, localFeatureRoots{}, nil)(&features.FNode{UserFeatureID: id})
103+
},
104+
} {
105+
t.Run(name, func(t *testing.T) {
106+
var logs bytes.Buffer
107+
set, err := process(log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: &logs}))
108+
if set != nil || err != nil {
109+
t.Fatalf("set, err = %+v, %v; want neither", set, err)
110+
}
111+
for _, line := range wantLogs {
112+
if !strings.Contains(logs.String(), line) {
113+
t.Errorf("logs = %q, want %q", logs.String(), line)
114+
}
115+
}
116+
})
117+
}
118+
if n := requests.Load(); n != 0 {
119+
t.Errorf("%d requests to the http:// URL, want none", n)
120+
}
121+
}
122+
123+
func TestUnparseableFeatureReferences(t *testing.T) {
124+
for id, want := range map[string]string{
125+
"owner/repo/feat@1@2": "Parse error. Use the '@' symbol only to designate a version tag.",
126+
"owner/repo/fe.at@bad": "Could not resolve Feature 'owner/repo/fe.at@bad'. Ensure the Feature is published and accessible from your current environment.",
127+
"owner/repo/sub/feat@v1": "Could not resolve Feature 'owner/repo/sub/feat@v1'. Ensure the Feature is published and accessible from your current environment.",
128+
"owner//feat@v1": "Could not resolve Feature 'owner//feat@v1'. Ensure the Feature is published and accessible from your current environment.",
129+
} {
130+
var logs bytes.Buffer
131+
set, err := newMetadataProcessFeature(nil, log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: &logs}), localFeatureRoots{}, nil)(&features.FNode{UserFeatureID: id})
132+
if set != nil || err != nil {
133+
t.Errorf("%s: set, err = %+v, %v; want neither", id, set, err)
134+
}
135+
if !strings.HasSuffix(logs.String(), want+"\n") {
136+
t.Errorf("%s: logs = %q, want them to end with %q", id, logs.String(), want)
137+
}
138+
}
139+
if _, err := newMetadataProcessFeature(nil, log.Null, localFeatureRoots{}, nil)(&features.FNode{UserFeatureID: "owner/repo/feat@v1"}); err == nil || !strings.HasPrefix(err.Error(), "ERR: Feature 'owner/repo/feat@v1' could not be processed. ") {
140+
t.Errorf("err = %v; want the reference's parse failure", err)
141+
}
142+
}

‎internal/cli/feature_tarball_test.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -214,11 +214,9 @@ func TestDownloadFeatureTarball_2xxAccepted(t *testing.T) {
214214
}
215215

216216
func TestDirectTarballDigestCheckedAgainstTheLockfile(t *testing.T) {
217-
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
217+
uri := serveTrustedTLS(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
218218
w.Write([]byte("not-a-real-tarball"))
219-
}))
220-
defer srv.Close()
221-
uri := srv.URL + "/feature.tgz"
219+
})) + "/feature.tgz"
222220
sum := sha256.Sum256([]byte("not-a-real-tarball"))
223221
digest := "sha256:" + hex.EncodeToString(sum[:])
224222

‎internal/features/resolve.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,7 @@ func ClassifyID(id string) SourceType {
2121
if !strings.ContainsAny(id, `/\`) {
2222
return SourceLegacyShorthand
2323
}
24-
// TS takes only https:// as a tarball URL.
25-
if strings.HasPrefix(id, "http://") || strings.HasPrefix(id, "https://") {
24+
if strings.HasPrefix(id, "https://") {
2625
return SourceDirectTarball
2726
}
2827
if strings.HasPrefix(id, "./") || strings.HasPrefix(id, "../") || strings.HasPrefix(id, "/") {

‎internal/features/resolve_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ func TestClassifyFeatureID(t *testing.T) {
2222
{"owner/feat", SourceOCI},
2323
{`a\b`, SourceOCI},
2424
{"https://example.com/feature.tgz", SourceDirectTarball},
25-
{"http://example.com/feature.tgz", SourceDirectTarball},
25+
{"http://example.com/feature.tgz", SourceOCI},
2626
{"./local-feature", SourceLocalPath},
2727
{"../sibling-feature", SourceLocalPath},
2828
{"/abs/feature", SourceLocalPath},

0 commit comments

Comments
 (0)