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

Should ECR provide support for escaping rendered output? #15078

Open
robacarp opened this issue Oct 11, 2024 · 3 comments
Open

Should ECR provide support for escaping rendered output? #15078

robacarp opened this issue Oct 11, 2024 · 3 comments

Comments

@robacarp
Copy link
Contributor

The docs for ECR state:

Embedded Crystal (ECR) is a template language for embedding Crystal code into other text, that includes but is not limited to HTML.

A naïve user (me) might assume that ECR is smart enough to escape syntax so that it doesn't end up causing an XSS vector on a HTML based project. My workaround for this has been laborious, and it includes manually sanitizing anything being rendered with a call to HTML.escape. The workflow is error prone because it requires me to remember to be a good steward of user input. In todays world, web frameworks have evolved to do this for me because it's been shown over and over again that I will fail to remember to do this 100% of the time.

This has led me to consider a bunch of web specific templating languages. Unfortunately there's a lot of bit-rot and abandoned projects in the shardverse, and I'm skiddish to attach my project to another which would require a rewrite when the author no longer has time for a passion project. So I keep coming back to ECR because it's built-in to crystal.

Would it be possible to upgrade ECR so that it can take a sanitation helper?

I'm humbly attempting to survey the ECR Processor for an easy victory. It seems like maybe wrapping this buffer_name in a call to a theoretical user_escape() method would suffice -- or maybe it would be elsewhere, I can't tell.

I'm inspired by the syntax for Log -- and I love it. Log = ::Log.for(self) is clean and elegant.

I'm imagining something like this, which would require a broader rework of ECR, but could be really powerful:

HtmlSafeECR = ECR.with_escape do |string|
  HTML.escape string
end

HtmlSafeECR.render template.ecr

ECR.with_escape would capture the block and apply it every time a <%= %> sequence is evaluated, providing a safe default render macro.

Instructing the escape block that the string is already escaped and shouldn't be handled gets tricky. Perl had taint and I remember using that to accomplish this on some protoframework. Ruby has both taint and trust -- though Rails implements something completely differenthttps://api.rubyonrails.org/classes/ActiveSupport/SafeBuffer.html) for it's usage of ERB.

What do you think?

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Oct 11, 2024

Duplicate of #13753?

NVM

@robacarp
Copy link
Contributor Author

Another approach which might be more helpful is to make ECR a generic, and the T is the buffer. So the current implementation is:

module ECR
  macro render(filename)
    ::String.build do |%io|
      ::ECR.embed({{filename}}, %io)
    end
  end
end

But it could also work like this:

module ECR(BufferClass)
  macro render(filename)
    BufferClass.build do |%io|
      ::ECR.embed({{filename}}, %io)
    end
  end
end

BufferClass must then have a build method, which yields an object (a buffer ingest object). The buffer ingest object must respond to <<.

That would allow more in depth inspection of safe/unsafe strings through some out-of-band syntax. The buffer ingest object could be made responsible for deciding what needs to be escaped and what doesn't.

@robacarp
Copy link
Contributor Author

Also somewhat related is that ERB (or Rail's extension of ERB) has a <%== operator which doesn't run the html sanitation step. It's subtle, maybe even too subtle. Enough that there are suggestions to avoid it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants