Validate route identifiers as single path segments

README.md

@@ -26,6 +26,8 @@ organization = client.organization('my-great-org')
 agents = organization.rels[:agents].get.data
 ```
 
+Identifier arguments (`org`, `pipeline`, `build`, `job`, `id`) are interpolated into the request path, so each must be a single URL path segment: letters, digits, `-`, `_`, `.`, `~`. Values containing `/`, `?`, `%`, or that are `.`, `..`, or empty raise `Buildkit::InvalidRouteSegment` (an `ArgumentError`) before any request is made. Validate or reject user-supplied identifiers at your application boundary rather than rescuing this error.
+
 ## Development
 
 After checking out the repo, run `bin/setup` to install dependencies. Then, run `bin/console` for an interactive prompt that will allow you to experiment.

lib/buildkit/client.rb

@@ -25,6 +25,11 @@ class Client
     # Header keys that can be passed in options hash to {#get},{#head}
     CONVENIENCE_HEADERS = Set.new(%i[accept content_type])
 
+    # Characters permitted in a route identifier: RFC 3986 unreserved set.
+    # Buildkite identifiers are slugs ([a-z0-9-]), UUIDs, or integers, so this is
+    # not restrictive in practice; it rejects every path/query delimiter and `%`.
+    ROUTE_SEGMENT = /\A[A-Za-z0-9\-._~]+\z/.freeze
+
     # In Faraday 0.9, Faraday::Builder was renamed to Faraday::RackBuilder
     RACK_BUILDER_CLASS = defined?(Faraday::RackBuilder) ? Faraday::RackBuilder : Faraday::Builder
 
@@ -118,6 +123,25 @@ def root
 
     private
 
+    # Validate that a caller-supplied identifier is exactly one URL path segment.
+    #
+    # Named helpers interpolate identifiers between fixed route parts; a value
+    # containing `/`, `?` or a dot-segment would otherwise re-route the request
+    # to a different Buildkite action.
+    #
+    # @param value [String, Integer]
+    # @param name [Symbol] parameter name, for the error message
+    # @return [String]
+    # @raise [Buildkit::InvalidRouteSegment]
+    def route_segment(value, name)
+      segment = value.to_s
+      if segment.match?(ROUTE_SEGMENT) && segment != '.' && segment != '..'
+        segment
+      else
+        raise InvalidRouteSegment, "#{name} must be a single URL path segment, got #{value.inspect}"
+      end
+    end
+
     def request(method, path, data, options = {})
       if data.is_a?(Hash)
         options = extract_query_and_headers_from data

lib/buildkit/client/agents.rb

@@ -4,6 +4,8 @@ module Buildkit
   class Client
     # Methods for the Agents API
     #
+    # Identifier arguments must be single URL path segments; see {Buildkit::InvalidRouteSegment}.
+    #
     # @see https://buildkite.com/docs/api/agents
     module Agents
       # List agents
@@ -13,7 +15,7 @@ module Agents
       # @example
       #   Buildkit.agents('my-great-org')
       def agents(org, options = {})
-        get("/v2/organizations/#{org}/agents", options)
+        get("/v2/organizations/#{route_segment(org, :org)}/agents", options)
       end
 
       # Get an agent
@@ -25,7 +27,7 @@ def agents(org, options = {})
       # @example
       #   Buildkit.agent('my-great-org', '0b461f65-e7be-4c80-888a-ef11d81fd971')
       def agent(org, id, options = {})
-        get("/v2/organizations/#{org}/agents/#{id}", options)
+        get("/v2/organizations/#{route_segment(org, :org)}/agents/#{route_segment(id, :id)}", options)
       end
 
       # Stop an agent
@@ -36,7 +38,7 @@ def agent(org, id, options = {})
       # @example Stop an  agent
       #   Buildkit.stop_agent('my-great-org', '16940c91-f12d-4122-8154-0edf6c0978c2')
       def stop_agent(org, id, options = {})
-        put("/v2/organizations/#{org}/agents/#{id}/stop", options)
+        put("/v2/organizations/#{route_segment(org, :org)}/agents/#{route_segment(id, :id)}/stop", options)
       end
     end
   end

lib/buildkit/client/artifacts.rb

@@ -4,6 +4,8 @@ module Buildkit
   class Client
     # Methods for the Artifacts API
     #
+    # Identifier arguments must be single URL path segments; see {Buildkit::InvalidRouteSegment}.
+    #
     # @see https://buildkite.com/docs/api/artifacts
     module Artifacts
       # List all artifacts for a build
@@ -13,7 +15,8 @@ module Artifacts
       # @example
       #   Buildkit.artifacts('my-great-org', 'great-pipeline', 42)
       def artifacts(org, pipeline, build, options = {})
-        get("/v2/organizations/#{org}/pipelines/#{pipeline}/builds/#{build}/artifacts", options)
+        get("/v2/organizations/#{route_segment(org, :org)}/pipelines/#{route_segment(pipeline, :pipeline)}" \
+            "/builds/#{route_segment(build, :build)}/artifacts", options)
       end
 
       # List all artifacts for a job
@@ -23,7 +26,8 @@ def artifacts(org, pipeline, build, options = {})
       # @example
       #   Buildkit.job_artifacts('my-great-org', 'great-pipeline', 42, '76365070-34d5-4104-8b91-952780f8029f')
       def job_artifacts(org, pipeline, build, job, options = {})
-        get("/v2/organizations/#{org}/pipelines/#{pipeline}/builds/#{build}/jobs/#{job}/artifacts", options)
+        get("/v2/organizations/#{route_segment(org, :org)}/pipelines/#{route_segment(pipeline, :pipeline)}" \
+            "/builds/#{route_segment(build, :build)}/jobs/#{route_segment(job, :job)}/artifacts", options)
       end
     end
   end

lib/buildkit/client/builds.rb

@@ -4,6 +4,8 @@ module Buildkit
   class Client
     # Methods for the Builds API
     #
+    # Identifier arguments must be single URL path segments; see {Buildkit::InvalidRouteSegment}.
+    #
     # @see https://buildkite.com/docs/api/builds
     module Builds
       # List all builds
@@ -24,7 +26,7 @@ def builds(options = {})
       # @example
       #   Buildkit.organization_builds('my-great-org'))
       def organization_builds(org, options = {})
-        get("/v2/organizations/#{org}/builds", options)
+        get("/v2/organizations/#{route_segment(org, :org)}/builds", options)
       end
 
       # List builds for a pipeline
@@ -36,7 +38,7 @@ def organization_builds(org, options = {})
       # @example
       #   Buildkit.pipeline_builds('my-great-org', 'great-pipeline')
       def pipeline_builds(org, pipeline, options = {})
-        get("/v2/organizations/#{org}/pipelines/#{pipeline}/builds", options)
+        get(builds_path(org, pipeline), options)
       end
 
       # Get a build
@@ -49,7 +51,7 @@ def pipeline_builds(org, pipeline, options = {})
       # @example
       #   Buildkit.build('my-great-org', 'great-pipeline', 42)
       def build(org, pipeline, number, options = {})
-        get("/v2/organizations/#{org}/pipelines/#{pipeline}/builds/#{number}", options)
+        get("#{builds_path(org, pipeline)}/#{route_segment(number, :number)}", options)
       end
 
       # Rebuild a build
@@ -61,7 +63,7 @@ def build(org, pipeline, number, options = {})
       # @example
       #   Buildkit.rebuild('my-great-org', 'great-pipeline', 42)
       def rebuild(org, pipeline, number, options = {})
-        put("/v2/organizations/#{org}/pipelines/#{pipeline}/builds/#{number}/rebuild", options)
+        put("#{builds_path(org, pipeline)}/#{route_segment(number, :number)}/rebuild", options)
       end
 
       # Create a build
@@ -81,7 +83,7 @@ def rebuild(org, pipeline, number, options = {})
       #   })
       #
       def create_build(org, pipeline, options = {})
-        post("/v2/organizations/#{org}/pipelines/#{pipeline}/builds", options)
+        post(builds_path(org, pipeline), options)
       end
 
       # Cancel a build
@@ -93,7 +95,13 @@ def create_build(org, pipeline, options = {})
       # @example
       #   Buildkit.cancel_build('my-great-org', 'great-pipeline', 42)
       def cancel_build(org, pipeline, number, options = {})
-        put("/v2/organizations/#{org}/pipelines/#{pipeline}/builds/#{number}/cancel", options)
+        put("#{builds_path(org, pipeline)}/#{route_segment(number, :number)}/cancel", options)
+      end
+
+      private
+
+      def builds_path(org, pipeline)
+        "/v2/organizations/#{route_segment(org, :org)}/pipelines/#{route_segment(pipeline, :pipeline)}/builds"
       end
     end
   end

lib/buildkit/client/jobs.rb

@@ -4,6 +4,8 @@ module Buildkit
   class Client
     # Methods for the Jobs API
     #
+    # Identifier arguments must be single URL path segments; see {Buildkit::InvalidRouteSegment}.
+    #
     # @see https://buildkite.com/docs/rest-api/jobs
     module Jobs
       # Retry a job
@@ -17,7 +19,7 @@ module Jobs
       # @example
       #   Buildkit.retry_job('my-great-org', 'great-pipeline', 123, 'my-job-id')
       def retry_job(org, pipeline, build, job, options = {})
-        put("/v2/organizations/#{org}/pipelines/#{pipeline}/builds/#{build}/jobs/#{job}/retry", options)
+        put("#{job_path(org, pipeline, build, job)}/retry", options)
       end
 
       # Get a job's environment variables
@@ -31,7 +33,7 @@ def retry_job(org, pipeline, build, job, options = {})
       # @example
       #   Buildkit.job_env('my-great-org', 'great-pipeline', 123, 'my-job-id')
       def job_env(org, pipeline, build, job, options = {})
-        get("/v2/organizations/#{org}/pipelines/#{pipeline}/builds/#{build}/jobs/#{job}/env", options)
+        get("#{job_path(org, pipeline, build, job)}/env", options)
       end
 
       # Get a job's log output
@@ -45,7 +47,7 @@ def job_env(org, pipeline, build, job, options = {})
       # @example
       #   Buildkit.job_log('my-great-org', 'great-pipeline', 123, 'my-job-id')
       def job_log(org, pipeline, build, job, options = {})
-        get("/v2/organizations/#{org}/pipelines/#{pipeline}/builds/#{build}/jobs/#{job}/log", options)
+        get("#{job_path(org, pipeline, build, job)}/log", options)
       end
 
       # Unblock a job
@@ -65,7 +67,14 @@ def job_log(org, pipeline, build, job, options = {})
       #     }
       #   })
       def unblock(org, pipeline, build, job, options = {})
-        put("/v2/organizations/#{org}/pipelines/#{pipeline}/builds/#{build}/jobs/#{job}/unblock", options)
+        put("#{job_path(org, pipeline, build, job)}/unblock", options)
+      end
+
+      private
+
+      def job_path(org, pipeline, build, job)
+        "/v2/organizations/#{route_segment(org, :org)}/pipelines/#{route_segment(pipeline, :pipeline)}" \
+          "/builds/#{route_segment(build, :build)}/jobs/#{route_segment(job, :job)}"
       end
     end
   end

lib/buildkit/client/organizations.rb

@@ -4,6 +4,8 @@ module Buildkit
   class Client
     # Methods for the Organizations API
     #
+    # Identifier arguments must be single URL path segments; see {Buildkit::InvalidRouteSegment}.
+    #
     # @see https://buildkite.com/docs/api/organizations
     module Organizations
       # List organizations
@@ -24,7 +26,7 @@ def organizations(options = {})
       # @example
       #   Buildkit.organization('my-great-org')
       def organization(org, options = {})
-        get("/v2/organizations/#{org}", options)
+        get("/v2/organizations/#{route_segment(org, :org)}", options)
       end
     end
   end

lib/buildkit/client/pipelines.rb

@@ -4,6 +4,8 @@ module Buildkit
   class Client
     # Methods for the pipelines API
     #
+    # Identifier arguments must be single URL path segments; see {Buildkit::InvalidRouteSegment}.
+    #
     # @see https://buildkite.com/docs/api/pipelines
     module Pipelines
       # List pipelines
@@ -13,7 +15,7 @@ module Pipelines
       # @example
       #   Buildkit.pipelines('my-great-org')
       def pipelines(org, options = {})
-        get("/v2/organizations/#{org}/pipelines", options)
+        get(pipelines_path(org), options)
       end
 
       # Get a pipeline
@@ -25,7 +27,7 @@ def pipelines(org, options = {})
       # @example
       #   Buildkit.pipeline('my-great-org', 'great-pipeline')
       def pipeline(org, pipeline, options = {})
-        get("/v2/organizations/#{org}/pipelines/#{pipeline}", options)
+        get("#{pipelines_path(org)}/#{route_segment(pipeline, :pipeline)}", options)
       end
 
       # Create a pipeline
@@ -48,7 +50,7 @@ def pipeline(org, pipeline, options = {})
       #   })
       #
       def create_pipeline(org, options = {})
-        post("/v2/organizations/#{org}/pipelines", options)
+        post(pipelines_path(org), options)
       end
 
       # Update a pipeline
@@ -63,7 +65,7 @@ def create_pipeline(org, options = {})
       #   })
       #
       def update_pipeline(org, pipeline, options = {})
-        patch("/v2/organizations/#{org}/pipelines/#{pipeline}", options)
+        patch("#{pipelines_path(org)}/#{route_segment(pipeline, :pipeline)}", options)
       end
 
       # Archive a pipeline
@@ -76,7 +78,7 @@ def update_pipeline(org, pipeline, options = {})
       #   Buildkit.archive_pipeline('my-great-org', 'great-pipeline')
       #
       def archive_pipeline(org, pipeline)
-        post("/v2/organizations/#{org}/pipelines/#{pipeline}/archive")
+        post("#{pipelines_path(org)}/#{route_segment(pipeline, :pipeline)}/archive")
       end
 
       # Unarchive a pipeline
@@ -89,7 +91,7 @@ def archive_pipeline(org, pipeline)
       #   Buildkit.unarchive_pipeline('my-great-org', 'great-pipeline')
       #
       def unarchive_pipeline(org, pipeline)
-        post("/v2/organizations/#{org}/pipelines/#{pipeline}/unarchive")
+        post("#{pipelines_path(org)}/#{route_segment(pipeline, :pipeline)}/unarchive")
       end
 
       # Delete a pipeline
@@ -101,7 +103,13 @@ def unarchive_pipeline(org, pipeline)
       #   Buildkit.delete_pipeline('my-great-org', 'great-pipeline')
       #
       def delete_pipeline(org, pipeline)
-        delete("/v2/organizations/#{org}/pipelines/#{pipeline}")
+        delete("#{pipelines_path(org)}/#{route_segment(pipeline, :pipeline)}")
+      end
+
+      private
+
+      def pipelines_path(org)
+        "/v2/organizations/#{route_segment(org, :org)}/pipelines"
       end
     end
   end

lib/buildkit/error.rb

@@ -166,4 +166,7 @@ class ServiceUnavailable < ServerError; end
 
   # Raised when client fails to provide valid Content-Type
   class MissingContentType < ArgumentError; end
+
+  # Raised when a route identifier (org, pipeline, build, job, agent id) is not a single URL path segment
+  class InvalidRouteSegment < ArgumentError; end
 end

lib/buildkit/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module Buildkit
-  VERSION = '1.6.1'
+  VERSION = '1.6.2'
 end

spec/client/route_segment_spec.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# Identifiers interpolated into route templates must be exactly one path
+# segment; otherwise a caller-supplied value can swallow a helper's fixed
+# action suffix and hit a different Buildkite endpoint.
+#
+# No cassettes: a request reaching Faraday under VCR raises
+# VCR::Errors::UnhandledHTTPRequestError, so these fail loudly without the fix.
+describe Buildkit::Client, 'route segment validation' do
+  it 'rejects a job id that re-routes job_log to the job env endpoint' do
+    expect { client.job_log('acme', 'pipe', 42, 'real-job-id/env?x') }
+      .to raise_error(Buildkit::InvalidRouteSegment)
+  end
+
+  it 'rejects a pipeline slug that substitutes another action' do
+    expect { client.archive_pipeline('acme', 'victim/unarchive?ignored') }
+      .to raise_error(Buildkit::InvalidRouteSegment)
+  end
+
+  it 'rejects a build number carrying only a query delimiter' do
+    expect { client.cancel_build('acme', 'pipe', '42?') }
+      .to raise_error(Buildkit::InvalidRouteSegment)
+  end
+
+  it 'rejects dot segments that URI resolution would collapse' do
+    expect { client.organization('..') }
+      .to raise_error(Buildkit::InvalidRouteSegment)
+    expect { client.pipeline('acme', '.') }
+      .to raise_error(Buildkit::InvalidRouteSegment)
+  end
+
+  it 'rejects percent-encoded delimiters in build identifiers' do
+    expect { client.artifacts('acme', 'pipe', '42%2Fjobs') }
+      .to raise_error(Buildkit::InvalidRouteSegment)
+  end
+
+  it 'rejects empty and nil identifiers' do
+    expect { client.pipeline('acme', '') }.to raise_error(Buildkit::InvalidRouteSegment)
+    expect { client.pipeline('acme', nil) }.to raise_error(Buildkit::InvalidRouteSegment)
+  end
+
+  it 'rejects slashes in agent ids with an ArgumentError' do
+    expect { client.stop_agent('acme', 'a/b') }.to raise_error(ArgumentError)
+  end
+end