Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: text/template,html/template: add ExecuteContext methods #31107

Open
empijei opened this issue Mar 28, 2019 · 15 comments

Comments

@empijei
Copy link
Contributor

commented Mar 28, 2019

This proposal is about providing a way to pass a context.Context to template execution.

Summary

Reasons:

  • Template execution cannot be gracefully interrupted, and it is a rather expensive and potentially long-running task.
  • Passing values to templates can only be achieved through the data interface{} parameter, and this makes the package not flexible enough for some purposes (expecially for html web nonces).

Advantages:

  • Provide an idiomatic way to cancel template execution.
  • Provade an idiomatic way to pass values that are not related to data being rendered, but to the context it is rendered in.

Detailed rationale

Cancellation

Templates might implement a rather complex logic, and they might import each other. It could be possible that given some specific conditions a template might run for a long time, even after the result output is not needed anymore.

Currently the only way to address this is to have a separate goroutine that closes the output io.Writer of the template.

This has three main downsides:

  • Requires a separate goroutine to run, which costs and must not be leaked
  • io.WriteCloser does not document that calling Close concurrently with Write is safe. This means that to cancel a template execution the programmer needs to wrap their io.WriteCloser in a safe one, and only then they can abort a template execution safely
  • If the programmer has only an io.Writer they need to implement a custom closing mechanism on it.

Contextual values

Sometimes data needs to be rendered differently based on the context it is rendered in.

In the following examples I'm referring to two html/template examples I struggled with:

  1. If I want to render data in a <form>, I also need to add an anti-CSRF token. This means that the template might need a value in some cases, and might not need it in others. If a toolkit or framework wants to provide protection against CSRF attacks, it must require the users to add the CSRF field to all their data structures being rendered. If the programmer misses one, that becomes a vulnerability.
  2. If an html template contains a <script> or <style> tag, they need to have a nonce in order to be executed when a strict Content Security Policy is enforced. This means that coders will have to manually pass the nonce value to all templates executing in their handlers. If a coder misses one, that breaks the service with a runtime error in the browser.

It would be nice to be able to protect an entire http.Handler in a http.ServeMux from CSRF and XSS and just add security tokens to the context. It would make it very hard for programmers to forget about security.

Example

Adding context values will make HTML templates usage from requiring this:

func handle(w http.ResponseWriter, r *http.Request) {
	data := struct {
		MyField      string
		MyOtherField string
		Nonce        string
		CSRFtok      string
	}{
		"Field",
		"OtherField",
	}
	// Retrieve nonce from r.Context()
	data.Nonce = nonce
	// Retrieve CSRFtok from r.Context()
	data.CSRFtok = csrftok
	err := tmpl.Execute(w, data)
	// Handle err
}

to this (which also handles cancellation):

func handle(w http.ResponseWriter, r *http.Request) {
	data := struct {
		MyField      string
		MyOtherField string
	}{
		"Field",
		"OtherField",
	}
	err := tmpl.ExecuteContext(r.Context(), w, data)
	// Handle err
}

Toolkits and frameworks could then provide ways to pre-parse and transform templates and add values to requests context.

Backward compatibility

This change would be backward compatible as it is about adding two func in each package:

  • (*Template).ExecuteTemplateContext(context.Context, io.Writer, string, interface{})
  • (*Template).ExecuteContext(context.Context, io.Writer, interface{})

That could add a context or ctx func to the default FuncMap. This would also be backward compatible because no-one is using ExecuteContext at the moment, and they could change their func names when migrating to this new API (only if they are using "context" as a name).
Alternatively a forbidden name (like ! or $_) could be used, but I'm not a fan of this option.

What do you think?

/cc @mvdan @esseks @mikesamuel @robpike @bradfitz @lweichselbaum

@gopherbot gopherbot added this to the Proposal milestone Mar 28, 2019
@gopherbot gopherbot added the Proposal label Mar 28, 2019
@empijei empijei changed the title proposal: text/template html/template add ExecuteWithContext functions and methods proposal: text/template html/template add ExecuteWithContext methods Mar 28, 2019
@mvdan mvdan changed the title proposal: text/template html/template add ExecuteWithContext methods proposal: text/template,html/template: add ExecuteWithContext methods Mar 28, 2019
@mvdan

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Thanks for spearheading this. I wonder - is there a reason you chose names like ExecuteWithContext instead of ExecuteContext? I think we've historically omitted With when adding context variants; for example, see net.DialContext, exec.CommandContext, or sql.DB.ExecContext.

@empijei empijei changed the title proposal: text/template,html/template: add ExecuteWithContext methods proposal: text/template,html/template: add ExecuteContext methods Mar 28, 2019
@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

It occurs to me that instead of (or perhaps as well as) adding a context entry to the function map, the function calling code could recognize functions that take context.Context as a first argument, and automatically pass the context as that argument (somewhat analogous to the way that error return values are hidden).

