fix(features): resolve Features in declaration order; stop treating http:// ids as tarballs

docs/migration.md

@@ -208,6 +208,20 @@ runs the official CLI is unaffected:
 - Only a registry whose host is `localhost` is reached over plain HTTP. Earlier
   releases also used plain HTTP for `127.0.0.1`, which is now addressed over HTTPS
   like any other host. Refer to a local plain-HTTP registry as `localhost:<port>`.
+- Only an `https://` Feature id is a tarball URL. An `http://` id is now handled
+  as the official CLI handles it: as a registry reference it cannot parse, so
+  `build`, `up`, `read-configuration` and `upgrade` exit 1 with `ERR: Feature
+  '<id>' could not be processed. ...` without downloading anything. Earlier
+  releases downloaded the tarball over plain HTTP. Serve the tarball over HTTPS
+  (with `NODE_EXTRA_CA_CERTS` or `SSL_CERT_FILE` naming a private CA if needed),
+  or publish the Feature to an OCI registry.
+- Features are resolved in the order the configuration declares them, as the
+  official CLI resolves them: integer-like ids such as `"9"` first, ascending,
+  then the others in declaration order, followed by the `--additional-features`
+  ids the configuration does not name. When several Features fail, the one
+  reported is the first in that order; earlier releases reported the first by
+  sorted id. `build` no longer reports a legacy Feature id before trying the
+  Features declared ahead of it, or before pulling the image.
 
 ## Rollback
 

docs/parity/parity-matrix.yaml

@@ -975,6 +975,71 @@ initial_cases:
     current_status: match
     notes: "With the configuration at the workspace root (.devcontainer.json), './feat' resolves outside the .devcontainer folder, which processFeatureIdentifier refuses; build reports the plain Error buildDependencyGraph throws as its message: {\"outcome\":\"error\",\"message\":\"ERR: Feature './feat' could not be processed.  You may not have permission ...\",\"description\":\"An error occurred building the container.\"}, exit 1. Go used to build the Feature."
 
+  - id: features.resolve-dependencies-declaration-order-success
+    lane: contract
+    command: features
+    priority: p1
+    docker_required: false
+    setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer/zeta $WORKSPACE/.devcontainer/alpha $WORKSPACE/.devcontainer/mid $WORKSPACE/.devcontainer/beta $WORKSPACE/.devcontainer/omega; printf '%s' '{\"id\":\"zeta\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/zeta/devcontainer-feature.json; printf '%s' '{\"id\":\"alpha\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/alpha/devcontainer-feature.json; printf '%s' '{\"id\":\"mid\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/mid/devcontainer-feature.json; printf '%s' '{\"id\":\"beta\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/beta/devcontainer-feature.json; printf '%s' '{\"id\":\"omega\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/omega/devcontainer-feature.json; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./zeta\":{},\"./alpha\":{},\"./mid\":{},\"./beta\":{},\"./omega\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE"
+    cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}"
+    ts_cmd: "features resolve-dependencies --workspace-folder ${WORKSPACE}"
+    asserts: [exit_code, stdout_text]
+    class: features
+    current_status: match
+    notes: "userFeaturesToArray (containerFeaturesConfiguration.ts) lists the configuration's Features in Object.keys order, and the mermaid graph lists the dependency graph's worklist in that order: ./zeta, ./alpha, ./mid, ./beta, ./omega. Go used to take them in Go map order."
+
+  - id: build.features-first-failure-in-declaration-order-error
+    lane: runtime
+    command: build
+    priority: p1
+    docker_required: true
+    setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer $WORKSPACE/zeta; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"../zeta\":{},\"zz\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE"
+    cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}"
+    ts_cmd: "build --workspace-folder ${WORKSPACE}"
+    asserts: [exit_code, stdout_normalized]
+    class: features
+    current_status: match
+    notes: "Features are processed in Object.keys order and the first that fails is reported: '../zeta' is refused (outside .devcontainer) before the legacy id 'zz' is classified, so build reports ERR: Feature '../zeta' could not be processed. Go used to check every id for a legacy one before resolving any Feature and reported 'Legacy feature 'zz' not supported.'."
+
+  - id: build.features-integer-like-ids-first-error
+    lane: runtime
+    command: build
+    priority: p1
+    docker_required: true
+    setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"../zeta\":{},\"10\":{},\"9\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE"
+    cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}"
+    ts_cmd: "build --workspace-folder ${WORKSPACE}"
+    asserts: [exit_code, stdout_normalized]
+    class: features
+    current_status: match
+    notes: "Object.keys lists integer-like keys first, ascending, so '9' is processed first and build reports 'Legacy feature '9' not supported.'. Go used to sort the ids and reported '10'."
+
+  - id: build.additional-features-after-config-error
+    lane: runtime
+    command: build
+    priority: p1
+    docker_required: true
+    setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer $WORKSPACE/zeta $WORKSPACE/alpha; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"../zeta\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE"
+    cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}"
+    ts_cmd: "build --workspace-folder ${WORKSPACE} --additional-features '{\"../alpha\":{}}'"
+    asserts: [exit_code, stdout_normalized]
+    class: features
+    current_status: match
+    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'."
+
+  - id: build.features-http-id-not-a-tarball-error
+    lane: runtime
+    command: build
+    priority: p1
+    docker_required: true
+    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"
+    cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}"
+    ts_cmd: "build --workspace-folder ${WORKSPACE}"
+    asserts: [exit_code, stdout_normalized]
+    class: features
+    current_status: match
+    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."
+
   - id: up.update-uid-remote-user-from-image-metadata-success
     lane: runtime
     command: up

internal/cli/build.go

@@ -12,7 +12,6 @@ import (
 	"github.com/devcontainers/cli/internal/config"
 	"github.com/devcontainers/cli/internal/docker"
 	coreerrors "github.com/devcontainers/cli/internal/errors"
-	"github.com/devcontainers/cli/internal/features"
 	"github.com/devcontainers/cli/internal/imagemeta"
 	"github.com/devcontainers/cli/internal/log"
 	"github.com/devcontainers/cli/internal/stringify"
@@ -329,10 +328,6 @@ func (r *buildRunner) buildImage(ctx context.Context, cfg *config.DevContainer,
 		return nil, fmt.Errorf("no image specified in devcontainer.json")
 	}
 
-	if err := checkLegacyFeatures(cfg.Features); err != nil {
-		return nil, err
-	}
-
 	logger.Write(fmt.Sprintf("Using image: %s", cfg.Image), log.LevelInfo)
 
 	folderImage := folderImageName(resolvePath(opts.workspaceFolder))
@@ -456,17 +451,6 @@ func imageBuildFor(cfg *config.DevContainer, opts *buildOpts) imageBuild {
 	return imageAsIs
 }
 
-func checkLegacyFeatures(feats map[string]any) error {
-	userFeatures, _ := features.UpdateDeprecatedFeaturesIntoOptions(sortedUserFeatures(feats, nil))
-	for _, f := range userFeatures {
-		id, _ := features.BackwardCompatibleID(f.UserFeatureID)
-		if features.ClassifyID(id) == features.SourceLegacyShorthand {
-			return &coreerrors.ContainerError{Description: fmt.Sprintf(msgLegacyFeature, id)}
-		}
-	}
-	return nil
-}
-
 func imageNamesOr(names []string, fallback string) []string {
 	if len(names) == 0 {
 		return []string{fallback}
@@ -693,6 +677,7 @@ func (r *buildRunner) featureBuildOptions(cfg *config.DevContainer, hostSub *con
 		Output:                      opts.output,
 		ContainerEnv:                cfg.ContainerEnv,
 		ContainerEnvOrder:           cfg.ContainerEnvOrder,
+		FeatureOrder:                cfg.FeatureOrder,
 		FeatureOptionOrder:          cfg.FeatureOptionOrder,
 		FrozenLockfile:              opts.experimentalFrozenLockfile,
 		NoLockfile:                  opts.noLockfile,

internal/cli/build_held_test.go

@@ -106,14 +106,14 @@ func TestImageBuildFor(t *testing.T) {
 	}
 }
 
-func TestCheckLegacyFeatures(t *testing.T) {
-	for _, id := range []string{"node", "golang", "gradle", "ghcr.io/devcontainers/features/node:1", "./local-feature", "owner/repo/feature"} {
-		if err := checkLegacyFeatures(map[string]any{id: true}); err != nil {
+func TestResolveFeatureIDRefusesLegacyIDs(t *testing.T) {
+	for _, id := range []string{"node", "golang", "ghcr.io/devcontainers/features/node:1", "./local-feature", "owner/repo/feature"} {
+		if _, _, err := resolveFeatureID(log.Null, id); err != nil {
 			t.Errorf("%s: unexpected error %v", id, err)
 		}
 	}
 	for _, id := range []string{"not-a-feature", "not-a-feature:1", "node:1", "node:18", "gradle:1"} {
-		err := checkLegacyFeatures(map[string]any{id: true})
+		_, _, err := resolveFeatureID(log.Null, id)
 		if err == nil || !strings.Contains(err.Error(), "Legacy feature '"+id+"' not supported") {
 			t.Errorf("%s: error = %v, want the legacy feature error naming it", id, err)
 		}

internal/cli/compose_up.go

@@ -274,7 +274,7 @@ func (r *upRunner) composeFeatureSets(ctx context.Context, cfg *config.DevContai
 			return nil, "", err
 		}
 	}
-	res, err := fetchFeatureSetsWithOrder(ctx, r.log, cfg.Features, cfg.FeatureOptionOrder, localFeatureRoots{filepath.Dir(cfg.ConfigFilePath), opts.workspaceFolder}, lockfile, cfg.OverrideFeatureInstallOrder)
+	res, err := fetchFeatureSetsWithOrder(ctx, r.log, cfg.Features, cfg.FeatureOrder, cfg.FeatureOptionOrder, localFeatureRoots{filepath.Dir(cfg.ConfigFilePath), opts.workspaceFolder}, lockfile, cfg.OverrideFeatureInstallOrder)
 	if err != nil {
 		return nil, "", err
 	}

internal/cli/compose_up_held_test.go

@@ -9,7 +9,6 @@ import (
 	"errors"
 	"io"
 	"net/http"
-	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -685,9 +684,7 @@ func TestComposeFeatureSets(t *testing.T) {
 	})
 	t.Run("lockfile pins a tarball", func(t *testing.T) {
 		tgz := featureTarball(t, "tarred")
-		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Write(tgz) }))
-		defer srv.Close()
-		uri := srv.URL + "/devcontainer-feature-tarred.tgz"
+		uri := serveTrustedTLS(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Write(tgz) })) + "/devcontainer-feature-tarred.tgz"
 		for _, noLockfile := range []bool{false, true} {
 			h := newComposeHarness(t)
 			h.r.opts.noLockfile = noLockfile

internal/cli/feature_deps.go

@@ -73,7 +73,7 @@ func newMetadataProcessFeature(client oci.Registry, logger log.Logger, local loc
 		default:
 			ref, err := oci.ParseRef(resolvedID)
 			if err != nil {
-				return nil, fmt.Errorf("ERR: Feature '%s' could not be processed.  %w", id, err)
+				return nil, unparseableFeatureRef(logger, id, resolvedID, err)
 			}
 			if lockfile != nil {
 				if integrity := lockfile.Features[resolvedID].Integrity; integrity != "" {

internal/cli/feature_install.go

@@ -18,7 +18,6 @@ import (
 	"path/filepath"
 	"regexp"
 	"slices"
-	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -48,6 +47,7 @@ type FeatureBuildOptions struct {
 	CABundle                    string
 	ContainerEnv                map[string]string
 	ContainerEnvOrder           []string
+	FeatureOrder                []string
 	FeatureOptionOrder          map[string][]string
 	SkipPersistCustoms          bool
 	FeaturesBasePath            string
@@ -67,16 +67,8 @@ type fetchFeatureResult struct {
 	TmpDir      string
 }
 
-func fetchFeatureSets(ctx context.Context, logger log.Logger, featuresCfg map[string]any, optionOrder map[string][]string, local localFeatureRoots) (*fetchFeatureResult, error) {
-	return fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, optionOrder, local, nil, nil)
-}
-
-func sortedUserFeatures(featuresCfg map[string]any, optionOrder map[string][]string) []features.DevContainerFeature {
-	userFeatures := features.UserFeaturesToArray(featuresCfg, optionOrder)
-	sort.Slice(userFeatures, func(i, j int) bool {
-		return userFeatures[i].UserFeatureID < userFeatures[j].UserFeatureID
-	})
-	return userFeatures
+func fetchFeatureSets(ctx context.Context, logger log.Logger, featuresCfg map[string]any, order []string, optionOrder map[string][]string, local localFeatureRoots) (*fetchFeatureResult, error) {
+	return fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, order, optionOrder, local, nil, nil)
 }
 
 type localFeatureRoots struct {
@@ -113,13 +105,12 @@ func createFeaturesFolder() (string, error) {
 	}
 }
 
-func fetchFeatureSetsWithOrder(ctx context.Context, logger log.Logger, featuresCfg map[string]any, optionOrder map[string][]string, local localFeatureRoots, lockfile *features.Lockfile, overrideOrder []string) (*fetchFeatureResult, error) {
+func fetchFeatureSetsWithOrder(ctx context.Context, logger log.Logger, featuresCfg map[string]any, order []string, optionOrder map[string][]string, local localFeatureRoots, lockfile *features.Lockfile, overrideOrder []string) (*fetchFeatureResult, error) {
 	if len(featuresCfg) == 0 {
 		return nil, nil
 	}
 
-	userFeatures := sortedUserFeatures(featuresCfg, optionOrder)
-	userFeatures, warnings := features.UpdateDeprecatedFeaturesIntoOptions(userFeatures)
+	userFeatures, warnings := features.UpdateDeprecatedFeaturesIntoOptions(features.UserFeaturesToArray(featuresCfg, order, optionOrder))
 	for _, w := range warnings {
 		logger.Write(w, log.LevelWarning)
 	}
@@ -306,7 +297,7 @@ func processInstallFeature(
 	default:
 		ref, err := oci.ParseRef(resolvedID)
 		if err != nil {
-			return nil, fmt.Errorf("ERR: Feature '%s' could not be processed.  %w", id, err)
+			return nil, unparseableFeatureRef(logger, id, resolvedID, err)
 		}
 
 		userRef := ref
@@ -588,7 +579,7 @@ func fetchBuildFeatures(ctx context.Context, logger log.Logger, featuresCfg map[
 			return nil, err
 		}
 	}
-	result, err := fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, fbOpts.FeatureOptionOrder, localFeatureRoots{fbOpts.FeaturesBasePath, fbOpts.WorkspaceFolder}, lockfile, fbOpts.OverrideFeatureInstallOrder)
+	result, err := fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, fbOpts.FeatureOrder, fbOpts.FeatureOptionOrder, localFeatureRoots{fbOpts.FeaturesBasePath, fbOpts.WorkspaceFolder}, lockfile, fbOpts.OverrideFeatureInstallOrder)
 	if err != nil {
 		return nil, err
 	}
@@ -995,6 +986,27 @@ func isPlainHTTPURL(u string) bool {
 	return parsed.Scheme == "http" || parsed.Hostname() == "localhost"
 }
 
+var githubReleaseFeatureName = regexp.MustCompile(`^[a-zA-Z0-9_-]*$`)
+
+// unparseableFeatureRef returns nil, after logging why, for a reference that
+// TS's GitHub Releases fallback refuses, which leaves the Feature
+// unprocessed; this CLI lacks that fallback, so it fails on any other.
+func unparseableFeatureRef(logger log.Logger, id, resolvedID string, parseErr error) error {
+	at := strings.Split(resolvedID, "@")
+	var refusal string
+	switch parts := strings.Split(at[0], "/"); {
+	case len(at) > 2:
+		refusal = "Parse error. Use the '@' symbol only to designate a version tag."
+	case len(parts) != 3 || slices.Contains(parts, "") || !githubReleaseFeatureName.MatchString(parts[2]):
+		refusal = fmt.Sprintf("Could not resolve Feature '%s'.  Ensure the Feature is published and accessible from your current environment.", resolvedID)
+	default:
+		return fmt.Errorf("ERR: Feature '%s' could not be processed.  %w", id, parseErr)
+	}
+	logger.Write(fmt.Sprintf("Could not resolve Feature manifest for '%s'.  If necessary, provide registry credentials with 'docker login <registry>'.", resolvedID), log.LevelWarning)
+	logger.Write(refusal, log.LevelError)
+	return nil
+}
+
 func resolveFeatureID(logger log.Logger, id string) (string, features.SourceType, error) {
 	resolvedID, warning := features.BackwardCompatibleID(id)
 	if warning != "" {

internal/cli/feature_install_held_test.go

@@ -5,6 +5,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
+	"encoding/pem"
 	"errors"
 	"fmt"
 	"io"
@@ -337,11 +338,23 @@ func TestProcessInstallFeatureOCIErrors(t *testing.T) {
 
 func serveBytes(t *testing.T, body []byte, status int) string {
 	t.Helper()
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	return serveTrustedTLS(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(status)
 		w.Write(body)
 	}))
+}
+
+func serveTrustedTLS(t *testing.T, h http.Handler) string {
+	t.Helper()
+	srv := httptest.NewTLSServer(h)
 	t.Cleanup(srv.Close)
+	caFile := filepath.Join(t.TempDir(), "ca.pem")
+	certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: srv.Certificate().Raw})
+	if err := os.WriteFile(caFile, certPEM, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("NODE_EXTRA_CA_CERTS", "")
+	t.Setenv("SSL_CERT_FILE", caFile)
 	return srv.URL
 }
 
@@ -429,7 +442,7 @@ func TestTarballFeatureIDUsesTheURLPath(t *testing.T) {
 
 func TestFetchFeatureSetsReportsTheFirstSortedFailure(t *testing.T) {
 	for range 5 {
-		_, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./b-missing": true, "./a-missing": true, "./c-missing": true}, nil, devcontainerRoots(t))
+		_, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./b-missing": true, "./a-missing": true, "./c-missing": true}, nil, nil, devcontainerRoots(t))
 		if err == nil || !strings.Contains(err.Error(), "'./a-missing'") {
 			t.Fatalf("err = %v, want the failure of ./a-missing", err)
 		}