From ea9cd5fc40ed313d09ce2b3b19d7e60cea359914 Mon Sep 17 00:00:00 2001
From: Roman Priesol <roman@priesol.net>
Date: Wed, 9 Sep 2026 18:21:47 +0200
Subject: [PATCH] Fix three bugs found reviewing this session's changes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- notes.js's ICON_LABELS wasn't extended for the shop_supermarket/
  shop_convenience keys icons.js added — the note icon picker showed
  the raw internal key as the title instead of a real label.
- Plugins.run mislabeled a malformed plugin.json as "plugin did not
  print valid JSON" — a message written for bad script stdout, not a
  bad manifest, since both paths shared one rescue.
- overpass_pois.rb's description_for joined raw OSM tag values with
  "\n" — an operator/opening_hours tag with an embedded newline could
  inject an extra line into the popup that gpx.js's formatDescription
  (built for p4n2gpx's own well-formed lines) would render as its own
  independent, legitimate-looking link. Tag values are stripped of
  embedded newlines before joining.
---
 lib/maptool/overpass_pois.rb | 15 +++++++++++++--
 lib/maptool/plugins.rb       |  6 +++++-
 public/js/notes.js           |  2 ++
 test/test_overpass_pois.rb   | 17 +++++++++++++++++
 test/test_plugins.rb         | 11 +++++++++++
 5 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/lib/maptool/overpass_pois.rb b/lib/maptool/overpass_pois.rb
index e3b2192..9c1c894 100644
--- a/lib/maptool/overpass_pois.rb
+++ b/lib/maptool/overpass_pois.rb
@@ -106,11 +106,22 @@ module Maptool
     private_class_method :category_for
 
     def self.description_for(tags)
-      lines = DESCRIPTION_TAGS.filter_map { |tag, label| "#{label}: #{tags[tag]}" if tags[tag] }
+      lines = DESCRIPTION_TAGS.filter_map { |tag, label| "#{label}: #{clean_tag(tags[tag])}" if tags[tag] }
       website = tags["website"] || tags["contact:website"]
-      lines << website if website
+      lines << clean_tag(website) if website
       lines.empty? ? nil : lines.join("\n")
     end
     private_class_method :description_for
+
+    # An OSM tag value is free text from a world-editable map — an embedded
+    # newline could otherwise inject an extra "line" of its own into the
+    # description, e.g. an operator tag ending in "\nhttps://evil.example"
+    # would render as its own separate, legitimate-looking link line once
+    # gpx.js's formatDescription (written for p4n2gpx's own well-formed
+    # "Label: value" lines) splits on "\n".
+    def self.clean_tag(value)
+      value.to_s.tr("\n\r", " ").strip
+    end
+    private_class_method :clean_tag
   end
 end
diff --git a/lib/maptool/plugins.rb b/lib/maptool/plugins.rb
index f42b937..bf86064 100644
--- a/lib/maptool/plugins.rb
+++ b/lib/maptool/plugins.rb
@@ -55,7 +55,11 @@ module Maptool
 
       manifest_path = File.join(plugin_dir, "plugin.json")
 
-      command = JSON.parse(File.read(manifest_path))["command"]
+      command = begin
+        JSON.parse(File.read(manifest_path))["command"]
+      rescue JSON::ParserError
+        raise RunError, "plugin has an invalid plugin.json"
+      end
       raise RunError, "plugin has no command" unless command
 
       south, west, north, east = bbox
diff --git a/public/js/notes.js b/public/js/notes.js
index 42bd372..8183668 100644
--- a/public/js/notes.js
+++ b/public/js/notes.js
@@ -24,6 +24,8 @@ const ICON_LABELS = {
   leisure_fishing: "Fishing",
   sport_climbing: "Climbing",
   route_hiking: "Hiking",
+  shop_supermarket: "Supermarket",
+  shop_convenience: "Convenience store",
 };
 
 const NEW_LAYER_VALUE = "__new__";
diff --git a/test/test_overpass_pois.rb b/test/test_overpass_pois.rb
index 91d979b..8f179e8 100644
--- a/test/test_overpass_pois.rb
+++ b/test/test_overpass_pois.rb
@@ -56,6 +56,23 @@ class OverpassPoisTest < Minitest::Test
     assert_equal "Opening hours: 24/7\nOperator: Town Council\nhttps://example.com", description
   end
 
+  def test_to_geojson_strips_embedded_newlines_from_tag_values
+    response = {
+      "elements" => [
+        {
+          "type" => "node",
+          "lat" => 48.1,
+          "lon" => 17.2,
+          "tags" => { "amenity" => "toilets", "operator" => "Acme Co\nhttps://evil.example" },
+        },
+      ],
+    }
+
+    description = Maptool::OverpassPois.to_geojson(response)["features"].first["properties"]["description"]
+
+    assert_equal "Operator: Acme Co https://evil.example", description, "an embedded newline must not inject a second, independently-rendered line"
+  end
+
   def test_to_geojson_omits_description_when_no_recognized_tags_are_present
     response = { "elements" => [{ "type" => "node", "lat" => 48.1, "lon" => 17.2, "tags" => { "amenity" => "toilets" } }] }
 
diff --git a/test/test_plugins.rb b/test/test_plugins.rb
index f250c58..901b4b9 100644
--- a/test/test_plugins.rb
+++ b/test/test_plugins.rb
@@ -53,6 +53,17 @@ class PluginsTest < Minitest::Test
     assert_raises(Maptool::Plugins::RunError) { Maptool::Plugins.run(@dir, "hello", [1, 2, 3, 4]) }
   end
 
+  # Distinct from the "did not print valid JSON" message above — that one's
+  # about the script's stdout, this is about plugin.json itself, before the
+  # script is ever run. Same exception class either way, but a clear
+  # message matters since this is what a "Failed" plugin run shows the user
+  # (see public/js/plugins.js).
+  def test_run_raises_a_distinct_message_for_an_invalid_manifest
+    File.write(File.join(@plugin_dir, "plugin.json"), "{not valid json")
+    error = assert_raises(Maptool::Plugins::RunError) { Maptool::Plugins.run(@dir, "hello", [1, 2, 3, 4]) }
+    assert_match(/invalid plugin\.json/, error.message)
+  end
+
   # Multiple dirs (app.rb's PLUGINS_DIRS: the built-in plugins/ plus an
   # optional private one under data/plugins/) — list/run both accept an
   # array, not just the single-dir form the tests above exercise.