refactoring of arguments generation (#387)

* refactoring of arguments generation

* simplify Add* signatures of Args struct

* add doc comments

* add unit tests

* resolving nitpicking comment

* parallelize more tests

config/confidential.go

@@ -15,6 +15,10 @@ type ConfidentialValue struct {
 	public, confidential string
 }
 
+func (c ConfidentialValue) HasValue() bool {
+	return c.confidential != ""
+}
+
 // Value returns the unmasked original value
 func (c ConfidentialValue) Value() string {
 	return c.confidential
@@ -206,19 +210,8 @@ func GetNonConfidentialArgs(profile *Profile, args *shell.Args) *shell.Args {
 		return nil
 	}
 
-	target := args.Clone()
-	confidentials := getAllConfidentialValues(profile)
+	modifier := shell.NewConfidentialArgModifier()
+	newArgs := args.Modify(modifier)
 
-	target.Walk(func(name string, arg *shell.Arg) *shell.Arg {
-		if arg.HasValue() {
-			value := convertToNonConfidential(confidentials, arg.Value())
-			if value != arg.Value() {
-				a := shell.NewArg(value, arg.Type())
-				return &a
-			}
-		}
-		return arg
-	})
-
-	return target
+	return newArgs
 }

config/confidential_test.go

@@ -26,6 +26,8 @@ var (
 )
 
 func TestConfidentialHideAll(t *testing.T) {
+	t.Parallel()
+
 	value := NewConfidentialValue("val")
 
 	assert.Equal(t, value.String(), value.Value())
@@ -37,6 +39,8 @@ func TestConfidentialHideAll(t *testing.T) {
 }
 
 func TestConfidentialHideSubmatch(t *testing.T) {
+	t.Parallel()
+
 	value := NewConfidentialValue("some-vAl-with-sEcRet-parts")
 
 	assert.Equal(t, value.String(), value.Value())
@@ -50,6 +54,8 @@ func TestConfidentialHideSubmatch(t *testing.T) {
 }
 
 func TestUpdateConfidentialValue(t *testing.T) {
+	t.Parallel()
+
 	v := NewConfidentialValue("abc")
 
 	v.setValue("xyz")
@@ -65,6 +71,8 @@ func TestUpdateConfidentialValue(t *testing.T) {
 }
 
 func TestFmtStringDoesntLeakConfidentialValues(t *testing.T) {
+	t.Parallel()
+
 	value := NewConfidentialValue("secret")
 	value.hideValue()
 
@@ -75,6 +83,8 @@ func TestFmtStringDoesntLeakConfidentialValues(t *testing.T) {
 }
 
 func TestStringifyPassesConfidentialValues(t *testing.T) {
+	t.Parallel()
+
 	value := NewConfidentialValue("secret")
 	value.hideValue()
 
@@ -85,6 +95,8 @@ func TestStringifyPassesConfidentialValues(t *testing.T) {
 }
 
 func TestConfidentialURLs(t *testing.T) {
+	t.Parallel()
+
 	// https://restic.readthedocs.io/en/latest/030_preparing_a_new_repo.html
 	urls := map[string]string{
 		"local:some/path":                                      "-",
@@ -122,6 +134,8 @@ repository = "%s"
 }
 
 func TestConfidentialEnvironment(t *testing.T) {
+	t.Parallel()
+
 	// https://restic.readthedocs.io/en/latest/030_preparing_a_new_repo.html
 	vars := map[string]string{
 		"MY_VALUE": "-",
@@ -194,6 +208,8 @@ func TestConfidentialEnvironment(t *testing.T) {
 }
 
 func TestShowConfigHidesConfidentialValues(t *testing.T) {
+	t.Parallel()
+
 	testConfig := `
 profile:
   repository: "local:user:pass@host"
@@ -223,6 +239,8 @@ profile:
 }
 
 func TestGetNonConfidentialValues(t *testing.T) {
+	t.Parallel()
+
 	testConfig := `
 profile:
   verbose: false
@@ -263,18 +281,51 @@ profile:
 }
 
 func TestGetNonConfidentialArgs(t *testing.T) {
+	t.Parallel()
+
 	repo := "local:user:%s@host/path with space"
 	testConfig := `
+global:
+  prevent-auto-repository-file: true
+profile:
+  repository: "` + fmt.Sprintf(repo, "password") + `"
+`
+	profile, err := getProfile("yaml", testConfig, "profile", "")
+	assert.NoError(t, err)
+	assert.NotNil(t, profile)
+
+	args := profile.GetCommandFlags(constants.CommandBackup)
+	result := GetNonConfidentialArgs(profile, args)
+
+	expectedSecret := shell.NewArg(fmt.Sprintf(repo, "password"), shell.ArgConfigEscape).String()
+	expectedPublic := shell.NewArg(fmt.Sprintf(repo, ConfidentialReplacement), shell.ArgConfigEscape).String()
+
+	assert.Equal(t, []string{"--repo=" + expectedSecret}, args.GetAll())
+	assert.Equal(t, []string{"--repo=" + expectedPublic}, result.GetAll())
+}
+
+func TestGetNonConfidentialArgsFromEnvironmentVariable(t *testing.T) {
+	t.Parallel()
+
+	if platform.IsWindows() {
+		t.Skip()
+	}
+	repo := "local:user:%s@host/path with space"
+	environment := []string{
+		"MY_REPOSITORY=" + fmt.Sprintf(repo, "password"),
+	}
+	testConfig := `
       global:
         prevent-auto-repository-file: true
       profile:
-        repository: "` + fmt.Sprintf(repo, "password") + `"
+        repository: $MY_REPOSITORY
       `
 	profile, err := getProfile("yaml", testConfig, "profile", "")
 	assert.NoError(t, err)
 	assert.NotNil(t, profile)
 
 	args := profile.GetCommandFlags(constants.CommandBackup)
+	args = args.Modify(shell.NewExpandEnvModifier(environment))
 	result := GetNonConfidentialArgs(profile, args)
 
 	expectedSecret := shell.NewArg(fmt.Sprintf(repo, "password"), shell.ArgConfigEscape).String()
@@ -376,6 +427,8 @@ func TestGetAutoRepositoryFileDisabledWithEnv(t *testing.T) {
 }
 
 func TestConfidentialToJSON(t *testing.T) {
+	t.Parallel()
+
 	t.Run("marshal", func(t *testing.T) {
 		value := NewConfidentialValue("plain")
 		assert.False(t, value.IsConfidential())
@@ -391,6 +444,8 @@ func TestConfidentialToJSON(t *testing.T) {
 	})
 
 	t.Run("unmarshal", func(t *testing.T) {
+		t.Parallel()
+
 		value := NewConfidentialValue("")
 		value.hideValue()
 		assert.True(t, value.IsConfidential())

config/flag.go

@@ -28,7 +28,7 @@ var allowedEmptyValueArgs = []string{
 func tryAddEmptyArg(args *shell.Args, name string, value any) bool {
 	sv, ok := value.(string)
 	if ok && sv == "" && slices.Contains(allowedEmptyValueArgs, name) {
-		args.AddFlag(name, shell.EmptyArgValue(), shell.NewEmptyValueArg().Type())
+		args.AddFlag(name, shell.NewEmptyValueArg())
 		return true
 	}
 	return false
@@ -49,13 +49,13 @@ func addArgsFromStruct(args *shell.Args, section any) {
 			if argument != "" {
 				convert, ok := stringifyConfidentialValue(valueOf.Field(i))
 				if ok {
-					argType := shell.ArgConfigEscape
-					// check if the argument type was specified
-					rawType, ok := field.Tag.Lookup("argument-type")
-					if ok && rawType == "no-glob" {
-						argType = shell.ArgConfigKeepGlobQuote
+					// FIXME quick hack for now
+					isConfidential := valueOf.Field(i).Type() == reflect.TypeOf(ConfidentialValue{})
+					flags := make([]shell.Arg, 0, len(convert))
+					for _, v := range convert {
+						flags = append(flags, shell.NewArg(v, getArgType(field), shell.NewConfidentialArgOption(isConfidential)))
 					}
-					args.AddFlags(argument, convert, argType)
+					args.AddFlags(argument, flags)
 				} else if len(convert) == 1 {
 					_ = tryAddEmptyArg(args, argument, convert[0])
 				}
@@ -89,13 +89,23 @@ func addArgsFromMap(args *shell.Args, argAliases map[string]string, argsMap map[
 			name = targetName
 		}
 		if convert, ok := stringifyValueOf(value); ok {
-			args.AddFlags(name, convert, shell.ArgConfigEscape)
+			args.AddFlags(name, shell.NewArgsSlice(convert, shell.ArgConfigEscape))
 		} else {
 			_ = tryAddEmptyArg(args, name, value)
 		}
 	}
 }
 
+func getArgType(field reflect.StructField) shell.ArgType {
+	argType := shell.ArgConfigEscape
+	// check if the argument type was specified
+	rawType, ok := field.Tag.Lookup("argument-type")
+	if ok && rawType == "no-glob" {
+		argType = shell.ArgConfigKeepGlobQuote
+	}
+	return argType
+}
+
 // stringifyValueOf returns a string representation of the value, and if it has any value at all
 func stringifyValueOf(value interface{}) ([]string, bool) {
 	return stringifyAnyValueOf(value, true)

config/profile.go

@@ -68,7 +68,6 @@ type Profile struct {
 	RunShellCommandsSection `mapstructure:",squash"`
 	OtherFlagsSection       `mapstructure:",squash"`
 	config                  *Config
-	legacyArg               bool
 	resticVersion           *semver.Version
 	Name                    string
 	Description             string                            `mapstructure:"description" description:"Describes the profile"`
@@ -420,7 +419,7 @@ func (s *CopySection) getCommandFlags(profile *Profile) (flags *shell.Args) {
 		// restic < 0.14: repo2, password-file2, etc. is the destination, repo, password-file, etc. the source
 		for name, value := range repositoryArgs {
 			if len(value) > 0 {
-				flags.AddFlag(fmt.Sprintf("%s2", name), value, shell.ArgConfigEscape)
+				flags.AddFlag(fmt.Sprintf("%s2", name), shell.NewArg(value, shell.ArgConfigEscape))
 			}
 		}
 	} else {
@@ -430,7 +429,7 @@ func (s *CopySection) getCommandFlags(profile *Profile) (flags *shell.Args) {
 		}
 		for name, value := range repositoryArgs {
 			if len(value) > 0 {
-				flags.AddFlag(name, value, shell.ArgConfigEscape)
+				flags.AddFlag(name, shell.NewArg(value, shell.ArgConfigEscape))
 			}
 		}
 	}
@@ -604,11 +603,6 @@ func (p *Profile) ResolveConfiguration() {
 	}
 }
 
-// SetLegacyArg is used to activate the legacy (broken) mode of sending arguments on the restic command line
-func (p *Profile) SetLegacyArg(legacy bool) {
-	p.legacyArg = legacy
-}
-
 // SetResticVersion sets the effective restic version for validation and to determine how to format flags.
 // Note that flags filtering happens later inside resticWrapper and is not necessary inside the profile.
 func (p *Profile) SetResticVersion(resticVersion string) (err error) {
@@ -819,7 +813,7 @@ func (p *Profile) GetCommonFlags() (flags *shell.Args) {
 	defer restore()
 
 	// Flags from the profile fields
-	flags = shell.NewArgs().SetLegacyArg(p.legacyArg)
+	flags = shell.NewArgs()
 	addArgsFromStruct(flags, p)
 	addArgsFromOtherFlags(flags, p, p)
 	return flags

config/profile_test.go

@@ -1572,6 +1572,6 @@ func TestGetCopyStructFields(t *testing.T) {
 		p := &(*profile)
 		assert.Nil(t, p.GetCopyInitializeFlags())
 		p.Copy = copy
-		assert.Equal(t, copy.getInitFlags(p), p.GetCopyInitializeFlags())
+		assert.Equal(t, copy.getInitFlags(p).GetAll(), p.GetCopyInitializeFlags().GetAll())
 	})
 }

context.go

@@ -34,6 +34,7 @@ type Context struct {
 	stopOnBattery int              // stop if running on battery
 	noLock        bool             // skip profile lock file
 	lockWait      time.Duration    // wait up to duration to acquire a lock
+	legacyArgs    bool             // I'm not even sure it's been used by anyone?
 }
 
 func CreateContext(flags commandLineFlags, global *config.Global, cfg *config.Config, ownCommands *OwnCommands) (*Context, error) {
@@ -63,6 +64,7 @@ func CreateContext(flags commandLineFlags, global *config.Global, cfg *config.Co
 		sigChan:       nil,
 		logTarget:     global.Log, // default to global (which can be empty)
 		commandOutput: global.CommandOutput,
+		legacyArgs:    global.LegacyArguments, // use the broken arguments escaping (before v0.15.0)
 	}
 	// own commands can check the context before running
 	if ownCommands.Exists(command, true) {

examples/dev.yaml

@@ -118,11 +118,13 @@ self:
         schedule-ignore-on-battery: true
         schedule-after-network-online: true
 
+    run-before:
+        - "echo DOW=`date +\"%u\"` >> {{ env }}"
     run-after-fail:
         - "echo restic returned an error, command line = ${ERROR_COMMANDLINE}"
         - "echo restic stderr = ${RESTIC_STDERR}"
     check:
-        read-data: true
+        read-data-subset: "$DOW/7"
         schedule:
             - "*:15"
     retention:
@@ -140,6 +142,7 @@ self:
     snapshots:
         host: true
         run-before:
+          - "env"
           - "echo STEP=Before >> {{ env }}"
           - "echo \"$STEP ($Step)\""
         run-after:

integration_test.go

@@ -155,22 +155,21 @@ func TestFromConfigFileToCommandLine(t *testing.T) {
 					continue
 				}
 
-				// legacy test
+				// legacy args test
 				t.Run(fixture.profileName+"/"+fixture.commandName+"/legacy", func(t *testing.T) {
 					profile, err := cfg.GetProfile(fixture.profileName)
 					require.NoError(t, err)
 					require.NotNil(t, profile)
 
-					profile.SetLegacyArg(true)
-
 					ctx := &Context{
 						request: Request{
 							profile:   fixture.profileName,
 							arguments: fixture.cmdlineArgs,
 						},
-						binary:  echoBinary,
-						profile: profile,
-						command: fixture.commandName,
+						binary:     echoBinary,
+						profile:    profile,
+						command:    fixture.commandName,
+						legacyArgs: true,
 					}
 					wrapper := newResticWrapper(ctx)
 					buffer := &bytes.Buffer{}

main.go

@@ -495,11 +495,6 @@ func runProfile(ctx *Context) error {
 		changeLevelFilter(clog.LevelDebug)
 	}
 
-	// use the broken arguments escaping (before v0.15.0)
-	if ctx.global.LegacyArguments {
-		profile.SetLegacyArg(true)
-	}
-
 	// tell the profile what version of restic is in use
 	if e := profile.SetResticVersion(ctx.global.ResticVersion); e != nil {
 		clog.Warningf("restic version %q is no valid semver: %s", ctx.global.ResticVersion, e.Error())

mask/mask.go

@@ -0,0 +1,24 @@
+package mask
+
+import "regexp"
+
+const maskReplacement = "×××"
+
+var (
+	// httpHeaderNames     = regexp.MustCompile("(?i)^(Authorization)$")
+	RepositoryConfidentialPart = regexp.MustCompile("[:/][^:/@]+?:([^:@]+?)@[^:/@]+?") // user:pass@host
+	// urlEnvKeys          = regexp.MustCompile("(?i)^.+(_AUTH|_URL)$")
+	// hiddenEnvKeys       = regexp.MustCompile("(?i)^(.+_KEY|.+_TOKEN|.*PASSWORD.*|.*SECRET.*)$")
+)
+
+func Submatches(pattern *regexp.Regexp, value string) string {
+	if matches := pattern.FindStringSubmatchIndex(value); len(matches) > 2 {
+		for i := len(matches) - 2; i > 1; i -= 2 {
+			start := matches[i]
+			end := matches[i+1]
+
+			value = value[0:start] + maskReplacement + value[end:]
+		}
+	}
+	return value
+}