From 823ef433d5200853ee908f2399164cb6cf0c00d6 Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 08:45:04 -0600 Subject: [PATCH 01/18] Add pstore as a dependency gemspec for ruby 4 Ensure pstore is a explicit dependency when ruby used is >= 4. Ruby 4 removes pstore from stdlib. Ruby 3 uses pstore from stdlib. --- procore.gemspec | 1 + 1 file changed, 1 insertion(+) diff --git a/procore.gemspec b/procore.gemspec index cdde3f6..36c3ae4 100644 --- a/procore.gemspec +++ b/procore.gemspec @@ -41,6 +41,7 @@ Gem::Specification.new do |spec| spec.add_dependency "activesupport", "> 2.4" spec.add_dependency "oauth2", "~> 2.0" + spec.add_dependency "pstore" spec.add_dependency "rest-client", "~> 2.0" spec.metadata['allowed_push_host'] = 'https://rubygems.org' From 7e305aa0818c12bfe0084c8e4b08b35418c03441 Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 08:46:39 -0600 Subject: [PATCH 02/18] Replace OpenStruct with Struct Replaced the OpenStruct objects with Structs on OAuthError, DuckResponse and DuckRequest. They use keyword_init: true because the gem still supports Ruby 3 --- lib/procore/errors.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/procore/errors.rb b/lib/procore/errors.rb index 7941964..0dcf211 100644 --- a/lib/procore/errors.rb +++ b/lib/procore/errors.rb @@ -1,5 +1,3 @@ -require 'ostruct' - module Procore # Generic / catch all error class. All other errors generated by the gem # inherit from this class. @@ -39,15 +37,20 @@ def initialize(message, response: nil) # Raised whenever there is a problem with OAuth. Possible causes: required # credentials are missing or an access token failed to refresh. class OAuthError < Error + # Minimal stand-ins exposing the parts of a Procore::Response that callers + # read from an error, built from the OAuth2 library's response. + DuckRequest = Struct.new(:options, :path, keyword_init: true) + DuckResponse = Struct.new(:code, :body, :headers, :request, keyword_init: true) + def initialize(message, response: nil) @message = message duck_response = if response - OpenStruct.new( + DuckResponse.new( code: response.status, body: response.parsed.presence || response.body, headers: response.headers, - request: OpenStruct.new( + request: DuckRequest.new( options: {}, path: nil, ), From 621a9d2345ea1c7d7fc822d77a81a98a2e54e612 Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 08:56:13 -0600 Subject: [PATCH 03/18] Update CONTRIBUTING.md to use modrn docker-compose command syntax --- CONTRIBUTING.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f5148f9..cf30553 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,7 +15,8 @@ Once a GitHub issue is accepted and assigned to you, please follow general workf - `bundle install` to install the dependencies - Make sure tests pass locally - - You'll need to spin up a few containers (redis and memcached) to run the tests. You can do this by running `docker-compose up -d` in the root of the project. + - You'll need to spin up a few containers (redis and memcached) to run the tests. You can do this by running + `docker compose up -d` in the root of the project. - Run tests with `COVERAGE=true bundle exec rake` 3. Commit your changes in the branch you created in step 2. All commits need to be signed-off. Check the [legal](#legal) section bellow for more details. From 437315f4ca7d9e082d9d5670af33c87d52205e38 Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 09:04:16 -0600 Subject: [PATCH 04/18] Add vendor/bundle to .gitignore --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index 5109efd..671150a 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,5 @@ /spec/reports/ /tmp/ /dump.rdb +/vendor/bundle/ + From a6fb0b94adf6f089178e0db3c45152433b118490 Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 09:06:50 -0600 Subject: [PATCH 05/18] Update supported Ruby versions to 3.3, 3.4, and 4.0; adjust gemspec to require Ruby >= 3.3 Drop EOL-ed rubies <= 3.2 --- .github/workflows/release.yaml | 2 +- .github/workflows/test.yaml | 2 +- CHANGELOG.md | 9 +++++++++ procore.gemspec | 2 +- 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 8a96e99..e5bcdc6 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -27,7 +27,7 @@ jobs: - name: Set up Ruby uses: ruby/setup-ruby@v1 with: - ruby-version: 3.2 + ruby-version: 3.3 bundler-cache: true - name: Installing dependencies run: bundle install diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 5b899bd..b8e45dc 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -11,7 +11,7 @@ jobs: strategy: matrix: os: [ubuntu-latest] - ruby: ['3.0', '3.1', '3.2', '3.3'] + ruby: ['3.3', '3.4', '4.0'] runs-on: ${{ matrix.os }} services: redis: diff --git a/CHANGELOG.md b/CHANGELOG.md index 90bfc77..3c303fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,12 @@ +## Unreleased + +* **Breaking:** Require Ruby 3.3 or newer. Ruby 3.0, 3.1 and 3.2 are end-of-life. + +* Support Ruby 3.4 and 4.0: + * Replace `OpenStruct` (no longer bundled with Ruby 4.0) with `Struct` in `Procore::OAuthError`. + * Add `pstore` as a runtime dependency (no longer bundled with Ruby 4.0). + * Fix `Procore::Auth::Stores::File` failing to load saved tokens on Ruby 3.4+ (`Psych::DisallowedClass`). + ## 1.3.1 (Jan 9, 2024) * Fix a bug with the retry logic in `with_response_handling` resulting in performing one more retry than expected . diff --git a/procore.gemspec b/procore.gemspec index 36c3ae4..bf77629 100644 --- a/procore.gemspec +++ b/procore.gemspec @@ -14,7 +14,7 @@ Gem::Specification.new do |spec| spec.description = "Procore Ruby Gem" spec.homepage = "https://github.com/procore-oss/ruby-sdk" spec.license = "MIT" - spec.required_ruby_version = Gem::Requirement.new('>= 3.0') + spec.required_ruby_version = Gem::Requirement.new('>= 3.3') spec.files = `git ls-files -z`.split("\x0").reject do |f| f.match(%r{^(test|spec|features)/}) From b8360d9687a5848f4e4148d9d267952bb3a218c1 Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 09:22:20 -0600 Subject: [PATCH 06/18] Fix file token store on Ruby 3.4+ YAML in ruby up to 3.3 reads the file with unsafe_load, which accepts any class, while from 3.4 on, it uses safe_load and only allows Symbol. One way to fix this mess would be to save tokens as hashes and rebuild Token on read. This would break token files written by old gems & require migrating. Instead, subclass YAML::Store, allow old style loading of expires_at (Time) and Procore::Auth::Token for backward compatibility. --- lib/procore/auth/stores/file.rb | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/procore/auth/stores/file.rb b/lib/procore/auth/stores/file.rb index 244f579..9628ecb 100644 --- a/lib/procore/auth/stores/file.rb +++ b/lib/procore/auth/stores/file.rb @@ -1,14 +1,25 @@ require "yaml/store" +require "procore/auth/token" module Procore module Auth module Stores class File + # YAML::Store only permits Symbol when loading on Ruby 3.4+, which + # rejects the serialized Token. Allow the classes a token file holds. + class TokenStore < YAML::Store + PERMITTED_CLASSES = [Symbol, Time, Procore::Auth::Token].freeze + + def load(content) + YAML.safe_load(content, permitted_classes: PERMITTED_CLASSES) || {} + end + end + attr_reader :key, :path def initialize(key:, path:) @key = key @path = path - @store = YAML::Store.new(path) + @store = TokenStore.new(path) end def save(token) From adb0e64dd1573d5fc952b2d1bb0dacc8423dba80 Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 10:23:51 -0600 Subject: [PATCH 07/18] Update minitest --- Rakefile | 7 +- procore.gemspec | 1 + .../auth/access_token_credentials_test.rb | 18 +- test/procore/auth/client_credentials_test.rb | 18 +- test/procore/requestable_test.rb | 319 +++++++++++++++++- test/test_helper.rb | 5 +- 6 files changed, 339 insertions(+), 29 deletions(-) diff --git a/Rakefile b/Rakefile index 70cbfa7..3d87232 100644 --- a/Rakefile +++ b/Rakefile @@ -1,14 +1,13 @@ require "bundler/gem_tasks" -require "rake/testtask" +require "minitest/test_task" require "rubocop/rake_task" RuboCop::RakeTask.new(:rubocop) do |t| t.options = ["--display-cop-names"] end -Rake::TestTask.new do |t| - t.libs << "test" - t.test_files = FileList["test/**/*_test.rb"] +Minitest::TestTask.create do |t| + t.test_globs = ["test/**/*_test.rb"] t.warning = false end diff --git a/procore.gemspec b/procore.gemspec index bf77629..1199efb 100644 --- a/procore.gemspec +++ b/procore.gemspec @@ -29,6 +29,7 @@ Gem::Specification.new do |spec| spec.add_development_dependency "dalli" spec.add_development_dependency "fakefs" spec.add_development_dependency "minitest" + spec.add_development_dependency "minitest-mock" spec.add_development_dependency "pry" spec.add_development_dependency "rake" spec.add_development_dependency "redis" diff --git a/test/procore/auth/access_token_credentials_test.rb b/test/procore/auth/access_token_credentials_test.rb index 205d506..50a6dad 100644 --- a/test/procore/auth/access_token_credentials_test.rb +++ b/test/procore/auth/access_token_credentials_test.rb @@ -58,11 +58,11 @@ def test_oauth_client_error_with_html credentials.refresh(token: "token", refresh: "refresh") end - assert_equal error.response.code, 500 - assert_equal error.response.body, "

This is very bad

" - assert_equal error.response.headers, "content-type" => "text/html" + assert_equal 500, error.response.code + assert_equal "

This is very bad

", error.response.body + assert_equal({ "content-type" => "text/html" }, error.response.headers) assert_nil error.response.request.path - assert_equal error.response.request.options, {} + assert_equal({}, error.response.request.options) end def test_oauth_client_error_with_json @@ -89,11 +89,11 @@ def test_oauth_client_error_with_json credentials.refresh(token: "token", refresh: "refresh") end - assert_equal error.response.code, 500 - assert_equal error.response.body, "error" => "Some bad error" - assert_equal error.response.headers, "content-type" => "application/json" + assert_equal 500, error.response.code + assert_equal({ "error" => "Some bad error" }, error.response.body) + assert_equal({ "content-type" => "application/json" }, error.response.headers) assert_nil error.response.request.path - assert_equal error.response.request.options, {} + assert_equal({}, error.response.request.options) end def test_connection_failed_error @@ -136,6 +136,6 @@ def test_bad_uri_error credentials.refresh(token: "token", refresh: "refresh") end - assert_equal error.message, "Host is not a valid URI. Check your host option to make sure it is a properly formed url" + assert_equal "Host is not a valid URI. Check your host option to make sure it is a properly formed url", error.message end end diff --git a/test/procore/auth/client_credentials_test.rb b/test/procore/auth/client_credentials_test.rb index f475f69..4657100 100644 --- a/test/procore/auth/client_credentials_test.rb +++ b/test/procore/auth/client_credentials_test.rb @@ -46,11 +46,11 @@ def test_oauth_client_error_with_html token.refresh end - assert_equal error.response.code, 500 - assert_equal error.response.body, "

This is very bad

" - assert_equal error.response.headers, "content-type" => "text/html" + assert_equal 500, error.response.code + assert_equal "

This is very bad

