fix: Django autoescape should be enable by default #326

gaby · 2024-01-08T05:52:07Z

The pongo2 library by default has autoescape set to True as seen here: https://github.com/flosch/pongo2/blob/master/context.go#L11 . We should do the same as this exposes the user to XSS.

Fixes #281

ReneWerner87 · 2024-01-08T14:53:44Z

@gaby
could you add a unitttest for this?
is this a breaking change?

gaby · 2024-01-08T14:56:46Z

@ReneWerner87 I'm not sure yet, the output is now escaped which broke all the unit tests.

Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-<!DOCTYPE html><html><head><title>Main</title></head><body><h2>Header</h2><h1>Hello, World!</h1><h2>Footer</h2></body></html>
+<!DOCTYPE html><html><head><title>Main</title></head><body>&lt;h2&gt;Header&lt;/h2&gt; &lt;h1&gt;Hello, World!&lt;/h1&gt; &lt;h2&gt;Footer&lt;/h2&gt;</body></html>

ReneWerner87 · 2024-01-08T15:02:23Z

Should we provide this feature as a config setting ? (otherwise its a breaking change i guess)

efectn · 2024-01-08T15:07:17Z

Should we provide this feature as a config setting ? (otherwise its a breaking change i guess)

I do agree

gaby · 2024-01-08T15:11:28Z

Ok, Will do that instead.

sixcolors · 2024-01-10T04:39:28Z

@ReneWerner87 I'm not sure yet, the output is now escaped which broke all the unit tests.

Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-<!DOCTYPE html><html><head><title>Main</title></head><body><h2>Header</h2><h1>Hello, World!</h1><h2>Footer</h2></body></html>
+<!DOCTYPE html><html><head><title>Main</title></head><body>&lt;h2&gt;Header&lt;/h2&gt; &lt;h1&gt;Hello, World!&lt;/h1&gt; &lt;h2&gt;Footer&lt;/h2&gt;</body></html>

Looks like the issue is related to {{embed}}in a layout, for example this works:

func Test_XSS(t *testing.T) {
	engine := New("./views", ".django")
	require.NoError(t, engine.Load())

	var buf bytes.Buffer
	err := engine.Render(&buf, "simple", map[string]interface{}{
		"Title": "<script>alert('XSS')</script>",
	})
	require.NoError(t, err)

	expect := `<h1>&lt;script&gt;alert(&#39;XSS&#39;)&lt;/script&gt;</h1>`
	result := trim(buf.String())
	require.Equal(t, expect, result)
}

but this does not:

func Test_XSS(t *testing.T) {
	engine := New("./views", ".django")
	require.NoError(t, engine.Load())

	var buf bytes.Buffer
	err := engine.Render(&buf, "index", map[string]interface{}{
		"Title": "<script>alert('XSS')</script>",
	}, "layouts/main")
	require.NoError(t, err)

	expect := `<!DOCTYPE html><html><head><title>Main</title></head><body><h2>Header</h2><h1>&lt;script&gt;alert(&#39;XSS&#39;)&lt;/script&gt;</h1><h2>Footer</h2></body></html>`
	result := trim(buf.String())
	require.Equal(t, expect, result)
}

sixcolors · 2024-01-10T04:43:51Z

Confirm, it has to do with {{embed}} since making the following change to views/layouts/main.django makes the previously failing test pass:

<!DOCTYPE html>
<html>

<head>
  <title>Main</title>
</head>

<body>
  {{embed | safe}}
</body>

</html>

sixcolors · 2024-01-10T05:12:51Z

The fiber/django template uses its own non-standard {{embed}} tag. I am not entirely sure why. Although I think it may have been done to create a common approach across different templating engines.

I believe that the appropriate way to do this in Django would be to use the following template code:

{% extends "layouts/main.django" %}

{% block content %}
    {% include "partials/header.django" %}
    <h1>{{ Title }}</h1>
    {% include "partials/footer.django" %}
{% endblock %}

and then this in the parent (layout):

<!DOCTYPE html>
<html>

<head>
  <title>Main</title>
</head>

<body>
  {% block content %}{% endblock %}
</body>

</html>

With that this would be the test:

