fix(cli): fail when --secrets-file can't be read or parsed

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

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

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,

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()