Skip to content

Commit a71aedf

Browse files
committed
fix: address Copilot review comments for auto-escape feature
- Fix io.Writer contract violation in replacerWriter.Write() The Write method now correctly returns len(p) as required by io.Writer interface, instead of returning the transformed string length - Fix potential nil panic in AddSafeFilter() Call ensureMapIsCreated() before checking c.filters["safe"] to prevent nil map access These issues were identified in PR #111 review comments but were intentionally merged without fixes in order to reduce the chance of a conflict with another incoming PR. This commit addresses both concerns to ensure proper runtime behavior.
1 parent 27634db commit a71aedf

4 files changed

Lines changed: 73 additions & 2 deletions

File tree

expressions/filters.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,8 @@ func (c *Config) AddFilter(name string, fn any) {
5757
}
5858

5959
func (c *Config) AddSafeFilter() {
60+
c.ensureMapIsCreated()
6061
if c.filters["safe"] == nil {
61-
c.ensureMapIsCreated()
6262
c.filters["safe"] = func(in interface{}) interface{} {
6363
if in, alreadySafe := in.(values.SafeValue); alreadySafe {
6464
return in

expressions/filters_test.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,3 +71,37 @@ func TestContext_runFilter(t *testing.T) {
7171
require.NoError(t, err)
7272
require.Equal(t, "(self, 11)", out)
7373
}
74+
75+
// TestAddSafeFilterNilMap verifies that AddSafeFilter doesn't panic
76+
// when called on a Config with nil filters map
77+
func TestAddSafeFilterNilMap(t *testing.T) {
78+
// Create a config without initializing filters map
79+
cfg := &Config{}
80+
81+
// This should not panic even though filters map is nil
82+
require.NotPanics(t, func() {
83+
cfg.AddSafeFilter()
84+
}, "AddSafeFilter should not panic with nil filters map")
85+
86+
// Verify the safe filter was added
87+
require.NotNil(t, cfg.filters)
88+
require.NotNil(t, cfg.filters["safe"])
89+
90+
// Test that calling AddSafeFilter again doesn't duplicate
91+
cfg.AddSafeFilter()
92+
require.NotNil(t, cfg.filters["safe"])
93+
94+
// Test the safe filter works correctly
95+
safeFilter := cfg.filters["safe"].(func(interface{}) interface{})
96+
97+
// Test with regular value
98+
result := safeFilter("test")
99+
safeVal, ok := result.(values.SafeValue)
100+
require.True(t, ok, "Should return SafeValue")
101+
require.Equal(t, "test", safeVal.Value)
102+
103+
// Test with already safe value
104+
alreadySafe := values.SafeValue{Value: "already safe"}
105+
result2 := safeFilter(alreadySafe)
106+
require.Equal(t, alreadySafe, result2, "Should return the same SafeValue if already safe")
107+
}

render/autoescape_test.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,3 +73,36 @@ func TestRenderEscapeFilter(t *testing.T) {
7373
)
7474
})
7575
}
76+
77+
// TestReplacerWriterIOContract verifies that replacerWriter.Write correctly
78+
// implements the io.Writer contract by returning len(p)
79+
func TestReplacerWriterIOContract(t *testing.T) {
80+
buf := new(bytes.Buffer)
81+
rw := &replacerWriter{
82+
replacer: HtmlEscaper,
83+
w: buf,
84+
}
85+
86+
// Test with input that gets escaped (different output length)
87+
input := []byte("<script>")
88+
n, err := rw.Write(input)
89+
require.NoError(t, err)
90+
require.Equal(t, len(input), n, "Write must return len(p) per io.Writer contract")
91+
require.Equal(t, "&lt;script&gt;", buf.String(), "Content should be escaped")
92+
93+
// Test with normal input (same output length)
94+
buf.Reset()
95+
input2 := []byte("hello world")
96+
n2, err2 := rw.Write(input2)
97+
require.NoError(t, err2)
98+
require.Equal(t, len(input2), n2, "Write must return len(p) for normal input")
99+
require.Equal(t, "hello world", buf.String())
100+
101+
// Test with empty input
102+
buf.Reset()
103+
input3 := []byte("")
104+
n3, err3 := rw.Write(input3)
105+
require.NoError(t, err3)
106+
require.Equal(t, 0, n3, "Write must return 0 for empty input")
107+
require.Equal(t, "", buf.String())
108+
}

render/render.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,11 @@ type replacerWriter struct {
164164
}
165165

166166
func (h *replacerWriter) Write(p []byte) (n int, err error) {
167-
return h.WriteString(string(p))
167+
_, err = h.WriteString(string(p))
168+
if err != nil {
169+
return 0, err
170+
}
171+
return len(p), nil
168172
}
169173

170174
func (h *replacerWriter) WriteString(s string) (n int, err error) {

0 commit comments

Comments
 (0)