I ran into a situation with .html_safe when communicating with a fellow programmer, and discovered that the method name isn't as clear as desired.
`
.html_safe does not mean “please make this html safe”, it’s the opposite - it is you the programmer telling rails that “this string is html safe, promise!”
This can be confused by programmers, and hence be a potential security risk. A programmer should be able to read the name of a method and unambiguously be able to predict what it does, precisely.
Renaming it to: .prevent_html_escaping would make it unambiguous, and directly refer to what is actually being done (so that the programmer doesn’t have to infer what “safe” means, precisely).
Another potential name could be something like: .render_html That name avoids the double negative: “preventing” an “escaping” (even though that that is what actually happens, since rails escapes html by default)
It’s not the best name, since “render” alludes to something that should be placed in a view or template, but this method should never be called from a view,
because “Code should never call html_safe on a string unless that code constructed the string and actually ensured it’s html-safety.”, according to this blog post.
Maybe .unescape_html is the best name for it. It states what you do with the string, and not what state it should be in afterwards (“this html is safe”).
Naming methods like verbs are also more in line with method naming convention, I think.
Renaming it to: .prevent_html_escaping would make it unambiguous, and directly refer to what is actually being done (so that the programmer doesn't have to infer what "safe" means, precisely).
Yes, we should rename it to something even longer so that people don't use it
Also, this is a hugely impactful breaking change to the API. Could it have a better name? Sure. But at this point it’s probably not a good idea to change it.
Also, this is a hugely impactful breaking change to the API. Could it have a better name? Sure. But at this point it’s probably not a good idea to change it.
I’m not weighing in on whether to rename it, but on this particular argument against the rename: I don’t think this is a valid reason not to rename it. Because:
There’s an easy upgrade path: alias the method, deprecate the old name, and finally remove it in a later release.
The upgrade is simple: search and replace. There’s no logic to change.
Even after removing it, it’s easy for people to put the alias back in if it’s really that painful for them to search-and-replace their code.
Rails has a long history of making breaking changes (much bigger ones than a method rename) when the core team believes the improvement warrants it.
Brian
Every Rails developer, even beginners should be aware of #html_safe and how it works since Rails escapes all strings by default. If the developer wants to use #html_safe then there are two possibility:
- he do wants to mark the string as safe so he searched how to do it and found #html_safe. Everything is OK.
- he saw #html_safe somewhere, didn’t even try to see if strings are already escaped or not, didn’t even read the API doc for #html_safe. He was just like « Ok I’m going to put this everywhere to secure my code ! »
This second developer is dumb… Sorry. It means he didn’t even read any doc or the Rails guide. Don’t let him touch your code until he knows some basis!
I think even a beginner should be aware that Rails escapes strings for you. If he isn’t aware of that he needs some training or at least to have some read.
Rails is supposed to be developer-friendly AFAICT, so if the name is indeed misleading and makes people assume this feature is meant for something else, we should probably rename it specially given there’s a major version being worked on.
This is a great example of bikeshedding. I am not sure that was the intention however.
The method name, for me, means “this string is safe to display as-is” in HTML.
Teaching people that this method does this is a simple matter of educating those people. Changing the method name to suit those who are uneducated is not a good direction. Educating those people is the best direction.
Re-naming is an inherently bikeshed-ing prone task.
Use data to make better arguments.
Are there stack overflow posts, or blog posts, or any kinds of numbers where we can point to people actually mis-using this? I agree that the name is ambiguous, however i’m pretty sure i’ve never seen a student mis-use it.
Deprecations aren’t taken lightly as even if it is “a simple find and replace” it adds a barrier to upgrading that all add up. Not saying we should never deprecate, but we should be careful we’re deprecating things that have high impact/value. Numbers can help.
Also not saying that these problems don’t exist if we can’t find evidence of them, merely that they might not be as impactful.
I ran into a situation with .html_safe when communicating with a fellow
programmer, and discovered that the method name isn't as clear as desired.
.html_safe does not mean "please make this html safe", it's the opposite
- it is you the programmer telling rails that "this string is html safe,
promise!"
This can be confused by programmers, and hence be a potential security
risk. A programmer should be able to read the name of a method and
unambiguously be able to predict what it does, precisely.
Renaming it to: .prevent_html_escaping would make it unambiguous, and
directly refer to what is actually being done (so that the programmer
doesn't have to infer what "safe" means, precisely).
Agreed.
Another potential name could be something like: .render_html
This communicates much less what the intent is. render is extremely
ambiguous.
Maybe .unescape_html is the best name for it. It states what you *do*
with the string, and not what state it should be in afterwards ("this html
is safe").
To me, this would imply that existing escaping would be removed from the
string. My understanding is that html_safe flags the string to be exempt
from a later escaping process. If you call it on a string where html-unsafe
characters have already been escaped by any means, that string remains
unchanged. Unless I'm wrong on that, unescape is ambiguous to me in this
context.
prevent_html_escaping seems the best of your suggestions.
Renaming it to: .prevent_html_escaping would make it unambiguous, and
directly refer to what is actually being done (so that the programmer
doesn't have to infer what "safe" means, precisely).
Agreed.
Well, on second thought, I'm not so sure if I find the effort warranted.
But if the method is renamed, I think this suggestion is much better than
the others.
In years of using Rails it has never occurred to me that the name “html_safe” can be ambiguous until just now when I read this thread.
When is a developer going to stumble across this method without knowing what it means and insert it blindly into their code? If it really is ambiguous, people will look it up to double-check and the problem will be averted.
IMHO, the target audience is largely an irrelevant concern. The more important concept is making users fall into the “pit of success”. In that light, the proposed renaming makes sense.
Don’t forget, the clearer the intention of the method the better it is for both novice and experienced programmers.
The main reason is: it should not be used by end users. It is an implementation detail of the framework and although it is exposed to the users we don’t recommend to use it directly.
So when Andrew said “we should rename it to something even longer so that people don’t use it” it was a joke, but just a half joke. We really don’t want to people to use it.
Almost 90% of the cases that people use html_safe what they really wanted is sanitize. Also, some day HTML escaping implementation could change html_safe be completely gone.
This is why we have the raw method, to expose a public API to disable the HTML escaping without exposing the implementation detail.
So I don’t think we should rename an “internal” method to make it explicit to end users.
Yeah. This is one of the example of things that were supposed to be implementation detail and became a widely used method, just like arel_table. It is not that you can’t use it at all, but that you should be very careful when using it. Like I said almost 90% of its usages can be avoided using sanitize or the proper tag helpers.
I agree we should improve the education about this subject.
I do agree with you; however, the same argument could be made for renaming it as well. If it is a private implementation concern and nobody should be using it, then there should be no problems with a rename to make sure that the name is scary enough to stop people from using it and/or break applications that are using it.
In our opinion this is will cause an unnecessary amount of work to people upgrading their applications. We want to avoid adding breaking changes/deprecation if we can.