func Test_XSS(t *testing.T) {
	engine := New("./views", ".django")
	require.NoError(t, engine.Load())

	var buf bytes.Buffer
	err := engine.Render(&buf, "index", map[string]interface{}{
		"Title": "<script>alert('XSS')</script>",
	})
	require.NoError(t, err)

	expect := `<!DOCTYPE html><html><head><title>Main</title></head><body><h2>Header</h2><h1>&lt;script&gt;alert(&#39;XSS&#39;)&lt;/script&gt;</h1><h2>Footer</h2></body></html>`
	result := trim(buf.String())
	require.Equal(t, expect, result)
}

By having a custom fiber way to do the same with {{embed}} and then doing pongo2.SetAutoescape(false) we have made the fiber/Django template insecure and IMO unusable.

sixcolors · 2024-01-10T05:41:08Z

@gaby a non-breaking change (eg. we still support {{embed}}) would be to replace django.go line 235 bind[e.LayoutName] = parsed with:

		// Wrokaround for custom {{embed}} tag
		// Mark the `embed` variable as safe
		// it has already been escaped above
		// e.LayoutName will be 'embed'
		safeEmbed := pongo2.AsSafeValue(parsed)
		// Add the safe value to the binding map
		bind[e.LayoutName] = safeEmbed

Then we can keep pongo2.SetAutoescape(true) and everything still works.

also recommend adding this test:

func Test_XSS(t *testing.T) {
	engine := New("./views", ".django")
	require.NoError(t, engine.Load())

	var buf bytes.Buffer
	err := engine.Render(&buf, "index", map[string]interface{}{
		"Title": "<script>alert('XSS')</script>",
	}, "layouts/main")
	require.NoError(t, err)

	expect := `<!DOCTYPE html><html><head><title>Main</title></head><body><h2>Header</h2><h1>&lt;script&gt;alert(&#39;XSS&#39;)&lt;/script&gt;</h1><h2>Footer</h2></body></html>`
	result := trim(buf.String())
	require.Equal(t, expect, result)
}

sixcolors

Please add fix on line 235 per my comment above and add XSS test.

sixcolors · 2024-01-10T06:19:05Z

@ReneWerner87 I think we should issue a CVE and GitHub security advisory for this too.

gaby · 2024-01-10T14:21:50Z

I'm not sure how to add a config variable for template engines, they are different than contrib/storage middlewares. Although I think we should just enable this by default to avoid issues in the future. The changes don't break existing functionality.

gaby · 2024-01-10T14:22:36Z

Ping @ReneWerner87 @efectn

sixcolors

While approving, we should review removing custom layout handling like {{embed}} and more directly supporting Django templates and their approach. Ref: #327

sixcolors · 2024-01-10T14:25:14Z

I'm not sure how to add a config variable for template engines, they are different than contrib/storage middlewares. However I think we should just enable this by default to avoid issues in the future. The changes don't break existing functionality.

I believe it's not a good idea to turn off AutoEscape globally. It can lead to serious issues and should be avoided.

Template authors can turn it off per block:

{% autoescape off %}
{{ "<script>alert('xss');</script>" }}
{% endautoescape %}

https://github.com/flosch/pongo2/blob/master/template_tests/autoescape.tpl

or per var:

<h1>{{ someSafeVar | safe }}</h1>
{{ "<script>"|safe }}

https://github.com/flosch/pongo2/blob/master/template_tests/filters.tpl

ReneWerner87 · 2024-01-10T14:59:09Z

I'm not sure how to add a config variable for template engines, they are different than contrib/storage middlewares. Although I think we should just enable this by default to avoid issues in the future. The changes don't break existing functionality.

@gaby
i was thinking of extending the engine config like here and adding a config property to the new function or proving a separate function to toggle auto escaping

// Create a new engine
engine := django.New("./views", ".django")
engine.SetAutoEscape(true)

@gaby @sixcolors could we add a part about escaping or security in the readme as a section for this -> best practices

The fiber/django template uses its own non-standard {{embed}} tag. I am not entirely sure why. Although I think it may have been done to create a common approach across different templating engines.

I believe that the appropriate way to do this in Django would be to use the following template code:
{% extends "layouts/main.django" %}