", error.response.body + assert_equal({ "content-type" => "text/html" }, error.response.headers) assert_nil error.response.request.path - assert_equal error.response.request.options, {} + assert_equal({}, error.response.request.options) end def test_oauth_client_error_with_json @@ -76,11 +76,11 @@ def test_oauth_client_error_with_json token.refresh end - assert_equal error.response.code, 500 - assert_equal error.response.body, "error" => "Some bad error" - assert_equal error.response.headers, "content-type" => "application/json" + assert_equal 500, error.response.code + assert_equal({ "error" => "Some bad error" }, error.response.body) + assert_equal({ "content-type" => "application/json" }, error.response.headers) assert_nil error.response.request.path - assert_equal error.response.request.options, {} + assert_equal({}, error.response.request.options) end def test_connection_failed_error @@ -124,7 +124,7 @@ def test_bad_uri_error token.refresh end - assert_equal error.message, "Host is not a valid URI. Check your host option to make sure it is a properly formed url" + assert_equal "Host is not a valid URI. Check your host option to make sure it is a properly formed url", error.message end def test_procore_oauth_error diff --git a/test/procore/requestable_test.rb b/test/procore/requestable_test.rb index a40ffb5..11d2098 100644 --- a/test/procore/requestable_test.rb +++ b/test/procore/requestable_test.rb @@ -21,6 +21,25 @@ def options end end + def with_max_retries(count) + max_retries_old = Procore.configuration.max_retries + Procore.configuration.max_retries = count + yield + ensure + Procore.configuration.max_retries = max_retries_old + end + + # Runs the block with retry backoff recorded instead of slept. + # + # @return [Array] the waits requested, in order + def record_sleeps(request) + sleeps = [] + request.stub(:sleep, ->(seconds) { sleeps << seconds }) do + yield + end + sleeps + end + def headers { "Accepts" => "application/json", @@ -217,7 +236,7 @@ def test_sync errors: { id: ["Entity with this ID not found"] }, }], } - assert response.body, expected_body + assert_equal expected_body.deep_stringify_keys, response.body end def test_post_with_idempotency_token @@ -326,11 +345,11 @@ def test_error_body headers: {}, ) - begin + error = assert_raises Procore::AuthorizationError do Request.new(token: "token").get("") - rescue Procore::AuthorizationError => e - assert_equal({ "errors" => "Unauthorized" }, e.response.body) end + + assert_equal({ "errors" => "Unauthorized" }, error.response.body) end def test_no_retry_logic @@ -340,7 +359,7 @@ def test_no_retry_logic begin Procore.configuration.max_retries = 0 stub_request(:get, "http://test.com/rest/v1.0/retry_test") - .to_raise(RestClient::Exceptions::Timeout) + .to_timeout # Attempt to perform the request and assert the correct exception is raised assert_raises(Procore::APIConnectionError) do @@ -352,4 +371,294 @@ def test_no_retry_logic Procore.configuration.max_retries = max_retries_old end end + + def test_get_encodes_nested_query + request = stub_request(:get, "http://test.com/rest/v1.0/home") + .with(query: { filters: { status: "open", ids: [1, 2] }, per_page: 5 }) + .to_return(status: 200, body: "", headers: {}) + + Request.new(token: "token").get( + "home", + query: { filters: { status: "open", ids: [1, 2] }, per_page: 5 }, + ) + + assert_requested request + end + + def test_delete_with_query + request = stub_request(:delete, "http://test.com/rest/v1.0/home") + .with(query: { force: true }, headers: headers) + .to_return(status: 200, body: "", headers: {}) + + Request.new(token: "token").delete("home", query: { force: true }) + + assert_requested request + end + + def test_post_multipart_sends_file_and_fields + request = stub_request(:post, "http://test.com/rest/v1.0/home") + .with { |req| + req.body.include?('name="file"; filename="pixel.png"') && + req.body.include?('name="description"') && + req.body.include?("A pixel") + } + .to_return(status: 200, body: "", headers: {}) + + pixel = File.new("test/support/pixel.png", "r") + + Request.new(token: "token").post( + "home", + body: { file: pixel, description: "A pixel" }, + ) + + assert_requested request + end + + def test_response_headers_use_symbol_keys + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return( + status: 200, + body: "", + headers: { "X-Request-Id" => "abc", "Content-Type" => "application/json" }, + ) + + response = Request.new(token: "token").get("home") + + assert_equal "abc", response.headers[:x_request_id] + assert_equal "application/json", response.headers[:content_type] + end + + def test_response_pagination_from_link_header + stub_request(:get, "http://test.com/rest/v1.0/projects") + .to_return( + status: 200, + body: "[]", + headers: { + "Link" => '; rel="next", ' \ + '; rel="last"', + }, + ) + + response = Request.new(token: "token").get("projects") + + assert_equal( + { next: "projects?page=2", last: "projects?page=9" }, + response.pagination, + ) + end + + def test_response_exposes_code_request_and_request_body + stub_request(:post, "http://test.com/rest/v1.0/home") + .to_return(status: 201, body: { id: 1 }.to_json, headers: {}) + + response = Request.new(token: "token").post("home", body: { name: "Name" }) + + assert_equal 201, response.code + assert_equal({ "id" => 1 }, response.body) + assert_equal({ name: "Name" }, response.request_body) + assert_equal "http://test.com/rest/v1.0/home", response.request.url + assert_equal "post", response.request.method.to_s + end + + def test_sync_caps_batch_size_at_1000 + updates = (1..1001).map { |id| { id: id } } + stub_request(:patch, "http://test.com/rest/v1.0/projects/sync") + .to_return(status: 200, body: { entities: [], errors: [] }.to_json) + + Request.new(token: "token").sync( + "projects/sync", + body: { updates: updates }, + options: { batch_size: 5000 }, + ) + + assert_requested :patch, "http://test.com/rest/v1.0/projects/sync", times: 2 + end + + def test_invalid_version_raises + assert_raises Procore::InvalidRequestError do + Request.new(token: "token").get("home", version: "v1") + end + end + + def test_bad_request_error + stub_request(:get, "http://test.com/rest/v1.0/") + .to_return(status: 400, body: "", headers: {}) + + error = assert_raises Procore::InvalidRequestError do + Request.new(token: "token").get("") + end + + assert_equal 400, error.response.code + end + + def test_unprocessable_entity_error + stub_request(:get, "http://test.com/rest/v1.0/") + .to_return(status: 422, body: "", headers: {}) + + error = assert_raises Procore::InvalidRequestError do + Request.new(token: "token").get("") + end + + assert_equal 422, error.response.code + end + + def test_rate_limit_error + stub_request(:get, "http://test.com/rest/v1.0/") + .to_return(status: 429, body: "", headers: {}) + + error = assert_raises Procore::RateLimitError do + Request.new(token: "token").get("") + end + + assert_equal 429, error.response.code + end + + def test_retries_timeout_with_backoff_then_raises + stub_request(:get, "http://test.com/rest/v1.0/retry_test").to_timeout + request = Request.new(token: "token") + + sleeps = with_max_retries(2) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.get("retry_test") + end + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 3 + assert_equal [1.5, 2.25], sleeps + end + + def test_retry_succeeds_after_transient_failure + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_timeout + .then + .to_return(status: 200, body: { ok: true }.to_json, headers: {}) + + request = Request.new(token: "token") + + response = nil + + with_max_retries(1) do + record_sleeps(request) do + response = request.get("retry_test") + end + end + + assert_equal({ "ok" => true }, response.body) + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_retries_connection_refused + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_raise(Errno::ECONNREFUSED) + request = Request.new(token: "token") + + with_max_retries(1) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.get("retry_test") + end + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_retries_connection_reset + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_raise(Errno::ECONNRESET) + request = Request.new(token: "token") + + with_max_retries(1) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.get("retry_test") + end + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_post_is_retried + stub_request(:post, "http://test.com/rest/v1.0/retry_test").to_timeout + request = Request.new(token: "token") + + with_max_retries(1) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.post("retry_test", body: { name: "Name" }) + end + end + end + + assert_requested :post, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_patch_is_retried + stub_request(:patch, "http://test.com/rest/v1.0/retry_test").to_timeout + request = Request.new(token: "token") + + with_max_retries(1) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.patch("retry_test", body: { name: "Name" }) + end + end + end + + assert_requested :patch, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_server_error_is_not_retried + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_return(status: 500, body: "", headers: {}) + + with_max_retries(2) do + assert_raises(Procore::ServerError) do + Request.new(token: "token").get("retry_test") + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 1 + end + + def test_rate_limit_is_not_retried + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_return(status: 429, body: "", headers: {}) + + with_max_retries(2) do + assert_raises(Procore::RateLimitError) do + Request.new(token: "token").get("retry_test") + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 1 + end + + def test_access_token_failure_is_retried + request = stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return(status: 200, body: "", headers: {}) + client = Request.new(token: "token") + token_attempts = 0 + flaky_token = -> { + token_attempts += 1 + if token_attempts == 1 + raise Procore::OAuthError.new("Token refresh failed") + else + "token" + end + } + + with_max_retries(1) do + record_sleeps(client) do + client.stub(:access_token, flaky_token) do + client.get("home") + end + end + end + + assert_requested request, times: 1 + end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 400c91b..46fc14a 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -9,12 +9,13 @@ ]) SimpleCov.start do - track_files "lib/**/*.{rb}" - add_filter "test/" + cover "lib/**/*.{rb}" + skip "test/" end end require "minitest/autorun" +require "minitest/mock" require "webmock/minitest" require "support/auth_stubs" From 5ed6df365f8c0c01d8a9c3477c47ce09b26b2fcd Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 11:56:43 -0600 Subject: [PATCH 08/18] **Redact Bearer Tokens in Logs and Add Logging Improvements** - Add `redact` method to replace Bearer tokens with `###` in logs for security. - Introduce tests to verify Bearer token redaction for `log_info`, `log_error`, and token handling in requests. - Fix `delete` request to include dynamic headers from `options`. - Add tests for `delete` with `company_id` and response request matching RestClient values. - Refactor constants and logging methods for maintainability. --- lib/procore/requestable.rb | 3 +-- lib/procore/util.rb | 27 +++++++++++++++++--- test/procore/requestable_test.rb | 44 ++++++++++++++++++++++++++++++++ test/procore/util_test.rb | 37 +++++++++++++++++++++++++++ 4 files changed, 105 insertions(+), 6 deletions(-) diff --git a/lib/procore/requestable.rb b/lib/procore/requestable.rb index fe0e025..35ab9e2 100644 --- a/lib/procore/requestable.rb +++ b/lib/procore/requestable.rb @@ -230,7 +230,6 @@ def delete(path, version: nil, query: {}, options: {}) "API Request Initiated", path: full_path, method: "DELETE", - headers: headers(options), query: query.to_s, ) @@ -238,7 +237,7 @@ def delete(path, version: nil, query: {}, options: {}) RestClient::Request.execute( method: :delete, url: full_path, - headers: headers.merge(params: query), + headers: headers(options).merge(params: query), timeout: Procore.configuration.timeout, ) end diff --git a/lib/procore/util.rb b/lib/procore/util.rb index 72751c1..833a6b0 100644 --- a/lib/procore/util.rb +++ b/lib/procore/util.rb @@ -9,8 +9,10 @@ def self.log_info(message, meta = {}) end.join(" ") Procore.configuration.logger.info( - "#{colorize('Procore', :yellow)} <<- " \ - "#{colorize(message.ljust(22), :cyan)} <<- #{meta_string}", + redact( + "#{colorize('Procore', :yellow)} <<- " \ + "#{colorize(message.ljust(22), :cyan)} <<- #{meta_string}", + ), ) end @@ -22,11 +24,24 @@ def self.log_error(message, meta = {}) end.join(" ") Procore.configuration.logger.info( - "#{colorize('Procore', :red)} <<- " \ - "#{colorize(message.ljust(22), :red)} <<- #{meta_string}", + redact( + "#{colorize('Procore', :red)} <<- " \ + "#{colorize(message.ljust(22), :red)} <<- #{meta_string}", + ), ) end + # Replaces bearer tokens with "###" so access tokens never reach the logs, + # whichever caller passes them in. + # + # @example + # redact('"Authorization" => "Bearer abc123"') + # #=> '"Authorization" => "Bearer ###"' + def self.redact(text) + text.gsub(BEARER_TOKEN, '\1###') + end + private_class_method :redact + def self.colorize(text, color, bold: false) mode = bold ? 1 : 0 foreground = 30 + COLOR_CODES.fetch(color) @@ -36,6 +51,10 @@ def self.colorize(text, color, bold: false) end private_class_method :colorize + # The token characters allowed by RFC 6750 (b64token). + BEARER_TOKEN = %r{(bearer\s+)[A-Za-z0-9\-._~+/]+=*}i + private_constant :BEARER_TOKEN + COLOR_CODES = { black: 0, red: 1, diff --git a/test/procore/requestable_test.rb b/test/procore/requestable_test.rb index 11d2098..8ab6c7c 100644 --- a/test/procore/requestable_test.rb +++ b/test/procore/requestable_test.rb @@ -287,6 +287,37 @@ def test_post_with_company_id assert_requested request end + def test_delete_with_company_id + request = stub_request(:delete, "http://test.com/rest/v1.0/home") + .with( + headers: headers.merge("procore-company-id" => "1") + ) + .to_return(status: 200, body: "", headers: {}) + + Request.new(token: "token").delete( + "home", + options: { company_id: 1 }, + ) + + assert_requested request + end + + def test_delete_does_not_log_access_token + stub_request(:delete, "http://test.com/rest/v1.0/home") + .to_return(status: 200, body: "", headers: {}) + output = StringIO.new + logger_old = Procore.configuration.logger + Procore.configuration.logger = Logger.new(output) + + begin + Request.new(token: "secret-token").delete("home") + ensure + Procore.configuration.logger = logger_old + end + + refute_includes output.string, "secret-token" + end + def test_post_with_multipart_body request = stub_request(:post, "http://test.com/rest/v1.0/home") .with( @@ -460,6 +491,19 @@ def test_response_exposes_code_request_and_request_body assert_equal "post", response.request.method.to_s end + # Response#request keeps the values RestClient::Request gave callers. + def test_response_request_matches_rest_client_values + stub_request(:get, "http://test.com/rest/v1.0/home") + .with(query: { per_page: 5 }) + .to_return(status: 200, body: "", headers: {}) + + request = Request.new(token: "token").get("home", query: { per_page: 5 }).request + + assert_equal "http://test.com/rest/v1.0/home?per_page=5", request.url + assert_equal "get", request.method + assert_equal "Bearer token", request.headers["Authorization"] + end + def test_sync_caps_batch_size_at_1000 updates = (1..1001).map { |id| { id: id } } stub_request(:patch, "http://test.com/rest/v1.0/projects/sync") diff --git a/test/procore/util_test.rb b/test/procore/util_test.rb index 49018a8..39ce1f4 100644 --- a/test/procore/util_test.rb +++ b/test/procore/util_test.rb @@ -6,8 +6,45 @@ def setup Procore.configuration.logger = Logger.new(@output) end + def teardown + Procore.configuration.logger = nil + end + def test_log_info Procore::Util.log_info("Hello") assert_match /Hello/, @output.string end + + def test_log_info_redacts_bearer_token + Procore::Util.log_info( + "Request", + headers: { "Authorization" => "Bearer abc.DEF-123_xyz=" }, + ) + + refute_includes @output.string, "abc.DEF-123_xyz" + assert_includes @output.string, "Bearer ###" + end + + def test_log_error_redacts_bearer_token + Procore::Util.log_error( + "Request failed", + headers: { "Authorization" => "Bearer abc.DEF-123_xyz=" }, + ) + + refute_includes @output.string, "abc.DEF-123_xyz" + assert_includes @output.string, "Bearer ###" + end + + def test_log_info_redacts_bearer_token_in_message + Procore::Util.log_info("Sent Authorization: bearer secret-token") + + refute_includes @output.string, "secret-token" + assert_includes @output.string, "bearer ###" + end + + def test_log_info_keeps_text_without_bearer_token + Procore::Util.log_info("Request", path: "/rest/v1.0/projects?per_page=5") + + assert_includes @output.string, "/rest/v1.0/projects?per_page=5" + end end From 34d1f7a7eb8f4d2a72f50749492111d2e863395b Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 13:50:35 -0600 Subject: [PATCH 09/18] Replace RestClient with Faraday for Procore API requests - Migrated HTTP logic in `Requestable` from RestClient to Faraday with multipart and retry middleware. - Introduced `Procore::Request` to expose request details while maintaining compatibility with existing behavior. - Updated error handling and connection retries to utilize Faraday's features. - Expanded test coverage, including response metadata, redirects, and retry scenarios. - Adjusted dependencies in `procore.gemspec` to remove RestClient and add Faraday-related gems. --- Rakefile | 11 + lib/procore.rb | 1 + lib/procore/auth/access_token_credentials.rb | 2 +- lib/procore/request.rb | 20 ++ lib/procore/requestable.rb | 243 +++++++++++++----- procore.gemspec | 7 +- .../auth/access_token_credentials_test.rb | 27 +- test/procore/auth/stores/dalli_store_test.rb | 5 +- test/procore/requestable_test.rb | 89 ++++++- test/procore/rest_client_leakage_test.rb | 149 +++++++++++ test/support/retry_sleeps.rb | 15 ++ test/test_helper.rb | 1 + 12 files changed, 492 insertions(+), 78 deletions(-) create mode 100644 lib/procore/request.rb create mode 100644 test/procore/rest_client_leakage_test.rb create mode 100644 test/support/retry_sleeps.rb diff --git a/Rakefile b/Rakefile index 3d87232..6b2c1b7 100644 --- a/Rakefile +++ b/Rakefile @@ -1,3 +1,4 @@ +require "bundler/audit/task" require "bundler/gem_tasks" require "minitest/test_task" require "rubocop/rake_task" @@ -6,6 +7,16 @@ RuboCop::RakeTask.new(:rubocop) do |t| t.options = ["--display-cop-names"] end +Bundler::Audit::Task.new + +namespace :security do + desc "Update the vulnerability database and audit Gemfile.lock" + task :audit do + Rake::Task["bundle:audit:update"].invoke + Rake::Task["bundle:audit"].invoke + end +end + Minitest::TestTask.create do |t| t.test_globs = ["test/**/*_test.rb"] t.warning = false diff --git a/lib/procore.rb b/lib/procore.rb index 91b1f65..0b02ef7 100644 --- a/lib/procore.rb +++ b/lib/procore.rb @@ -17,6 +17,7 @@ module Procore require "procore/configuration" require "procore/defaults" require "procore/errors" +require "procore/request" require "procore/response" require "procore/util" require "procore/version" diff --git a/lib/procore/auth/access_token_credentials.rb b/lib/procore/auth/access_token_credentials.rb index fc53ab6..eca3051 100644 --- a/lib/procore/auth/access_token_credentials.rb +++ b/lib/procore/auth/access_token_credentials.rb @@ -39,7 +39,7 @@ def revoke(token:) token: token.access_token, } client.request(:post, "/oauth/revoke", body: request) - rescue RestClient::ExceptionWithResponse + rescue OAuth2::Error => e raise OAuthError.new(e.description, response: e.response) end diff --git a/lib/procore/request.rb b/lib/procore/request.rb new file mode 100644 index 0000000..acee5d6 --- /dev/null +++ b/lib/procore/request.rb @@ -0,0 +1,20 @@ +module Procore + # The request that produced a Response. Exposes the same values callers read + # from RestClient::Request before the gem moved to Faraday. + # + # @example + # response = client.get("projects", query: { per_page: 5 }) + # response.request.url #=> "https://api.procore.com/rest/v1.0/projects?per_page=5" + # response.request.method #=> "get" + # + # @!attribute [r] method + # @return [String] HTTP method, lowercase. + # @!attribute [r] url + # @return [String] Full URL, including the query string. + # @!attribute [r] headers + # @return [Hash] Request headers as sent. + # @!attribute [r] payload + # @return [String, nil] Request body, or nil when there was none or it was + # a multipart upload. + Request = Struct.new(:method, :url, :headers, :payload, keyword_init: true) +end diff --git a/lib/procore/requestable.rb b/lib/procore/requestable.rb index 35ab9e2..f5eb5fa 100644 --- a/lib/procore/requestable.rb +++ b/lib/procore/requestable.rb @@ -1,5 +1,9 @@ -require "rest-client" +require "faraday" +require "faraday/multipart" +require "faraday/retry" +require "mime/types" require "procore/errors" +require "procore/request" module Procore # Module which defines HTTP verbs GET, POST, PUT, PATCH and DELETE. Is @@ -11,13 +15,41 @@ module Procore # @example Using #post: # client.post("projects", name: "New Project") module Requestable + # Failures raised as APIConnectionError once retries are used up. HTTP_EXCEPTIONS = [ + Faraday::ConnectionFailed, + Faraday::SSLError, + Faraday::TimeoutError, + Procore::OAuthError, + ].freeze + + # Connection failures worth retrying: the connection timed out, or the + # server refused, reset or closed it. DNS, unreachable-host and TLS + # failures are not retried. faraday-net_http raises connect timeouts + # (Net::OpenTimeout) as ConnectionFailed, hence Timeout::Error here. + RETRIABLE_CONNECTION_ERRORS = [ Errno::ECONNREFUSED, Errno::ECONNRESET, - Procore::OAuthError, - RestClient::Exceptions::Timeout, - RestClient::ServerBrokeConnection, + EOFError, + Timeout::Error, ].freeze + + RETRY_IF = lambda do |_env, exception| + if exception.is_a?(Faraday::ConnectionFailed) + RETRIABLE_CONNECTION_ERRORS.any? do |error| + exception.wrapped_exception.is_a?(error) + end + else + true + end + end + private_constant :RETRIABLE_CONNECTION_ERRORS, :RETRY_IF + + # GET follows redirects, as it did with rest-client. + REDIRECT_STATUSES = [301, 302, 303, 307, 308].freeze + MAX_REDIRECTS = 10 + private_constant :REDIRECT_STATUSES, :MAX_REDIRECTS + # @param path [String] URL path # @param version [String] API version # @param query [Hash] Query options to pass along with the request @@ -38,12 +70,7 @@ def get(path, version: nil, query: {}, options: {}) ) with_response_handling do - RestClient::Request.execute( - method: :get, - url: full_path, - headers: headers(options).merge(params: query), - timeout: Procore.configuration.timeout, - ) + get_following_redirects(full_path, query, headers(options)) end end @@ -72,13 +99,7 @@ def post(path, version: nil, body: {}, options: {}) ) with_response_handling(request_body: body) do - RestClient::Request.execute( - method: :post, - url: full_path, - payload: payload(body), - headers: headers(options), - timeout: Procore.configuration.timeout, - ) + perform(:post, full_path, body: payload(body), headers: headers(options)) end end @@ -103,13 +124,7 @@ def put(path, version: nil, body: {}, options: {}) ) with_response_handling(request_body: body) do - RestClient::Request.execute( - method: :put, - url: full_path, - payload: payload(body), - headers: headers(options), - timeout: Procore.configuration.timeout, - ) + perform(:put, full_path, body: payload(body), headers: headers(options)) end end @@ -138,13 +153,7 @@ def patch(path, version: nil, body: {}, options: {}) ) with_response_handling(request_body: body) do - RestClient::Request.execute( - method: :patch, - url: full_path, - payload: payload(body), - headers: headers(options), - timeout: Procore.configuration.timeout, - ) + perform(:patch, full_path, body: payload(body), headers: headers(options)) end end @@ -193,12 +202,11 @@ def sync(path, version: nil, body: {}, options: {}) responses = groups.map do |group| batched_body = body.merge(updates: group) with_response_handling(request_body: batched_body) do - RestClient::Request.execute( - method: :patch, - url: full_path, - payload: payload(batched_body), + perform( + :patch, + full_path, + body: payload(batched_body), headers: headers(options), - timeout: Procore.configuration.timeout, ) end end @@ -234,12 +242,7 @@ def delete(path, version: nil, query: {}, options: {}) ) with_response_handling do - RestClient::Request.execute( - method: :delete, - url: full_path, - headers: headers(options).merge(params: query), - timeout: Procore.configuration.timeout, - ) + perform(:delete, full_path, params: query, headers: headers(options)) end end @@ -251,52 +254,43 @@ def with_response_handling(request_body: nil) begin result = yield - rescue *HTTP_EXCEPTIONS => e + rescue Procore::OAuthError + # The access token is fetched before Faraday sends the request, so the + # retry middleware never sees this failure. Retry it here instead. if retries < Procore.configuration.max_retries retries += 1 sleep 1.5**retries retry else - raise APIConnectionError.new( - "Cannot connect to the Procore API. Double check your timeout " \ - "settings to ensure requests are not being cancelled before they " \ - "can complete. Try setting the timeout and max_retries to larger " \ - "values.", - ), e + raise_connection_error end - rescue RestClient::ExceptionWithResponse => e - result = e.response + rescue *HTTP_EXCEPTIONS + raise_connection_error end - response = Procore::Response.new( - body: result.body, - headers: result.headers, - code: result.code, - request: result.request, - request_body: request_body, - ) + response = build_response(result, request_body) - case result.code + case response.code when 200..299 Util.log_info( "API Request Finished ", - path: result.request.url, - status: result.code.to_s, + path: response.request.url, + status: response.code.to_s, duration: "#{((Time.now - request_start_time) * 1000).round(0)}ms", - request_id: result.headers["x-request-id"], + request_id: response.headers[:x_request_id], ) else Util.log_error( "API Request Failed", - path: result.request.url, - status: result.code.to_s, + path: response.request.url, + status: response.code.to_s, duration: "#{((Time.now - request_start_time) * 1000).round(0)}ms", - request_id: result.headers["x-request-id"], + request_id: response.headers[:x_request_id], retries: retries, ) end - case result.code + case response.code when 200..299 response when 401 @@ -347,25 +341,134 @@ def headers(options = {}) "User-Agent" => Procore.configuration.user_agent, }.tap do |headers| if options[:idempotency_token] - headers["Idempotency-Token"] = options[:idempotency_token] + headers["Idempotency-Token"] = options[:idempotency_token].to_s end if options[:company_id] - headers["procore-company-id"] = options[:company_id] + headers["procore-company-id"] = options[:company_id].to_s end end end + # Raised from inside a rescue, so the original error becomes the cause. + def raise_connection_error + raise APIConnectionError.new( + "Cannot connect to the Procore API. Double check your timeout " \ + "settings to ensure requests are not being cancelled before they " \ + "can complete. Try setting the timeout and max_retries to larger " \ + "values.", + ) + end + + def connection + Faraday.new do |conn| + conn.options.timeout = Procore.configuration.timeout + # Retry runs before multipart so each attempt re-encodes the upload. + conn.request :retry, + max: Procore.configuration.max_retries, + interval: 1.5, + backoff_factor: 1.5, + exceptions: [Faraday::TimeoutError, Faraday::ConnectionFailed], + methods: [], + retry_if: RETRY_IF + conn.request :multipart + conn.adapter Faraday.default_adapter + end + end + + def perform(method, url, headers:, params: nil, body: nil) + # Multipart bodies arrive as a Hash; let the middleware set the + # multipart Content-Type and boundary. + if body.is_a?(Hash) + headers = headers.except("Content-Type") + end + + connection.run_request(method, url, body, headers) do |request| + request.params.update(params) if params + end + end + + def get_following_redirects(url, query, headers) + response = perform(:get, url, params: query, headers: headers) + MAX_REDIRECTS.times do + location = response.headers["location"] + break unless REDIRECT_STATUSES.include?(response.status) && location + + response = perform( + :get, + URI.join(response.env.url.to_s, location).to_s, + headers: headers, + ) + end + response + end + + def build_response(result, request_body) + env = result.env + + Procore::Response.new( + body: result.body, + headers: response_headers(result.headers), + code: result.status, + request: Procore::Request.new( + method: env.method.to_s, + url: env.url.to_s, + headers: env.request_headers.to_h, + payload: (env.request_body if env.request_body.is_a?(String)), + ), + request_body: request_body, + ) + end + + # Keys headers the way rest-client did (:x_request_id, :content_type), + # which callers and Response#pagination rely on. + def response_headers(headers) + headers.to_h.to_h do |name, value| + [name.to_s.downcase.tr("-", "_").to_sym, value] + end + end + def payload(body) if multipart?(body) - body + multipart_body(body) else body.to_json end end def multipart?(body) - RestClient::Payload::has_file?(body) + case body + when Hash then body.values.any? { |value| multipart?(value) } + when Array then body.any? { |value| multipart?(value) } + else file?(body) + end + end + + def multipart_body(body) + case body + when Hash then body.transform_values { |value| multipart_body(value) } + when Array then body.map { |value| multipart_body(value) } + else + if file?(body) + Faraday::Multipart::FilePart.new( + body, + content_type(body.path), + File.basename(body.path), + ) + else + body + end + end + end + + # Guesses from the file extension, as rest-client did. + def content_type(path) + MIME::Types.type_for(path).first&.content_type || + "application/octet-stream" + end + + def file?(value) + value.respond_to?(:path) && value.respond_to?(:read) end def full_path(path, version) diff --git a/procore.gemspec b/procore.gemspec index 1199efb..698c1d4 100644 --- a/procore.gemspec +++ b/procore.gemspec @@ -26,8 +26,10 @@ Gem::Specification.new do |spec| spec.add_development_dependency "actionpack" spec.add_development_dependency "activerecord" spec.add_development_dependency "bundler" + spec.add_development_dependency "bundler-audit" spec.add_development_dependency "dalli" spec.add_development_dependency "fakefs" + spec.add_development_dependency "irb" spec.add_development_dependency "minitest" spec.add_development_dependency "minitest-mock" spec.add_development_dependency "pry" @@ -41,9 +43,12 @@ Gem::Specification.new do |spec| spec.add_development_dependency "webmock" spec.add_dependency "activesupport", "> 2.4" + spec.add_dependency "faraday", "~> 2.0" + spec.add_dependency "faraday-multipart", "~> 1.0" + spec.add_dependency "faraday-retry", "~> 2.0" + spec.add_dependency "mime-types", "~> 3.0" spec.add_dependency "oauth2", "~> 2.0" spec.add_dependency "pstore" - spec.add_dependency "rest-client", "~> 2.0" spec.metadata['allowed_push_host'] = 'https://rubygems.org' spec.metadata['rubygems_mfa_required'] = 'true' diff --git a/test/procore/auth/access_token_credentials_test.rb b/test/procore/auth/access_token_credentials_test.rb index 50a6dad..0851bab 100644 --- a/test/procore/auth/access_token_credentials_test.rb +++ b/test/procore/auth/access_token_credentials_test.rb @@ -1,6 +1,6 @@ require "test_helper" -class Procore::Auth::ClientCredentialsTest < Minitest::Test +class Procore::Auth::AccessTokenCredentialsTest < Minitest::Test def test_refresh_token stub_request(:post, "https://procore.example.com/oauth/token") .with(body: { @@ -138,4 +138,29 @@ def test_bad_uri_error assert_equal "Host is not a valid URI. Check your host option to make sure it is a properly formed url", error.message end + + def test_revoke_failure_raises_oauth_error + stub_request(:post, "https://procore.example.com/oauth/revoke") + .to_return( + status: 500, + body: { error: "Some bad error" }.to_json, + headers: { "Content-Type" => "application/json" }, + ) + credentials = Procore::Auth::AccessTokenCredentials.new( + client_id: "id", + client_secret: "secret", + host: "https://procore.example.com", + ) + token = Procore::Auth::Token.new( + access_token: "token", + refresh_token: "refresh", + expires_at: 0, + ) + + error = assert_raises Procore::OAuthError do + credentials.revoke(token: token) + end + + assert_equal 500, error.response.code + end end diff --git a/test/procore/auth/stores/dalli_store_test.rb b/test/procore/auth/stores/dalli_store_test.rb index 71b3e9f..99f51ba 100644 --- a/test/procore/auth/stores/dalli_store_test.rb +++ b/test/procore/auth/stores/dalli_store_test.rb @@ -3,7 +3,10 @@ class Procore::Auth::Stores::DalliTest < Minitest::Test def setup - @store = Procore::Auth::Stores::Dalli.new(key: "key", dalli: Dalli::Client.new) + @store = Procore::Auth::Stores::Dalli.new( + key: "key", + dalli: Dalli::Client.new(nil, serializer: Dalli::JSONSerializer), + ) @store.save( Procore::Auth::Token.new( access_token: "token", diff --git a/test/procore/requestable_test.rb b/test/procore/requestable_test.rb index 8ab6c7c..930dc61 100644 --- a/test/procore/requestable_test.rb +++ b/test/procore/requestable_test.rb @@ -29,15 +29,16 @@ def with_max_retries(count) Procore.configuration.max_retries = max_retries_old end - # Runs the block with retry backoff recorded instead of slept. + # Runs the block with retry backoff recorded instead of slept. Covers both + # the retry middleware and the access-token retry in Requestable. # # @return [Array] the waits requested, in order def record_sleeps(request) - sleeps = [] - request.stub(:sleep, ->(seconds) { sleeps << seconds }) do + RetrySleeps.recorded.clear + request.stub(:sleep, ->(seconds) { RetrySleeps.recorded << seconds }) do yield end - sleeps + RetrySleeps.recorded.dup end def headers @@ -318,6 +319,22 @@ def test_delete_does_not_log_access_token refute_includes output.string, "secret-token" end + def test_logs_request_id + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return(status: 200, body: "", headers: { "X-Request-Id" => "req-123" }) + output = StringIO.new + logger_old = Procore.configuration.logger + Procore.configuration.logger = Logger.new(output) + + begin + Request.new(token: "token").get("home") + ensure + Procore.configuration.logger = logger_old + end + + assert_includes output.string, "req-123" + end + def test_post_with_multipart_body request = stub_request(:post, "http://test.com/rest/v1.0/home") .with( @@ -445,6 +462,35 @@ def test_post_multipart_sends_file_and_fields assert_requested request end + def test_post_multipart_sends_file_content_type + request = stub_request(:post, "http://test.com/rest/v1.0/home") + .with { |req| + req.body.match?(/filename="pixel.png"\r\nContent-Length: \d+\r\nContent-Type: image\/png/) + } + .to_return(status: 200, body: "", headers: {}) + + pixel = File.new("test/support/pixel.png", "r") + + Request.new(token: "token").post("home", body: { file: pixel }) + + assert_requested request + end + + def test_post_multipart_sends_file_arrays + request = stub_request(:post, "http://test.com/rest/v1.0/home") + .with { |req| req.body.scan('filename="pixel.png"').size == 2 } + .to_return(status: 200, body: "", headers: {}) + + files = [ + File.new("test/support/pixel.png", "r"), + File.new("test/support/pixel.png", "r"), + ] + + Request.new(token: "token").post("home", body: { files: files }) + + assert_requested request + end + def test_response_headers_use_symbol_keys stub_request(:get, "http://test.com/rest/v1.0/home") .to_return( @@ -593,6 +639,41 @@ def test_retry_succeeds_after_transient_failure assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 2 end + def test_retries_read_timeout + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_raise(Net::ReadTimeout) + request = Request.new(token: "token") + + with_max_retries(1) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.get("retry_test") + end + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_access_token_failure_raises_connection_error_after_retries + client = Request.new(token: "token") + failing_token = -> { raise Procore::OAuthError.new("Token refresh failed") } + + error = nil + + with_max_retries(1) do + record_sleeps(client) do + client.stub(:access_token, failing_token) do + error = assert_raises(Procore::APIConnectionError) do + client.get("home") + end + end + end + end + + assert_instance_of Procore::OAuthError, error.cause + end + def test_retries_connection_refused stub_request(:get, "http://test.com/rest/v1.0/retry_test") .to_raise(Errno::ECONNREFUSED) diff --git a/test/procore/rest_client_leakage_test.rb b/test/procore/rest_client_leakage_test.rb new file mode 100644 index 0000000..3b4c2d6 --- /dev/null +++ b/test/procore/rest_client_leakage_test.rb @@ -0,0 +1,149 @@ +require "test_helper" + +# The HTTP library behind Procore::Requestable is an implementation detail. +# These tests fail when rest-client types reach callers of the client, and pin +# rest-client behaviour that callers rely on. See +# docs/adr/0001-replace-rest-client-with-faraday.md. +class Procore::RestClientLeakageTest < Minitest::Test + class Request + include Procore::Requestable + + def access_token + "token" + end + + def base_api_path + "http://test.com" + end + + def options + { + default_version: "v1.0", + } + end + end + + def setup + @max_retries_old = Procore.configuration.max_retries + Procore.configuration.max_retries = 0 + end + + def teardown + Procore.configuration.max_retries = @max_retries_old + end + + def assert_not_rest_client(object) + refute object.class.name.to_s.start_with?("RestClient"), + "#{object.class} leaked to the caller" + end + + def test_response_request_is_not_a_rest_client_object + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return(status: 200, body: "", headers: {}) + + response = Request.new.get("home") + + assert_not_rest_client response.request + end + + def test_error_response_request_is_not_a_rest_client_object + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return(status: 404, body: "", headers: {}) + + error = assert_raises Procore::NotFoundError do + Request.new.get("home") + end + + assert_not_rest_client error.response.request + end + + def test_response_headers_are_a_plain_hash_with_symbol_keys + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return(status: 200, body: "", headers: { "X-Request-Id" => "abc" }) + + response = Request.new.get("home") + + assert_instance_of Hash, response.headers + assert response.headers.keys.all?(Symbol) + end + + def test_timeout_cause_is_not_a_rest_client_exception + stub_request(:get, "http://test.com/rest/v1.0/home").to_timeout + + error = assert_raises Procore::APIConnectionError do + Request.new.get("home") + end + + assert_not_rest_client error.cause + end + + def test_broken_connection_cause_is_not_a_rest_client_exception + stub_request(:get, "http://test.com/rest/v1.0/home").to_raise(EOFError) + + error = assert_raises Procore::APIConnectionError do + Request.new.get("home") + end + + assert_not_rest_client error.cause + end + + def test_ssl_failure_raises_procore_error + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_raise(OpenSSL::SSL::SSLError.new("certificate verify failed")) + + assert_raises Procore::APIConnectionError do + Request.new.get("home") + end + end + + def test_dns_failure_raises_procore_error + stub_request(:get, "http://test.com/rest/v1.0/home").to_raise(SocketError) + + assert_raises Procore::APIConnectionError do + Request.new.get("home") + end + end + + def test_unreachable_host_raises_procore_error + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_raise(Errno::EHOSTUNREACH) + + assert_raises Procore::APIConnectionError do + Request.new.get("home") + end + end + + def test_http_exceptions_constant_has_no_rest_client_classes + names = Procore::Requestable::HTTP_EXCEPTIONS.map(&:name) + + assert_empty names.grep(/\ARestClient\b/) + end + + def test_requiring_procore_does_not_load_rest_client + loaded = system( + RbConfig.ruby, + "-Ilib", + "-e", + 'require "procore"; exit(defined?(RestClient) ? 1 : 0)', + ) + + assert loaded, "require \"procore\" defined RestClient" + end + + # rest-client follows redirects on GET. Faraday does not unless configured, + # so this pins the behaviour across the switch. + def test_get_follows_redirects + stub_request(:get, "http://test.com/rest/v1.0/old") + .to_return( + status: 301, + headers: { "Location" => "http://test.com/rest/v1.0/new" }, + ) + stub_request(:get, "http://test.com/rest/v1.0/new") + .to_return(status: 200, body: { moved: true }.to_json, headers: {}) + + response = Request.new.get("old") + + assert_equal 200, response.code + assert_equal({ "moved" => true }, response.body) + end +end diff --git a/test/support/retry_sleeps.rb b/test/support/retry_sleeps.rb new file mode 100644 index 0000000..78dbbd9 --- /dev/null +++ b/test/support/retry_sleeps.rb @@ -0,0 +1,15 @@ +require "faraday/retry" + +# Records faraday-retry's backoff instead of sleeping, so retry tests run fast +# and can assert on the waits. +module RetrySleeps + def self.recorded + Thread.current[:procore_retry_sleeps] ||= [] + end + + def sleep(seconds) + RetrySleeps.recorded << seconds + end +end + +Faraday::Retry::Middleware.prepend(RetrySleeps) diff --git a/test/test_helper.rb b/test/test_helper.rb index 46fc14a..a238f27 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -20,6 +20,7 @@ require "support/auth_stubs" require "support/database" +require "support/retry_sleeps" require "procore" From f7a108da618a5981fbd992d2b265776d0a72fd2a Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 13:51:05 -0600 Subject: [PATCH 10/18] Add pre-commit hooks for gitleaks and security audit, update SECURITY.md for supported Ruby versions --- .pre-commit-config.yaml | 20 ++++++++++++++++++++ SECURITY.md | 7 +++---- 2 files changed, 23 insertions(+), 4 deletions(-) create mode 100644 .pre-commit-config.yaml diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..ba0196b --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,20 @@ +# Hooks run by prek (https://github.com/j178/prek), or pre-commit. Install them +# with `prek install --hook-type pre-commit`. +repos: + - repo: https://github.com/gitleaks/gitleaks + rev: v8.30.1 + hooks: + - id: gitleaks + stages: [pre-commit] + + - repo: local + hooks: + # Runs on every commit, not just Gemfile.lock changes, so new advisories + # against unchanged gems block the commit too. + - id: security-audit + name: Security Audit + entry: bundle exec rake security:audit + language: system + pass_filenames: false + always_run: true + stages: [pre-commit] diff --git a/SECURITY.md b/SECURITY.md index c009fd7..2f97c00 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -6,11 +6,10 @@ Ruby versions that are currently being supported with security updates. | Version | Supported | | ------- | ------------------ | -| <=2.7 | :x: | -| 3.0 | :white_check_mark: | -| 3.1 | :white_check_mark: | -| 3.2 | :white_check_mark: | +| <=3.2 | :x: | | 3.3 | :white_check_mark: | +| 3.4 | :white_check_mark: | +| 4.0 | :white_check_mark: | ## Reporting a Vulnerability From 7668cf6c98b9ec73986e76e353b64b887a3985a6 Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 13:51:32 -0600 Subject: [PATCH 11/18] Update RuboCop config for Ruby 3.3 compatibility - Set `TargetRubyVersion` to 3.3 and disable `NewCops`. - Replace deprecated cops with their updated equivalents (e.g., `Style/FlipFlop` to `Lint/FlipFlop`). - Adjust options like `ExcludedMethods` to `AllowedMethods`. - Disable Rails-specific cop `Rails/RakeEnvironment`, as the project is a gem. - Refine layout and naming configurations for consistency. --- .rubocop.yml | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index c06ba19..6e9142d 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,9 +1,11 @@ -require: +plugins: - rubocop-rails - rubocop-performance AllCops: - TargetRubyVersion: 2.5 + TargetRubyVersion: 3.3 + NewCops: disable + SuggestExtensions: false Include: - '**/Rakefile' - '**/config.ru' @@ -112,7 +114,7 @@ Metrics/BlockLength: Enabled: false CountComments: true # count full line comments? Max: 25 - ExcludedMethods: [] + AllowedMethods: [] Exclude: - "**/*.rake" - "spec/**/*" @@ -172,7 +174,7 @@ Style/FrozenStringLiteralComment: to help transition from Ruby 2.3.0 to Ruby 3.0. Enabled: false -Style/FlipFlop: +Lint/FlipFlop: Description: 'Checks for flip flops' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-flip-flops' Enabled: false @@ -225,7 +227,7 @@ Style/LineEndConcatenation: line end. Enabled: false -Metrics/LineLength: +Layout/LineLength: Description: 'Limit lines to 80 characters.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#80-character-limits' Max: 150 @@ -306,7 +308,7 @@ Style/PerlBackrefs: StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-perl-regexp-last-matchers' Enabled: false -Naming/PredicateName: +Naming/PredicatePrefix: Description: 'Check the names of predicate methods.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#bool-methods-qmark' ForbiddenPrefixes: @@ -498,7 +500,7 @@ Lint/CircularArgumentReference: Description: "Don't refer to the keyword argument in the default value." Enabled: false -Lint/ConditionPosition: +Layout/ConditionPosition: Description: >- Checks for condition placed in a confusing position relative to the keyword. @@ -606,7 +608,7 @@ Performance/ReverseEach: Reference: 'https://github.com/JuanitoFatas/fast-ruby#enumerablereverseeach-vs-enumerablereverse_each-code' Enabled: false -Performance/Sample: +Style/Sample: Description: >- Use `sample` instead of `shuffle.first`, `shuffle.last`, and `shuffle[Fixnum]`. @@ -675,3 +677,7 @@ Rails/TimeZone: Rails/Validation: Description: 'Use validates :attribute, hash of validations.' Enabled: false + +# This is a gem, not a Rails app: there is no :environment task to depend on. +Rails/RakeEnvironment: + Enabled: false From df2eb58594bc09405b5651c7959ce0c48c912cdd Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Wed, 14 Oct 2026 13:52:01 -0600 Subject: [PATCH 12/18] Release v2.0.0 - **Breaking:** Drop support for Ruby < 3.3 (EOL). Require Ruby 3.3+. - **Breaking:** Replace `rest-client` with `Faraday` (`Procore::Request`, improved error handling, TLS/dns failure reporting). - Fix token revoke errors raising `OAuth2::Error` instead of `Procore::OAuthError`. - Fix request logging to include `X-Request-Id` and redact sensitive tokens. - Fix DELETE requests ignoring `options` (e.g., `Procore-Company-Id`). - Add Ruby 3.4/4.0 support (replace `OpenStruct` with `Struct`, add `pstore`, YAML compatibility in `File` store). --- CHANGELOG.md | 16 +- README.md | 262 +++++++++++++++++- .../0001-replace-rest-client-with-faraday.md | 220 +++++++++++++++ lib/procore/version.rb | 2 +- 4 files changed, 488 insertions(+), 12 deletions(-) create mode 100644 docs/adr/0001-replace-rest-client-with-faraday.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c303fe..11aa089 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,12 +1,26 @@ -## Unreleased +## 2.0.0 (Unreleased) * **Breaking:** Require Ruby 3.3 or newer. Ruby 3.0, 3.1 and 3.2 are end-of-life. +* **Breaking:** Replace rest-client with Faraday (see `docs/adr/0001-replace-rest-client-with-faraday.md` for the upgrade guide): + * `Response#request` is now a `Procore::Request` (`url`, `method`, `headers`, `payload`) instead of a `RestClient::Request`. + * TLS certificate errors, DNS failures and unreachable hosts now raise `Procore::APIConnectionError`. + * `APIConnectionError#cause` and `Requestable::HTTP_EXCEPTIONS` use Faraday exception classes. + * The gem no longer loads rest-client. + +* Fix token revoke failures raising `OAuth2::Error` instead of `Procore::OAuthError`. + +* Fix request logs never including the response's `X-Request-Id`. + * Support Ruby 3.4 and 4.0: * Replace `OpenStruct` (no longer bundled with Ruby 4.0) with `Struct` in `Procore::OAuthError`. * Add `pstore` as a runtime dependency (no longer bundled with Ruby 4.0). * Fix `Procore::Auth::Stores::File` failing to load saved tokens on Ruby 3.4+ (`Psych::DisallowedClass`). +* Fix `delete` ignoring `options:`, so `company_id` was never sent as the `Procore-Company-Id` header on DELETE requests. + +* **Security:** stop logging request headers on DELETE requests. The log line included the `Authorization` header, exposing the access token to anything with access to the logs. All log lines now also replace bearer tokens with `###`. + ## 1.3.1 (Jan 9, 2024) * Fix a bug with the retry logic in `with_response_handling` resulting in performing one more retry than expected . diff --git a/README.md b/README.md index 2aa5f6e..f339461 100644 --- a/README.md +++ b/README.md @@ -6,6 +6,7 @@ #### Table of Contents - [Installation](#installation) +- [Upgrading to 2.0](#upgrading-to-20) - [Configuration](#configuration) - [Making Requests](#making-requests) - [Usage](#usage) @@ -15,12 +16,14 @@ - [Change Number of Results](#change-number-of-results) - [Sync Actions](#sync-actions) - [Stores](#stores) + - [Choosing a Store](#choosing-a-store) - [Session Store](#session-store) - [Redis Store](#redis-store) - [Dalli Store](#dalli-store) - [ActiveRecord Store](#activerecord-store) - [File Store](#file-store) - [Memory Store](#memory-store) + - [Custom Stores](#custom-stores) - [Full Example](#full-example) - [Contributing](#contributing) @@ -32,6 +35,88 @@ Add this line to your application's Gemfile: gem "procore" ``` +The gem requires Ruby 3.3 or newer. + +## Upgrading to 2.0 + +Version 2.0 requires Ruby 3.3 or newer, and sends requests with +[Faraday](https://lostisland.github.io/faraday/) instead of rest-client. The +request methods (`get`, `post`, `put`, `patch`, `delete`, `sync`), their +arguments, `Procore::Response` and the configuration options are unchanged. +What changes is mostly which exceptions you see. The reasoning is recorded in +[docs/adr/0001-replace-rest-client-with-faraday.md](docs/adr/0001-replace-rest-client-with-faraday.md). + +### Exceptions + +Every failure now raises a `Procore::Error` subclass, so +`rescue Procore::Error` catches anything the client raises for a failed request. + +| Failure | 1.x raises | 2.0 raises | `cause` in 1.x | `cause` in 2.0 | +|-----------------------------------|-----------------------------------------|--------------------------|---------------------------------------|-----------------------------| +| HTTP 401 | `AuthorizationError` | unchanged | – | – | +| HTTP 403 | `ForbiddenError` | unchanged | – | – | +| HTTP 404 | `NotFoundError` | unchanged | – | – | +| HTTP 400, 422 | `InvalidRequestError` | unchanged | – | – | +| HTTP 429 | `RateLimitError` | unchanged | – | – | +| Any other non-2xx status | `ServerError` | unchanged | – | – | +| Connect timeout | `APIConnectionError` | unchanged | `RestClient::Exceptions::OpenTimeout` | `Faraday::ConnectionFailed` | +| Read timeout | `APIConnectionError` | unchanged | `RestClient::Exceptions::ReadTimeout` | `Faraday::TimeoutError` | +| Connection refused | `APIConnectionError` | unchanged | `Errno::ECONNREFUSED` | `Faraday::ConnectionFailed` | +| Connection reset | `APIConnectionError` | unchanged | `Errno::ECONNRESET` | `Faraday::ConnectionFailed` | +| Server closed the connection | `APIConnectionError` | unchanged | `RestClient::ServerBrokeConnection` | `Faraday::ConnectionFailed` | +| Access token could not be fetched | `APIConnectionError` | unchanged | `Procore::OAuthError` | unchanged | +| TLS certificate error | `RestClient::SSLCertificateNotVerified` | **`APIConnectionError`** | – | `Faraday::SSLError` | +| DNS lookup failed | `SocketError` | **`APIConnectionError`** | – | `Faraday::ConnectionFailed` | +| Host unreachable | `Errno::EHOSTUNREACH` | **`APIConnectionError`** | – | `Faraday::ConnectionFailed` | + +Retries work as before: timeouts, refused and reset connections, and token +fetch failures are retried `max_retries` times with backoff before +`APIConnectionError` is raised. HTTP error statuses, including 429 and 5xx, are +not retried. Every request method is retried, including `post` and `patch`. + +To upgrade: + +- **If you rescue rest-client or socket errors,** rescue + `Procore::APIConnectionError` instead. + + ```ruby + # 1.x + rescue RestClient::SSLCertificateNotVerified, SocketError, Errno::EHOSTUNREACH => e + + # 2.0 + rescue Procore::APIConnectionError => e + ``` + +- **If you inspect `cause`,** check for the Faraday class from the table. + Faraday keeps the underlying error, so the original `Errno` or `SocketError` + is still reachable as `e.cause.wrapped_exception`. + + ```ruby + # 1.x + retry_later if e.cause.is_a?(Errno::ECONNREFUSED) + + # 2.0 + retry_later if e.cause.wrapped_exception.is_a?(Errno::ECONNREFUSED) + ``` + +- **If you read `Procore::Requestable::HTTP_EXCEPTIONS`,** it now lists Faraday + exception classes instead of `RestClient::Exceptions::Timeout` and + `RestClient::ServerBrokeConnection`. + +### Other changes + +- **`Response#request` is no longer a `RestClient::Request`.** It is a + `Procore::Request`. `url`, `method` (a lowercase string such as `"get"`) and + `headers` return the same values as before. `payload` now returns the request + body as a string; in 1.x, reading it raised `IOError`. +- **The gem no longer loads rest-client.** If your application uses + `RestClient` directly but only got it through this gem, add it to your + Gemfile: + + ```ruby + gem "rest-client" + ``` + ## Configuration The Procore Gem exposes a configuration with several options. @@ -115,11 +200,11 @@ delete(path, version: "", query: {}, options: {}) All paths are relative, the gem will handle expanding them. An API version can be specified in the `version:` argument, otherwise the `default_version` in the configuration will be used. -| Example | Requested URL | -| --- | --- | -| `client.get("me")` | `https://api.procore.com/rest/v1.0/me` | -| `client.get("me", version: "v1.1")` | `https://api.procore.com/rest/v1.1/me` | -| `client.get("me", version: "vapid")` | `https://api.procore.com/vapid/me` | +| Example | Requested URL | +|--------------------------------------|----------------------------------------| +| `client.get("me")` | `https://api.procore.com/rest/v1.0/me` | +| `client.get("me", version: "v1.1")` | `https://api.procore.com/rest/v1.1/me` | +| `client.get("me", version: "vapid")` | `https://api.procore.com/vapid/me` | #### Requiring `company_id` in the `options` @@ -417,6 +502,58 @@ Stores contain logic for accessing, storing, and managing access tokens. The Procore API uses expiring tokens - this gem abstracts away the need to manually refresh tokens. +How the client uses its store: + +- Before every request, the client reads the token from the store. +- When the access token has expired, the client refreshes it with the refresh + token and saves the new access and refresh tokens back to the store. +- If the store has no token, the request raises `Procore::MissingTokenError` + and the user has to sign in again. + +So a store must keep the token for as long as you want the user to stay signed +in, and must be writable from everywhere the client runs, including background +jobs. + +### Choosing a Store + +| Store | Where the token lives | Survives a restart | Shared between processes and servers | Best for | +|---|---|---|---|---| +| [ActiveRecord](#activerecord-store) | A row in your database, usually the user | Yes | Yes | Rails apps with a user model, including background jobs | +| [Redis](#redis-store) | A Redis key | Yes, if Redis persistence is on | Yes | Apps without a suitable model, or tokens not tied to a user | +| [Dalli](#dalli-store) | A memcached key | No; memcached may also evict it | Yes | Only when an occasional forced sign-in is acceptable | +| [Session](#session-store) | The user's session | As long as the session lasts | Only within that user's requests | Not recommended; see the warning below | +| [File](#file-store) | A YAML file on disk | Yes | Same machine only | Command-line tools and scripts | +| [Memory](#memory-store) | A Ruby object | No | No, not even between store instances | Tests | + +Which to use: + +- **Rails app where users connect their Procore account:** use the + [ActiveRecord store](#activerecord-store) on the user model. The token lives + next to the user, survives deploys and is available to background jobs. Use + [Active Record encryption](https://guides.rubyonrails.org/active_record_encryption.html) + (`encrypts :access_token, :refresh_token`) so tokens aren't stored in plain + text. +- **Background jobs or several app servers, without a model to hold the + token** (for example, one integration-wide token): use the + [Redis store](#redis-store) with persistence turned on. +- **Command-line tool or script:** use the [File store](#file-store), so users + don't sign in for every command. +- **Tests:** use the [Memory store](#memory-store). +- **Avoid** the [Session store](#session-store): the token is unavailable to + background jobs, and depending on your session store it may be logged or kept + client-side. Use the [Dalli store](#dalli-store) only if losing a token now + and then is acceptable, because memcached forgets data on restart and evicts + it under memory pressure. + +Every built-in store keeps tokens in plain text: as JSON in Redis, memcached and +the session, as YAML in the File store, and in your table's columns for +ActiveRecord unless you encrypt them. Protect the storage the way you would a +password. + +The gem doesn't lock around token refresh. If several processes share a token +and it expires, more than one may refresh it at the same time, and the last +save wins. + Available stores: ### Session Store @@ -437,6 +574,12 @@ unencrypted. Tokens should not be stored client-side if it can be avoided. store = Procore::Auth::Stores::Session.new(session: session) ``` +- **Use it when:** prototyping a web app where every API call happens inside + the signed-in user's own requests. +- **Watch out for:** background jobs can't reach the token. With Rails' default + cookie session store, the token travels to and from the browser on every + request. + ### Redis Store Options: `redis`: Instance of Redis @@ -451,6 +594,12 @@ The key will usually be the id of the current user. store = Procore::Auth::Stores::Redis.new(redis: Redis.new, key: current_user.id) ``` +- **Use it when:** several processes or servers need the token, for example + web servers and background jobs, and there's no model to hold it. +- **Watch out for:** the token is stored under `procore-redis-{key}` with no + expiry. It only survives a Redis restart if persistence (RDB or AOF) is on, + and an eviction policy such as `allkeys-lru` can remove it. + ### Dalli Store Options: `dalli`: Instance of Dalli @@ -461,10 +610,21 @@ There's two required options, `dalli` which is an instance of a Dalli client, and `key` which is a unique key which will be used to save / retrieve an access token. The key will usually be the id of the current user. +The store saves tokens as JSON strings, so the client doesn't need Marshal. +With Dalli 5 or newer, pass `serializer: Dalli::JSONSerializer` to avoid +deserializing with Marshal and to silence Dalli's security warning. + ```ruby -store = Procore::Auth::Stores::Dalli.new(dalli: Dalli.new, key: current_user.id) +dalli = Dalli::Client.new(servers, serializer: Dalli::JSONSerializer) +store = Procore::Auth::Stores::Dalli.new(dalli: dalli, key: current_user.id) ``` +- **Use it when:** you already run memcached and users signing in again after + a cache restart is acceptable. +- **Watch out for:** memcached is a cache. A restart clears every token, and + under memory pressure it evicts keys, so users are asked to sign in again + without warning. + ### ActiveRecord Store Options: `object`: Instance of an ActiveRecord model. @@ -478,6 +638,14 @@ The following columns **must** exist on the model you pass in: store = Procore::Auth::Stores::ActiveRecord.new(object: current_user) ``` +- **Use it when:** tokens belong to a record you already have, usually the + user. This is the best default for Rails apps. +- **Watch out for:** `expires_at` holds a Unix timestamp, so make it an integer + column. Encrypt `access_token` and `refresh_token`, for example with + `encrypts :access_token, :refresh_token`. Each save runs `update`, so the + model's validations and callbacks run too, and if validation fails the new + token is silently not saved. + ### File Store Options: `path`: Path to a file to store access tokens @@ -491,6 +659,13 @@ authenticate every single command. store = Procore::Auth::Stores::File.new(path: "./tokens.yml", key: current_user.id) ``` +- **Use it when:** building a command-line tool or script that runs on one + machine. One file can hold tokens for several keys, and file locking keeps + concurrent processes on that machine from corrupting it. +- **Watch out for:** the file is plain YAML, created with your default + permissions, which usually lets every user on the machine read it. Restrict + it (`chmod 600 tokens.yml`) and keep it out of version control. + ### Memory Store Options: `key`: Unique identifier to an access token @@ -503,6 +678,62 @@ used in tests. store = Procore::Auth::Stores::Memory.new(key: current_user.id) ``` +- **Use it when:** writing tests. +- **Watch out for:** each store instance keeps its own token. A new + `Memory.new(key: ...)` with the same key starts empty, so reuse the same + instance. + +### Custom Stores + +Any object can be a store if it responds to three methods: + +- `save(token)`: persist a `Procore::Auth::Token`. +- `fetch`: return the saved `Procore::Auth::Token`, or `nil` if there is none. +- `delete`: remove the saved token. + +The client also logs the store with `to_s`, so define it to identify the store +without printing the token. + +This example keeps tokens in HashiCorp Vault with the +[vault](https://github.com/hashicorp/vault-ruby) gem: + +```ruby +class VaultStore + def initialize(key:) + @key = key + end + + def save(token) + Vault.logical.write("secret/procore/#{@key}", **token_attributes(token)) + end + + def fetch + secret = Vault.logical.read("secret/procore/#{@key}") + if secret + Procore::Auth::Token.new(**secret.data.slice(:access_token, :refresh_token, :expires_at)) + end + end + + def delete + Vault.logical.delete("secret/procore/#{@key}") + end + + def to_s + "Vault, Key: #{@key}" + end + + private + + def token_attributes(token) + { + access_token: token.access_token, + refresh_token: token.refresh_token, + expires_at: token.expires_at, + } + end +end +``` + ## Full Example ```ruby @@ -559,15 +790,26 @@ To contribute to the gem, please clone the repo and cut a new branch. In the PR, Please **do not** bump the gem version in your PR. This will be done in a follow up by the gem maintainers. +The repository has pre-commit hooks for secret scanning (Gitleaks), a +vulnerability audit of `Gemfile.lock` (bundler-audit), and a guard against +committing directly to `main`. Install them with +[prek](https://github.com/j178/prek) (or `pre-commit`): + +```sh +prek install --hook-type pre-commit +``` + ## Releasing Gem maintainer should follow these steps to release a new version: 1. Merge all PRs into the `main` branch that you want included in the release. -2. Update `version.rb` to a new semantic version, and move unreleased items in `CHANGELOG.md` into a section for that version. -3. Stage and commit changes. `git commit -a` -4. Create a tag for the new version. `git tag -a {version}` -5. Push the tagged commit `git push --follow-tags` and a new version will be published to RubyGems. +2. Create a release branch from `main`. `git switch -c release-{version}` +3. Update `version.rb` to a new semantic version, and move unreleased items in `CHANGELOG.md` into a section for that version. +4. Stage and commit changes. `git commit -a` +5. Open a PR for the release branch and merge it into `main`. Direct commits to `main` are blocked by the pre-commit hook. +6. Once the Test workflow passes on `main`, the Release workflow sees the change to `version.rb` and publishes the new version to RubyGems. +7. Tag the merge commit for the new version. `git switch main && git pull && git tag -a {version} && git push --tags` ### Tests diff --git a/docs/adr/0001-replace-rest-client-with-faraday.md b/docs/adr/0001-replace-rest-client-with-faraday.md new file mode 100644 index 0000000..9947641 --- /dev/null +++ b/docs/adr/0001-replace-rest-client-with-faraday.md @@ -0,0 +1,220 @@ +# 1. Replace rest-client with Faraday + +Date: 2026-10-14 + +## Status + +Accepted + +## Context + +`Procore::Requestable` sends every API request through `rest-client`. Its last +release, 2.1.0, was in 2019, and the project is effectively unmaintained. It +holds back the dependency tree: `http-accept` is pinned below 2.0 because of +it. + +rest-client is used almost entirely in `lib/procore/requestable.rb`, but its +types show through the public API in a few places: + +- `Response#headers` is rest-client's header hash, with keys converted to + symbols (`:link`, `:content_type`). The README tells users to inspect + `e.response.headers`, and pagination reads `headers[:link]`. +- `Response#request` returns a `RestClient::Request`. It is public but + undocumented. +- File uploads work by passing a `File` in `body:`. rest-client detects it and + sends `multipart/form-data`. +- The cause attached to `APIConnectionError` is a rest-client exception. + +Two existing bugs come from the rest-client coupling: + +- `AccessTokenCredentials#revoke` rescues `RestClient::ExceptionWithResponse`, + but oauth2 makes its calls through Faraday, so the rescue never runs. It also + refers to an unbound `e`. +- The request logs look up `headers["x-request-id"]` with a string key, but + rest-client stores headers under symbol keys (`:x_request_id`), so the + logged request ID is likely always `nil`. + +Options considered: + +- **Faraday.** Actively maintained, and already installed because oauth2 + depends on it. Each client gets its own connection object, and the HTTP + adapter can be swapped. Multipart and retries come from the + `faraday-multipart` and `faraday-retry` gems, both from the same maintainers. +- **HTTParty.** Maintained, with multipart built in. It would be a new gem with + its own dependencies (`multi_xml`, `csv`, `mini_mime`), and it is built around + configuring a class, which suits applications more than libraries. +- **Net::HTTP.** Ships with Ruby, so no new gems. Multipart encoding, retries and + nested query encoding would all be code we write and maintain. + +## Decision + +Replace rest-client with Faraday, using `faraday-multipart` for uploads and +`faraday-retry` for retries. Keep the public API of `Procore::Client` +unchanged: + +- **Request methods:** `get`, `post`, `put`, `patch`, `delete` and `sync` keep + their signatures and return types. +- **Errors:** responses keep mapping to the existing `Procore::*` errors. + Connection failures that remain after retries still raise + `Procore::APIConnectionError`. +- **Configuration:** `Procore.configuration.timeout` maps to Faraday's + `request.timeout`. `max_retries` maps to the retry middleware's `max`. +- **Headers:** `Response#headers` keeps its symbol-keyed shape (`:link`, + `:x_request_id`). The gem converts Faraday's headers into that shape inside + `Procore::Response`. +- **Request:** `Response#request` returns a small Procore-owned struct instead + of a `RestClient::Request`, following the stand-in structs `OAuthError` + already uses. It answers the same calls with the same values rest-client + gives today, so code reading them keeps working: + - `url`: the full URL including the query string + (`http://…/rest/v1.0/projects?per_page=5`). + - `method`: the HTTP method as a lowercase string (`"get"`), not a symbol. + - `headers`: the request headers as sent, keyed by header name + (`"Authorization"`). + - `payload`: the request body as a string. With rest-client this is a + `RestClient::Payload` whose stream is already closed, so reading it raises + `IOError`; no caller can depend on it, and it is not a compatibility + promise. + + The gem does not define any `RestClient::*` classes to imitate rest-client. + An app that also loads the real rest-client would get clashing definitions. +- **Uploads:** the gem detects `File`/`IO` values in `body:` and wraps them in + `Faraday::Multipart::FilePart`, so upload calls do not change. Each file + part's content type is guessed from its extension with `mime-types`, as + rest-client did, which keeps `mime-types` as a direct dependency. +- **Connection failures:** DNS failures, unreachable hosts and TLS errors raise + `Procore::APIConnectionError`. Today they escape as `SocketError`, + `Errno::EHOSTUNREACH` and `RestClient::SSLCertificateNotVerified`. +- **Redirects:** GET requests keep following redirects, as rest-client does. + Faraday does not follow them by default, so this needs either the + `faraday-follow_redirects` gem or a small redirect loop in `Requestable`. +- **`HTTP_EXCEPTIONS`:** this public constant lists only Ruby and Faraday + exception classes, with no rest-client ones. +- **Loading the gem:** `require "procore"` no longer defines `RestClient` in the + caller's process. + +`test/procore/rest_client_leakage_test.rb` holds tests for these points. Nine of +them fail until the migration is done; that file is the migration's acceptance +check. + +The retry middleware is configured to match the current retry behaviour, not +Faraday's defaults: + +- **Methods:** retry all methods, including `post` and `patch`. Faraday retries + only idempotent methods by default. The current code retries every method, + and `post`/`patch` can carry idempotency tokens. +- **Exceptions:** retry on timeouts and connection failures + (`Faraday::TimeoutError`, `Faraday::ConnectionFailed`, `Errno::ECONNREFUSED`, + `Errno::ECONNRESET`). +- **Statuses:** do not retry on HTTP status codes. The current code does not + retry 429 or 5xx responses, and changing that is a separate decision. +- **Backoff:** `interval: 1.5` and `backoff_factor: 1.5`, which gives the + current waits of 1.5 s, 2.25 s, 3.375 s and so on. + +`Procore::OAuthError` is also in today's retry list. It is raised while the +access token is fetched, before Faraday sends the request, so the retry +middleware cannot see it. Token fetching keeps its own retry inside +`Requestable`. + +## Consequences + +- One unmaintained runtime dependency (`rest-client`, and with it + `http-accept`, `http-cookie` and `netrc`) is replaced by `faraday-multipart` + and `faraday-retry`. `faraday` and `mime-types` are now declared directly; + before, they arrived through oauth2 and rest-client. +- `Response#request` changes type. Code that calls rest-client-specific methods + on it breaks. This ships in 2.0.0 alongside the Ruby 3.3 minimum and is listed + in the CHANGELOG. +- `APIConnectionError#cause` becomes a Faraday exception instead of a + rest-client one. +- The `revoke` rescue and the request-ID logging are fixed as part of the + migration. +- Before the switch, tests pin the behaviour users depend on: the shape of + `Response#headers`, nested query encoding (`filters[foo]=1`, + `ids[]=1&ids[]=2`), multipart uploads, and the retry count and backoff. + +## Upgrade guide + +This section is written for users of the gem and is meant to be copied into the +README or the 2.0.0 release notes once the migration ships. + +### Upgrading to 2.0: rest-client replaced by Faraday + +The gem now sends requests with [Faraday](https://lostisland.github.io/faraday/) +instead of rest-client. The request methods (`get`, `post`, `put`, `patch`, +`delete`, `sync`), their arguments, `Procore::Response` and the configuration +options are unchanged. What changes is mostly which exceptions you see. + +#### Exceptions + +Every failure now raises a `Procore::Error` subclass, so +`rescue Procore::Error` catches anything the client raises for a failed request. + +| Failure | 1.x raises | 2.0 raises | `cause` in 1.x | `cause` in 2.0 | +|-----------------------------------|-----------------------------------------|--------------------------|-------------------------------------------------------|-----------------------------| +| HTTP 401 | `AuthorizationError` | unchanged | – | – | +| HTTP 403 | `ForbiddenError` | unchanged | – | – | +| HTTP 404 | `NotFoundError` | unchanged | – | – | +| HTTP 400, 422 | `InvalidRequestError` | unchanged | – | – | +| HTTP 429 | `RateLimitError` | unchanged | – | – | +| Any other non-2xx status | `ServerError` | unchanged | – | – | +| Connect timeout | `APIConnectionError` | unchanged | `RestClient::Exceptions::OpenTimeout` | `Faraday::ConnectionFailed` | +| Read timeout | `APIConnectionError` | unchanged | `RestClient::Exceptions::ReadTimeout` | `Faraday::TimeoutError` | +| Connection refused | `APIConnectionError` | unchanged | `Errno::ECONNREFUSED` | `Faraday::ConnectionFailed` | +| Connection reset | `APIConnectionError` | unchanged | `Errno::ECONNRESET` | `Faraday::ConnectionFailed` | +| Server closed the connection | `APIConnectionError` | unchanged | `RestClient::ServerBrokeConnection` | `Faraday::ConnectionFailed` | +| Access token could not be fetched | `APIConnectionError` | unchanged | `Procore::OAuthError` | unchanged | +| TLS certificate error | `RestClient::SSLCertificateNotVerified` | **`APIConnectionError`** | – | `Faraday::SSLError` | +| DNS lookup failed | `SocketError` | **`APIConnectionError`** | – | `Faraday::ConnectionFailed` | +| Host unreachable | `Errno::EHOSTUNREACH` | **`APIConnectionError`** | – | `Faraday::ConnectionFailed` | + +The retry behaviour is unchanged: timeouts, refused and reset connections, and +token fetch failures are retried `max_retries` times with backoff before +`APIConnectionError` is raised. HTTP error statuses, including 429 and 5xx, are +not retried. Every request method is retried, including `post` and `patch`. + +**What to change** + +- **Rescuing rest-client or socket errors:** rescue `Procore::APIConnectionError` + instead. + + ```ruby + # 1.x + rescue RestClient::SSLCertificateNotVerified, SocketError, Errno::EHOSTUNREACH => e + + # 2.0 + rescue Procore::APIConnectionError => e + ``` + +- **Inspecting `cause`:** code that checks for a specific rest-client or `Errno` + class needs the Faraday class from the table. Faraday keeps the underlying + error, so the original `Errno` is still reachable as + `e.cause.wrapped_exception`. + + ```ruby + # 1.x + retry_later if e.cause.is_a?(Errno::ECONNREFUSED) + + # 2.0 + retry_later if e.cause.wrapped_exception.is_a?(Errno::ECONNREFUSED) + ``` + +- **`Procore::Requestable::HTTP_EXCEPTIONS`:** if you read this constant, it now + lists Faraday exception classes instead of `RestClient::Exceptions::Timeout` + and `RestClient::ServerBrokeConnection`. + +#### Other changes + +- **`Response#request` is no longer a `RestClient::Request`.** It is a small + Procore object. `url`, `method` (a lowercase string such as `"get"`) and + `headers` return the same values as before. `payload` now returns the request + body as a string; in 1.x, reading it raised `IOError`. +- **Integer header values are sent as strings.** `company_id` and + `idempotency_token` may still be passed as Integers. +- **The gem no longer loads rest-client.** If your application uses + `RestClient` directly but only got it through this gem, add it to your + Gemfile: + + ```ruby + gem "rest-client" + ``` diff --git a/lib/procore/version.rb b/lib/procore/version.rb index 1538020..f1c0bff 100644 --- a/lib/procore/version.rb +++ b/lib/procore/version.rb @@ -1,3 +1,3 @@ module Procore - VERSION = "1.3.1".freeze + VERSION = "2.0.0".freeze end From b45ac43c7294b8ebae6812d265e616f8b43dad76 Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Mon, 26 Oct 2026 18:23:19 -0600 Subject: [PATCH 13/18] Fix gem loading outside Rails The gem only required active_support/hash_with_indifferent_access but used three core extensions it never loaded. Without a full ActiveSupport load, the Session, Redis and Dalli stores serialized a token as "#" so every request raised MissingTokenError, sync raised NoMethodError on deep_merge, and OAuthError raised NoMethodError on presence. Give Token an explicit to_h/to_json, require the two remaining extensions where they are used, and require activesupport >= 7.0. Covered by subprocess tests that load the gem with nothing else. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 2 + docs/todos.md | 233 +++++++++++++++++++++++++++ lib/procore/auth/token.rb | 17 ++ lib/procore/errors.rb | 2 + lib/procore/requestable.rb | 1 + procore.gemspec | 2 +- test/procore/standalone_load_test.rb | 91 +++++++++++ 7 files changed, 347 insertions(+), 1 deletion(-) create mode 100644 docs/todos.md create mode 100644 test/procore/standalone_load_test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 11aa089..b67c9c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,8 @@ * Add `pstore` as a runtime dependency (no longer bundled with Ruby 4.0). * Fix `Procore::Auth::Stores::File` failing to load saved tokens on Ruby 3.4+ (`Psych::DisallowedClass`). +* Fix the gem failing outside Rails: the Session, Redis and Dalli stores, `sync` and `OAuthError` relied on ActiveSupport core extensions the gem never required. `Procore::Auth::Token` now defines `to_h` and `to_json`, and the gem requires `activesupport` 7.0 or newer. + * Fix `delete` ignoring `options:`, so `company_id` was never sent as the `Procore-Company-Id` header on DELETE requests. * **Security:** stop logging request headers on DELETE requests. The log line included the `Authorization` header, exposing the access token to anything with access to the logs. All log lines now also replace bearer tokens with `###`. diff --git a/docs/todos.md b/docs/todos.md new file mode 100644 index 0000000..5ec9f39 --- /dev/null +++ b/docs/todos.md @@ -0,0 +1,233 @@ +# TODOs + +Run each command from the repository root. The first three bugs are already +fixed in the working tree (the first two by the Faraday migration) and only +need filing for the record. The fourth is a security issue: report it +privately, not as a public issue. + +The commands use bash heredocs, which fish does not support. Start `bash` +first, then paste a command. Each command adds the `bug` label; drop +`--label bug` if the repository has no such label. + +## - [ ] File: `revoke` raises `OAuth2::Error` instead of `Procore::OAuthError` + +~~~sh +gh issue create --label bug --title 'Token revoke failure raises OAuth2::Error instead of Procore::OAuthError' --body-file - <<'EOF' +## Summary + +When the token revoke request fails, `Procore::Client#revoke` raises a raw +`OAuth2::Error` instead of `Procore::OAuthError`, so callers who rescue the +gem's documented errors don't catch it. + +## Steps to reproduce + +```ruby +credentials = Procore::Auth::AccessTokenCredentials.new( + client_id: "id", + client_secret: "secret", + host: "https://login.example.com", +) +# With POST /oauth/revoke returning HTTP 500: +credentials.revoke(token: token) +``` + +## Expected + +`Procore::OAuthError`, with the failed response attached. + +## Actual + +`OAuth2::Error` (a `StandardError`). + +## Cause + +`lib/procore/auth/access_token_credentials.rb`: + +```ruby +def revoke(token:) + # ... + client.request(:post, "/oauth/revoke", body: request) +rescue RestClient::ExceptionWithResponse + raise OAuthError.new(e.description, response: e.response) +end +``` + +- The request is made by the oauth2 gem, which uses Faraday and raises + `OAuth2::Error`, never `RestClient::ExceptionWithResponse`, so the rescue + never matches. +- If it did match, it would raise `NameError`, because the exception isn't + bound (`=> e` is missing). +- `Procore::Client#revoke` only rescues `RuntimeError`, which `OAuth2::Error` + is not, so the error reaches the caller unchanged. + +## Suggested fix + +Rescue `OAuth2::Error => e` and raise `OAuthError.new(e.description, response: e.response)`, +matching `#refresh` in the same class. Add a test for a failed revoke. +EOF +~~~ + +## - [ ] File: request ID is never logged + +~~~sh +gh issue create --label bug --title 'Request logs never include the X-Request-Id' --body-file - <<'EOF' +## Summary + +The "API Request Finished" and "API Request Failed" log lines are meant to +include the response's `X-Request-Id`, but the value is always empty, so logs +can't be matched to Procore's server-side request IDs. + +## Steps to reproduce + +1. Set `Procore.configuration.logger` to a `Logger`. +2. Make any request whose response includes `X-Request-Id: req-123`. + +## Expected + +The log line contains `request_id: req-123`. + +## Actual + +`request_id` is empty. `response.headers[:x_request_id]` returns `"req-123"`, +so the header is received; only the log lookup is wrong. + +## Cause + +`lib/procore/requestable.rb`, in `with_response_handling`: + +```ruby +request_id: result.headers["x-request-id"], +``` + +rest-client stores response headers under underscored symbol keys +(`:x_request_id`), so looking up the string `"x-request-id"` always returns +`nil`. + +## Suggested fix + +Read `response.headers[:x_request_id]` from the `Procore::Response`, which +keeps the same key once the HTTP client is replaced. Add a test that captures +the log output and checks for the request ID. +EOF +~~~ + +## - [ ] File: `delete` ignores `company_id` + +~~~sh +gh issue create --label bug --title 'delete ignores options:, so company_id is never sent' --body-file - <<'EOF' +## Summary + +`Procore::Client#delete` ignores its `options:` argument, so +`options: { company_id: ... }` never produces the `Procore-Company-Id` header. +Every other request method sends it. On multi-company (MPZ) accounts, DELETE +requests that need the header can fail or hit the wrong company. + +## Steps to reproduce + +```ruby +client.delete("projects/1", options: { company_id: 42 }) +``` + +## Expected + +The request includes `Procore-Company-Id: 42`. + +## Actual + +The header is missing. + +## Cause + +`lib/procore/requestable.rb`, in `#delete`: + +```ruby +headers: headers.merge(params: query), +``` + +It calls `headers` without `options`, so the company and idempotency headers +are never added. The other methods call `headers(options)`. + +## Fix + +Use `headers(options)`, as the other methods do. Covered by +`test_delete_with_company_id` in `test/procore/requestable_test.rb`. +EOF +~~~ + +## - [ ] Report privately: DELETE requests log the access token + +`SECURITY.md` asks for vulnerabilities to be reported privately. Don't open a +public issue for this one. Use **Report a vulnerability** at +https://github.com/procore-oss/ruby-sdk/security and paste the text below. +The fix is already in the working tree. + +**Title:** Access token written to logs on DELETE requests + +**Weakness:** CWE-532: Insertion of Sensitive Information into Log File + +**Affected versions:** 1.3.1 and earlier (the first affected version has not +been checked) + +**Description:** + +> When a logger is configured (`Procore.configuration.logger`), every +> `Procore::Client#delete` call writes the full request headers to the log, +> including `Authorization: Bearer `. Anyone who can read the +> application's logs, or any log aggregation service they're shipped to, can +> obtain a valid Procore access token and call the API as that user until the +> token expires. No other request method logs headers. +> +> **Steps to reproduce** +> +> ```ruby +> Procore.configure { |config| config.logger = Logger.new($stdout) } +> client.delete("projects/1") +> # The "API Request Initiated" line includes +> # "Authorization" => "Bearer " +> ``` +> +> **Cause:** `lib/procore/requestable.rb`, `#delete`, passes +> `headers: headers(options)` to `Util.log_info`. +> +> **Fix:** remove `headers:` from that log call. A test that captures the log +> output and checks the token is absent +> (`test_delete_does_not_log_access_token`) covers it. +> +> **Advice for users:** after upgrading, revoke or rotate access tokens that may +> have been logged, and purge DELETE request lines from existing logs. + +## Bugs + +Found in the 2026-10-26 review against `reference/combined_OAS.json`. Fix +test-first; each item names the test that should fail before the fix. + +- [x] **Gem breaks without a full ActiveSupport load.** `lib/procore.rb` only + requires `active_support/hash_with_indifferent_access`, but the code uses + three core extensions it never requires. Outside Rails: the Session, Redis + and Dalli stores serialize a token as `"#"` + (`Object#to_json`), so every request raises `MissingTokenError`; `sync` + raises `NoMethodError` on `deep_merge`; `OAuthError` with a response raises + `NoMethodError` on `presence`. Require the extensions where they are used + and tighten the gemspec's `activesupport > 2.4` constraint. + Test: `test/procore/standalone_load_test.rb` (subprocess, like + `test_requiring_procore_does_not_load_rest_client`). +- [x] **Empty response bodies parse to the String `"{}"`.** + `lib/procore/response.rb` defaults the raw body to `"{}".to_json`, which is + the JSON string literal `"\"{}\""`. The spec has 235 operations returning + 204 No Content; `response.body` on those is a String, not a Hash. + Test: `test/procore/response_test.rb`, empty and nil bodies give `{}`. +- [x] **Pagination drops the API version.** `Response#parse_pagination` + strips the `rest/vX.Y` or `vapid` prefix, so `client.get(pagination[:next])` + uses `default_version`. About half the spec's paths are v1.1, v2.0 or v2.1, + and following their links silently hits the wrong version. Decide the + public shape first (keep the version in the URL, or accept absolute URLs). + Test: `test/procore/requestable_test.rb`, following a v1.1 next link + requests `/rest/v1.1/...`. +- [x] **README documents methods that do not exist.** `e.response.errors` + raises `NoMethodError` (`Response` has `body`, not `errors`), and + `rescue Procore:Error` has a single colon. No test; documentation only. +- [x] **`Accepts` is not an HTTP header.** `Requestable#headers` sends + `Accepts: application/json`; the header is `Accept`. Harmless today because + Procore defaults to JSON, but it is wrong and the spec has an endpoint that + requires `Accept: text/csv`. + Test: `test/procore/requestable_test.rb` header expectations use `Accept`. diff --git a/lib/procore/auth/token.rb b/lib/procore/auth/token.rb index 67da303..80baf33 100644 --- a/lib/procore/auth/token.rb +++ b/lib/procore/auth/token.rb @@ -1,3 +1,5 @@ +require "json" + module Procore module Auth class Token @@ -8,6 +10,21 @@ def initialize(access_token:, refresh_token:, expires_at:) @expires_at = expires_at end + # The shape the Session, Redis and Dalli stores persist and read back. + # + # @return [Hash{Symbol => Object}] + def to_h + { + access_token: access_token, + refresh_token: refresh_token, + expires_at: expires_at, + } + end + + def to_json(*args) + to_h.to_json(*args) + end + def invalid? access_token.nil? end diff --git a/lib/procore/errors.rb b/lib/procore/errors.rb index 0dcf211..1f8e888 100644 --- a/lib/procore/errors.rb +++ b/lib/procore/errors.rb @@ -1,3 +1,5 @@ +require "active_support/core_ext/object/blank" + module Procore # Generic / catch all error class. All other errors generated by the gem # inherit from this class. diff --git a/lib/procore/requestable.rb b/lib/procore/requestable.rb index f5eb5fa..40cfb73 100644 --- a/lib/procore/requestable.rb +++ b/lib/procore/requestable.rb @@ -1,3 +1,4 @@ +require "active_support/core_ext/hash/deep_merge" require "faraday" require "faraday/multipart" require "faraday/retry" diff --git a/procore.gemspec b/procore.gemspec index 698c1d4..6cdd7a2 100644 --- a/procore.gemspec +++ b/procore.gemspec @@ -42,7 +42,7 @@ Gem::Specification.new do |spec| spec.add_development_dependency "sqlite3" spec.add_development_dependency "webmock" - spec.add_dependency "activesupport", "> 2.4" + spec.add_dependency "activesupport", ">= 7.0" spec.add_dependency "faraday", "~> 2.0" spec.add_dependency "faraday-multipart", "~> 1.0" spec.add_dependency "faraday-retry", "~> 2.0" diff --git a/test/procore/standalone_load_test.rb b/test/procore/standalone_load_test.rb new file mode 100644 index 0000000..b8d4291 --- /dev/null +++ b/test/procore/standalone_load_test.rb @@ -0,0 +1,91 @@ +require "test_helper" + +# The test suite loads ActiveRecord and ActionPack, which pull in every +# ActiveSupport core extension. An application that is not Rails only gets +# what the gem itself requires, so these tests run a script in a fresh +# process with nothing but `require "procore"`. +class Procore::StandaloneLoadTest < Minitest::Test + def run_standalone(script) + output = IO.popen( + [RbConfig.ruby, "-Ilib", "-e", script], + err: [:child, :out], + &:read + ) + + [$?.success?, output] + end + + def test_session_store_round_trips_a_token + passed, output = run_standalone(<<~RUBY) + require "procore" + + store = Procore::Auth::Stores::Session.new(session: {}) + store.save( + Procore::Auth::Token.new( + access_token: "access", + refresh_token: "refresh", + expires_at: 123, + ), + ) + token = store.fetch + + exit(token.access_token == "access" && token.expires_at == 123) + RUBY + + assert passed, output + end + + def test_sync_combines_batched_responses + passed, output = run_standalone(<<~RUBY) + require "procore" + + class Request + include Procore::Requestable + + def access_token = "token" + def base_api_path = "http://test.com" + def options = { default_version: "v1.0" } + + private + + # Answers each batch without a network, with the shape a sync returns. + def perform(_method, _url, headers:, params: nil, body: nil) + Faraday::Response.new( + Faraday::Env.from( + status: 200, + body: { entities: [JSON.parse(body)["updates"].first], errors: [] }.to_json, + response_headers: {}, + request_headers: headers, + url: URI("http://test.com/rest/v1.0/projects/sync"), + method: :patch, + ), + ) + end + end + + response = Request.new.sync( + "projects/sync", + body: { updates: [{ id: 1 }, { id: 2 }] }, + options: { batch_size: 1 }, + ) + + exit(response.body["entities"].map { |e| e["id"] } == [1, 2]) + RUBY + + assert passed, output + end + + def test_oauth_error_keeps_the_failed_response + passed, output = run_standalone(<<~RUBY) + require "procore" + + oauth_response = Struct.new(:status, :parsed, :body, :headers) + .new(401, nil, "invalid_grant", {}) + error = Procore::OAuthError.new("refresh failed", response: oauth_response) + + exit(error.response.code == 401 && error.response.body == "invalid_grant") + RUBY + + assert passed, output + end +end From 3f5f93b088438e15856b33c759f6b1495a0ea82f Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Mon, 26 Oct 2026 18:25:45 -0600 Subject: [PATCH 14/18] Parse empty response bodies to an empty Hash The raw body defaulted to "{}".to_json, which is the JSON string literal "\"{}\"", so a 204 No Content response gave a String body. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 2 ++ lib/procore/response.rb | 2 +- test/procore/response_test.rb | 24 ++++++++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b67c9c8..97a782f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ * Fix the gem failing outside Rails: the Session, Redis and Dalli stores, `sync` and `OAuthError` relied on ActiveSupport core extensions the gem never required. `Procore::Auth::Token` now defines `to_h` and `to_json`, and the gem requires `activesupport` 7.0 or newer. +* Fix empty response bodies (such as 204 No Content) parsing to the String `"{}"` instead of an empty Hash. + * Fix `delete` ignoring `options:`, so `company_id` was never sent as the `Procore-Company-Id` header on DELETE requests. * **Security:** stop logging request headers on DELETE requests. The log line included the `Authorization` header, exposing the access token to anything with access to the logs. All log lines now also replace bearer tokens with `###`. diff --git a/lib/procore/response.rb b/lib/procore/response.rb index d13c008..b21b771 100644 --- a/lib/procore/response.rb +++ b/lib/procore/response.rb @@ -53,7 +53,7 @@ def initialize(body:, headers:, code:, request:, request_body:) @pagination = parse_pagination @request = request @request_body = request_body - @raw_body = !body.to_s.empty? ? body : "{}".to_json + @raw_body = body.to_s.empty? ? "{}" : body end # @return [Array, Hash] Ruby representation of JSON response. Hashes are diff --git a/test/procore/response_test.rb b/test/procore/response_test.rb index 6ca89d3..971cdc4 100644 --- a/test/procore/response_test.rb +++ b/test/procore/response_test.rb @@ -13,6 +13,30 @@ def test_response_body assert_equal({ "key" => "value" }, response.body) end + def test_empty_body_parses_to_an_empty_hash + response = Procore::Response.new( + body: "", + code: 204, + headers: {}, + request: nil, + request_body: nil, + ) + + assert_equal({}, response.body) + end + + def test_nil_body_parses_to_an_empty_hash + response = Procore::Response.new( + body: nil, + code: 204, + headers: {}, + request: nil, + request_body: nil, + ) + + assert_equal({}, response.body) + end + def test_response_body_as_collection response_body = [{ "key" => "value" }] From a73c6fc6562c52f98b9a0c7f3d604be533e4978c Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Mon, 26 Oct 2026 18:27:20 -0600 Subject: [PATCH 15/18] Keep the API version in pagination paths Response#pagination stripped the rest/vX.Y or vapid prefix from Link header URLs, so client.get(pagination[:next]) always used default_version. Following a next link from a v1.1, v2.0 or vapid response silently requested v1.0. Pagination paths now keep the version segment, and get/delete pass a path that already names its version through without prefixing it. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 2 ++ README.md | 19 ++++++++------ lib/procore/requestable.rb | 8 +++++- lib/procore/response.rb | 14 +++++++--- test/procore/requestable_test.rb | 45 +++++++++++++++++++++++++++++++- test/procore/response_test.rb | 28 +++++++++++--------- 6 files changed, 90 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97a782f..a33aa5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ * Fix the gem failing outside Rails: the Session, Redis and Dalli stores, `sync` and `OAuthError` relied on ActiveSupport core extensions the gem never required. `Procore::Auth::Token` now defines `to_h` and `to_json`, and the gem requires `activesupport` 7.0 or newer. +* **Breaking:** `Response#pagination` paths now include the API version (`"rest/v1.1/projects?page=2"`, `"vapid/projects?page=2"`) instead of stripping it, and `get`/`delete` accept such paths without adding `default_version`. Following a next link from a v1.1, v2.0 or vapid response previously requested the default version. + * Fix empty response bodies (such as 204 No Content) parsing to the String `"{}"` instead of an empty Hash. * Fix `delete` ignoring `options:`, so `company_id` was never sent as the `Procore-Company-Id` header on DELETE requests. diff --git a/README.md b/README.md index f339461..64bbb8e 100644 --- a/README.md +++ b/README.md @@ -404,8 +404,8 @@ like: response.pagination { - next: "projects?per_page=100&page=2", - last: "projects?per_page=100&page=5" + next: "rest/v1.0/projects?per_page=100&page=2", + last: "rest/v1.0/projects?per_page=100&page=5" } ``` @@ -413,6 +413,9 @@ The `next` value will return the second page of results - which is expected as all paginated responses start on page 1. The `last` value ends on page 5, so there are another 4 pages to consume in order to get all the possible results. +Each path carries the API version the page was requested with, so passing it +back to `client#get` requests the same version without a `version:` argument. + ### Navigating Through Paginated Results To get the next page of results, just pass the url into `client#get`. You may @@ -428,10 +431,10 @@ end puts next_page.pagination { - first: "projects?per_page=100&page=1", - next: "projects?per_page=100&page=3", - prev: "projects?per_page=100&page=1", - last: "projects?per_page=100&page=5" + first: "rest/v1.0/projects?per_page=100&page=1", + next: "rest/v1.0/projects?per_page=100&page=3", + prev: "rest/v1.0/projects?per_page=100&page=1", + last: "rest/v1.0/projects?per_page=100&page=5" } ``` @@ -445,8 +448,8 @@ first_page = client.get("projects", query: { per_page: 250 }) puts first_page.pagination { - next: "projects?per_page=250&page=2", - last: "projects?per_page=250&page=2" + next: "rest/v1.0/projects?per_page=250&page=2", + last: "rest/v1.0/projects?per_page=250&page=2" } ``` diff --git a/lib/procore/requestable.rb b/lib/procore/requestable.rb index 40cfb73..119d7b2 100644 --- a/lib/procore/requestable.rb +++ b/lib/procore/requestable.rb @@ -472,9 +472,15 @@ def file?(value) value.respond_to?(:path) && value.respond_to?(:read) end + # A path that already names its version, as Response#pagination returns. + VERSIONED_PATH = %r{\A/?(?:vapid|rest/v\d+\.\d+)(?:/|\z)} + private_constant :VERSIONED_PATH + def full_path(path, version) version ||= options[:default_version] - if version == "vapid" + if VERSIONED_PATH.match?(path) + File.join(base_api_path, path) + elsif version == "vapid" File.join(base_api_path, "vapid", path) elsif /\Av\d+\.\d+\z/.match?(version) File.join(base_api_path, "rest", version, path) diff --git a/lib/procore/response.rb b/lib/procore/response.rb index b21b771..9c612c4 100644 --- a/lib/procore/response.rb +++ b/lib/procore/response.rb @@ -27,12 +27,13 @@ module Procore # first_page.pagination[:first] # #=> nil # first_page.pagination[:next] - # #=> "projects?per_page=20&page=2" + # #=> "rest/v1.0/projects?per_page=20&page=2" # - # # Any other page will have all keys + # # Any other page will have all keys. The path carries the API version, + # # so #get requests the same version without a version: argument. # next_page = client.get(first_page.pagination[:next]) # next_page.pagination[:first] - # #=> "projects?per_page=20&page=1" + # #=> "rest/v1.0/projects?per_page=20&page=1" # # # The last page will only have URLs for :first & :next # last_page = client.get(first_page.pagination[:last]) @@ -70,9 +71,14 @@ def parse_body JSON.parse(raw_body, object_class: HashWithIndifferentAccess) end + # Keeps the version segment ("rest/v1.1/..." or "vapid/...") so the + # returned path requests the same API version when passed back to #get. + LINK = %r{((?:vapid|rest/v\d+\.\d+)/.*?)>; rel="(\w+)"} + private_constant :LINK + def parse_pagination headers[:link].to_s.split(", ").map(&:strip).reduce({}) do |links, link| - url, name = link.match(/(?:vapid|rest\/.*?)\/(.*?)>; rel="(\w+)"/).captures + url, name = link.match(LINK).captures links.merge!(name.to_sym => url) end end diff --git a/test/procore/requestable_test.rb b/test/procore/requestable_test.rb index 930dc61..bbc50b3 100644 --- a/test/procore/requestable_test.rb +++ b/test/procore/requestable_test.rb @@ -519,11 +519,54 @@ def test_response_pagination_from_link_header response = Request.new(token: "token").get("projects") assert_equal( - { next: "projects?page=2", last: "projects?page=9" }, + { next: "rest/v1.0/projects?page=2", last: "rest/v1.0/projects?page=9" }, response.pagination, ) end + def test_following_a_next_link_keeps_the_requested_version + stub_request(:get, "http://test.com/rest/v1.1/projects") + .to_return( + status: 200, + body: "[]", + headers: { "Link" => '; rel="next"' }, + ) + next_page = stub_request(:get, "http://test.com/rest/v1.1/projects?page=2") + .to_return(status: 200, body: "[]", headers: {}) + client = Request.new(token: "token") + + first_page = client.get("projects", version: "v1.1") + client.get(first_page.pagination[:next]) + + assert_requested next_page + end + + def test_following_a_vapid_next_link_stays_on_vapid + stub_request(:get, "http://test.com/vapid/projects") + .to_return( + status: 200, + body: "[]", + headers: { "Link" => '; rel="next"' }, + ) + next_page = stub_request(:get, "http://test.com/vapid/projects?page=2") + .to_return(status: 200, body: "[]", headers: {}) + client = Request.new(token: "token") + + first_page = client.get("projects", version: "vapid") + client.get(first_page.pagination[:next]) + + assert_requested next_page + end + + def test_versioned_path_is_not_prefixed_again + request = stub_request(:delete, "http://test.com/rest/v2.0/companies/1/projects/2") + .to_return(status: 204, body: "", headers: {}) + + Request.new(token: "token").delete("rest/v2.0/companies/1/projects/2") + + assert_requested request + end + def test_response_exposes_code_request_and_request_body stub_request(:post, "http://test.com/rest/v1.0/home") .to_return(status: 201, body: { id: 1 }.to_json, headers: {}) diff --git a/test/procore/response_test.rb b/test/procore/response_test.rb index 971cdc4..fe36e2a 100644 --- a/test/procore/response_test.rb +++ b/test/procore/response_test.rb @@ -51,7 +51,7 @@ def test_response_body_as_collection assert_equal(response_body, response.body) end - def shared_response_pagination_parsing_test(base_path) + def shared_response_pagination_parsing_test(base_path, version_path) links = %W( <#{base_path}/projects?page=1>; rel="first", <#{base_path}/projects?page=173>; rel="last", @@ -69,24 +69,28 @@ def shared_response_pagination_parsing_test(base_path) assert_equal( { - first: "projects?page=1", - last: "projects?page=173", - next: "projects?page=6", - prev: "projects?page=4", + first: "#{version_path}/projects?page=1", + last: "#{version_path}/projects?page=173", + next: "#{version_path}/projects?page=6", + prev: "#{version_path}/projects?page=4", }, response.pagination, ) end def test_rest_response_pagination_parsing - shared_response_pagination_parsing_test("http://localhost:3000/rest/v1.0") + shared_response_pagination_parsing_test("http://localhost:3000/rest/v1.0", "rest/v1.0") + end + + def test_rest_v2_response_pagination_parsing + shared_response_pagination_parsing_test("http://localhost:3000/rest/v2.0", "rest/v2.0") end def test_vapid_response_pagination_parsing - shared_response_pagination_parsing_test("http://localhost:3000/vapid") + shared_response_pagination_parsing_test("http://localhost:3000/vapid", "vapid") end - def shared_response_pagination_parsing_on_first_page(base_path) + def shared_response_pagination_parsing_on_first_page(base_path, version_path) links = %W( <#{base_path}/projects?page=173>; rel="last", <#{base_path}/projects?page=6>; rel="next" @@ -102,19 +106,19 @@ def shared_response_pagination_parsing_on_first_page(base_path) assert_equal( { - last: "projects?page=173", - next: "projects?page=6", + last: "#{version_path}/projects?page=173", + next: "#{version_path}/projects?page=6", }, response.pagination, ) end def test_vapid_response_pagination_parsing_on_first_page - shared_response_pagination_parsing_on_first_page("http://localhost:3000/vapid") + shared_response_pagination_parsing_on_first_page("http://localhost:3000/vapid", "vapid") end def test_rest_response_pagination_parsing_on_first_page - shared_response_pagination_parsing_on_first_page("http://localhost:3000/rest/v1.0") + shared_response_pagination_parsing_on_first_page("http://localhost:3000/rest/v1.0", "rest/v1.0") end def test_response_pagination_no_links From f77758f1e658c815aa125165bc436170f5a43c86 Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Mon, 26 Oct 2026 18:28:19 -0600 Subject: [PATCH 16/18] Fix error handling examples in README e.response.errors does not exist; Response exposes the parsed body as e.response.body. Also fix the single-colon Procore:Error rescue. Co-Authored-By: Claude Fable 5.1 --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 64bbb8e..bd47cdc 100644 --- a/README.md +++ b/README.md @@ -352,7 +352,7 @@ errors generated by this gem. ```ruby begin client.get("projects") -rescue Procore:Error => e +rescue Procore::Error => e # Something went wrong. # Print the error class @@ -364,8 +364,8 @@ rescue Procore:Error => e # Print the HTTP code puts e.response.code - # Print the json error response - puts e.response.errors + # Print the parsed error response body + puts e.response.body # Print the headers puts e.response.headers From 49833f7c3d863bf6d991999d9d82b46889d23d7e Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Mon, 26 Oct 2026 18:29:39 -0600 Subject: [PATCH 17/18] Send Accept instead of the misspelled Accepts header Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 2 ++ lib/procore/requestable.rb | 2 +- test/procore/requestable_test.rb | 12 +++++++++++- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a33aa5f..f2852a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ * **Breaking:** `Response#pagination` paths now include the API version (`"rest/v1.1/projects?page=2"`, `"vapid/projects?page=2"`) instead of stripping it, and `get`/`delete` accept such paths without adding `default_version`. Following a next link from a v1.1, v2.0 or vapid response previously requested the default version. +* Fix the `Accept` header being sent as `Accepts`. + * Fix empty response bodies (such as 204 No Content) parsing to the String `"{}"` instead of an empty Hash. * Fix `delete` ignoring `options:`, so `company_id` was never sent as the `Procore-Company-Id` header on DELETE requests. diff --git a/lib/procore/requestable.rb b/lib/procore/requestable.rb index 119d7b2..3dd9b03 100644 --- a/lib/procore/requestable.rb +++ b/lib/procore/requestable.rb @@ -334,7 +334,7 @@ def with_response_handling(request_body: nil) def headers(options = {}) { - "Accepts" => "application/json", + "Accept" => "application/json", "Authorization" => "Bearer #{access_token}", "Content-Type" => "application/json", "Procore-Sdk-Version" => Procore::VERSION, diff --git a/test/procore/requestable_test.rb b/test/procore/requestable_test.rb index bbc50b3..bc37d54 100644 --- a/test/procore/requestable_test.rb +++ b/test/procore/requestable_test.rb @@ -43,7 +43,7 @@ def record_sleeps(request) def headers { - "Accepts" => "application/json", + "Accept" => "application/json", "Authorization" => "Bearer token", "Content-Type" => "application/json", "Procore-Sdk-Version" => Procore::VERSION, @@ -64,6 +64,16 @@ def test_get assert_requested request end + def test_get_does_not_send_a_misspelled_accept_header + request = stub_request(:get, "http://test.com/rest/v1.0/home") + .with { |req| !req.headers.key?("Accepts") } + .to_return(status: 200, body: "", headers: {}) + + Request.new(token: "token").get("home") + + assert_requested request + end + def test_rest_get request = stub_request(:get, "http://test.com/rest/v1.1/home") .with( From 79159061f63be4105cfa3e268d8c32a5b215891d Mon Sep 17 00:00:00 2001 From: Pawel Osiczko Date: Mon, 26 Oct 2026 18:43:30 -0600 Subject: [PATCH 18/18] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2852a6..a55ab3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -## 2.0.0 (Unreleased) +## 2.0.0 (Oct 26, 2026) * **Breaking:** Require Ruby 3.3 or newer. Ruby 3.0, 3.1 and 3.2 are end-of-life.