From bf3e795c2fa0ddbbb0e0cfc3484bdba20e249278 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 12 Nov 2026 21:23:14 -0300 Subject: [PATCH 1/2] fix(cli): fail when --secrets-file can't be read or parsed A missing or unparsable --secrets-file was ignored: up, run-user-commands and build ran with no secrets injected or masked and could exit 0. As the reference CLI's readSecretsFromFile, the file is now read once, before anything else, and a failure ends the command with {"outcome":"error","message":,"description":"Failed to read/parse secrets"} and exit 1. The file is parsed as jsonc.parse does: comments and any JSON value are accepted, and trailing commas, a BOM and trailing data are not. Values become env vars as JS String() makes them, in JS key order. build, whose --secrets-file the reference lacks, fails the same way. Eight hermetic parity cases compare the error and acceptance paths with the reference for up and run-user-commands. Co-Authored-By: Claude Opus 5.5 --- docs/go-only-features.md | 6 + docs/parity/parity-matrix.yaml | 100 ++++++++++ internal/cli/build.go | 16 +- internal/cli/run_user_commands.go | 14 +- internal/cli/secrets.go | 259 ++++++++++++++++++++++--- internal/cli/secrets_cmd_test.go | 94 +++++++++ internal/cli/secrets_held_test.go | 110 ++++++++++- internal/cli/secrets_repo_held_test.go | 22 +-- internal/cli/up.go | 29 ++- internal/cli/up_held_test.go | 28 --- 10 files changed, 575 insertions(+), 103 deletions(-) create mode 100644 internal/cli/secrets_cmd_test.go diff --git a/docs/go-only-features.md b/docs/go-only-features.md index 17af27a..9b0c21f 100644 --- a/docs/go-only-features.md +++ b/docs/go-only-features.md @@ -202,6 +202,12 @@ with a warning. `RUN --mount=type=secret` the same way, and the lifecycle commands as environment variables. Neither leaves the value in a layer, the image config or a container label. +The file is read as the reference CLI reads it for `up` and `run-user-commands`: JSON with +comments, and a file that cannot be read or parsed (a missing file, a syntax error, a +trailing comma) fails the command with exit code 1 and +`{"outcome":"error","message":…,"description":"Failed to read/parse secrets"}` before +anything is built. `build` fails the same way. + ## `--ca-bundle` Trust extra certificate authorities, such as a TLS-intercepting proxy's, in image builds diff --git a/docs/parity/parity-matrix.yaml b/docs/parity/parity-matrix.yaml index 06d7d83..7d896c6 100644 --- a/docs/parity/parity-matrix.yaml +++ b/docs/parity/parity-matrix.yaml @@ -2079,6 +2079,56 @@ initial_cases: class: required-args current_status: match + - id: run-user-commands.secrets-file-missing + lane: contract + command: run-user-commands + priority: p1 + docker_required: false + ts_cmd: "run-user-commands --workspace-folder path-that-does-not-exist --secrets-file path-that-does-not-exist.json" + asserts: [exit_code, stdout_normalized] + class: secrets-file + current_status: match + notes: "readSecretsFromFile throws ContainerError{description: 'Failed to read/parse secrets'} with Node's ENOENT message, before the configuration is looked up. Go used to ignore an unreadable file and run with no secrets." + + - id: run-user-commands.secrets-file-invalid-json + lane: contract + command: run-user-commands + priority: p1 + docker_required: false + setup_cmd: "D=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $D; mkdir -p $D; printf '%s' '{\"A\":\"1\",}' > $D/secrets.json; echo SECRETS_FILE=$D/secrets.json" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "run-user-commands --workspace-folder path-that-does-not-exist --secrets-file ${SECRETS_FILE}" + asserts: [exit_code, stdout_normalized] + class: secrets-file + current_status: match + notes: "jsonc.parse reports a trailing comma as a parse error, so the message is 'Invalid json data'. Go used to ignore a malformed file and run with no secrets." + + - id: run-user-commands.secrets-file-jsonc-config-not-found + lane: contract + command: run-user-commands + priority: p1 + docker_required: false + setup_cmd: "D=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $D; mkdir -p $D; printf '%s' '{ /* c */ \"A\": 1, \"B\": [1, 2], \"C\": {\"x\": true} }' > $D/secrets.json; echo SECRETS_FILE=$D/secrets.json" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "run-user-commands --workspace-folder path-that-does-not-exist --secrets-file ${SECRETS_FILE}" + asserts: [exit_code, stdout_normalized] + class: secrets-file + current_status: match + notes: "jsonc.parse accepts comments and non-string values, so the command gets past the secrets file to the configuration lookup. Go json.Unmarshal into map[string]string used to reject non-string values." + + - id: run-user-commands.secrets-file-null-value-failure + lane: contract + command: run-user-commands + priority: p1 + docker_required: false + setup_cmd: "D=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $D; mkdir -p $D; printf '%s' '{\"A\": null}' > $D/secrets.json; echo SECRETS_FILE=$D/secrets.json" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "run-user-commands --workspace-folder path-that-does-not-exist --secrets-file ${SECRETS_FILE}" + asserts: [exit_code, stdout_normalized] + class: secrets-file + current_status: match + notes: "createLog masks Object.values(secrets).filter(v => v.length), which throws a TypeError on a null value; the command's catch-all reports it." + - id: set-up.missing-container-id lane: contract command: set-up @@ -2362,6 +2412,56 @@ initial_cases: class: required-args current_status: match + - id: up.secrets-file-missing + lane: contract + command: up + priority: p1 + docker_required: false + ts_cmd: "up --workspace-folder path-that-does-not-exist --secrets-file path-that-does-not-exist.json" + asserts: [exit_code, stdout_normalized] + class: secrets-file + current_status: match + notes: "readSecretsFromFile throws ContainerError{description: 'Failed to read/parse secrets'} with Node's ENOENT message, before the configuration is looked up. Go used to ignore an unreadable file and run with no secrets." + + - id: up.secrets-file-invalid-json + lane: contract + command: up + priority: p1 + docker_required: false + setup_cmd: "D=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $D; mkdir -p $D; printf '%s' '{\"A\":\"1\",}' > $D/secrets.json; echo SECRETS_FILE=$D/secrets.json" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "up --workspace-folder path-that-does-not-exist --secrets-file ${SECRETS_FILE}" + asserts: [exit_code, stdout_normalized] + class: secrets-file + current_status: match + notes: "jsonc.parse reports a trailing comma as a parse error, so the message is 'Invalid json data'. Go used to ignore a malformed file and run with no secrets." + + - id: up.secrets-file-jsonc-config-not-found + lane: contract + command: up + priority: p1 + docker_required: false + setup_cmd: "D=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $D; mkdir -p $D; printf '%s' '{ /* c */ \"A\": 1, \"B\": [1, 2], \"C\": {\"x\": true} }' > $D/secrets.json; echo SECRETS_FILE=$D/secrets.json" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "up --workspace-folder path-that-does-not-exist --secrets-file ${SECRETS_FILE}" + asserts: [exit_code, stdout_normalized] + class: secrets-file + current_status: match + notes: "jsonc.parse accepts comments and non-string values, so the command gets past the secrets file to the configuration lookup. Go json.Unmarshal into map[string]string used to reject non-string values." + + - id: up.secrets-file-null-value-failure + lane: contract + command: up + priority: p1 + docker_required: false + setup_cmd: "D=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $D; mkdir -p $D; printf '%s' '{\"A\": null}' > $D/secrets.json; echo SECRETS_FILE=$D/secrets.json" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "up --workspace-folder path-that-does-not-exist --secrets-file ${SECRETS_FILE}" + asserts: [exit_code, stdout_normalized] + class: secrets-file + current_status: match + notes: "createLog masks Object.values(secrets).filter(v => v.length), which throws a TypeError on a null value; the command's catch-all reports it." + - id: up.override-config-missing-error lane: contract command: up diff --git a/internal/cli/build.go b/internal/cli/build.go index 9a04663..530e1c6 100644 --- a/internal/cli/build.go +++ b/internal/cli/build.go @@ -44,6 +44,7 @@ type buildOpts struct { noLockfile bool frozenLockfile bool secretsFile string + secrets fileSecrets caBundle string lockfileExcludeIDs map[string]bool } @@ -150,6 +151,10 @@ func runBuild(ctx context.Context, out Output, opts *buildOpts) error { configPath = resolvePath(opts.configPath) } + if opts.secrets, err = loadSecretsFile(opts.secretsFile); err != nil { + return writeErrorJSON(out, coreerrors.ToErrorOutput(err)) + } + logger := log.New(log.Options{ Version: cliVersion(), Level: log.ParseLevel(opts.logLevel), @@ -390,14 +395,7 @@ func (r *buildRunner) buildDockerfile(ctx context.Context, cfg *config.DevContai authEnv, authCleanup := bridgeBuildAuth(logger, baseImage, imageNames, cacheFrom, opts.cacheTo) defer authCleanup() - var buildSecrets []string - if opts.secretsFile != "" { - secrets, secErr := readSecretsFile(opts.secretsFile) - if secErr != nil { - return nil, fmt.Errorf("read secrets file: %w", secErr) - } - buildSecrets = buildxSecrets(secrets, useBuildx, logger) - } + buildSecrets := buildxSecrets(opts.secrets.env(), useBuildx, logger) caSecrets := caBundleBuildSecrets(opts.caBundle, useBuildx, logger) buildOpts := docker.BuildOptions{ @@ -829,7 +827,7 @@ func labelComposeImage(ctx context.Context, dockerClient *docker.Client, imageNa func (r *buildRunner) featureBuildOptions(cfg *config.DevContainer, hostSub *config.HostSubContext) *FeatureBuildOptions { opts := r.opts return &FeatureBuildOptions{ - Secrets: buildSecretsFromFile(opts.secretsFile), + Secrets: opts.secrets.env(), CABundle: opts.caBundle, OverrideFeatureInstallOrder: cfg.OverrideFeatureInstallOrder, NoCache: opts.noCache, diff --git a/internal/cli/run_user_commands.go b/internal/cli/run_user_commands.go index 2fa61dd..c01ef2b 100644 --- a/internal/cli/run_user_commands.go +++ b/internal/cli/run_user_commands.go @@ -35,6 +35,7 @@ type runUserCommandsOpts struct { defaultUserEnvProbe string containerSessionDataFolder string secretsFile string + secrets fileSecrets terminalColumns givenInt terminalRows givenInt } @@ -97,12 +98,21 @@ func runUserCommands(ctx context.Context, out Output, opts *runUserCommandsOpts) } defer closeLog() + var err error + if opts.secrets, err = loadSecretsFile(opts.secretsFile); err != nil { + return writeErrorJSON(out, coreerrors.ToErrorOutput(err)) + } + maskValues, err := opts.secrets.maskValues() + if err != nil { + return writeErrorJSON(out, coreerrors.ToErrorOutput(&coreerrors.ContainerError{Description: "An error occurred running user commands in the container.", OriginalError: err})) + } + logger := log.New(log.Options{ Version: cliVersion(), Level: log.ParseLevel(opts.logLevel), Format: opts.logFormat, Writer: logDst, - Secrets: secretValuesFromFile(opts.secretsFile), + Secrets: maskValues, }) var loadResult *config.LoadResult @@ -212,7 +222,7 @@ func runUserCommandsInContainer(ctx context.Context, out Output, opts *runUserCo remoteUser := lifecycleRemoteUser(merged, cfg) probed := lifecycle.ProbedRemoteEnv(ctx, engine, logger, containerID, remoteUser, lifecycleUserEnvProbe(opts.defaultUserEnvProbe, cfg), opts.containerSessionDataFolder) - remoteEnv := lifecycleRemoteEnv(probed, buildSecretsFromFile(opts.secretsFile), opts.remoteEnvs, cfg) + remoteEnv := lifecycleRemoteEnv(probed, opts.secrets.env(), opts.remoteEnvs, cfg) shellServer := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, remoteEnv...) defer shellServer.Close() diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index ca2404c..0d4ee29 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -1,54 +1,263 @@ package cli import ( + "bytes" "encoding/json" + "errors" "fmt" + "math" "os" + "slices" + "strconv" "strings" + "syscall" + + "github.com/tailscale/hujson" + + coreerrors "github.com/devcontainers/cli/internal/errors" ) -func readSecretsFile(path string) ([]string, error) { +const secretsFileDescription = "Failed to read/parse secrets" + +type secret struct { + name string + value any +} + +type fileSecrets []secret + +func loadSecretsFile(path string) (fileSecrets, error) { + if path == "" { + return nil, nil + } data, err := os.ReadFile(path) if err != nil { - return nil, err + return nil, &coreerrors.ContainerError{Description: secretsFileDescription, OriginalError: nodeReadFileError(path, err)} } - var secrets map[string]string - if err := json.Unmarshal(data, &secrets); err != nil { - return nil, fmt.Errorf("parse secrets: %w", err) + value, err := parseJSONCStrict(data) + if err != nil { + return nil, &coreerrors.ContainerError{Description: secretsFileDescription, OriginalError: errors.New("Invalid json data")} } - var envs []string - for k, v := range secrets { - envs = append(envs, fmt.Sprintf("%s=%s", k, v)) + return jsSpreadEntries(value), nil +} + +func nodeReadFileError(path string, err error) error { + switch { + case errors.Is(err, syscall.ENOENT): + return fmt.Errorf("ENOENT: no such file or directory, open '%s'", path) + case errors.Is(err, syscall.EACCES): + return fmt.Errorf("EACCES: permission denied, open '%s'", path) + case errors.Is(err, syscall.ENOTDIR): + return fmt.Errorf("ENOTDIR: not a directory, open '%s'", path) + case errors.Is(err, syscall.EISDIR): + return errors.New("EISDIR: illegal operation on a directory, read") } - return envs, nil + return err } -func secretValuesFromFile(path string) []string { - if path == "" { - return nil +// parseJSONCStrict accepts what jsonc-parser's parse does without reporting +// an error under its default options: JSON plus comments. Unlike +// internal/jsonc it rejects trailing commas and a leading BOM, as TS does +// for a secrets file (Buffer.toString keeps the BOM). +func parseJSONCStrict(data []byte) (any, error) { + v, err := hujson.Parse(data) + if err != nil { + return nil, err + } + for node := range v.All() { + var last *hujson.Value + switch c := node.Value.(type) { + case *hujson.Object: + if n := len(c.Members); n > 0 { + last = &c.Members[n-1].Value + } + case *hujson.Array: + if n := len(c.Elements); n > 0 { + last = &c.Elements[n-1] + } + } + if last != nil && last.AfterExtra != nil { + return nil, errors.New("trailing comma") + } } - envs, err := readSecretsFile(path) + v.Standardize() + dec := json.NewDecoder(bytes.NewReader(v.Pack())) + dec.UseNumber() + return decodeOrdered(dec) +} + +func decodeOrdered(dec *json.Decoder) (any, error) { + tok, err := dec.Token() if err != nil { - return nil + return nil, err } - values := make([]string, 0, len(envs)) - for _, e := range envs { - if _, after, ok := strings.Cut(e, "="); ok { - values = append(values, after) + switch t := tok.(type) { + case json.Delim: + switch t { + case '{': + members := []secret{} + for dec.More() { + nameTok, err := dec.Token() + if err != nil { + return nil, err + } + value, err := decodeOrdered(dec) + if err != nil { + return nil, err + } + members = append(members, secret{nameTok.(string), value}) + } + _, err := dec.Token() + return members, err + case '[': + elems := []any{} + for dec.More() { + value, err := decodeOrdered(dec) + if err != nil { + return nil, err + } + elems = append(elems, value) + } + _, err := dec.Token() + return elems, err } } - return values + return tok, nil } -func buildSecretsFromFile(path string) []string { - if path == "" { - return nil +// jsSpreadEntries is the [key, value] list of `{...value}`: an object's own +// keys (a repeated key keeps its first position and its last value; array +// index keys come first, ascending), an array's indexes, a string's +// characters, and nothing for null, a number or a boolean. +func jsSpreadEntries(value any) fileSecrets { + switch v := value.(type) { + case []secret: + pos := map[string]int{} + var out fileSecrets + for _, m := range v { + if i, ok := pos[m.name]; ok { + out[i].value = m.value + continue + } + pos[m.name] = len(out) + out = append(out, m) + } + slices.SortStableFunc(out, func(a, b secret) int { + ai, aok := jsArrayIndex(a.name) + bi, bok := jsArrayIndex(b.name) + switch { + case aok && bok: + return compareUint(ai, bi) + case aok: + return -1 + case bok: + return 1 + } + return 0 + }) + return out + case []any: + out := make(fileSecrets, len(v)) + for i, e := range v { + out[i] = secret{strconv.Itoa(i), e} + } + return out + case string: + out := fileSecrets{} + for i, r := range []rune(v) { + out = append(out, secret{strconv.Itoa(i), string(r)}) + } + return out } - secrets, err := readSecretsFile(path) - if err != nil { + return fileSecrets{} +} + +func compareUint(a, b uint64) int { + switch { + case a < b: + return -1 + case a > b: + return 1 + } + return 0 +} + +// jsArrayIndex reports whether name is a canonical array index (0 to +// 2^32-2 without leading zeros), which JS enumerates before other keys. +func jsArrayIndex(name string) (uint64, bool) { + if name == "" || (len(name) > 1 && name[0] == '0') { + return 0, false + } + n, err := strconv.ParseUint(name, 10, 64) + if err != nil || n > 1<<32-2 { + return 0, false + } + return n, true +} + +func (s fileSecrets) env() []string { + if s == nil { return nil } - return secrets + envs := make([]string, 0, len(s)) + for _, e := range s { + envs = append(envs, e.name+"="+jsString(e.value)) + } + return envs +} + +// maskValues is what TS maskSecrets masks: Object.values(secrets) filtered +// by `v.length`, so strings and arrays (as String(array)) are masked and +// numbers, booleans and objects are not. A null value fails there as it +// fails TS ("Cannot read properties of null (reading 'length')"). +func (s fileSecrets) maskValues() ([]string, error) { + var values []string + for _, e := range s { + switch v := e.value.(type) { + case nil: + return nil, errors.New("Cannot read properties of null (reading 'length')") + case string: + values = append(values, v) + case []any: + if len(v) > 0 { + values = append(values, jsString(v)) + } + } + } + return values, nil +} + +func jsString(value any) string { + switch v := value.(type) { + case nil: + return "null" + case string: + return v + case bool: + return strconv.FormatBool(v) + case json.Number: + // jsonc-parser reads numbers with JS Number(): one past the float64 + // range is Infinity, which ParseFloat returns along with ErrRange. + f, _ := strconv.ParseFloat(v.String(), 64) + switch { + case math.IsInf(f, 1): + return "Infinity" + case math.IsInf(f, -1): + return "-Infinity" + } + // encoding/json writes a finite float64 as JS does. + data, _ := json.Marshal(f) + return string(data) + case []any: + parts := make([]string, len(v)) + for i, e := range v { + if e != nil { + parts[i] = jsString(e) + } + } + return strings.Join(parts, ",") + } + return "[object Object]" } func buildSecretIDs(secrets []string) []string { diff --git a/internal/cli/secrets_cmd_test.go b/internal/cli/secrets_cmd_test.go new file mode 100644 index 0000000..5aa8a44 --- /dev/null +++ b/internal/cli/secrets_cmd_test.go @@ -0,0 +1,94 @@ +package cli + +import ( + "path/filepath" + "strings" + "testing" +) + +var secretsCommands = []struct { + name string + run func(t *testing.T, out *captureOutput, workspace, secretsFile string) error +}{ + {"up", func(t *testing.T, out *captureOutput, workspace, secretsFile string) error { + return runUp(t.Context(), out, &upOpts{workspaceFolder: workspace, secretsFile: secretsFile, logLevel: "info", logFormat: "text", buildkit: "auto", gpuAvailability: "detect", workspaceMountConsistency: "cached", defaultUserEnvProbe: "loginInteractiveShell", updateRemoteUserUIDDefault: "on"}) + }}, + {"run-user-commands", func(t *testing.T, out *captureOutput, workspace, secretsFile string) error { + opts := validRunUserCommandsOpts() + opts.workspaceFolder, opts.secretsFile = workspace, secretsFile + return runUserCommands(t.Context(), out, opts) + }}, + {"build", func(t *testing.T, out *captureOutput, workspace, secretsFile string) error { + return runBuild(t.Context(), out, &buildOpts{workspaceFolder: workspace, secretsFile: secretsFile, logLevel: "info", logFormat: "text", buildkit: "auto"}) + }}, +} + +func TestSecretsFileErrorFailsTheCommand(t *testing.T) { + t.Setenv("DOCKER_HOST", "unix:///nonexistent/devcontainer-cli-test.sock") + missing := filepath.Join(t.TempDir(), "missing.json") + for _, cmd := range secretsCommands { + for _, tc := range []struct { + name, path, message string + }{ + {"missing", missing, "ENOENT: no such file or directory, open '" + missing + "'"}, + {"invalid json", writeSecretsFile(t, `{"A":"1",}`), "Invalid json data"}, + } { + t.Run(cmd.name+"/"+tc.name, func(t *testing.T) { + out := &captureOutput{} + err := cmd.run(t, out, t.TempDir(), tc.path) + if ece := asExitCode(t, err); ece.Code != 1 { + t.Errorf("exit code = %d, want 1", ece.Code) + } + want := `{"outcome":"error","message":"` + tc.message + `","description":"Failed to read/parse secrets"}` + if got := strings.TrimSpace(out.out.String()); got != want { + t.Errorf("stdout = %s\nwant %s", got, want) + } + }) + } + } +} + +func TestSecretsFileInJSONCGetsToTheConfiguration(t *testing.T) { + t.Setenv("DOCKER_HOST", "unix:///nonexistent/devcontainer-cli-test.sock") + secrets := writeSecretsFile(t, "{\n// comment\n\"A\": 1, \"B\": [1, 2], \"C\": {\"x\": true} /* end */\n}") + for _, cmd := range secretsCommands { + t.Run(cmd.name, func(t *testing.T) { + out := &captureOutput{} + err := cmd.run(t, out, t.TempDir(), secrets) + if ece := asExitCode(t, err); ece.Code != 1 { + t.Errorf("exit code = %d, want 1", ece.Code) + } + if got := out.out.String(); !strings.Contains(got, "Dev container config") || strings.Contains(got, "secrets") { + t.Errorf("stdout = %q, want the configuration not found, past the secrets file", got) + } + }) + } +} + +func TestSecretsFileNullValueFailsLikeTS(t *testing.T) { + t.Setenv("DOCKER_HOST", "unix:///nonexistent/devcontainer-cli-test.sock") + secrets := writeSecretsFile(t, `{"A":null}`) + for _, tc := range []struct { + command, description string + }{ + {"up", "An error occurred setting up the container."}, + {"run-user-commands", "An error occurred running user commands in the container."}, + } { + t.Run(tc.command, func(t *testing.T) { + for _, cmd := range secretsCommands { + if cmd.name != tc.command { + continue + } + out := &captureOutput{} + err := cmd.run(t, out, t.TempDir(), secrets) + if ece := asExitCode(t, err); ece.Code != 1 { + t.Errorf("exit code = %d, want 1", ece.Code) + } + want := `{"outcome":"error","message":"Cannot read properties of null (reading 'length')","description":"` + tc.description + `"}` + if got := strings.TrimSpace(out.out.String()); got != want { + t.Errorf("stdout = %s\nwant %s", got, want) + } + } + }) + } +} diff --git a/internal/cli/secrets_held_test.go b/internal/cli/secrets_held_test.go index 0aac15e..e029097 100644 --- a/internal/cli/secrets_held_test.go +++ b/internal/cli/secrets_held_test.go @@ -5,27 +5,117 @@ import ( "path/filepath" "slices" "testing" + + coreerrors "github.com/devcontainers/cli/internal/errors" ) +func writeSecretsFile(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "secrets.json") + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + return path +} + func TestSecretValuesFromFile(t *testing.T) { - dir := t.TempDir() - good := filepath.Join(dir, "secrets.json") - if err := os.WriteFile(good, []byte(`{"TOKEN":"s3cr=t","USER":"bob"}`), 0o600); err != nil { + s, err := loadSecretsFile(writeSecretsFile(t, `{"TOKEN":"s3cr=t","USER":"bob"}`)) + if err != nil { + t.Fatal(err) + } + got, err := s.maskValues() + if err != nil { t.Fatal(err) } - got := secretValuesFromFile(good) slices.Sort(got) if want := []string{"bob", "s3cr=t"}; !slices.Equal(got, want) { t.Errorf("values = %q, want %q", got, want) } - bad := filepath.Join(dir, "bad.json") - if err := os.WriteFile(bad, []byte(`[1,2]`), 0o600); err != nil { + s, err = loadSecretsFile("") + if err != nil || s != nil { + t.Errorf("no --secrets-file = %q, %v; want no secrets and no error", s, err) + } +} + +func TestLoadSecretsFileFailsAsTS(t *testing.T) { + dir := t.TempDir() + missing := filepath.Join(dir, "missing.json") + for _, tc := range []struct { + name, path, message string + }{ + {"missing file", missing, "ENOENT: no such file or directory, open '" + missing + "'"}, + {"relative missing file", "missing-secrets.json", "ENOENT: no such file or directory, open 'missing-secrets.json'"}, + {"directory", dir, "EISDIR: illegal operation on a directory, read"}, + {"not json", writeSecretsFile(t, `not json`), "Invalid json data"}, + {"empty", writeSecretsFile(t, ``), "Invalid json data"}, + {"trailing comma", writeSecretsFile(t, `{"A":"1",}`), "Invalid json data"}, + {"array trailing comma", writeSecretsFile(t, `[1,2,]`), "Invalid json data"}, + {"trailing data", writeSecretsFile(t, `{"A":"1"} x`), "Invalid json data"}, + {"bom", writeSecretsFile(t, "\xEF\xBB\xBF{\"A\":\"1\"}"), "Invalid json data"}, + {"unquoted name", writeSecretsFile(t, `{A:"1"}`), "Invalid json data"}, + } { + t.Run(tc.name, func(t *testing.T) { + s, err := loadSecretsFile(tc.path) + if s != nil { + t.Errorf("secrets = %q, want none", s) + } + ce, ok := coreerrors.AsContainerError(err) + if !ok { + t.Fatalf("error = %v (%T), want a ContainerError", err, err) + } + got := coreerrors.ToErrorOutput(ce) + want := coreerrors.ErrorOutput{Outcome: "error", Message: tc.message, Description: "Failed to read/parse secrets"} + if got != want { + t.Errorf("error output = %+v, want %+v", got, want) + } + }) + } +} + +func TestLoadSecretsFileAcceptsWhatJSONCParses(t *testing.T) { + for _, tc := range []struct { + name, content string + env, mask []string + }{ + {"comments", "{\n// line\n\"A\":\"x\" /* block */\n}", []string{"A=x"}, []string{"x"}}, + {"non-string values", `{"N":1,"F":1.5,"E":1e21,"T":true,"O":{"x":1},"L":[1,null,"b"],"Z":[]}`, + []string{"N=1", "F=1.5", "E=1e+21", "T=true", "O=[object Object]", "L=1,,b", "Z="}, []string{"1,,b"}}, + {"array", `[1,"two"]`, []string{"0=1", "1=two"}, []string{"two"}}, + {"string", `"ab"`, []string{"0=a", "1=b"}, []string{"a", "b"}}, + {"null", `null`, []string{}, nil}, + {"number", `5`, []string{}, nil}, + {"repeated and index keys", `{"B":"1","2":"x","A":"2","B":"3","1":"y"}`, + []string{"1=y", "2=x", "B=3", "A=2"}, []string{"y", "x", "3", "2"}}, + } { + t.Run(tc.name, func(t *testing.T) { + s, err := loadSecretsFile(writeSecretsFile(t, tc.content)) + if err != nil { + t.Fatal(err) + } + if got := s.env(); !slices.Equal(got, tc.env) { + t.Errorf("env = %q, want %q", got, tc.env) + } + mask, err := s.maskValues() + if err != nil { + t.Fatal(err) + } + if !slices.Equal(mask, tc.mask) { + t.Errorf("mask values = %q, want %q", mask, tc.mask) + } + }) + } +} + +func TestSecretsMaskValuesFailsOnNullAsTS(t *testing.T) { + s, err := loadSecretsFile(writeSecretsFile(t, `{"A":"x","B":null}`)) + if err != nil { t.Fatal(err) } - for _, p := range []string{"", filepath.Join(dir, "missing.json"), bad} { - if got := secretValuesFromFile(p); got != nil { - t.Errorf("secretValuesFromFile(%q) = %q, want nil", p, got) - } + if got := s.env(); !slices.Equal(got, []string{"A=x", "B=null"}) { + t.Errorf("env = %q", got) + } + if _, err := s.maskValues(); err == nil || err.Error() != "Cannot read properties of null (reading 'length')" { + t.Errorf("mask error = %v, want the TS TypeError message", err) } } diff --git a/internal/cli/secrets_repo_held_test.go b/internal/cli/secrets_repo_held_test.go index 48e7dac..2656c40 100644 --- a/internal/cli/secrets_repo_held_test.go +++ b/internal/cli/secrets_repo_held_test.go @@ -1,32 +1,28 @@ package cli import ( - "os" - "path/filepath" "slices" "testing" ) func TestBuildSecretsFromFile(t *testing.T) { - dir := t.TempDir() - good := filepath.Join(dir, "secrets.json") - if err := os.WriteFile(good, []byte(`{"NPM_TOKEN":"abc","GH_TOKEN":"x=y"}`), 0o600); err != nil { + s, err := loadSecretsFile(writeSecretsFile(t, `{"NPM_TOKEN":"abc","GH_TOKEN":"x=y"}`)) + if err != nil { t.Fatal(err) } - got := buildSecretsFromFile(good) + got := s.env() slices.Sort(got) if want := []string{"GH_TOKEN=x=y", "NPM_TOKEN=abc"}; !slices.Equal(got, want) { t.Errorf("secrets = %q, want %q", got, want) } - bad := filepath.Join(dir, "bad.json") - if err := os.WriteFile(bad, []byte(`not json`), 0o600); err != nil { - t.Fatal(err) - } - for _, p := range []string{"", filepath.Join(dir, "missing.json"), bad} { - if got := buildSecretsFromFile(p); got != nil { - t.Errorf("buildSecretsFromFile(%q) = %q, want nil", p, got) + for _, content := range []string{`not json`, `{"A":"1",}`} { + if s, err := loadSecretsFile(writeSecretsFile(t, content)); err == nil { + t.Errorf("loadSecretsFile(%q) = %q, want an error", content, s) } } + if s, err := loadSecretsFile(""); err != nil || s.env() != nil { + t.Errorf("no --secrets-file = %q, %v; want no secrets", s.env(), err) + } } func TestBuildSecretIDsOneLetterName(t *testing.T) { diff --git a/internal/cli/up.go b/internal/cli/up.go index bf437d8..124771d 100644 --- a/internal/cli/up.go +++ b/internal/cli/up.go @@ -62,6 +62,7 @@ type upOpts struct { terminalColumns givenInt terminalRows givenInt secretsFile string + secrets fileSecrets caBundle string experimentalLockfile bool experimentalFrozenLockfile bool @@ -247,12 +248,20 @@ func runUp(ctx context.Context, out Output, opts *upOpts) error { } defer closeLog() + if opts.secrets, err = loadSecretsFile(opts.secretsFile); err != nil { + return writeErrorJSON(out, coreerrors.ToErrorOutput(err)) + } + maskValues, err := opts.secrets.maskValues() + if err != nil { + return writeErrorJSON(out, coreerrors.ToErrorOutput(&coreerrors.ContainerError{Description: "An error occurred setting up the container.", OriginalError: err})) + } + logger := log.New(log.Options{ Version: cliVersion(), Level: log.ParseLevel(opts.logLevel), Format: opts.logFormat, Writer: logDst, - Secrets: secretValuesFromFile(opts.secretsFile), + Secrets: maskValues, }) ociAuthPolicy(ctx).SetLogger(logger) @@ -575,7 +584,7 @@ func (r *upRunner) fromDockerfile(ctx context.Context, cfg *config.DevContainer, defer authCleanup() buildResult, err := dockerClient.Build(ctx, docker.BuildOptions{ - Secrets: buildxSecrets(buildSecretsFromFile(opts.secretsFile), useBuildx, logger), + Secrets: buildxSecrets(opts.secrets.env(), useBuildx, logger), FileSecrets: caBundleBuildSecrets(opts.caBundle, useBuildx, logger), Dockerfile: dockerfilePath, ContextPath: contextPath, @@ -1031,7 +1040,7 @@ func (r *upRunner) runLifecycleForUp(ctx context.Context, containerID string, cf logger, engine, opts := r.log, r.engine, r.opts probeStrategy := lifecycle.UserEnvProbeStrategy(cmp.Or(cfg.UserEnvProbe, opts.defaultUserEnvProbe)) probed := lifecycle.ProbedRemoteEnv(ctx, engine, logger, containerID, remoteUser, probeStrategy, opts.containerSessionDataFolder) - extra := append(secretsRemoteEnv(opts.secretsFile, logger), opts.remoteEnvs...) + extra := append(opts.secrets.env(), opts.remoteEnvs...) mergedRemoteEnv := hookRemoteEnv(probed, extra, cfg.RemoteEnv) shellServer := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, mergedRemoteEnv...) @@ -1078,18 +1087,6 @@ func (r *upRunner) checkContainerSubstitution(inspectResp container.InspectRespo return checkContainerSubstitution(containerEnv, cfg.ConfigFilePath, raw, imagemeta.MergeConfiguration(entries)) } -func secretsRemoteEnv(path string, logger log.Logger) []string { - if path == "" { - return nil - } - envs, err := readSecretsFile(path) - if err != nil { - logger.Write(fmt.Sprintf("Warning: secrets file: %v", err), log.LevelWarning) - return nil - } - return envs -} - func hookRemoteEnv(probed map[string]string, extra []string, cfgRemoteEnv map[string]*string) []string { var env []string for k, v := range probed { @@ -1178,7 +1175,7 @@ func (r *upRunner) featureBuildOptions(cfg *config.DevContainer, loadResult *con opts := r.opts return &FeatureBuildOptions{ Platform: platform, - Secrets: buildSecretsFromFile(opts.secretsFile), + Secrets: opts.secrets.env(), CABundle: opts.caBundle, OverrideFeatureInstallOrder: cfg.OverrideFeatureInstallOrder, FeaturesBasePath: filepath.Dir(cfg.ConfigFilePath), diff --git a/internal/cli/up_held_test.go b/internal/cli/up_held_test.go index d0bdd4a..eba0384 100644 --- a/internal/cli/up_held_test.go +++ b/internal/cli/up_held_test.go @@ -464,34 +464,6 @@ func TestParseComposeServiceBuildReadsTheServicesBuild(t *testing.T) { } } -func TestSecretsRemoteEnv(t *testing.T) { - dir := t.TempDir() - good := filepath.Join(dir, "secrets.json") - if err := os.WriteFile(good, []byte(`{"TOKEN":"s3cr3t"}`), 0o600); err != nil { - t.Fatal(err) - } - for _, tc := range []struct { - name, path string - want []string - warns bool - }{ - {"no file", "", nil, false}, - {"file", good, []string{"TOKEN=s3cr3t"}, false}, - {"missing file", filepath.Join(dir, "missing.json"), nil, true}, - } { - t.Run(tc.name, func(t *testing.T) { - var logged bytes.Buffer - got := secretsRemoteEnv(tc.path, log.New(log.Options{Writer: &logged})) - if !reflect.DeepEqual(got, tc.want) { - t.Errorf("env = %q, want %q", got, tc.want) - } - if warned := strings.Contains(logged.String(), "Warning: secrets file:"); warned != tc.warns { - t.Errorf("log = %q, want a warning: %v", logged.String(), tc.warns) - } - }) - } -} - func TestHookRemoteEnvOrdersLaterSourcesLast(t *testing.T) { got := hookRemoteEnv( map[string]string{"P": "probed"}, From fe07db0af679be85b5a0b89cc84eac2748c00e40 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 12 Nov 2026 22:18:36 -0300 Subject: [PATCH 2/2] fix(cli): print -0 and mask secrets from --secrets-file as the reference Order a secrets object's keys with the Features package's JS object instead of a second copy of the array-index rules, write -0 as "0" as String() does, and leave empty strings out of the values to mask, as replaceAllLog's length filter does. Pin array-index boundaries, number formatting and masking by length with cases computed by node from the reference's jsonc-parser. Co-Authored-By: Claude Opus 5.5 --- internal/cli/secrets.go | 83 ++++++++++--------------------- internal/cli/secrets_held_test.go | 14 ++++++ internal/features/jsobject.go | 11 ++++ 3 files changed, 51 insertions(+), 57 deletions(-) diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 0d4ee29..63c56e2 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -7,7 +7,6 @@ import ( "fmt" "math" "os" - "slices" "strconv" "strings" "syscall" @@ -15,6 +14,7 @@ import ( "github.com/tailscale/hujson" coreerrors "github.com/devcontainers/cli/internal/errors" + "github.com/devcontainers/cli/internal/features" ) const secretsFileDescription = "Failed to read/parse secrets" @@ -126,35 +126,22 @@ func decodeOrdered(dec *json.Decoder) (any, error) { } // jsSpreadEntries is the [key, value] list of `{...value}`: an object's own -// keys (a repeated key keeps its first position and its last value; array -// index keys come first, ascending), an array's indexes, a string's +// keys in the order JavaScript enumerates them (a repeated key keeps its +// first position and its last value), an array's indexes, a string's // characters, and nothing for null, a number or a boolean. func jsSpreadEntries(value any) fileSecrets { switch v := value.(type) { case []secret: - pos := map[string]int{} + names := make([]string, len(v)) + last := make(map[string]any, len(v)) + for i, m := range v { + names[i] = m.name + last[m.name] = m.value + } var out fileSecrets - for _, m := range v { - if i, ok := pos[m.name]; ok { - out[i].value = m.value - continue - } - pos[m.name] = len(out) - out = append(out, m) + for _, name := range features.KeyOrder(names) { + out = append(out, secret{name, last[name]}) } - slices.SortStableFunc(out, func(a, b secret) int { - ai, aok := jsArrayIndex(a.name) - bi, bok := jsArrayIndex(b.name) - switch { - case aok && bok: - return compareUint(ai, bi) - case aok: - return -1 - case bok: - return 1 - } - return 0 - }) return out case []any: out := make(fileSecrets, len(v)) @@ -172,29 +159,6 @@ func jsSpreadEntries(value any) fileSecrets { return fileSecrets{} } -func compareUint(a, b uint64) int { - switch { - case a < b: - return -1 - case a > b: - return 1 - } - return 0 -} - -// jsArrayIndex reports whether name is a canonical array index (0 to -// 2^32-2 without leading zeros), which JS enumerates before other keys. -func jsArrayIndex(name string) (uint64, bool) { - if name == "" || (len(name) > 1 && name[0] == '0') { - return 0, false - } - n, err := strconv.ParseUint(name, 10, 64) - if err != nil || n > 1<<32-2 { - return 0, false - } - return n, true -} - func (s fileSecrets) env() []string { if s == nil { return nil @@ -207,9 +171,9 @@ func (s fileSecrets) env() []string { } // maskValues is what TS maskSecrets masks: Object.values(secrets) filtered -// by `v.length`, so strings and arrays (as String(array)) are masked and -// numbers, booleans and objects are not. A null value fails there as it -// fails TS ("Cannot read properties of null (reading 'length')"). +// by `v.length`, so non-empty strings and arrays (as String(array)) are +// masked and numbers, booleans and objects are not. A null value fails there +// as it fails TS ("Cannot read properties of null (reading 'length')"). func (s fileSecrets) maskValues() ([]string, error) { var values []string for _, e := range s { @@ -217,7 +181,9 @@ func (s fileSecrets) maskValues() ([]string, error) { case nil: return nil, errors.New("Cannot read properties of null (reading 'length')") case string: - values = append(values, v) + if v != "" { + values = append(values, v) + } case []any: if len(v) > 0 { values = append(values, jsString(v)) @@ -238,14 +204,17 @@ func jsString(value any) string { case json.Number: // jsonc-parser reads numbers with JS Number(): one past the float64 // range is Infinity, which ParseFloat returns along with ErrRange. - f, _ := strconv.ParseFloat(v.String(), 64) - switch { - case math.IsInf(f, 1): + f, _ := v.Float64() + if math.IsInf(f, 0) { + if math.Signbit(f) { + return "-Infinity" + } return "Infinity" - case math.IsInf(f, -1): - return "-Infinity" } - // encoding/json writes a finite float64 as JS does. + if f == 0 { + f = 0 // String(-0) is "0" + } + // encoding/json writes any other finite float64 as JS does. data, _ := json.Marshal(f) return string(data) case []any: diff --git a/internal/cli/secrets_held_test.go b/internal/cli/secrets_held_test.go index e029097..8f457a9 100644 --- a/internal/cli/secrets_held_test.go +++ b/internal/cli/secrets_held_test.go @@ -51,6 +51,8 @@ func TestLoadSecretsFileFailsAsTS(t *testing.T) { {"empty", writeSecretsFile(t, ``), "Invalid json data"}, {"trailing comma", writeSecretsFile(t, `{"A":"1",}`), "Invalid json data"}, {"array trailing comma", writeSecretsFile(t, `[1,2,]`), "Invalid json data"}, + {"one-element array trailing comma", writeSecretsFile(t, `{"A":["x",]}`), "Invalid json data"}, + {"one-member object trailing comma", writeSecretsFile(t, `{"A":{"x":1,}}`), "Invalid json data"}, {"trailing data", writeSecretsFile(t, `{"A":"1"} x`), "Invalid json data"}, {"bom", writeSecretsFile(t, "\xEF\xBB\xBF{\"A\":\"1\"}"), "Invalid json data"}, {"unquoted name", writeSecretsFile(t, `{A:"1"}`), "Invalid json data"}, @@ -87,6 +89,18 @@ func TestLoadSecretsFileAcceptsWhatJSONCParses(t *testing.T) { {"number", `5`, []string{}, nil}, {"repeated and index keys", `{"B":"1","2":"x","A":"2","B":"3","1":"y"}`, []string{"1=y", "2=x", "B=3", "A=2"}, []string{"y", "x", "3", "2"}}, + // Expected values below are what node prints for the reference's + // jsonc.parse followed by `{...secrets}`. + {"array index boundaries", + `{"B":"b","4294967295":"max","4294967294":"top","01":"lead","0":"zero","-1":"neg","99999999999999999999":"long","10":"ten","2":"two"}`, + []string{"0=zero", "2=two", "10=ten", "4294967294=top", "B=b", "4294967295=max", "01=lead", "-1=neg", "99999999999999999999=long"}, + []string{"zero", "two", "ten", "top", "b", "max", "lead", "neg", "long"}}, + {"String of numbers", + `{"Z":-0,"M":-0.0e5,"BIG":1.7976931348623157e308,"NBIG":-1.7976931348623157e308,"I":1e400,"NI":-1e400,"E20":1e20,"E21":1e21,"L":123456789012345678901234,"S":1e-7,"T":0.000001}`, + []string{"Z=0", "M=0", "BIG=1.7976931348623157e+308", "NBIG=-1.7976931348623157e+308", "I=Infinity", "NI=-Infinity", + "E20=100000000000000000000", "E21=1e+21", "L=1.2345678901234569e+23", "S=1e-7", "T=0.000001"}, nil}, + {"masking by length", `{"A":["x"],"N":[null],"NN":[[null,1],null],"E":[],"S":""}`, + []string{"A=x", "N=", "NN=,1,", "E=", "S="}, []string{"x", "", ",1,"}}, } { t.Run(tc.name, func(t *testing.T) { s, err := loadSecretsFile(writeSecretsFile(t, tc.content)) diff --git a/internal/features/jsobject.go b/internal/features/jsobject.go index c3d7e1f..3c918cc 100644 --- a/internal/features/jsobject.go +++ b/internal/features/jsobject.go @@ -76,6 +76,17 @@ func (o *jsObject) order() []string { return append(idx, rest...) } +// KeyOrder lists the distinct keys of an object that JavaScript gave the +// properties keys, in that order, as it enumerates them: the integer-index +// keys ascending, then the others where each was first set. +func KeyOrder(keys []string) []string { + o := newJSObject() + for _, k := range keys { + o.set(k, nil) + } + return o.order() +} + func (o *jsObject) MarshalJSON() ([]byte, error) { var buf bytes.Buffer buf.WriteByte('{')