That way, it wouldn't be necessary to burden the template text itself with needing to know which functions expect a context argument, meaning context support could be more easily retrofitted without changing all external template text.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

@rogpeppe Regarding "the function calling code could recognize functions that take context.Context as a first argument, and automatically pass the context as that argument", we don't implicitly supply context during a function call anywhere in Go. Starting in text/template seems like an odd place. If we do add it, it seems like it should be explicit on use like everywhere else.

ExecuteTemplate is a tiny wrapper around Execute. If we add a context-aware execute we can just use one, ExecuteContext, not also ExecuteContextTemplate.

The built-in function context to return the context is the most interesting part of the proposal. How do people feel about that?

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

we don't implicitly supply context during a function call anywhere in Go

Is there anywhere else that automatically turns an error-returning function into a function that doesn't appear to return an error? AFAIK that's a convenience provided by text/template but not elsewhere, which is why I thought its dual (context is in some senses dual to error, I think) might also be appropriate there.

I can totally understand your argument too though.

@empijei

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

I agree with @rogpeppe but I have not a strong opinion on this as long as we provide an API to access context values from the templates.

@andybons

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@rsc are you saying you're in favor of having a built-in context method alongside the original function signatures described above (ExecuteContext, ExecuteTemplateContext)?

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

The last time we discussed this we were focused on the propagation of context values to invoked functions. It still seems like that would be best done explicitly. Is it a common pattern today to do something like start a template with {{$context := $.Context}} and then refer to $context throughout? That's basically what we'd be making slightly nicer.

I don't understand the cancellation part. Is the idea that the template executor would occasionally poll the context to decide whether to stop early? Is template generation really so expensive that it would happen often enough that the result is no longer needed? How often would/should the template executor be checking?

@empijei

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Is it a common pattern today to do something like start a template with {{$context := $.Context}} and then refer to $context throughout?

I haven't seen that pattern, but I've seen in a lot of places people taking some values out of ctx and enrich a struct with those values, as in my examples in the initial post. Most of the time that operation happens at every call to Execute with the same parameters, which are usually nonces, tokens or other values that are unrelated to the data being rendered but they are there to make a service secure (I'm talking about html/template here).

Is template generation really so expensive that it would happen often enough that the result is no longer needed?

I tried putting some code into the cancellation part and it makes the implementation clunky and slower than I initially thought. Since I don't have a strong case for supporting cancellation I'm ok if we were to drop that part of the proposal.

What I'm most concerned about is to have a way to easily propagate context values into templates so that it reduces friction for developers to get it right. I don't want people to have to read and pass N additional values at every Execute call to make their web applications secure.

Moreover if someone is already using a token and they want to add one more, they can just change the middleware and the templates if they use context as per this proposal. Otherwise they would need to change every call site of Execute and that is an expensive change to make.

One concrete example

Let's say I'm develping a web application and I use html/template. Since I don't want to be vulnerable to CSRF I use somethings along the lines of gorilla/csrf.
This means that every call into Execute will have this code in it:

func RenderFoo(w http.ResponseWriter, r *http.Request) {
    t.ExecuteTemplate(w, "foo.tmpl", map[string]interface{}{
        csrf.TemplateTag: csrf.TemplateField(r),
        "data": foo,
    })
}

Not only this is hard to read, it also makes templates a lot clunkier to write and use. If devs used to pass a struct in the template instead of writing {{.Foo}} they now have to use {{.data.Foo}} everywhere.

Let's now say that I want to add strict CSP to my service. I need to find all calls to Execute and add one more line to provide the nonce value to the template.

One might say that a team could just agree to never call Execute directly and go though a wrapper that will put their struct in a "data" field of a map, but that adds some cognitive overhead and would probably be different from project to project, from company to company and would make some libraries not compatible with each other. I'd like everyone to be able to share code and agree on a single way to do things.

Note

I work in Google security enhancement for the web. The way we deal with web security is to have developers write logic and templates and the security team owns part of the middleware and a wrapper around the templating library.
This way we can own the security bits so developers get security fixes and improvements without having to know all the quirks and details of modern web mitigations. We as security engineers don't want to get in the way of developers.
This proposal comes from the idea of having coders just focus on the code and write

func RenderFoo(w http.ResponseWriter, r *http.Request) {
    t.ExecuteTemplateContext(r.Context(), w, "foo.tmpl", foo)
}

And access data in the template with {{.Foo}}.
If then the security team finds a vulnerability that requires an additional quirk, they can just add a middleware that puts a token in the context and have some tooling (or pre-parse step) to change templates accordingly. The code change would be minimal, unlike the change we would currently have to go through (in my case ~3K changes every time we need to tweak something). This means that all the users of the libraries would get security improvements for free.

Since we are planning to open-source all the internal libraries to secure web applications in Go (starting from the html wrapper), it'd be nice to agree on a single way to do this kind of things. This would make it possible to have a non-fragmented ecosystem in Go web apps 😃 .

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

To summarize the discussion so far, there have been at least four different things suggested:

  1. Add an Execute method variant that takes an explicit context arg.
  2. Make the context available from a builtin "context" function.
  3. Supply the context implicitly and automatically to called methods/funcs that have a leading context.Context argument.
  4. Attempt cancellation in the template execution itself.

It sounds like 4 is quite involved and probably not necessary.
3 is not in keeping with our previous 'contexts are explicit' approach.
But it seems OK to do 1 and 2 if there is widespread need.

I'd like to hear from more template users to gauge whether having
the context built-in function would benefit a variety of use cases.

/cc @bep and feel free to add any other heavy template users. Thanks.

@bep

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

  1. Supply the context implicitly and automatically to called methods/funcs that have a leading context.Context argument.

For this to be really usable (and it really would be), you need to also do 3), which is also somehow in line with how you handle errors in templates (a method has 1 or 2 return values):

  • It would not be realistic to add context to an existing API with many users.
  • Many Go template writers are not Go programmers, and they do template work in text editors without any code completion etc., so asking them to always pass context as the first argument (with no compiler errors if they don't) is just not realistic.
@empijei

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

Looking at this feedback and considering the use case for it I'd then say that we could go for:

  1. ExecuteContext that only cares about values, not cancellation
  2. Automatic passing of context to funcs that accept it.

I'm not sure about a context built-in at this point, as it would be redundant with the auto-passing.

@rsc I'd say that this is still coherent with how we manage context in Go and idiomatic. Context must be passed to ExecuteContext, and that is the Go part. Templates are a DSL that does not resemble Go (pipes for call chains, automatic error handling, dot operator to access maps values) so implicit context passing feels more in line with it compared with the explicit approach we want for Go APIs.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Automatically supplying the context as a first argument seems like a step too far, for the reasons I gave above (it's just not in keeping with Go being explicit about context, and why is context special?).

