coder · Emyrk · Sep 29, 2023 · Sep 28, 2023 · Sep 28, 2023 · Sep 28, 2023
diff --git a/cli/clibase/option.go b/cli/clibase/option.go
@@ -1,6 +1,8 @@
 package clibase
 
 import (
+	"bytes"
+	"encoding/json"
 	"os"
 	"strings"
 
@@ -65,6 +67,20 @@ type Option struct {
 	ValueSource ValueSource `json:"value_source,omitempty"`
 }
 
+// optionNoMethods is just a wrapper around Option so we can defer to the
+// default json.Unmarshaler behavior.
+type optionNoMethods Option
+
+func (o *Option) UnmarshalJSON(data []byte) error {
+	// If an option has no values, we have no idea how to unmarshal it.
+	// So just discard the json data.
+	if o.Value == nil {
+		o.Value = &DiscardValue
+	}
+
+	return json.Unmarshal(data, (*optionNoMethods)(o))
+}
+
 func (o Option) YAMLPath() string {
 	if o.YAML == "" {
 		return ""
@@ -79,15 +95,94 @@ func (o Option) YAMLPath() string {
 // OptionSet is a group of options that can be applied to a command.
 type OptionSet []Option
 
+// UnmarshalJSON implements json.Unmarshaler for OptionSets. Options have an
+// interface Value type that cannot handle unmarshalling because the types cannot
+// be inferred. Since it is a slice, instantiating the Options first does not
+// help.
+//
+// However, we typically do instantiate the slice to have the correct types.
+// So this unmarshaller will attempt to find the named option in the existing
+// set, if it cannot, the value is discarded. If the option exists, the value
+// is unmarshalled into the existing option, and replaces the existing option.
+//
+// The value is discarded if it's type cannot be inferred. This behavior just
+// feels "safer", although it should never happen if the correct option set
+// is passed in.
+func (optSet *OptionSet) UnmarshalJSON(data []byte) error {
+	dec := json.NewDecoder(bytes.NewBuffer(data))
+	// Should be a json array, so consume the starting open bracket.
+	t, err := dec.Token()
+	if err != nil {
+		return xerrors.Errorf("read array open bracket: %w", err)
+	}
+	if t != json.Delim('[') {
+		return xerrors.Errorf("expected array open bracket, got %q", t)
+	}
+
+	// As long as json elements exist, consume them. The counter is used for
+	// better errors.
+	var i int
+OptionSetDecodeLoop:
+	for dec.More() {
+		var opt Option
+		// jValue is a placeholder value that allows us to capture the
+		// raw json for the value to attempt to unmarshal later.
+		var jValue jsonValue
+		opt.Value = &jValue
+		err := dec.Decode(&opt)
+		if err != nil {
+			return xerrors.Errorf("decode %d option: %w", i, err)
+		}
+
+		// Try to see if the option already exists in the option set.
+		// If it does, just update the existing option.
+		for i, have := range *optSet {
+			if have.Name == opt.Name {
+				if jValue != nil {
+					err := json.Unmarshal(jValue, &(*optSet)[i].Value)
+					if err != nil {
+						return xerrors.Errorf("decode option %q value: %w", have.Name, err)
+					}
+					// Set the opt's value
+					opt.Value = (*optSet)[i].Value
+				} else {
+					// Discard the value if it's nil.
+					opt.Value = DiscardValue
+				}
+				// Override the existing.
+				(*optSet)[i] = opt
+				// Go to the next option to decode.
+				continue OptionSetDecodeLoop
+			}
+		}
+
+		// If the option doesn't exist, the value will be discarded.
+		// We do this because we cannot infer the type of the value.
+		opt.Value = DiscardValue
+		*optSet = append(*optSet, opt)
+		i++
+	}
+
+	t, err = dec.Token()
+	if err != nil {
+		return xerrors.Errorf("read array close bracket: %w", err)
+	}
+	if t != json.Delim(']') {
+		return xerrors.Errorf("expected array close bracket, got %q", t)
+	}
+
+	return nil
+}
+
 // Add adds the given Options to the OptionSet.
-func (s *OptionSet) Add(opts ...Option) {
-	*s = append(*s, opts...)
+func (optSet *OptionSet) Add(opts ...Option) {
+	*optSet = append(*optSet, opts...)
 }
 
 // Filter will only return options that match the given filter. (return true)
-func (s OptionSet) Filter(filter func(opt Option) bool) OptionSet {
+func (optSet OptionSet) Filter(filter func(opt Option) bool) OptionSet {
 	cpy := make(OptionSet, 0)
-	for _, opt := range s {
+	for _, opt := range optSet {
 		if filter(opt) {
 			cpy = append(cpy, opt)
 		}
@@ -96,13 +191,13 @@ func (s OptionSet) Filter(filter func(opt Option) bool) OptionSet {
 }
 
 // FlagSet returns a pflag.FlagSet for the OptionSet.
-func (s *OptionSet) FlagSet() *pflag.FlagSet {
-	if s == nil {
+func (optSet *OptionSet) FlagSet() *pflag.FlagSet {
+	if optSet == nil {
 		return &pflag.FlagSet{}
 	}
 
 	fs := pflag.NewFlagSet("", pflag.ContinueOnError)
-	for _, opt := range *s {
+	for _, opt := range *optSet {
 		if opt.Flag == "" {
 			continue
 		}
@@ -139,8 +234,8 @@ func (s *OptionSet) FlagSet() *pflag.FlagSet {
 
 // ParseEnv parses the given environment variables into the OptionSet.
 // Use EnvsWithPrefix to filter out prefixes.
-func (s *OptionSet) ParseEnv(vs []EnvVar) error {
-	if s == nil {
+func (optSet *OptionSet) ParseEnv(vs []EnvVar) error {
+	if optSet == nil {
 		return nil
 	}
 
@@ -154,7 +249,7 @@ func (s *OptionSet) ParseEnv(vs []EnvVar) error {
 		envs[v.Name] = v.Value
 	}
 
-	for i, opt := range *s {
+	for i, opt := range *optSet {
 		if opt.Env == "" {
 			continue
 		}
@@ -172,7 +267,7 @@ func (s *OptionSet) ParseEnv(vs []EnvVar) error {
 			continue
 		}
 
-		(*s)[i].ValueSource = ValueSourceEnv
+		(*optSet)[i].ValueSource = ValueSourceEnv
 		if err := opt.Value.Set(envVal); err != nil {
 			merr = multierror.Append(
 				merr, xerrors.Errorf("parse %q: %w", opt.Name, err),
@@ -185,14 +280,14 @@ func (s *OptionSet) ParseEnv(vs []EnvVar) error {
 
 // SetDefaults sets the default values for each Option, skipping values
 // that already have a value source.
-func (s *OptionSet) SetDefaults() error {
-	if s == nil {
+func (optSet *OptionSet) SetDefaults() error {
+	if optSet == nil {
 		return nil
 	}
 
 	var merr *multierror.Error
 
-	for i, opt := range *s {
+	for i, opt := range *optSet {
 		// Skip values that may have already been set by the user.
 		if opt.ValueSource != ValueSourceNone {
 			continue
@@ -212,7 +307,7 @@ func (s *OptionSet) SetDefaults() error {
 			)
 			continue
 		}
-		(*s)[i].ValueSource = ValueSourceDefault
+		(*optSet)[i].ValueSource = ValueSourceDefault
 		if err := opt.Value.Set(opt.Default); err != nil {
 			merr = multierror.Append(
 				merr, xerrors.Errorf("parse %q: %w", opt.Name, err),
@@ -224,9 +319,9 @@ func (s *OptionSet) SetDefaults() error {
 
 // ByName returns the Option with the given name, or nil if no such option
 // exists.
-func (s *OptionSet) ByName(name string) *Option {
-	for i := range *s {
-		opt := &(*s)[i]
+func (optSet *OptionSet) ByName(name string) *Option {
+	for i := range *optSet {
+		opt := &(*optSet)[i]
 		if opt.Name == name {
 			return opt
 		}

diff --git a/cli/clibase/values.go b/cli/clibase/values.go
@@ -430,6 +430,35 @@ func (discardValue) Type() string {
 	return "discard"
 }
 
+func (discardValue) UnmarshalJSON([]byte) error {
+	return nil
+}
+
+// jsonValue is intentionally not exported. It is just used to store the raw JSON
+// data for a value to defer it's unmarshal. It implements the pflag.Value to be
+// usable in an Option.
+type jsonValue json.RawMessage
+
+func (jsonValue) Set(string) error {
+	return xerrors.Errorf("json value is read-only")
+}
+
+func (jsonValue) String() string {
+	return ""
+}
+
+func (jsonValue) Type() string {
+	return "json"
+}
+
+func (j *jsonValue) UnmarshalJSON(data []byte) error {
+	if j == nil {
+		return xerrors.New("json.RawMessage: UnmarshalJSON on nil pointer")
+	}
+	*j = append((*j)[0:0], data...)
+	return nil
+}
+
 var _ pflag.Value = (*Enum)(nil)
 
 type Enum struct {

diff --git a/cli/clibase/yaml.go b/cli/clibase/yaml.go
@@ -51,12 +51,12 @@ func deepMapNode(n *yaml.Node, path []string, headComment string) *yaml.Node {
 // the stack.
 //
 // It is isomorphic with FromYAML.
-func (s *OptionSet) MarshalYAML() (any, error) {
+func (optSet *OptionSet) MarshalYAML() (any, error) {
 	root := yaml.Node{
 		Kind: yaml.MappingNode,
 	}
 
-	for _, opt := range *s {
+	for _, opt := range *optSet {
 		if opt.YAML == "" {
 			continue
 		}
@@ -222,7 +222,7 @@ func (o *Option) setFromYAMLNode(n *yaml.Node) error {
 
 // UnmarshalYAML converts the given YAML node into the option set.
 // It is isomorphic with ToYAML.
-func (s *OptionSet) UnmarshalYAML(rootNode *yaml.Node) error {
+func (optSet *OptionSet) UnmarshalYAML(rootNode *yaml.Node) error {
 	// The rootNode will be a DocumentNode if it's read from a file. We do
 	// not support multiple documents in a single file.
 	if rootNode.Kind == yaml.DocumentNode {
@@ -240,8 +240,8 @@ func (s *OptionSet) UnmarshalYAML(rootNode *yaml.Node) error {
 	matchedNodes := make(map[string]*yaml.Node, len(yamlNodes))
 
 	var merr error
-	for i := range *s {
-		opt := &(*s)[i]
+	for i := range *optSet {
+		opt := &(*optSet)[i]
 		if opt.YAML == "" {
 			continue
 		}

diff --git a/cli/server.go b/cli/server.go
@@ -617,6 +617,10 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
 				MetricsCacheRefreshInterval: vals.MetricsCacheRefreshInterval.Value(),
 				AgentStatsRefreshInterval:   vals.AgentStatRefreshInterval.Value(),
 				DeploymentValues:            vals,
+				// Do not pass secret values to DeploymentOptions. All values should be read from
+				// the DeploymentValues instead, this just serves to indicate the source of each
+				// option. This is just defensive to prevent accidentally leaking.
+				DeploymentOptions:           codersdk.DeploymentOptionsWithoutSecrets(opts),
 				PrometheusRegistry:          prometheus.NewRegistry(),
 				APIRateLimit:                int(vals.RateLimit.API.Value()),
 				LoginRateLimit:              loginRateLimit,

diff --git a/cli/server_test.go b/cli/server_test.go
@@ -1188,6 +1188,22 @@ func TestServer(t *testing.T) {
 			require.Equal(t, map[string]string{"serious_business_unit": "serious_business_unit"}, deploymentConfig.Values.OIDC.GroupMapping.Value)
 			require.Equal(t, "Sign In With Coder", deploymentConfig.Values.OIDC.SignInText.Value())
 			require.Equal(t, "https://example.com/icon.png", deploymentConfig.Values.OIDC.IconURL.Value().String())
+
+			// Verify the option values
+			for _, opt := range deploymentConfig.Options {
+				switch opt.Flag {
+				case "access-url":
+					require.Equal(t, "http://example.com", opt.Value.String())
+				case "oidc-icon-url":
+					require.Equal(t, "https://example.com/icon.png", opt.Value.String())
+				case "oidc-sign-in-text":
+					require.Equal(t, "Sign In With Coder", opt.Value.String())
+				case "redirect-to-access-url":
+					require.Equal(t, "false", opt.Value.String())
+				case "derp-server-region-id":
+					require.Equal(t, "999", opt.Value.String())
+				}
+			}
 		})
 	})
 

diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -40,6 +40,7 @@ import (
 
 	"cdr.dev/slog"
 	"github.com/coder/coder/v2/buildinfo"
+	"github.com/coder/coder/v2/cli/clibase"
 	"github.com/coder/coder/v2/coderd/audit"
 	"github.com/coder/coder/v2/coderd/awsidentity"
 	"github.com/coder/coder/v2/coderd/batchstats"
@@ -152,7 +153,12 @@ type Options struct {
 	MetricsCacheRefreshInterval time.Duration
 	AgentStatsRefreshInterval   time.Duration
 	DeploymentValues            *codersdk.DeploymentValues
-	UpdateCheckOptions          *updatecheck.Options // Set non-nil to enable update checking.
+	// DeploymentOptions do contain the copy of DeploymentValues, but contain
+	// contextual information about how the values were set.
+	// Do not use DeploymentOptions to retrieve values, use DeploymentValues instead.
 codersdk.DeploymentConfig{ 
 	Values:  values, 
 	Options: api.DeploymentOptions, 
 }, 
 codersdk.DeploymentConfig{ 
 	Values:  values, 
 	Options: api.DeploymentOptions, 
 }, 
+	// All secrets values are stripped.
+	DeploymentOptions  clibase.OptionSet
+	UpdateCheckOptions *updatecheck.Options // Set non-nil to enable update checking.
 
 	// SSHConfig is the response clients use to configure config-ssh locally.
 	SSHConfig codersdk.SSHConfigResponse

diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -417,6 +417,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
 			MetricsCacheRefreshInterval:        options.MetricsCacheRefreshInterval,
 			AgentStatsRefreshInterval:          options.AgentStatsRefreshInterval,
 			DeploymentValues:                   options.DeploymentValues,
+			DeploymentOptions:                  codersdk.DeploymentOptionsWithoutSecrets(options.DeploymentValues.Options()),
 			UpdateCheckOptions:                 options.UpdateCheckOptions,
 			SwaggerEndpoint:                    options.SwaggerEndpoint,
 			AppSecurityKey:                     AppSecurityKey,

diff --git a/coderd/deployment.go b/coderd/deployment.go
@@ -33,7 +33,7 @@ func (api *API) deploymentValues(rw http.ResponseWriter, r *http.Request) {
 		r.Context(), rw, http.StatusOK,
 		codersdk.DeploymentConfig{
 			Values:  values,
-			Options: values.Options(),
+			Options: api.DeploymentOptions,
 		},
 	)
 }

diff --git a/codersdk/deployment.go b/codersdk/deployment.go
@@ -1762,6 +1762,26 @@ type LinkConfig struct {
 	Icon   string `json:"icon" yaml:"icon"`
 }
 
+// DeploymentOptionsWithoutSecrets returns a copy of the OptionSet with secret values omitted.
+func DeploymentOptionsWithoutSecrets(set clibase.OptionSet) clibase.OptionSet {
+	cpy := make(clibase.OptionSet, 0, len(set))
+	for _, opt := range set {
+		cpyOpt := opt
+		if IsSecretDeploymentOption(cpyOpt) {
+			switch cpyOpt.Value.Type() {
+			case "string-array":
+				var empty clibase.StringArray
+				cpyOpt.Value = &empty
+			default:
+				var empty clibase.String
+				cpyOpt.Value = &empty
+			}
+		}
+		cpy = append(cpy, cpyOpt)
+	}
+	return cpy
+}
+
 // WithoutSecrets returns a copy of the config without secret values.
 func (c *DeploymentValues) WithoutSecrets() (*DeploymentValues, error) {
 	var ff DeploymentValues