Harden RDoc::Server: security, concurrency, and robustness fixes

Address review findings from the server-mode implementation:

Security:
- Prevent path traversal in serve_asset via expand_path containment check
- Bind to 127.0.0.1 instead of 0.0.0.0 (localhost-only)

Concurrency:
- Wrap entire reparse_and_refresh in mutex so store mutations are atomic
- Protect @last_change_time read in route with mutex

Correctness:
- Remove old file data before re-parsing to prevent stale methods/constants
- Wrap individual parse_file calls in rescue so one failure doesn't skip
  store.complete for remaining files
- Recurse into nested classes/modules in Store#remove_file
- Clean up C variable maps on file removal

Robustness:
- Add 5-second IO.select timeout on client socket reads
- Return 405 Method Not Allowed for non-GET requests
- Return 400 Bad Request for missing or malformed URIs
- Track watcher thread with @running flag; join on shutdown

Nits:
- Fix docstring referencing WEBrick in rdoc.rb
- Fix doc comments in ri/servlet.rb to reference RDoc::RI::Servlet
- Use begin/rescue/end inside while for Ruby 3.0 compatibility

Author: st0012

lib/rdoc/rdoc.rb

@@ -459,7 +459,7 @@ def document(options)
     if @options.server_port
       @store.load_cache
 
-      file_info = parse_files @options.files
+      parse_files @options.files
 
       @options.default_title = "RDoc Documentation"
 
@@ -530,7 +530,7 @@ def generate
   end
 
   ##
-  # Starts a live-reloading WEBrick server for previewing documentation.
+  # Starts a live-reloading HTTP server for previewing documentation.
   # Called from #document when <tt>--server</tt> is given.
 
   def start_server

lib/rdoc/ri/servlet.rb

@@ -24,12 +24,12 @@
 #
 #   server = WEBrick::HTTPServer.new Port: 8000
 #
-#   server.mount '/', RDoc::Servlet
+#   server.mount '/', RDoc::RI::Servlet
 #
 # If you want to mount the servlet some other place than the root, provide the
 # base path when mounting:
 #
-#   server.mount '/rdoc', RDoc::Servlet, '/rdoc'
+#   server.mount '/rdoc', RDoc::RI::Servlet, '/rdoc'
 
 class RDoc::RI::Servlet < WEBrick::HTTPServlet::AbstractServlet
 

lib/rdoc/server.rb

@@ -38,6 +38,14 @@ class RDoc::Server
     '.json' => 'application/json',
   }.freeze
 
+  STATUS_TEXTS = {
+    200 => 'OK',
+    400 => 'Bad Request',
+    404 => 'Not Found',
+    405 => 'Method Not Allowed',
+    500 => 'Internal Server Error',
+  }.freeze
+
   ##
   # Creates a new server.
   #
@@ -56,15 +64,17 @@ def initialize(rdoc, port)
     @search_index_cache = nil
     @last_change_time = Time.now.to_f
     @mutex = Mutex.new
+    @running = false
   end
 
   ##
   # Starts the server.  Blocks until interrupted.
 
   def start
-    @tcp_server = TCPServer.new('0.0.0.0', @port)
+    @tcp_server = TCPServer.new('127.0.0.1', @port)
+    @running = true
 
-    start_watcher(@rdoc.last_modified.keys)
+    @watcher_thread = start_watcher(@rdoc.last_modified.keys)
 
     url = "http://localhost:#{@port}"
     $stderr.puts "\nServing documentation at: \e]8;;#{url}\e\\#{url}\e]8;;\e\\"
@@ -82,7 +92,9 @@ def start
   # Shuts down the server.
 
   def shutdown
+    @running = false
     @tcp_server&.close
+    @watcher_thread&.join(2)
   end
 
   private
@@ -101,18 +113,27 @@ def create_generator
   def handle_client(client)
     client.binmode
 
+    return unless IO.select([client], nil, nil, 5)
+
     request_line = client.gets("\n")
     return unless request_line
 
     method, request_uri, = request_line.split(' ', 3)
-    path = URI.parse(request_uri).path
+    return write_response(client, 400, 'text/plain', 'Bad Request') unless request_uri
+
+    begin
+      path = URI.parse(request_uri).path
+    rescue URI::InvalidURIError
+      return write_response(client, 400, 'text/plain', 'Bad Request')
+    end
 
-    # Consume remaining headers (we don't need them)
     while (line = client.gets("\n"))
       break if line.strip.empty?
     end
 
-    return unless method == 'GET'
+    unless method == 'GET'
+      return write_response(client, 405, 'text/plain', 'Method Not Allowed')
+    end
 
     status, content_type, body = route(path)
     write_response(client, status, content_type, body)
@@ -134,8 +155,10 @@ def handle_client(client)
   def route(path)
     case path
     when '/__status'
-      [200, 'application/json', JSON.generate(last_change: @last_change_time)]
+      t = @mutex.synchronize { @last_change_time }
+      [200, 'application/json', JSON.generate(last_change: t)]
     when '/js/search_data.js'
+      # Search data is dynamically generated, not a static asset
       serve_page(path)
     when %r{\A/(?:css|js)/}
       serve_asset(path)
@@ -148,10 +171,9 @@ def route(path)
   # Writes an HTTP/1.1 response to +client+.
 
   def write_response(client, status, content_type, body)
-    status_text = { 200 => 'OK', 404 => 'Not Found', 500 => 'Internal Server Error' }
     body_bytes = body.b
 
