-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
html/template: add support for template strings (backticks) #9200
Comments
I really should have seen that coming since I wrote the spec strawman: https://code.google.com/p/js-quasis-libraries-and-repl/ This doesn't actually introduce a new execution vector for client-side code since the backticks are significant inside a block of JavaScript code, not inside HTML. Automatic interpretation of '{{' inside <template> elements does that. (sigh) Will fix. |
Might also be an issue inside of JS attributes, as in this demo: https://play.golang.org/p/iahSK9oqLc - pops up alert in modern browsers |
#29406 was about malformed template output since backticks aren't parsed properly. I've re-titled this issue to be about the overall support of this ES6 feature, and closed the newer issue as a duplicate. I'm marking it as NeedsDecision, just in case anyone disagrees with adding this feature. It's part of the JS standard, so as long as |
I'm not sure if anyone else is working on this, but I have given it some thought and I thought I'd share in case anyone else has some insight. First, some notes about template literals:
I am not super-familiar with this codebase but to me there are two main issues that need to be solved:
I think if we can solve problem 1 then problem 2 is straightforward: simply replace "$" with "\x24" when inserting into a template-literal context. In the regular javascript context (including the placeholder context) any data with a backtick would already have quotes wrapped around it so it would not be considered a template literal. Solving problem 1 is a harder given the approach of the existing code, which steps through the characters of the JS code but doesn't really parse the JS. In the code, the state of being in regular Javascript code is called I implemented the above as my initial solution, but then I realized it still isn't quite right, because the placeholder expression could contain "}"s. For example, At this point it seems to me that I am thinking of a level of complexity beyond how the JS-handing code currently works. So I'm wondering if I'm missing a simpler solution, or if these template literals complicate the situation enough that it is simply necessary to have a more complex Javascript "parser" for html/template to work with them. |
/cc @empijei |
Thanks @hundt for your analysis. It seems to me that fixing this issue would require this package to implement a JS parser to keep track of the current context. If not a fully-fledged JS parser it would at least need to correctly understand the context-switch between Template Literals and JavaScript context. While this is possible, it would complicate the current logic more than I'm comfortable with, and seems very tricky to get right. I played with this a little and stuff like the following line came up: var tmpl = `"${a="`${"}}`; My opinion would be to not support this and disallow JS template literals. This would need the documentation to be updated accordingly. I have some ideas on how to do this:
I would honestly already update the docs to say that we currently do not support template literals. We also need to decide what to do for cases like
Which are valid JS function calls. @mikesamuel do you have a different opinion? @hundt and @eternal-flame-AD what is the use case for JS template literals inside html templates? (I can guess hypothetical ones but I'm interested in your concrete one if you can publish it) |
A few notes:
I think this kind of thing is pretty well handled by the internal state machine once it understands the backticks, because it already knows that characters inside quotes lose the special meaning they have outside quotes. I tried this input with my initial implementation that I mentioned above, with some logging to indicate state transitions, and it did the right thing:
Here
I'm not sure it would be necessary to explicitly handle this. As long as backticks are implemented, I believe the current code will correctly understand that the "alert" part is in the normal Javascript context and we leave that context with the backtick.
Honestly in my case it was just a case of being lazy and using an embedded script with rendering logic inside of it on a page that also used html/template to inject in some server-side variables. My experience with this issue and my later reading of #27926 has convinced me that this was a bad approach, and I refactored my code to put substantially all of the Javascript into separately loaded modules which no longer pass through any server-side templating, which is what I probably should have done in the first place. Given that it is probably not good practice to do what I had been doing, in my opinion it would be reasonable not to support HTML templates with JS template literals (and fail when it encounters them), or to support it only with template literals formatted in the subset of possible ways that html/template can understand (provided it can reliably detect and error out when it encounters an unsupported situation). To me the biggest problem is that the API currently promises "safe" HTML but does not deliver it. Better to simply fail in that situation. In case it is of interest: I gathered that Google's closure-templates library is well-regarded, so I checked what they do. Just like html/template, they do a character-by-character "context" analysis of Javascript that falls short of fully parsing it. Their approach to template literals appears to be identical to the initial implementation I described above, including the failure to deal with braces inside |
Do you want to share a link to the patched code?
I agree. Plus I see little to no reason to nest Go and JS templates. The solution I'm more inclined to implement and document is to just have a @hundt if we can get to an agreement on this would you like to work on a CL to address it? |
Ah, sorry, wasn't sure the right way to share it without requesting a PR. Here's a patch, does that work for you? https://gist.github.com/hundt/fc27e649a01722a8466987b0f102fe5d
I think if we are going to support template literals then we would need to support
I'm interested but I will likely not be available in the short term. After another day or so I will likely have little time to work on this for at least two weeks. But if you don't mind waiting I can probably pick it up then. |
In case it helps illustrate some of the issues, here are some test cases indicating potential failure modes that allow code injection: https://play.golang.org/p/61h5vn53A4H The current code fails the first, second, and fourth tests because it does not understand template literals. My patch fails the fifth test because it understands template literals but not braces inside |
Right. I didn't notice that, good catch (I reported it to the maintainers of that package). |
A recap for this issue so far. We have several possibilities in order of backward-compatibility breakage and security improvement:
All of the last four would address the "url breaks if in backticks" case. Did I miss anything? |
To be a little more precise, your solution (4) would ban interpolation/placeholders inside template literals but would allow simple template literals (like |
Updated, thanks. |
I ran into this today, took me a while to figure it out since one template with javascript backticks was impacted and another was not. It would be great if the html/template documentation mentioned that javascript template strings aren't currently supported. I can open a PR for that if it's wanted. |
Is there any objection to adding a A note in the docs, similar to what is in https://pkg.go.dev/reflect#pkg-notes, would possibly have saved me some time and frustration today until I found this open issue. Looking at the references this issue is getting, I'm sure I'm not the only one bitten by unexpected behavior (I had a URL inside a JS backtick string in a template; seemed to work in the first occurrence, but was treated as comment in the second and broke the script). |
Support for JS template strings changed with #59234 (a security fix). |
Thanks. I'm closing this issue as fixed, or avoided, by #59234. Please comment if you disagree. |
@ianlancetaylor I do not think this completely fixes the issue. Consider this program
Go's JS "state machine" incorrectly thinks that "{{.V}}" appears outside of a JS template literal and escapes it as if it were the javascript context (by just adding quotes around it). As a result the output of this program is
which, if inserted as HTML in a web page, will run the alert function. |
@hundt Thanks. |
@ianlancetaylor Should this issue be marked as Security and associated with CVE-2023-24538 |
Change https://go.dev/cl/484075 mentions this issue: |
@rolandshoemaker Since I'm interested I took a look at that CL. It looks like it sort of extends the simple state machine with one more state ("inside a string interpolation"). However, as discussed above, knowing when a string interpolation ends is not as simple as looking for the next |
Bah, that is a very good point. I keep looking for a cheat here, and it really seems like without a significantly more complex JS state machine (which I don't think we can really justify sinking time into), there isn't a clear solution here. At some point I think we probably have to draw a line, and say that, as is documented, template authors are generally trusted. We can attempt to prevent them from doing things we consider dangerous, but there is a limit to what we can prevent them from doing by working around the restrictions we put in place. I think what we have now (just disabling actions in basic template literals) and perhaps the suggested change (disallow actions in nested template literals) is perhaps enough? 🤷 |
@rolandshoemaker I'm not sure it's about trusting template authors. To me the main issue is that template authors may, without realizing it, provide a template that The initial fix makes it less likely that template authors will run into a problem, and the latest CL would presumably make it even less likely, but still there is some undocumented limit to how complex your Javascript can be before this breaks down in unpredictable ways. Personally I wouldn't want to use such a library because I wouldn't want to have to try to keep this information in my head. And indeed I did stop putting any Javascript through That all said, as far as I can tell other popular template libraries (closure, safehtml/template) use roughly the approach in your latest CL, so maybe that is generally considered a good balance. |
Yesterday, I had to help rescue a production site that broke due to the introduction of
I appreciate the security fix and all the work on this issue, and know there are no guarantees for backward compatibility for security fixes. The site would also have kept working if If I can "throw out a brick to attract jade" -- the main reason backticks were used in this case was for a multi-line string. I would be fine with no support for template literals, but would be grateful for some programmatic way of handling multi-line strings. |
We've discussed this internally and come to the conclusion that with the current state of the very basic JS parser state machine we currently have in the template package, we cannot reasonably handle the complex cases @hundt et al have brought up (as evidenced by the same issues occurring in the significantly more complex google/safehtml implementation). Rather than adding a partial fix (such as the one suggested in my CL above, in the same vein to what google/safehtml currently implements), we are leaning towards simply detecting these particular complex cases (i.e. anything beyond This will obviously break some use cases, but I think it does strike a reasonable balance between supporting the common use cases of JS template literals and preventing unsafe, hard to understand behavior (i.e. in many current cases it is likely extremely confusing how a Go action inside of a template literal will currently be treated). |
@rolandshoemaker I don't know enough to have an opinion about what we should do, but I do know that what you are describing should take the form of a new proposal. Thanks. |
by opennota:
The text was updated successfully, but these errors were encountered: