fix(oci,mounts,config): identity-token build auth, mount target subst, comma-path mount

Three more bugs from the adversarial edge-case review, each verified against the
TS reference with a test that fails on the old code:

  - oci/buildauth: a token-only credential (identitytoken / OAuth refresh token,
    e.g. ACR) has an empty base64 auth, so the `base64Encoded == ""` guard dropped
    it and the build subprocess authenticated anonymously — the base-image pull
    then failed even though the CLI's own OCI auth worked. Skip only when there is
    no usable credential at all; the identitytoken branch below is now reachable.
    (auth entry tags gain omitempty so a token-only entry omits the empty auth.)

  - cli/mounts: ${devcontainerId} was substituted in an object mount's source but
    not its target, so `"target":"/cache/${devcontainerId}"` reached the container
    literal — diverging from the string form which substitutes the whole spec.

  - config/loader: the workspace bind mount did not quote a source/target path
    containing a comma, which Docker reads as a mount-option boundary (broken
    mount). Quote the affected segment, matching the TS srcQuote/tgtQuote guard.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: aledbf

internal/cli/mounts.go

@@ -108,6 +108,10 @@ func mountFromMetadata(entry interface{}, devcontainerID string) (dockermount.Mo
 		return docker.ParseMountSpec(spec)
 	case map[string]interface{}:
 		target, _ := raw["target"].(string)
+		// ${devcontainerId} must resolve in target too, not only source — the
+		// string form substitutes the whole spec, so the object form has to match
+		// (e.g. target "/cache/${devcontainerId}").
+		target = strings.ReplaceAll(target, "${devcontainerId}", devcontainerID)
 		if target == "" {
 			return dockermount.Mount{}, fmt.Errorf("mount requires a target/destination")
 		}

internal/cli/upstream_fixes_test.go

@@ -119,3 +119,23 @@ func TestBuildPositionalPath(t *testing.T) {
 		t.Errorf("#8 build must accept a single positional arg: %v", err)
 	}
 }
+
+// TestMountFromMetadata_DevcontainerIdInTarget covers the fix: ${devcontainerId}
+// must resolve in an object mount's target, not only its source (the string form
+// already substitutes the whole spec).
+func TestMountFromMetadata_DevcontainerIdInTarget(t *testing.T) {
+	m, err := mountFromMetadata(map[string]interface{}{
+		"type":   "volume",
+		"source": "cache-${devcontainerId}",
+		"target": "/cache/${devcontainerId}",
+	}, "abc123")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if m.Source != "cache-abc123" {
+		t.Errorf("source = %q, want cache-abc123", m.Source)
+	}
+	if m.Target != "/cache/abc123" {
+		t.Errorf("target = %q, want /cache/abc123 (was left literal)", m.Target)
+	}
+}

internal/config/loader.go

@@ -324,7 +324,19 @@ func computeWorkspaceConfig(workspace *Workspace, config *DevContainer, mountWor
 		if runtime.GOOS != "linux" {
 			consistency = ",consistency=consistent"
 		}
-		wc.WorkspaceMount = fmt.Sprintf("type=bind,source=%s,target=/workspaces/%s%s", sourceFolder, filepath.Base(sourceFolder), consistency)
+		// A comma in the source/target path would otherwise be read by Docker as a
+		// mount-option boundary and break the mount — quote the affected segment,
+		// matching the TS srcQuote/tgtQuote guard.
+		containerMountFolder := "/workspaces/" + filepath.Base(sourceFolder)
+		srcQuote, tgtQuote := "", ""
+		if strings.Contains(sourceFolder, ",") {
+			srcQuote = `"`
+		}
+		if strings.Contains(containerMountFolder, ",") {
+			tgtQuote = `"`
+		}
+		wc.WorkspaceMount = fmt.Sprintf("type=bind,%ssource=%s%s,%starget=%s%s%s",
+			srcQuote, sourceFolder, srcQuote, tgtQuote, containerMountFolder, tgtQuote, consistency)
 	}
 
 	if config.WorkspaceFolder != "" {

internal/config/workspacemount_test.go

@@ -0,0 +1,26 @@
+package config
+
+import (
+	"strings"
+	"testing"
+)
+
+// TestWorkspaceMount_QuotesCommaPath covers the fix: a workspace path containing
+// a comma must be quoted in the bind spec, or Docker parses the comma as a mount-
+// option boundary and the mount breaks (matches the TS srcQuote/tgtQuote guard).
+func TestWorkspaceMount_QuotesCommaPath(t *testing.T) {
+	ws := &Workspace{RootFolderPath: "/home/user/project,v2"}
+	wc := computeWorkspaceConfig(ws, &DevContainer{}, false)
+	if !strings.Contains(wc.WorkspaceMount, `"source=/home/user/project,v2"`) {
+		t.Errorf("comma source not quoted: %q", wc.WorkspaceMount)
+	}
+	if !strings.Contains(wc.WorkspaceMount, `"target=/workspaces/project,v2"`) {
+		t.Errorf("comma target not quoted: %q", wc.WorkspaceMount)
+	}
+
+	// A normal path is NOT quoted.
+	wc2 := computeWorkspaceConfig(&Workspace{RootFolderPath: "/home/user/project"}, &DevContainer{}, false)
+	if strings.Contains(wc2.WorkspaceMount, `"`) {
+		t.Errorf("plain path should not be quoted: %q", wc2.WorkspaceMount)
+	}
+}

internal/oci/auth.go

@@ -64,8 +64,8 @@ type dockerConfigFile struct {
 }
 
 type dockerConfigAuth struct {
-	Auth          string `json:"auth"`
-	IdentityToken string `json:"identitytoken"`
+	Auth          string `json:"auth,omitempty"`
+	IdentityToken string `json:"identitytoken,omitempty"`
 }
 
 func getCredentialFromDockerConfig(env map[string]string, registry string, logger log.Logger) *credential {

internal/oci/buildauth.go

@@ -34,7 +34,12 @@ func ResolveBuildAuth(env map[string]string, registries []string, logger log.Log
 		}
 		seen[reg] = true
 		cred := getCredential(env, reg, logger)
-		if cred == nil || cred.base64Encoded == "" {
+		// Skip only when there is no usable credential at all. A token-only
+		// credential (identitytoken / OAuth refresh token, e.g. ACR) has an empty
+		// base64Encoded but a non-empty refreshToken — the old `base64Encoded == ""`
+		// guard dropped it, so the build subprocess authenticated anonymously and
+		// the base-image pull failed even though the CLI's own auth worked.
+		if cred == nil || (cred.base64Encoded == "" && cred.refreshToken == "") {
 			continue
 		}
 		entry := dockerConfigAuth{Auth: cred.base64Encoded}

internal/oci/buildauth_test.go

@@ -89,3 +89,32 @@ func TestResolveBuildAuthDedupesRegistries(t *testing.T) {
 		t.Fatalf("want 1 deduped auth, got %d", len(auths))
 	}
 }
+
+// TestResolveBuildAuth_IdentityToken covers the fix for token-only credentials
+// (ACR / OAuth refresh token): they carry an identitytoken but no base64 auth, and
+// were dropped by the old guard, leaving the build subprocess unauthenticated.
+func TestResolveBuildAuth_IdentityToken(t *testing.T) {
+	cfgDir := t.TempDir()
+	cfg := `{"auths":{"myacr.azurecr.io":{"identitytoken":"eyJTOKEN"}}}`
+	if err := os.WriteFile(filepath.Join(cfgDir, "config.json"), []byte(cfg), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	env := map[string]string{"DOCKER_CONFIG": cfgDir}
+	dir, cleanup, ok, err := ResolveBuildAuth(env, []string{"myacr.azurecr.io"}, log.Null)
+	if err != nil || !ok {
+		t.Fatalf("ok=%v err=%v — a token-only credential must still produce an auth entry", ok, err)
+	}
+	defer cleanup()
+
+	entry, present := readConfigAuths(t, dir)["myacr.azurecr.io"]
+	if !present {
+		t.Fatal("identity-token registry missing from the build auth config")
+	}
+	if entry.IdentityToken != "eyJTOKEN" {
+		t.Errorf("IdentityToken = %q, want eyJTOKEN", entry.IdentityToken)
+	}
+	if entry.Auth != "" {
+		t.Errorf("Auth should be empty for a token-only credential, got %q", entry.Auth)
+	}
+}