{% block content %}
    {% include "partials/header.django" %}
    <h1>{{ Title }}</h1>
    {% include "partials/footer.django" %}
{% endblock %}
and then this in the parent (layout):
<!DOCTYPE html>
<html>

<head>
  <title>Main</title>
</head>

<body>
  {% block content %}{% endblock %}
</body>

</html>
With that this would be the test:
func Test_XSS(t *testing.T) {
	engine := New("./views", ".django")
	require.NoError(t, engine.Load())

	var buf bytes.Buffer
	err := engine.Render(&buf, "index", map[string]interface{}{
		"Title": "<script>alert('XSS')</script>",
	})
	require.NoError(t, err)

	expect := `<!DOCTYPE html><html><head><title>Main</title></head><body><h2>Header</h2><h1>&lt;script&gt;alert(&#39;XSS&#39;)&lt;/script&gt;</h1><h2>Footer</h2></body></html>`
	result := trim(buf.String())
	require.Equal(t, expect, result)
}
By having a custom fiber way to do the same with {{embed}} and then doing pongo2.SetAutoescape(false) we have made the fiber/Django template insecure and IMO unusable.

@sixcolors yes i also think that fenny wanted a common way that works for everyone and therefore added the custom {{embed}}
if the native version doesn't have any disadvantages and does what we wanted to achieve, then we are welcome to change it

I believe it's not a good idea to turn off AutoEscape globally. It can lead to serious issues and should be avoided.

as long as it has no negative consequences, we could also activate it by default
would still add a way to disable it for this engine

so either add a specific config setting in this engine where you can control AutoEscape via setter, or as config property in another argument to the NEW functions
or in the engine core as a general setter for all engines, so that we can add this to the others later (although this may be confusing if this switch does not yet trigger anything there and is already present)

django/django.go

spelling fix

gaby · 2024-01-10T15:04:56Z

@ReneWerner87 I like this approach you suggested:

// Create a new engine
engine := django.New("./views", ".django")
engine.SetAutoEscape(true)

I will update that today and also add a note on the README.

sixcolors · 2024-01-11T00:02:22Z

@ReneWerner87 I like this approach you suggested:
// Create a new engine
engine := django.New("./views", ".django")
engine.SetAutoEscape(true)
I will update that today and also add a note on the README.

Shouldn't the default setting be secure, eg the zero value would be the secure option?

gaby · 2024-01-11T00:11:08Z

@ReneWerner87 I like this approach you suggested:
// Create a new engine
engine := django.New("./views", ".django")
engine.SetAutoEscape(true)
I will update that today and also add a note on the README.
Shouldn't the default setting be secure, eg the zero value would be the secure option?

Yes, just an example.

gaby · 2024-01-11T04:20:43Z

@sixcolors @ReneWerner87 @efectn Added support for making autoescape configurable, update docs, and added unit-tests for XSS.

sixcolors

Maybe a note about using per variable | safe or turning auto escape off in blocks of template code?

gaby · 2024-01-11T05:28:37Z

Maybe a note about using per variable | safe or turning auto escape off in blocks of template code?

Done, thanks for the suggestion.

django/README.md

django/django.go

gaby · 2024-01-11T13:40:52Z

@ReneWerner87 @efectn Updated based on last comments, should be good to merge.

Enable Django Autoescape by default

06c6256

github-actions bot added the ☢️ Bug Something isn't working label Jan 8, 2024

gaby requested review from sixcolors, ReneWerner87 and efectn January 8, 2024 05:52

sixcolors mentioned this pull request Jan 10, 2024

fiber/django template uses its own non-standard {{embed}} tag. #327

Closed

sixcolors requested changes Jan 10, 2024

View reviewed changes

Add unit-test for xss, and workaround for safe variables

d7d74b5

gaby requested a review from sixcolors January 10, 2024 14:20

sixcolors approved these changes Jan 10, 2024

View reviewed changes

ReneWerner87 reviewed Jan 10, 2024

View reviewed changes

django/django.go Outdated Show resolved Hide resolved

Update django/django.go

5be100c

spelling fix

ReneWerner87 approved these changes Jan 10, 2024

