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,6 +95,85 @@ 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 (os *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 *os {
+			if have.Name == opt.Name {
+				if jValue != nil {
+					err := json.Unmarshal(jValue, &(*os)[i].Value)
+					if err != nil {
+						return xerrors.Errorf("decode option %q value: %w", have.Name, err)
+					}
+					// Set the opt's value
+					opt.Value = (*os)[i].Value
+				} else {
+					// Discard the value if it's nil.
+					opt.Value = DiscardValue
+				}
+				// Override the existing.
+				(*os)[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
+		*os = append(*os, 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...)

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(data []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/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

diff --git a/site/src/components/DeploySettingsLayout/Option.tsx b/site/src/components/DeploySettingsLayout/Option.tsx
@@ -58,21 +58,31 @@ export const OptionValue: FC<{ children?: unknown }> = ({ children }) => {
   return <span className={styles.optionValue}>{JSON.stringify(children)}</span>;
 };
 
-export const OptionConfig = (props: BoxProps) => {
+// OptionalConfig takes a source bool to indicate if the Option is the source of the configured value.
+export const OptionConfig = ({
+  source,
+  ...boxProps
+}: { source?: boolean } & BoxProps) => {
   return (
     <Box
-      {...props}
+      {...boxProps}
       sx={{
         fontSize: 13,
         fontFamily: MONOSPACE_FONT_FAMILY,
         fontWeight: 600,
-        backgroundColor: (theme) => theme.palette.background.paperLight,
+        backgroundColor: (theme) =>
+          source
+            ? theme.palette.primary.dark
+            : theme.palette.background.paperLight,
         display: "inline-flex",
         alignItems: "center",
         borderRadius: 0.25,
         padding: (theme) => theme.spacing(0, 1),
-        border: (theme) => `1px solid ${theme.palette.divider}`,
-        ...props.sx,
+        border: (theme) =>
+          `1px solid ${
+            source ? theme.palette.primary.main : theme.palette.divider
+          }`,
+        ...boxProps.sx,
       }}
     />
   );

diff --git a/site/src/components/DeploySettingsLayout/OptionsTable.tsx b/site/src/components/DeploySettingsLayout/OptionsTable.tsx
@@ -58,25 +58,25 @@ const OptionsTable: FC<{
                     }}
                   >
                     {option.flag && (
-                      <OptionConfig>
+                      <OptionConfig source={option.value_source === "flag"}>
                         <OptionConfigFlag>CLI</OptionConfigFlag>
                         --{option.flag}
                       </OptionConfig>
                     )}
                     {option.flag_shorthand && (
-                      <OptionConfig>
+                      <OptionConfig source={option.value_source === "flag"}>
                         <OptionConfigFlag>CLI</OptionConfigFlag>-
                         {option.flag_shorthand}
                       </OptionConfig>
                     )}
                     {option.env && (
-                      <OptionConfig>
+                      <OptionConfig source={option.value_source === "env"}>
                         <OptionConfigFlag>ENV</OptionConfigFlag>
                         {option.env}
                       </OptionConfig>
                     )}
                     {option.yaml && (
-                      <OptionConfig>
+                      <OptionConfig source={option.value_source === "yaml"}>
                         <OptionConfigFlag>YAML</OptionConfigFlag>
                         {option.yaml}
                       </OptionConfig>