html/template: add break and continue #23683
Comments
|
I'll send a CL. |
|
Why do the 1.10 release notes say that I've sent a CL in case we decide to go with the latter. |
|
|
ALTree
added
the
NeedsDecision
|
The fix looks small but it's not up to me so labelling this as |
|
Change https://golang.org/cl/91815 mentions this issue: |
|
/cc @ianlancetaylor |
|
It sure is unfortunate that we never tested that break and continue work in html templates. I don't believe CL 91815 is sufficient to add them. Break and continue stop the control flow midway through a range loop iteration. The flow-sensitive analysis of HTML context assumes that the loop body runs in full. It would need to be updated to take the early stop into account. I don't see an easy way to do that in the code. Go 1.10 was supposed to have shipped already. It seems to me far too late to add a change this subtle. It seems like the safest course of action is to remove break and continue from both text/template and html/template for Go 1.10, and then add them back in Go 1.11, with appropriate attention spent on html/template. Thoughts? |
|
I agree, the change to text/template should be rolled back. |
|
CL 91815 actually looks fine to me. I don't think that html/template assumes that the loop body will run in full; it escapes the range loop so that it will always produce safe output, no matter how it is executed at run-time. Specifically, it unconditionally escapes the nodes in the if and else branches of a range loop, and checks that executing the body of the range loop multiple times does not change its escaping context. I can't come up with an edge case involving continue or break that the current escaping logic will not handle. I might be missing something, though. It probably makes sense to delay this change to Go 1.11 so we have more time to consider and test this change. |
|
Change https://golang.org/cl/92155 mentions this issue: |
|
On Feb 5, 2018 2:29 PM, "Samuel Tan" ***@***.***> wrote:
CL 91815 actually looks fine to me. I don't html/template assumes that the
loop body will run in full; it escapes the range loop so that it will
always produce safe output, no matter how many times the body is executed,
if any at all. Specifically, it unconditionally escapes the nodes in the
if
<https://github.com/golang/go/blob/master/src/html/template/escape.go#L477>
and else
<https://github.com/golang/go/blob/master/src/html/template/escape.go#L493>
branches of a range loop, and checks
<https://github.com/golang/go/blob/master/src/html/template/escape.go#L479>
that executing the body of the range loop multiple times does not change
its escaping context.
I can't come up with an edge case involving continue or break that the
current escaping logic will not handle. I might be missing something,
though. It probably makes sense to delay this change to Go 1.11 so we have
more time to consider and test this change.
So it's not a problem where a loop body does something like
<script>{{if .C}}{{..}}{{else}}{{break}}{{end}}</script>
|
The new break and continue actions do not work in html/template, and
fixing them requires thinking about security issues that seem too
tricky at this stage of the release. We will try again for 1.11.
Original CL description:
text/template: add break, continue actions in ranges
Adds the two range control actions "break" and "continue". They act the
same as the Go keywords break and continue, but are simplified in that
only the innermost range statement can be broken out of or continued.
Fixes #20531
Updates #20531
Updates #23683
Change-Id: Ia7fd3c409163e3bcb5dc42947ae90b15bdf89853
Reviewed-on: https://go-review.googlesource.com/92155
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
|
Exactly. This is why we're rolling it back. |
<script>{{if .C}}{{..}}{{else}}{{break}}{{end}}</script> Ah, that makes sense. We can probably fix this by storing the context whenever we see break or continue, and comparing that context with the end-of-loop context when we get there. |
ALTree
added
NeedsFix
|
On Wed, Feb 7, 2018 at 1:25 PM, Samuel Tan ***@***.***> wrote:
<script>{{if .C}}{{..}}{{else}}{{break}}{{end}}</script>
Ah, that makes sense. We can probably fix this by storing the context
whenever we see break or continue, and comparing that context with the
end-of-loop context when we get there.
Yep. I think just treating both break and continue as a jump to end of
body would do it since the loop increment does nothing to the output
buffer, and we already reconcile the loop body end context with the loop
start context.
|
This is copied verbatim from https://go-review.googlesource.com/c/go/+/66410/. The above commit was reverted due to issues with html/template (see golang/go#23683 ). However, as we don't have such issues, there seems to be no issue in adding it to this fork. Changes pass all tests.
This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork.
This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork.
This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork.
This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork.
This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork. all: add break & continue loop actions This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork.
This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork.
This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork.
This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork.
This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork.
This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork.
This is mostly taken from https://go-review.googlesource.com/c/go/+/66410/, with some edits to support while actions. The above commit was reverted due to issues with html/template (see golang/go#23683). However, as we don't have such issues, there should be no issue adding it to this fork.
|
This was added in 1.18 |
#20531 added
breakandcontinuetotext/template, but these keywords are not available inhtml/template.Running a simple template (https://github.com/campoy/gotalks/blob/master/go1.10/template/main.go) with
html/templatefails at runtime withpanic: escaping {{continue}} is unimplementedThe text was updated successfully, but these errors were encountered: