security: add slug validation, username validation, and template escaping
- Enforce SLUG_REGEX (alphanumeric + dot/dash/underscore only) in creation_error to prevent path traversal (C-1) and stored XSS (C-3). - Add File.expand_path containment check in default_path_for as defense-in-depth. - Escape slug in form action attributes (list, form views). - Validate username in change_username using the same regex as create_user (C-4). - Escape username in settings form action attributes. - Treat submitting the current username as a no-op success instead of 'taken'.
This commit is contained in:
@@ -17,6 +17,7 @@ module Volumen
|
||||
class Server < Sinatra::Base
|
||||
MAX_LOGIN_ATTEMPTS = 10
|
||||
LOGIN_WINDOW = 60 # seconds
|
||||
SLUG_REGEX = /\A[a-z0-9](?:[a-z0-9._-]*[a-z0-9])?\z/
|
||||
|
||||
register ApiRoutes
|
||||
register AdminRoutes
|
||||
@@ -260,6 +261,11 @@ module Volumen
|
||||
def change_username
|
||||
new_name = presence(params["username"])
|
||||
return render_settings(error: "Username cannot be empty.") if new_name.nil?
|
||||
unless new_name.match?(/\A[a-zA-Z0-9._-]+\z/)
|
||||
return render_settings(error: "Username may use letters, numbers, dot, dash, underscore.")
|
||||
end
|
||||
|
||||
return render_settings(notice: "Username unchanged.") if new_name == current_user
|
||||
|
||||
if users.rename(current_user, new_name)
|
||||
session[:user] = new_name
|
||||
@@ -335,6 +341,7 @@ module Volumen
|
||||
|
||||
def creation_error(post)
|
||||
return "Slug is required." if presence(post.slug).nil?
|
||||
return "Invalid slug." unless post.slug.match?(SLUG_REGEX)
|
||||
return "A post with that slug already exists." if store.find(post.slug)
|
||||
|
||||
nil
|
||||
|
||||
@@ -85,7 +85,12 @@ module Volumen
|
||||
end
|
||||
|
||||
def default_path_for(post)
|
||||
File.join(@content_dir, "#{post.slug}.md")
|
||||
target = File.join(@content_dir, "#{post.slug}.md")
|
||||
unless File.expand_path(target).start_with?(File.expand_path(@content_dir) + File::SEPARATOR)
|
||||
raise ArgumentError, "slug escapes content directory"
|
||||
end
|
||||
|
||||
target
|
||||
end
|
||||
|
||||
def safe_media_name(original)
|
||||
|
||||
@@ -5,7 +5,7 @@
|
||||
<% end %>
|
||||
|
||||
<form id="post-form" method="post"
|
||||
action="<%= @mode == :new ? to("/admin/posts") : to("/admin/posts/#{@post.slug}") %>">
|
||||
action="<%= @mode == :new ? to("/admin/posts") : h(to("/admin/posts/#{@post.slug}")) %>">
|
||||
<input type="hidden" name="_csrf" value="<%= csrf_token %>">
|
||||
|
||||
<div class="fields">
|
||||
|
||||
@@ -19,9 +19,9 @@
|
||||
<td><%= h(post.date_string) %></td>
|
||||
<td><%= post.draft? ? "yes" : "" %></td>
|
||||
<td class="actions">
|
||||
<a href="<%= to("/admin/posts/#{post.slug}/edit") %>">Edit</a>
|
||||
<a href="<%= h(to("/admin/posts/#{post.slug}/edit")) %>">Edit</a>
|
||||
<form class="inline" method="post"
|
||||
action="<%= to("/admin/posts/#{post.slug}/delete") %>"
|
||||
action="<%= h(to("/admin/posts/#{post.slug}/delete")) %>"
|
||||
onsubmit="return confirm('Delete this post?');">
|
||||
<input type="hidden" name="_csrf" value="<%= csrf_token %>">
|
||||
<button type="submit" class="danger">Delete</button>
|
||||
|
||||
@@ -41,7 +41,7 @@
|
||||
<%= h(user.role) %>
|
||||
<% else %>
|
||||
<form class="inline" method="post"
|
||||
action="<%= to("/admin/settings/users/#{user.username}/role") %>">
|
||||
action="<%= h(to("/admin/settings/users/#{user.username}/role")) %>">
|
||||
<input type="hidden" name="_csrf" value="<%= csrf_token %>">
|
||||
<select name="role" onchange="this.form.submit()">
|
||||
<% Volumen::Users::ROLES.each do |role| %>
|
||||
@@ -54,7 +54,7 @@
|
||||
<td class="actions">
|
||||
<% unless user.username == current_user %>
|
||||
<form class="inline" method="post"
|
||||
action="<%= to("/admin/settings/users/#{user.username}/delete") %>"
|
||||
action="<%= h(to("/admin/settings/users/#{user.username}/delete")) %>"
|
||||
onsubmit="return confirm('Remove this user?');">
|
||||
<input type="hidden" name="_csrf" value="<%= csrf_token %>">
|
||||
<button type="submit" class="danger">Remove</button>
|
||||
|
||||
@@ -229,6 +229,40 @@ class AdminTest < Minitest::Test
|
||||
"sweep should remove IPs with only stale entries"
|
||||
end
|
||||
|
||||
def test_create_post_rejects_invalid_slug
|
||||
login
|
||||
get "/admin/posts/new"
|
||||
post "/admin/posts",
|
||||
"_csrf" => csrf(last_response.body), "title" => "X", "slug" => "../../etc/passwd",
|
||||
"lang" => "en", "body" => "y"
|
||||
assert_includes last_response.body, "Invalid slug"
|
||||
end
|
||||
|
||||
def test_create_post_rejects_xss_in_slug
|
||||
login
|
||||
get "/admin/posts/new"
|
||||
post "/admin/posts",
|
||||
"_csrf" => csrf(last_response.body), "title" => "X", "slug" => "foo\" onclick",
|
||||
"lang" => "en", "body" => "y"
|
||||
assert_includes last_response.body, "Invalid slug"
|
||||
end
|
||||
|
||||
def test_change_username_rejects_special_chars
|
||||
login
|
||||
get "/admin/settings"
|
||||
post "/admin/settings/username",
|
||||
"_csrf" => csrf(last_response.body), "username" => "foo\" bar"
|
||||
assert_includes last_response.body, "Username may use letters"
|
||||
end
|
||||
|
||||
def test_change_username_accepts_same_name
|
||||
login
|
||||
get "/admin/settings"
|
||||
post "/admin/settings/username",
|
||||
"_csrf" => csrf(last_response.body), "username" => "admin"
|
||||
assert_includes last_response.body, "Username unchanged"
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def csrf(body)
|
||||
|
||||
@@ -49,4 +49,10 @@ class StoreTest < Minitest::Test
|
||||
assert store.find("global", lang: "cs")
|
||||
assert store.find("global", lang: "en")
|
||||
end
|
||||
|
||||
def test_save_rejects_path_traversal_slug
|
||||
store = Volumen::Store.new(@dir)
|
||||
post = Volumen::Post.new(metadata: { "slug" => "../../etc/passwd" }, body: "x")
|
||||
assert_raises(ArgumentError, "slug escapes content directory") { store.save(post) }
|
||||
end
|
||||
end
|
||||
|
||||
Reference in New Issue
Block a user