-    header = +"HTTP/1.1 #{status} #{status_text[status] || 'Unknown'}\r\n"
+    header = +"HTTP/1.1 #{status} #{STATUS_TEXTS[status] || 'Unknown'}\r\n"
     header << "Content-Type: #{content_type}\r\n"
     header << "Content-Length: #{body_bytes.bytesize}\r\n"
     header << "Connection: close\r\n"
@@ -168,14 +190,16 @@ def write_response(client, status, content_type, body)
   def serve_asset(path)
     rel_path = path.sub(%r{\A/}, '')
     asset_path = File.join(@generator.template_dir, rel_path)
+    real_asset = File.expand_path(asset_path)
+    real_template = File.expand_path(@generator.template_dir)
 
-    unless File.file?(asset_path)
+    unless real_asset.start_with?("#{real_template}/") && File.file?(real_asset)
       return [404, 'text/plain', "Asset not found: #{rel_path}"]
     end
 
     ext = File.extname(rel_path)
     content_type = CONTENT_TYPES[ext] || 'application/octet-stream'
-    [200, content_type, File.read(asset_path)]
+    [200, content_type, File.read(real_asset)]
   end
 
   ##
@@ -242,10 +266,8 @@ def generate_page(name)
   # Builds the search index JavaScript.
 
   def build_search_index
-    @search_index_cache ||= begin
-      index = @generator.build_search_index
-      "var search_data = #{JSON.generate(index: index)};"
-    end
+    @search_index_cache ||=
+      "var search_data = #{JSON.generate(index: @generator.build_search_index)};"
   end
 
   ##
@@ -273,11 +295,13 @@ def start_watcher(source_files)
     end
 
     Thread.new do
-      loop do
-        sleep 1
-        check_for_changes
-      rescue => e
-        $stderr.puts "RDoc server watcher error: #{e.message}"
+      while @running
+        begin
+          sleep 1
+          check_for_changes
+        rescue => e
+          $stderr.puts "RDoc server watcher error: #{e.message}"
+        end
       end
     end
   end
@@ -290,7 +314,6 @@ def check_for_changes
     changed = []
     removed = []
 
-    # Check existing tracked files for modifications or deletions
     @file_mtimes.each do |file, old_mtime|
       unless File.exist?(file)
         removed << file
@@ -302,7 +325,6 @@ def check_for_changes
       changed << file if old_mtime.nil? || current_mtime > old_mtime
     end
 
-    # Scan for new files using the same directory walking logic
     file_list = @rdoc.normalized_file_list(
       @options.files.empty? ? [@options.root.to_s] : @options.files,
       true, @options.exclude
@@ -346,24 +368,32 @@ def relative_path_for(filename)
   # refreshes the generator, and invalidates caches.
 
   def reparse_and_refresh(changed_files, removed_files)
-    unless removed_files.empty?
-      $stderr.puts "Removed: #{removed_files.join(', ')}"
-      removed_files.each do |f|
-        @file_mtimes.delete(f)
-        relative = relative_path_for(f)
-        @store.remove_file(relative)
+    @mutex.synchronize do
+      unless removed_files.empty?
+        $stderr.puts "Removed: #{removed_files.join(', ')}"
+        removed_files.each do |f|
+          @file_mtimes.delete(f)
+          relative = relative_path_for(f)
+          @store.remove_file(relative)
+        end
       end
-    end
 
-    unless changed_files.empty?
-      $stderr.puts "Re-parsing: #{changed_files.join(', ')}"
-      changed_files.each { |f| @rdoc.parse_file(f) }
-      changed_files.each { |f| @file_mtimes[f] = File.mtime(f) rescue nil }
-    end
+      unless changed_files.empty?
+        $stderr.puts "Re-parsing: #{changed_files.join(', ')}"
+        changed_files.each do |f|
+          begin
+            relative = relative_path_for(f)
+            @store.remove_file(relative)
+            @rdoc.parse_file(f)
+            @file_mtimes[f] = File.mtime(f) rescue nil
+          rescue => e
+            $stderr.puts "Error parsing #{f}: #{e.message}"
+          end
+        end
+      end
 
-    @rdoc.store.complete(@options.visibility)
+      @store.complete(@options.visibility)
 
-    @mutex.synchronize do
       @generator.refresh_store_data
       invalidate_all_caches
       @last_change_time = Time.now.to_f

lib/rdoc/store.rb

@@ -206,15 +206,11 @@ def add_file(absolute_name, relative_name: absolute_name, parser: nil)
   def remove_file(relative_name)
     top_level = @files_hash.delete(relative_name)
     @text_files_hash.delete(relative_name)
+    @c_class_variables.delete(relative_name)
+    @c_singleton_class_variables.delete(relative_name)
     return unless top_level
 
-    top_level.classes_or_modules.each do |cm|
-      if cm.is_a?(RDoc::NormalModule)
-        @modules_hash.delete(cm.full_name)
-      else
-        @classes_hash.delete(cm.full_name)
-      end
-    end
+    remove_classes_and_modules(top_level.classes_or_modules)
   end
 
   ##
@@ -1002,6 +998,19 @@ def unique_modules
   end
 
   private
+
+  def remove_classes_and_modules(cms)
+    cms.each do |cm|
+      remove_classes_and_modules(cm.classes_and_modules)
+
+      if cm.is_a?(RDoc::NormalModule)
+        @modules_hash.delete(cm.full_name)
+      else
+        @classes_hash.delete(cm.full_name)
+      end
+    end
+  end
+
   def marshal_load(file)
     File.open(file, 'rb') {|io| Marshal.load(io, MarshalFilter)}
   end