Escape URL to render HTML

[yous]

DIR_FILE have <a href='%s'>, so single quotes in URL should be escaped.

[yous]

You can simply reproduce this problem.

mkdir test
mkdir test/foo\'bar
touch test/foo\'bar/omg\'omg.txt

config.ru:

run Rack::Directory.new('test')

rackup config.ru

When I click the link to foo'bar, it actually goes to /foo.

[tonytonyjan]

It's caused by the changed behavior of Rack::Utils#escape_path:

# rack 1.6.5
Rack::Utils.escape_path(?') # => "%27"

# rack master
Rack::Utils.escape_path(?') # => "'"

Why not patch #escape_path instead, I wonder.

[yous]

That change was came from the patch for #265. Seeing #265 (comment), ' is a valid character for path.

And actually this patch is needed only because DIR_FILE uses <a href='%s'>. If we change this to <a href="%s">, then we don't need gsub as ::URI::DEFAULT_PARSER.escape replaces " to %22. Maybe changing DIR_FILE is more proper solution. How do you think?

[tonytonyjan]

I found it's been URI escaped here, but not yet HTML escaped.

Since the #DIR_FILE_escape will return contents (url, basename, size, type, mtime) which are going to be used to render HTML here, they should be all HTML escaped already.

In my opinion, this way can be more decent:

def each
  # ...
  listings = files.map{|f| DIR_FILE % DIR_FILE_escape(f) }*"\n"
  # ...
end
# ...
def DIR_FILE_escape(html)
  html.map { |e| Utils.escape_html(e) }
end

[yous]

Yes, I think that's more proper. I just updated.

[yous]

Any update on this? I can reproduce this with rack 2.0.7.

[jeremyevans]

This change looks good to me. CI failures look unrelated. I'll push the changes as a new PR and if CI passes I'll merge it.

[jeremyevans]

Merged in #1524

[yous]

Thanks!