View reviewed changes

gaby added 2 commits January 10, 2024 23:02

Make autoescape configurable

3e00a49

Update django docs, add unit-test for XSS without escaping

f02c83a

sixcolors reviewed Jan 11, 2024

View reviewed changes

Add section about using Django built-in template tags

36d033b

gaby changed the title ~~fix: django autoescape should be enable by default~~ fix: Django autoescape should be enable by default Jan 11, 2024

sixcolors approved these changes Jan 11, 2024

View reviewed changes

ReneWerner87 approved these changes Jan 11, 2024

View reviewed changes

ReneWerner87 reviewed Jan 11, 2024

View reviewed changes

django/README.md Outdated Show resolved Hide resolved

efectn reviewed Jan 11, 2024

View reviewed changes

django/django.go Outdated Show resolved Hide resolved

Updates based on code review

93123c9

ReneWerner87 approved these changes Jan 11, 2024

View reviewed changes

ReneWerner87 merged commit 28cff3a into master Jan 11, 2024
16 checks passed

gaby deleted the django-autoescape branch January 11, 2024 14:01

ReneWerner87 mentioned this pull request Feb 29, 2024

🤗 [Question]: #347

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Django autoescape should be enable by default #326

fix: Django autoescape should be enable by default #326

gaby commented Jan 8, 2024

ReneWerner87 commented Jan 8, 2024

gaby commented Jan 8, 2024 •

edited

Loading

ReneWerner87 commented Jan 8, 2024

efectn commented Jan 8, 2024

gaby commented Jan 8, 2024

sixcolors commented Jan 10, 2024

sixcolors commented Jan 10, 2024

sixcolors commented Jan 10, 2024 •

edited

Loading

sixcolors commented Jan 10, 2024 •

edited

Loading

sixcolors left a comment •

edited

Loading

sixcolors commented Jan 10, 2024

gaby commented Jan 10, 2024

gaby commented Jan 10, 2024

sixcolors left a comment •

edited

Loading

sixcolors commented Jan 10, 2024 •

edited

Loading

ReneWerner87 commented Jan 10, 2024 •

edited

Loading

gaby commented Jan 10, 2024

sixcolors commented Jan 11, 2024

gaby commented Jan 11, 2024

gaby commented Jan 11, 2024 •

edited

Loading

sixcolors left a comment

gaby commented Jan 11, 2024

gaby commented Jan 11, 2024

fix: Django autoescape should be enable by default #326

fix: Django autoescape should be enable by default #326

Conversation

gaby commented Jan 8, 2024

ReneWerner87 commented Jan 8, 2024

gaby commented Jan 8, 2024 • edited Loading

ReneWerner87 commented Jan 8, 2024

efectn commented Jan 8, 2024

gaby commented Jan 8, 2024

sixcolors commented Jan 10, 2024

sixcolors commented Jan 10, 2024

sixcolors commented Jan 10, 2024 • edited Loading

sixcolors commented Jan 10, 2024 • edited Loading

sixcolors left a comment • edited Loading

Choose a reason for hiding this comment

sixcolors commented Jan 10, 2024

gaby commented Jan 10, 2024

gaby commented Jan 10, 2024

sixcolors left a comment • edited Loading

Choose a reason for hiding this comment

sixcolors commented Jan 10, 2024 • edited Loading

ReneWerner87 commented Jan 10, 2024 • edited Loading

gaby commented Jan 10, 2024

sixcolors commented Jan 11, 2024

gaby commented Jan 11, 2024

gaby commented Jan 11, 2024 • edited Loading

sixcolors left a comment

Choose a reason for hiding this comment

gaby commented Jan 11, 2024

gaby commented Jan 11, 2024

gaby commented Jan 8, 2024 •

edited

Loading

sixcolors commented Jan 10, 2024 •

edited

Loading

sixcolors commented Jan 10, 2024 •

edited

Loading

sixcolors left a comment •

edited

Loading

sixcolors left a comment •

edited

Loading

sixcolors commented Jan 10, 2024 •

edited

Loading

ReneWerner87 commented Jan 10, 2024 •

edited

Loading

gaby commented Jan 11, 2024 •

edited

Loading