I would feel more comfortable if we had a standard "template execution state" type that we wanted to make available to template functions and that was being passed automatically. Maybe the argument is to reuse context for that. I'm wary of overloading context too much though. I'm also wary of exposing too much template execution state (magic functions that rewrite variables etc seems like too much).

If we only do (1) and (2), I understand it won't solve everyone's problems. Does it solve anyone's problems? How many people?

@empijei

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

I would feel more comfortable if we had a standard "template execution state" type that we wanted to make available to template functions and that was being passed automatically.

Ho would this look? Do you have something in mind?

I'm wary of overloading context too much though.

Context seemed to me like the best option to be coherent with request-scoped values and other affine concepts that are already in the standard library.

I'm also wary of exposing too much template execution state (magic functions that rewrite variables etc seems like too much).

What do you mean by that? I was thinking about something way simpler, just to provide rendering-contextual values alongside with data to render.

If we only do (1) and (2), I understand it won't solve everyone's problems. Does it solve anyone's problems? How many people?

If we had a way to pass and retrieve the context or something like it (potentially the data structure you were proposing above) we could build middleware or tooling to pre-process or transform templates before they are parsed. This would address the problem in the original report with very little more work than I thought. That said I don't know about other use cases, for example what @bep was referring to.

@mikesamuel

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@rsc said

it's just not in keeping with Go being explicit about context, and why is context special?

A CSP nonce has effect because it appears in the response header, so it is scoped to the document as a whole.

Most inputs to a template are properly scoped to a substring of the body, but one way to look at context is that it is special because it relates to a small amount of per-response or per-document scoped metadata.

I'm not familiar enough with the internals to know there's a way to get auto-noncing without provisions for per-response scope, but in case you're not sold on the use case, nonce-based CSP is the way ISE recommends doing CSP.
https://csp.withgoogle.com/docs/strict-csp.html :

To enable a strict CSP policy, most applications will need to make the following changes:

  1. Add a nonce attribute to all <script> elements. Some template systems can do this automatically.
  2. Refactor any markup with inline event handlers (onclick, etc.) and javascript: URIs (details).
  3. For every page load, generate a new nonce, pass it the to the template system, and use the same value in the policy.

In ISE we think it's a bad idea for template authors to get in the habit of sprinkling nonce attributes throughout templates.
Auto-noncing is more convenient for template authors, but the effect on code reviewers is more significant.
If authors have to specify <script nonce={{.Nonce}}> where it's obvious ok, then code reviewers gloss over it as boilerplate, but if the template system automatically handles it for 99% of obviously safe cases, then it stands out in code review in the remaining cases where it's not obviously safe to add a nonce to a static analyzer.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Given @mikesamuel's comment, it seems like all the discussion of context here would be creating the wrong solution to the problem.

Perhaps instead we should be talking about how to let html/template automatically add the nonces?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.