Repository: maragudk/gomponents
Pull request #77 (2021-05-11 17:38:22)

Escape Attr values
Because this can be a place of injection if untrusted data is passed, escape all attribute values.

This slows down rendering a bit, but we're still talking tiny increments compared to everything else.

### Before

```
$ make benchmark
go test -bench=.
goos: darwin
goarch: arm64
pkg: github.com/maragudk/gomponents
BenchmarkAttr/boolean_attributes-8         	13095948	        91.92 ns/op
BenchmarkAttr/name-value_attributes-8      	 9124303	       131.5 ns/op
BenchmarkEl/normal_elements-8              	 6384505	       187.4 ns/op
PASS
ok  	github.com/maragudk/gomponents	5.124s
```

### After

```
$ make benchmark
go test -bench=.
goos: darwin
goarch: arm64
pkg: github.com/maragudk/gomponents
BenchmarkAttr/boolean_attributes-8         	11524586	        92.07 ns/op
BenchmarkAttr/name-value_attributes-8      	 7606478	       157.0 ns/op
BenchmarkEl/normal_elements-8              	 6390315	       187.0 ns/op
PASS
ok  	github.com/maragudk/gomponents	4.000s
```

See #74.

oderwat: I think this is the right way to go. XSS is one thing, strange bugs are the other. Automagic escaping really helps to not break stuff by accident.
markuswustenberg: @oderwat thanks for the feedback. 😊 Have you seen the discussion in #74 as well? I think some of this will still be changed, I'm not sure I'm keeping attribute name escaping.
oderwat: Yes, I saw it. I actually think that the system should probably "panic" for invalid attribute names. Alternative mangle them into something valid (replacing invalid characters with an _ maybe). Valid could be everything except "whitespace" and `/>"'=`.
markuswustenberg: > Yes, I saw it. I actually think that the system should probably "panic" for invalid attribute names. Alternative mangle them into something valid (replacing invalid characters with an _ maybe). Valid could be everything except "whitespace" and `/>"'=`.

Hmm. I think I'd rather not panic, because I would be very sad as a library user if the library panicked and I didn't expect it. I think I'll hope that the developer knows what s/he is doing instead, and let them misuse the library. I think that's the least surprising thing to do.

If this turns out to be an issue, I can revisit the decision.

Patch:
diff --git a/gomponents.go b/gomponents.go
index a8b0f95..9cf4297 100644
--- a/gomponents.go
+++ b/gomponents.go
@@ -160,7 +160,7 @@ func (a *attr) Render(w io.Writer) error {
 		_, err := w.Write([]byte(" " + a.name))
 		return err
 	}
-	_, err := w.Write([]byte(" " + a.name + `="` + *a.value + `"`))
+	_, err := w.Write([]byte(" " + a.name + `="` + template.HTMLEscapeString(*a.value) + `"`))
 	return err
 }
 


Test patch:
diff --git a/gomponents_test.go b/gomponents_test.go
index c88a796..dffa64c 100644
--- a/gomponents_test.go
+++ b/gomponents_test.go
@@ -54,6 +54,11 @@ func TestAttr(t *testing.T) {
 			t.FailNow()
 		}
 	})
+
+	t.Run("escapes attribute values", func(t *testing.T) {
+		a := g.Attr(`id`, `hat"><script`)
+		assert.Equal(t, ` id="hat&#34;&gt;&lt;script"`, a)
+	})
 }
 
 func BenchmarkAttr(b *testing.B) {
@@ -132,6 +137,15 @@ func TestEl(t *testing.T) {
 	})
 }
 
+func BenchmarkEl(b *testing.B) {
+	b.Run("normal elements", func(b *testing.B) {
+		for i := 0; i < b.N; i++ {
+			e := g.El("div")
+			_ = e.Render(&strings.Builder{})
+		}
+	})
+}
+
 type erroringWriter struct{}
 
 func (w *erroringWriter) Write(p []byte) (n int, err error) {