-
Notifications
You must be signed in to change notification settings - Fork 0
Add workflow validator to enforce policy #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| module R3x | ||
| module Workflow | ||
| module Policy | ||
| STRICT_FORBIDDEN_CONSTANTS = %w[ENV].freeze | ||
| STRICT_FORBIDDEN_MODULE_PREFIXES = %w[R3x::Env ::R3x::Env].freeze | ||
| STRICT_FORBIDDEN_METHODS = %i[eval system exec spawn send __send__ instance_eval class_eval const_get].freeze | ||
|
zewelor marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The strict forbidden-method list omits Useful? React with 👍 / 👎. |
||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| module R3x | ||
| module Workflow | ||
| class Validator | ||
| class ForbiddenAccessError < StandardError; end | ||
|
|
||
| def self.scan_file(file_path, policy: :strict) | ||
| new(file_path, policy: policy).scan | ||
| end | ||
|
|
||
| def initialize(file_path, policy:) | ||
| @file_path = file_path | ||
| @policy = policy | ||
| @violations = [] | ||
| end | ||
|
|
||
| def scan | ||
| return if @policy == :permissive | ||
|
|
||
| source = File.read(@file_path) | ||
| ast = RubyVM::AbstractSyntaxTree.parse(source) | ||
| walk(ast) | ||
| raise_on_violations | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def walk(node) | ||
| return if node.nil? | ||
| return unless node.is_a?(RubyVM::AbstractSyntaxTree::Node) | ||
|
|
||
| check_node(node) | ||
| node.children.each { |child| walk(child) if child.is_a?(RubyVM::AbstractSyntaxTree::Node) } | ||
| end | ||
|
|
||
| def check_node(node) | ||
| case node.type | ||
| when :CONST | ||
| check_forbidden_constant(node.children[0]) | ||
| when :COLON3 | ||
| check_forbidden_constant(node.children[0]) | ||
| when :COLON2 | ||
| check_forbidden_prefix(node) | ||
| when :FCALL | ||
| check_forbidden_method(node.children[0]) | ||
| when :CALL | ||
| check_forbidden_method(node.children[1]) | ||
|
zewelor marked this conversation as resolved.
|
||
| when :XSTR | ||
| @violations << "Backtick shell execution is forbidden (use declared capabilities instead)" | ||
| end | ||
| end | ||
|
|
||
| def check_forbidden_constant(name) | ||
| return unless Policy::STRICT_FORBIDDEN_CONSTANTS.include?(name.to_s) | ||
|
|
||
| @violations << "Direct ENV access is forbidden (use declared capabilities instead)" | ||
| end | ||
|
|
||
| def check_forbidden_method(name) | ||
| return unless Policy::STRICT_FORBIDDEN_METHODS.include?(name) | ||
|
|
||
| @violations << "Calling #{name} is forbidden (use declared capabilities instead)" | ||
| end | ||
|
|
||
| def check_forbidden_prefix(node) | ||
| constant_path = resolve_constant_path(node) | ||
| return if constant_path.nil? | ||
|
|
||
| if Policy::STRICT_FORBIDDEN_MODULE_PREFIXES.any? { |prefix| constant_path == prefix } | ||
| @violations << "Direct #{constant_path} access is forbidden (use declared capabilities instead)" | ||
| end | ||
| end | ||
|
|
||
| def resolve_constant_path(node) | ||
| return nil unless node.is_a?(RubyVM::AbstractSyntaxTree::Node) && node.type == :COLON2 | ||
|
|
||
| child = node.children[0] | ||
| name = node.children[1] | ||
|
|
||
| if child.nil? | ||
| name.to_s | ||
| elsif child.is_a?(RubyVM::AbstractSyntaxTree::Node) && child.type == :CONST | ||
| "#{child.children[0]}::#{name}" | ||
| elsif child.is_a?(RubyVM::AbstractSyntaxTree::Node) && child.type == :COLON3 | ||
| "::#{child.children[0]}::#{name}" | ||
| end | ||
| end | ||
|
|
||
| def raise_on_violations | ||
| return if @violations.empty? | ||
|
|
||
| unique = @violations.uniq | ||
| message = "Workflow policy violation in #{@file_path}:\n#{unique.map { |v| " - #{v}" }.join("\n")}" | ||
| raise ForbiddenAccessError, message | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,257 @@ | ||
| require "test_helper" | ||
| require "tmpdir" | ||
|
|
||
| module R3x | ||
| module Workflow | ||
| class ValidatorTest < ActiveSupport::TestCase | ||
| def scan_with(code, policy: :strict) | ||
| Dir.mktmpdir do |dir| | ||
| file = File.join(dir, "test_workflow.rb") | ||
| File.write(file, code) | ||
| Validator.scan_file(file, policy: policy) | ||
| end | ||
| end | ||
|
|
||
| def assert_forbidden(code, message: nil) | ||
| error = assert_raises(Validator::ForbiddenAccessError) { scan_with(code) } | ||
| assert_match(/forbidden/i, error.message) if message.nil? | ||
| end | ||
|
|
||
| # --- Allowed patterns --- | ||
|
|
||
| test "allows clean workflow code" do | ||
| code = <<~RUBY | ||
| module Workflows | ||
| class Clean < R3x::Workflow::Base | ||
| def run(ctx) | ||
| { ok: true } | ||
| end | ||
| end | ||
| end | ||
| RUBY | ||
| assert_nothing_raised { scan_with(code) } | ||
| end | ||
|
|
||
| test "allows ctx.client.http usage" do | ||
| code = <<~RUBY | ||
| def run(ctx) | ||
| ctx.client.http.get("https://example.com") | ||
| end | ||
| RUBY | ||
| assert_nothing_raised { scan_with(code) } | ||
| end | ||
|
|
||
| test "allows ctx.client.llm usage" do | ||
| code = <<~RUBY | ||
| def run(ctx) | ||
| ctx.client.llm.message(model: "gemini-2.0-flash", prompt: "hi") | ||
| end | ||
| RUBY | ||
| assert_nothing_raised { scan_with(code) } | ||
| end | ||
|
|
||
| test "allows ctx.client.prometheus usage" do | ||
| code = <<~RUBY | ||
| def run(ctx) | ||
| ctx.client.prometheus.query("up") | ||
| end | ||
| RUBY | ||
| assert_nothing_raised { scan_with(code) } | ||
| end | ||
|
|
||
| test "allows usages of R3x modules in class inheritance" do | ||
| code = <<~RUBY | ||
| module Workflows | ||
| class MyWf < R3x::Workflow::Base | ||
| end | ||
| end | ||
| RUBY | ||
| assert_nothing_raised { scan_with(code) } | ||
| end | ||
|
|
||
| test "allows api_key_env option string" do | ||
| code = <<~RUBY | ||
| module Workflows | ||
| class LlmWf < R3x::Workflow::Base | ||
| uses :llm, api_key_env: "GEMINI_API_KEY_MICHAL" | ||
| def run(ctx); end | ||
| end | ||
| end | ||
| RUBY | ||
| assert_nothing_raised { scan_with(code) } | ||
| end | ||
|
|
||
| test "allows local variables and method calls" do | ||
| code = <<~RUBY | ||
| def run(ctx) | ||
| url = "https://example.com" | ||
| response = ctx.client.http.get(url) | ||
| response.body | ||
| end | ||
| RUBY | ||
| assert_nothing_raised { scan_with(code) } | ||
| end | ||
|
|
||
| # --- Forbidden: ENV --- | ||
|
|
||
| test "forbids ENV subscript access" do | ||
| assert_forbidden 'ENV["FOO"]' | ||
| end | ||
|
|
||
| test "forbids ENV.fetch" do | ||
| assert_forbidden 'ENV.fetch("FOO")' | ||
| end | ||
|
|
||
| test "forbids ENV.fetch with default" do | ||
| assert_forbidden 'ENV.fetch("FOO", "default")' | ||
| end | ||
|
|
||
| test "forbids ENV assignment" do | ||
| assert_forbidden 'ENV["FOO"] = "bar"' | ||
| end | ||
|
|
||
| test "forbids ENV.each" do | ||
| assert_forbidden "ENV.each { |k, v| puts k }" | ||
| end | ||
|
|
||
| test "forbids ENV.key?" do | ||
| assert_forbidden 'ENV.key?("FOO")' | ||
| end | ||
|
|
||
| test "forbids top-level ::ENV" do | ||
| assert_forbidden '::ENV["FOO"]' | ||
| end | ||
|
|
||
| test "forbids ENV as value" do | ||
| assert_forbidden "config = ENV" | ||
| end | ||
|
|
||
| test "forbids ENV.to_h" do | ||
| assert_forbidden "ENV.to_h" | ||
| end | ||
|
|
||
| test "forbids ENV in assignment" do | ||
| assert_forbidden 'my_var = ENV["FOO"]' | ||
| end | ||
|
|
||
| # --- Forbidden: R3x::Env --- | ||
|
|
||
| test "forbids R3x::Env.fetch!" do | ||
| assert_forbidden 'R3x::Env.fetch!("MY_KEY")' | ||
| end | ||
|
|
||
| test "forbids R3x::Env.fetch" do | ||
| assert_forbidden 'R3x::Env.fetch("MY_KEY")' | ||
| end | ||
|
|
||
| test "forbids R3x::Env.present?" do | ||
| assert_forbidden 'R3x::Env.present?("MY_KEY")' | ||
| end | ||
|
|
||
| test "forbids R3x::Env.load_from_vault" do | ||
| assert_forbidden 'R3x::Env.load_from_vault("secret/foo")' | ||
| end | ||
|
|
||
| # --- Forbidden: ::R3x::Env (top-level constant path) --- | ||
|
|
||
| test "forbids ::R3x::Env.fetch" do | ||
| assert_forbidden '::R3x::Env.fetch("MY_KEY")' | ||
| end | ||
|
|
||
| test "forbids ::R3x::Env.fetch!" do | ||
| assert_forbidden '::R3x::Env.fetch!("MY_KEY")' | ||
| end | ||
|
|
||
| # --- Forbidden: dangerous methods --- | ||
|
|
||
| test "forbids eval" do | ||
| assert_forbidden 'eval("ENV[\"SECRET\"]")' | ||
| end | ||
|
|
||
| test "forbids system" do | ||
| assert_forbidden 'system("ls")' | ||
| end | ||
|
|
||
| test "forbids exec" do | ||
| assert_forbidden 'exec("ls")' | ||
| end | ||
|
|
||
| test "forbids spawn" do | ||
| assert_forbidden 'spawn("ls")' | ||
| end | ||
|
|
||
| test "forbids backtick execution" do | ||
| assert_forbidden "`ls`" | ||
| end | ||
|
|
||
| test "forbids send" do | ||
| assert_forbidden "obj.send(:dangerous_method)" | ||
| end | ||
|
|
||
| test "forbids __send__" do | ||
| assert_forbidden "obj.__send__(:dangerous_method)" | ||
| end | ||
|
|
||
| test "forbids instance_eval" do | ||
| assert_forbidden 'obj.instance_eval("code")' | ||
| end | ||
|
|
||
| test "forbids class_eval" do | ||
| assert_forbidden 'Klass.class_eval("code")' | ||
| end | ||
|
|
||
| test "forbids const_get" do | ||
| assert_forbidden "Module.const_get(name)" | ||
| end | ||
|
|
||
| # --- permissive policy --- | ||
|
|
||
| test "allows ENV under permissive policy" do | ||
| code = <<~RUBY | ||
| def run(ctx) | ||
| ENV["FOO"] | ||
| end | ||
| RUBY | ||
| assert_nothing_raised { scan_with(code, policy: :permissive) } | ||
| end | ||
|
|
||
| test "allows R3x::Env under permissive policy" do | ||
| code = <<~RUBY | ||
| def run(ctx) | ||
| R3x::Env.fetch!("KEY") | ||
| end | ||
| RUBY | ||
| assert_nothing_raised { scan_with(code, policy: :permissive) } | ||
| end | ||
|
|
||
| # --- error messages --- | ||
|
|
||
| test "error message includes file path" do | ||
| Dir.mktmpdir do |dir| | ||
| file = File.join(dir, "bad_workflow.rb") | ||
| File.write(file, 'ENV["X"]') | ||
| error = assert_raises(Validator::ForbiddenAccessError) do | ||
| Validator.scan_file(file) | ||
| end | ||
| assert_includes error.message, file | ||
| end | ||
| end | ||
|
|
||
| test "error message deduplicates violations" do | ||
| code = <<~RUBY | ||
| a = ENV["X"] | ||
| b = ENV["Y"] | ||
| c = ENV["Z"] | ||
| RUBY | ||
| Dir.mktmpdir do |dir| | ||
| file = File.join(dir, "multi.rb") | ||
| File.write(file, code) | ||
| error = assert_raises(Validator::ForbiddenAccessError) do | ||
| Validator.scan_file(file) | ||
| end | ||
| assert_equal 1, error.message.scan("ENV").size | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PackLoader.load!only validates eachworkflow.rbentrypoint, then immediatelyrequires it. Inlib/r3x/workflow/pack_loader.rb, this lets a workflow bypass the new policy by moving forbidden code into a secondary file (for examplerequire_relative "unsafe"whereunsafe.rbreadsENVor shells out), because that file is never scanned. Since this commit is meant to enforce workflow policy at load time, limiting validation to entrypoints leaves a straightforward enforcement gap in production.Useful? React with 👍 / 👎.