Treat targets_modified overlay as data, not code

openc3-cosmos-cmd-tlm-api/app/controllers/storage_controller.rb

@@ -418,7 +418,6 @@ def get_upload_presigned_request
     params.require(:bucket)
     bucket_name = ENV.fetch(params[:bucket]) { |name| raise StorageError, "Unknown bucket #{name}" }
     path = sanitize_path(params[:object_id])
-    key_split = path.split('/')
 
     # Check scope-based RBAC for config and logs buckets
     if bucket_requires_rbac?(params[:bucket])
@@ -429,10 +428,10 @@ def get_upload_presigned_request
       end
     end
 
-    # Anywhere other than config/SCOPE/targets_modified or config/SCOPE/tmp requires admin
-    if !(params[:bucket] == 'OPENC3_CONFIG_BUCKET' && (key_split[1] == 'targets_modified' || key_split[1] == 'tmp'))
-      return unless authorization('admin')
-    end
+    # Non-admins may only write the user-writable overlay (config/SCOPE/targets_modified
+    # or config/SCOPE/tmp), and never the cmd_tlm overlay, which PacketConfig ERB-renders
+    # and whose GENERIC_*_CONVERSION blocks it evaluates as code. Everything else is admin.
+    return if !non_admin_config_overlay_write?(params[:bucket], path) && !authorization('admin')
 
     bucket = OpenC3::Bucket.getClient()
     result = bucket.presigned_request(bucket: bucket_name,
@@ -601,6 +600,30 @@ def repair_candidates
 
   private
 
+  # True if the given config-bucket key is an overlay path a non-admin is allowed
+  # to write. Non-admins may write config/SCOPE/targets_modified/... and
+  # config/SCOPE/tmp/..., but NOT the cmd_tlm overlay
+  # (config/SCOPE/targets_modified/TARGET/cmd_tlm/...), which is loaded and
+  # executed as code by PacketConfig (ERB rendering + GENERIC_*_CONVERSION eval).
+  #
+  # The key must be canonical: a positional segment check (parts[1], parts[3])
+  # can otherwise be bypassed by a key the object store normalizes differently,
+  # so empty '//' segments, '.'/'..' segments, and leading/trailing slashes are
+  # rejected here (forcing the admin path). sanitize_path already rejects '..'.
+  def non_admin_config_overlay_write?(bucket_param, path)
+    return false unless bucket_param == 'OPENC3_CONFIG_BUCKET'
+    return false if path.nil? || path.empty?
+    return false if path.start_with?('/') || path.end_with?('/')
+    parts = path.split('/')
+    return false if parts.any? { |p| p.empty? || p == '.' || p == '..' }
+    # parts: SCOPE / <area> / TARGET / <subdir> / ...
+    area = parts[1]
+    return false unless area == 'targets_modified' || area == 'tmp'
+    # cmd_tlm overlay (parts[3]) is code-execution territory; require admin.
+    return false if area == 'targets_modified' && parts[3] == 'cmd_tlm'
+    true
+  end
+
   def sanitize_path(path)
     return '' if path.nil?
     # path is passed as a parameter thus we have to sanitize it or the code scanner detects:
@@ -619,7 +642,6 @@ def delete_bucket_item(params)
     params.require(:bucket)
     bucket_name = ENV.fetch(params[:bucket]) { |name| raise StorageError, "Unknown bucket #{name}" }
     path = sanitize_path(params[:object_id])
-    key_split = path.split('/')
 
     # Check scope-based RBAC for config and logs buckets
     if bucket_requires_rbac?(params[:bucket])
@@ -630,11 +652,9 @@ def delete_bucket_item(params)
       end
     end
 
-    # Anywhere other than config/SCOPE/targets_modified or config/SCOPE/tmp requires admin
-    authorized = true
-    if !(params[:bucket] == 'OPENC3_CONFIG_BUCKET' && (key_split[1] == 'targets_modified' || key_split[1] == 'tmp'))
-      authorized = false unless authorization('admin')
-    end
+    # Non-admins may only touch the user-writable config overlay (targets_modified or
+    # tmp), never the cmd_tlm overlay which is evaluated as code. Everything else is admin.
+    authorized = non_admin_config_overlay_write?(params[:bucket], path) || authorization('admin')
 
     if authorized
       if ENV.fetch('OPENC3_LOCAL_MODE', false)
@@ -667,8 +687,10 @@ def delete_bucket_directory(params)
     bucket_name = ENV.fetch(params[:bucket]) { |name| raise StorageError, "Unknown bucket #{name}" }
 
     path = sanitize_path(params[:object_id])
+    # Evaluate the overlay check on the canonical (un-slashed) path before appending
+    # the trailing slash, since the helper rejects trailing slashes.
+    overlay_write = non_admin_config_overlay_write?(params[:bucket], path)
     path = "#{path}/" unless path.end_with?('/')
-    key_split = path.split('/')
 
     # Check scope-based RBAC
     if bucket_requires_rbac?(params[:bucket]) && !authorize_bucket_path(params[:bucket], path, permission: 'system_set')
@@ -677,10 +699,9 @@ def delete_bucket_directory(params)
       return false
     end
 
-    # Require admin for most bucket directories
-    unless (params[:bucket] == 'OPENC3_CONFIG_BUCKET' && (key_split[1] == 'targets_modified' || key_split[1] == 'tmp'))
-      return false unless authorization('admin')
-    end
+    # Non-admins may only touch the user-writable config overlay (targets_modified or
+    # tmp), never the cmd_tlm overlay which is evaluated as code. Everything else is admin.
+    return false unless overlay_write || authorization('admin')
 
     # List all objects under the prefix (same pattern as TargetModel#undeploy)
     bucket = OpenC3::Bucket.getClient()

openc3-cosmos-cmd-tlm-api/app/controllers/tables_controller.rb

@@ -110,6 +110,7 @@ def save
     return unless authorization('system')
     scope, binary, definition = sanitize_params([:scope, :binary, :definition], require_params: false, allow_forward_slash: true)
     return unless scope
+    return unless authorize_overlay_write(binary)
     begin
       Table.save(scope, binary, definition, params[:tables])
       head :ok
@@ -123,6 +124,7 @@ def save_as
     return unless authorization('system')
     scope, name, new_name = sanitize_params([:scope, :name, :new_name], require_params: true, allow_forward_slash: true)
     return unless scope
+    return unless authorize_overlay_write(new_name)
     begin
       Table.save_as(scope, name, new_name)
       head :ok
@@ -136,6 +138,9 @@ def generate
     return unless authorization('system')
     scope, definition = sanitize_params([:scope, :definition], require_params: false, allow_forward_slash: true)
     return unless scope
+    # generate writes a binary derived from the definition path under the same
+    # TARGET/<area> prefix, so gating on the definition covers the write target.
+    return unless authorize_overlay_write(definition)
     begin
       filename = Table.generate(scope, definition)
       render json: { filename: filename }
@@ -166,6 +171,7 @@ def destroy
     return unless authorization('system')
     scope, name = sanitize_params([:scope, :name], require_params: true, allow_forward_slash: true)
     return unless scope
+    return unless authorize_overlay_write(name)
     # destroy returns no indication of success or failure so just assume it worked
     Table.destroy(scope, name)
     OpenC3::Logger.info(
@@ -175,4 +181,37 @@ def destroy
     )
     head :ok
   end
+
+  private
+
+  # Single choke point gating every Table writer (save, save_as, generate,
+  # destroy) that funnels through TargetFile.create/destroy into the
+  # targets_modified overlay. The cmd_tlm overlay
+  # (targets_modified/<TARGET>/cmd_tlm/...) is loaded and executed as code by
+  # PacketConfig (ERB rendering + GENERIC_*_CONVERSION eval), so writing it
+  # requires admin even though general Table Manager operations only require
+  # 'system' (which under Enterprise RBAC every role down to Viewer holds).
+  # This mirrors storage_controller#non_admin_config_overlay_write?, which gates
+  # the other writer (the presigned S3 upload). `name` is the overlay-relative
+  # path written under targets_modified/ (e.g. "<TARGET>/cmd_tlm/..."). Returns
+  # true if allowed; otherwise renders the 401/403 and returns false.
+  def authorize_overlay_write(name)
+    return true unless cmd_tlm_overlay?(name)
+    return false unless authorization('admin')
+    true
+  end
+
+  # True if the overlay-relative name targets the cmd_tlm subtree (code-execution
+  # territory). The name must be canonical: a positional segment check (parts[1])
+  # can otherwise be bypassed by a name the object store normalizes differently,
+  # so empty '//', '.'/'..' segments, and leading/trailing slashes fail closed to
+  # the admin path. sanitize_params already neutralizes '..', backslashes, etc.
+  def cmd_tlm_overlay?(name)
+    return true if name.nil? || name.empty?
+    return true if name.start_with?('/') || name.end_with?('/')
+    parts = name.split('/')
+    return true if parts.any? { |p| p.empty? || p == '.' || p == '..' }
+    # parts: <TARGET> / <area> / ...
+    parts[1] == 'cmd_tlm'
+  end
 end

openc3-cosmos-cmd-tlm-api/app/models/table.rb

@@ -168,7 +168,11 @@ def self.locked?(scope, name)
   # Private helper methods
 
   def self.get_definitions(scope, definition_filename, binary_filename = nil)
-    definition = body(scope, definition_filename)
+    # Definitions are ERB-rendered and their GENERIC_*_CONVERSION blocks are
+    # evaluated as code by TableConfig, so they must come from the read-only
+    # plugin-installed targets/ tree only. Reading the user-writable
+    # targets_modified/ overlay here would be arbitrary code execution.
+    definition = body(scope, definition_filename, original: true)
     # We might not find the definition, especially if the binary isn't named
     # like the convention. If not, and they pass us a binary filename,
     # then look through all the definitions and try to find a match.
@@ -193,7 +197,7 @@ def self.get_definitions(scope, definition_filename, binary_filename = nil)
         end
       end
       if found
-        definition = body(scope, definition_filename)
+        definition = body(scope, definition_filename, original: true)
       else
         return [nil, definition_filename, nil]
       end
@@ -208,7 +212,7 @@ def self.get_definitions(scope, definition_filename, binary_filename = nil)
       definition.split("\n").each do |line|
         if line.strip =~ /^TABLEFILE (.*)/
           filename = File.join(base_dir, $1.remove_quotes)
-          file = body(scope, filename)
+          file = body(scope, filename, original: true)
           raise NotFound, "Could not find file #{filename}" unless file
           File.write(File.join(temp_dir, File.basename(filename)), file)
         end

openc3-cosmos-cmd-tlm-api/spec/controllers/storage_controller_spec.rb

@@ -524,6 +524,40 @@
       expect(response).to have_http_status(:internal_server_error)
       expect(JSON.parse(response.body)["message"]).to eq("General error")
     end
+
+    context "with a non-admin user (system_set but not admin)" do
+      before(:each) do
+        # Grant every permission except admin to simulate a non-admin operator
+        allow(controller).to receive(:authorize) do |args|
+          raise OpenC3::AuthError.new("admin required") if args[:permission] == 'admin'
+          'authorized_user'
+        end
+        bucket_client = instance_double(OpenC3::Bucket)
+        allow(OpenC3::Bucket).to receive(:getClient).and_return(bucket_client)
+        allow(bucket_client).to receive(:presigned_request).and_return({url: "https://bucket/x"})
+      end
+
+      it "allows a non-cmd_tlm targets_modified overlay upload" do
+        get :get_upload_presigned_request, params: {
+          bucket: "OPENC3_CONFIG_BUCKET", object_id: "DEFAULT/targets_modified/INST/screens/poc.txt", scope: "DEFAULT"
+        }
+        expect(response).to have_http_status(:created)
+      end
+
+      it "rejects a cmd_tlm overlay upload (requires admin)" do
+        get :get_upload_presigned_request, params: {
+          bucket: "OPENC3_CONFIG_BUCKET", object_id: "DEFAULT/targets_modified/INST/cmd_tlm/tlm.txt", scope: "DEFAULT"
+        }
+        expect(response).to have_http_status(:unauthorized)
+      end
+
+      it "rejects a non-canonical overlay key that hides cmd_tlm (requires admin)" do
+        get :get_upload_presigned_request, params: {
+          bucket: "OPENC3_CONFIG_BUCKET", object_id: "DEFAULT/targets_modified/INST//cmd_tlm/tlm.txt", scope: "DEFAULT"
+        }
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
   end
 
   describe "POST download_multiple_files" do
@@ -911,6 +945,36 @@
       end
     end
 
+    describe "non_admin_config_overlay_write?" do
+      let(:m) { :non_admin_config_overlay_write? }
+
+      it "allows non-cmd_tlm targets_modified and tmp overlay writes" do
+        expect(controller.send(m, 'OPENC3_CONFIG_BUCKET', 'DEFAULT/targets_modified/INST/screens/poc.txt')).to be true
+        expect(controller.send(m, 'OPENC3_CONFIG_BUCKET', 'DEFAULT/targets_modified/INST/procedures/x.rb')).to be true
+        expect(controller.send(m, 'OPENC3_CONFIG_BUCKET', 'DEFAULT/tmp/foo.txt')).to be true
+      end
+
+      it "requires admin (returns false) for the cmd_tlm overlay" do
+        expect(controller.send(m, 'OPENC3_CONFIG_BUCKET', 'DEFAULT/targets_modified/INST/cmd_tlm/tlm.txt')).to be false
+      end
+
+      it "requires admin for non-overlay and non-config paths" do
+        expect(controller.send(m, 'OPENC3_CONFIG_BUCKET', 'DEFAULT/targets/INST/screens/x.txt')).to be false
+        expect(controller.send(m, 'OPENC3_CONFIG_BUCKET', 'DEFAULT/target_archives/INST/x.zip')).to be false
+        expect(controller.send(m, 'OPENC3_LOGS_BUCKET', 'DEFAULT/targets_modified/INST/screens/x.txt')).to be false
+        expect(controller.send(m, 'OPENC3_TOOLS_BUCKET', 'DEFAULT/targets_modified/INST/screens/x.txt')).to be false
+      end
+
+      it "rejects non-canonical keys so the positional check cannot be bypassed" do
+        # Empty '//' segments, '.'/'..' segments, leading/trailing slashes all force admin
+        expect(controller.send(m, 'OPENC3_CONFIG_BUCKET', 'DEFAULT/targets_modified/INST//cmd_tlm/x.txt')).to be false
+        expect(controller.send(m, 'OPENC3_CONFIG_BUCKET', 'DEFAULT/targets_modified/INST/./cmd_tlm/x.txt')).to be false
+        expect(controller.send(m, 'OPENC3_CONFIG_BUCKET', '/DEFAULT/targets_modified/INST/screens/x.txt')).to be false
+        expect(controller.send(m, 'OPENC3_CONFIG_BUCKET', 'DEFAULT/targets_modified/INST/screens/')).to be false
+        expect(controller.send(m, 'OPENC3_CONFIG_BUCKET', '')).to be false
+      end
+    end
+
     describe "target_list_depth" do
       it "returns 2 for config bucket target directories" do
         expect(controller.send(:target_list_depth, 'OPENC3_CONFIG_BUCKET', 'DEFAULT/targets')).to eq(2)

openc3-cosmos-cmd-tlm-api/spec/controllers/tables_controller_spec.rb

@@ -561,4 +561,86 @@
       expect(response).to have_http_status(:unauthorized)
     end
   end
+
+  # The cmd_tlm overlay (targets_modified/<TARGET>/cmd_tlm/...) is ERB-rendered
+  # and eval'd as code by PacketConfig, so every Table writer must require admin
+  # to write it, not just the presigned-upload path in storage_controller.
+  describe "cmd_tlm overlay gate" do
+    describe "cmd_tlm_overlay?" do
+      let(:m) { :cmd_tlm_overlay? }
+
+      it "flags cmd_tlm overlay names (admin required)" do
+        expect(controller.send(m, "INST/cmd_tlm/tlm.txt")).to be true
+        expect(controller.send(m, "INST/cmd_tlm/config/extra.txt")).to be true
+      end
+
+      it "allows non-cmd_tlm overlay names" do
+        expect(controller.send(m, "INST/tables/bin/table.bin")).to be false
+        expect(controller.send(m, "INST/tables/config/table_def.txt")).to be false
+        expect(controller.send(m, "INST/screens/x.txt")).to be false
+      end
+
+      it "fails closed on non-canonical names so the positional check cannot be bypassed" do
+        expect(controller.send(m, "INST//cmd_tlm/x.txt")).to be true
+        expect(controller.send(m, "INST/./cmd_tlm/x.txt")).to be true
+        expect(controller.send(m, "INST/../cmd_tlm/x.txt")).to be true
+        expect(controller.send(m, "/INST/tables/x.bin")).to be true
+        expect(controller.send(m, "INST/tables/")).to be true
+        expect(controller.send(m, "")).to be true
+        expect(controller.send(m, nil)).to be true
+      end
+    end
+
+    context "when the caller is non-admin (system but not admin)" do
+      before do
+        allow(controller).to receive(:authorization).with('system').and_return(true)
+        allow(controller).to receive(:authorization).with('admin').and_return(false)
+      end
+
+      it "blocks save into the cmd_tlm overlay" do
+        allow(Table).to receive(:save)
+        post :save, params: {scope: "DEFAULT", name: "INST/cmd_tlm/tlm.txt", binary: "INST/cmd_tlm/tlm.txt", definition: "INST/tables/config/table_def.txt", tables: '{"tables": []}'}
+        expect(Table).not_to have_received(:save)
+      end
+
+      it "blocks save_as into the cmd_tlm overlay" do
+        allow(Table).to receive(:save_as)
+        post :save_as, params: {scope: "DEFAULT", name: "INST/tables/bin/table.bin", new_name: "INST/cmd_tlm/poc.txt"}
+        expect(Table).not_to have_received(:save_as)
+      end
+
+      it "blocks generate into the cmd_tlm overlay" do
+        allow(Table).to receive(:generate)
+        post :generate, params: {scope: "DEFAULT", definition: "INST/cmd_tlm/config/poc_def.txt"}
+        expect(Table).not_to have_received(:generate)
+      end
+
+      it "blocks destroy of the cmd_tlm overlay" do
+        allow(Table).to receive(:destroy)
+        delete :destroy, params: {scope: "DEFAULT", name: "INST/cmd_tlm/tlm.txt"}
+        expect(Table).not_to have_received(:destroy)
+      end
+
+      it "still allows non-cmd_tlm table writes" do
+        allow(Table).to receive(:save_as)
+        post :save_as, params: {scope: "DEFAULT", name: "INST/tables/bin/table.bin", new_name: "INST/tables/bin/copy.bin"}
+        expect(response).to have_http_status(:ok)
+        expect(Table).to have_received(:save_as)
+      end
+    end
+
+    context "when the caller is admin" do
+      before do
+        allow(controller).to receive(:authorization).with('system').and_return(true)
+        allow(controller).to receive(:authorization).with('admin').and_return(true)
+      end
+
+      it "allows save_as into the cmd_tlm overlay" do
+        allow(Table).to receive(:save_as)
+        post :save_as, params: {scope: "DEFAULT", name: "INST/tables/bin/table.bin", new_name: "INST/cmd_tlm/tlm.txt"}
+        expect(response).to have_http_status(:ok)
+        expect(Table).to have_received(:save_as).with("DEFAULT", "INST/tables/bin/table.bin", "INST/cmd_tlm/tlm.txt")
+      end
+    end
+  end
 end