Repository: uber-go/fx
Pull request #1051 (2023-02-28 02:38:49)

[fx-annotate] Verify invalid tags passed to fx.Annotate #835
Tags passed with fx.Annotate follow the struct format
```
fx.Annotate(f, fx.ParamTags(`group:”myGroup”`))
```
Where the tag is of the format `<tag_name>:”tag_value”` and tags are separated by space.

If a user accidentally passes an invalid tag such as mismatched quotes(#835), the issue is not identified and no error is thrown. This is not desired as the tag gets passed to dig, where it’s ignored as it’s not a valid group tag.

This commit adds checks to identify if invalid tags are passed to fx.Annotate. If identified then it throws an error with the right tag format specified. 
The following checks are now enforced -
 - tag key can only be group, optional or name ( dig only accepts these tags as valid keys currently)
 - In cases of multiple tags  they must be separated by space . Eg ``group:"some" optional:"true"``vim 
 - the tag values must be contained within double quotes.

For e.g. -
```
fx.Annotate(f, fx.ParamTags(`group:'myGroup"`))
```
Should be an invalid tag as it starts from a single quote and ends with a double quote (mismatched quotes instead of “...”) We added checks for ParamTag and ResultTag to verify and capture this invalid tag.
Now, when we pass this invalid tag we get the following error :
```
 Encountered error while applying annotation using fx.Annotate to function, 
tag value should start with double quote i.e. key:"value"
```
Components Modified
annotated.go  - add checks to verify invalid tag usage
annotated_test.go - Add test cases to verify functionality of changes
module_test.go - FIx a typo in testcase
Refs : #835

codecov[bot]: # [Codecov](https://codecov.io/gh/uber-go/fx/pull/1051?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) Report
> Merging [#1051](https://codecov.io/gh/uber-go/fx/pull/1051?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) (9dea8c4) into [master](https://codecov.io/gh/uber-go/fx/commit/1d726c7fe773533212e658563766c75bc68baa90?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) (1d726c7) will **decrease** coverage by `0.08%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
- Coverage   98.50%   98.42%   -0.08%     
==========================================
  Files          39       33       -6     
  Lines        2937     2789     -148     
==========================================
- Hits         2893     2745     -148     
  Misses         37       37              
  Partials        7        7              
```


| [Impacted Files](https://codecov.io/gh/uber-go/fx/pull/1051?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) | Coverage Δ | |
|---|---|---|
| [annotated.go](https://codecov.io/gh/uber-go/fx/pull/1051?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-YW5ub3RhdGVkLmdv) | `99.20% <100.00%> (+<0.01%)` | :arrow_up: |
| [docs/internal/apptest/run.go](https://codecov.io/gh/uber-go/fx/pull/1051?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-ZG9jcy9pbnRlcm5hbC9hcHB0ZXN0L3J1bi5nbw==) | | |
| [docs/internal/exectest/output.go](https://codecov.io/gh/uber-go/fx/pull/1051?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-ZG9jcy9pbnRlcm5hbC9leGVjdGVzdC9vdXRwdXQuZ28=) | | |
| [docs/internal/exectest/cmd.go](https://codecov.io/gh/uber-go/fx/pull/1051?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-ZG9jcy9pbnRlcm5hbC9leGVjdGVzdC9jbWQuZ28=) | | |
| [docs/internal/httptest/http.go](https://codecov.io/gh/uber-go/fx/pull/1051?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-ZG9jcy9pbnRlcm5hbC9odHRwdGVzdC9odHRwLmdv) | | |
| [docs/internal/iotest/read.go](https://codecov.io/gh/uber-go/fx/pull/1051?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-ZG9jcy9pbnRlcm5hbC9pb3Rlc3QvcmVhZC5nbw==) | | |
| [docs/internal/test/fake.go](https://codecov.io/gh/uber-go/fx/pull/1051?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-ZG9jcy9pbnRlcm5hbC90ZXN0L2Zha2UuZ28=) | | |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go)


Patch:
diff --git a/annotated.go b/annotated.go
index 2027226..516010b 100644
--- a/annotated.go
+++ b/annotated.go
@@ -144,6 +144,75 @@ type paramTagsAnnotation struct {
 }
 
 var _ Annotation = paramTagsAnnotation{}
+var (
+	errTagSyntaxSpace            = errors.New(`multiple tags are not separated by space`)
+	errTagKeySyntax              = errors.New("tag key is invalid, Use group, name or optional as tag keys")
+	errTagValueSyntaxQuote       = errors.New(`tag value should start with double quote. i.e. key:"value" `)
+	errTagValueSyntaxEndingQuote = errors.New(`tag value should end in double quote. i.e. key:"value" `)
+)
+
+// Collections of key value pairs within a tag should be separated by a space.
+// Eg: `group:"some" optional:"true"`.
+func verifyTagsSpaceSeparated(tagIdx int, tag string) error {
+	if tagIdx > 0 && tag != "" && tag[0] != ' ' {
+		return errTagSyntaxSpace
+	}
+	return nil
+}
+
+// verify tag values are delimited with double quotes.
+func verifyValueQuote(value string) (string, error) {
+	// starting quote should be a double quote
+	if value[0] != '"' {
+		return "", errTagValueSyntaxQuote
+	}
+	// validate tag value is within quotes
+	i := 1
+	for i < len(value) && value[i] != '"' {
+		if value[i] == '\\' {
+			i++
+		}
+		i++
+	}
+	if i >= len(value) {
+		return "", errTagValueSyntaxEndingQuote
+	}
+	return value[i+1:], nil
+
+}
+
+// Check whether the tag follows valid struct.
+// format and returns an error if it's invalid. (i.e. not following
+// tag:"value" space-separated list )
+// Currently dig accepts only 'name', 'group', 'optional' as valid tag keys.
+func verifyAnnotateTag(tag string) error {
+	tagIdx := 0
+	validKeys := map[string]struct{}{"group": {}, "optional": {}, "name": {}}
+	for ; tag != ""; tagIdx++ {
+		if err := verifyTagsSpaceSeparated(tagIdx, tag); err != nil {
+			return err
+		}
+		i := 0
+		if strings.TrimSpace(tag) == "" {
+			return nil
+		}
+		// parsing the key i.e. till reaching colon :
+		for i < len(tag) && tag[i] != ':' {
+			i++
+		}
+		key := strings.TrimSpace(tag[:i])
+		if _, ok := validKeys[key]; !ok {
+			return errTagKeySyntax
+		}
+		value, err := verifyValueQuote(tag[i+1:])
+		if err != nil {
+			return err
+		}
+		tag = value
+	}
+	return nil
+
+}
 
 // Given func(T1, T2, T3, ..., TN), this generates a type roughly
 // equivalent to,
@@ -159,11 +228,19 @@ var _ Annotation = paramTagsAnnotation{}
 //
 // If there has already been a ParamTag that was applied, this
 // will return an error.
+//
+// If the tag is invalid and has mismatched quotation for example,
+// (`tag_name:"tag_value') , this will return an error.
 
 func (pt paramTagsAnnotation) apply(ann *annotated) error {
 	if len(ann.ParamTags) > 0 {
 		return errors.New("cannot apply more than one line of ParamTags")
 	}
+	for _, tag := range pt.tags {
+		if err := verifyAnnotateTag(tag); err != nil {
+			return err
+		}
+	}
 	ann.ParamTags = pt.tags
 	return nil
 }
@@ -285,10 +362,18 @@ var _ Annotation = resultTagsAnnotation{}
 //
 // If there has already been a ResultTag that was applied, this
 // will return an error.
+//
+// If the tag is invalid and has mismatched quotation for example,
+// (`tag_name:"tag_value') , this will return an error.
 func (rt resultTagsAnnotation) apply(ann *annotated) error {
 	if len(ann.ResultTags) > 0 {
 		return errors.New("cannot apply more than one line of ResultTags")
 	}
+	for _, tag := range rt.tags {
+		if err := verifyAnnotateTag(tag); err != nil {
+			return err
+		}
+	}
 	ann.ResultTags = rt.tags
 	return nil
 }


Test patch:
diff --git a/annotated_test.go b/annotated_test.go
index b7d5217..39b9e5e 100644
--- a/annotated_test.go
+++ b/annotated_test.go
@@ -1474,6 +1474,131 @@ func TestAnnotate(t *testing.T) {
 	})
 }
 
+func TestAnnotateApplyFail(t *testing.T) {
+	type a struct{}
+	type b struct{ a *a }
+	newA := func() *a { return &a{} }
+	newB := func(a *a) *b {
+		return &b{a}
+	}
+
+	var (
+		errTagSyntaxSpace            = `multiple tags are not separated by space`
+		errTagKeySyntax              = "tag key is invalid, Use group, name or optional as tag keys"
+		errTagValueSyntaxQuote       = `tag value should start with double quote. i.e. key:"value" `
+		errTagValueSyntaxEndingQuote = `tag value should end in double quote. i.e. key:"value" `
+	)
+	tests := []struct {
+		give                 string
+		wantErr              string
+		giveAnnotationParam  fx.Annotation
+		giveAnnotationResult fx.Annotation
+	}{
+		{
+			give:                 "Tags value invalid ending quote",
+			wantErr:              errTagValueSyntaxEndingQuote,
+			giveAnnotationParam:  fx.ParamTags(`name:"something'`),
+			giveAnnotationResult: fx.ResultTags(`name:"something'`),
+		},
+		{
+			give:                 "Tags value wrong starting quote",
+			wantErr:              errTagValueSyntaxQuote,
+			giveAnnotationParam:  fx.ParamTags(`name:"something" optional:'true"`),
+			giveAnnotationResult: fx.ResultTags(`name:"something" optional:'true"`),
+		},
+		{
+			give:                 "Tags multiple tags not separated by space",
+			wantErr:              errTagSyntaxSpace,
+			giveAnnotationParam:  fx.ParamTags(`name:"something"group:"something"`),
+			giveAnnotationResult: fx.ResultTags(`name:"something"group:"something"`),
+		},
+		{
+			give:                 "Tags key not equal to group, name or optional",
+			wantErr:              errTagKeySyntax,
+			giveAnnotationParam:  fx.ParamTags(`name1:"something"`),
+			giveAnnotationResult: fx.ResultTags(`name1:"something"`),
+		},
+		{
+			give:                 "Tags key empty",
+			wantErr:              errTagKeySyntax,
+			giveAnnotationParam:  fx.ParamTags(`:"something"`),
+			giveAnnotationResult: fx.ResultTags(`:"something"`),
+		},
+	}
+	for _, tt := range tests {
+		t.Run("Param "+tt.give, func(t *testing.T) {
+			app := NewForTest(t,
+				fx.Provide(
+					fx.Annotate(
+						newA,
+						tt.giveAnnotationParam,
+					),
+				),
+				fx.Invoke(newB),
+			)
+			assert.ErrorContains(t, app.Err(), tt.wantErr)
+		})
+		t.Run("Result "+tt.give, func(t *testing.T) {
+			app := NewForTest(t,
+				fx.Provide(
+					fx.Annotate(
+						newA,
+						tt.giveAnnotationResult,
+					),
+				),
+				fx.Invoke(newB),
+			)
+			assert.ErrorContains(t, app.Err(), tt.wantErr)
+		})
+	}
+}
+
+func TestAnnotateApplySuccess(t *testing.T) {
+	type a struct{}
+	type b struct{ a *a }
+	newA := func() *a { return &a{} }
+	newB := func(a *a) *b {
+		return &b{a}
+	}
+
+	tests := []struct {
+		give                 string
+		giveAnnotationParam  fx.Annotation
+		giveAnnotationResult fx.Annotation
+	}{
+		{
+			give:                 "ParamTags Tag Empty",
+			giveAnnotationParam:  fx.ParamTags(`  `),
+			giveAnnotationResult: fx.ResultTags(`  `),
+		},
+		{
+			give:                 "ParamTags Tag Empty with extra spaces",
+			giveAnnotationParam:  fx.ParamTags(`name:"versionNum"`, `  `),
+			giveAnnotationResult: fx.ResultTags(`   `, `group:"versionNum"`),
+		},
+		{
+			give:                 "ParamTags Tag with \\ ",
+			giveAnnotationParam:  fx.ParamTags(`name:"version\\Num"`, `  `),
+			giveAnnotationResult: fx.ResultTags(``, `group:"version\\Num"`),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.give, func(t *testing.T) {
+			app := NewForTest(t,
+				fx.Provide(
+					fx.Annotate(
+						newA,
+						tt.giveAnnotationParam,
+						tt.giveAnnotationResult,
+					),
+				),
+				fx.Invoke(newB),
+			)
+			require.NoError(t, app.Err())
+		})
+	}
+
+}
 func assertApp(
 	t *testing.T,
 	app interface {
diff --git a/module_test.go b/module_test.go
index 8dd5eae..f8a7ace 100644
--- a/module_test.go
+++ b/module_test.go
@@ -518,8 +518,8 @@ func TestModuleFailures(t *testing.T) {
 		app := NewForTest(t,
 			fx.Module("module",
 				fx.Provide(fx.Annotate(newA,
-					fx.ParamTags(`"name:"A1"`),
-					fx.ParamTags(`"name:"A2"`),
+					fx.ParamTags(`name:"A1"`),
+					fx.ParamTags(`name:"A2"`),
 				)),
 			),
 		)