diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/builder/agent_factory.rb b/packages/forest_admin_agent/lib/forest_admin_agent/builder/agent_factory.rb index e4f7fba41..1974d6baa 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/builder/agent_factory.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/builder/agent_factory.rb @@ -13,6 +13,8 @@ class AgentFactory attr_reader :customizer, :container, :has_env_secret attr_accessor :schema_only_mode + ENV_SECRET_FORMAT = /\A[0-9a-f]{64}\z/ + def initialize super @reloading = false @@ -20,7 +22,7 @@ def initialize def setup(options) @options = options - @has_env_secret = options.to_h.key?(:env_secret) + @has_env_secret = !options.to_h[:env_secret].nil? @customizer = ForestAdminDatasourceCustomizer::DatasourceCustomizer.new build_container build_cache @@ -115,6 +117,7 @@ def send_schema(force: false) end return unless @has_env_secret + return unless secrets_format_valid? schema = generate_schema_file @@ -127,7 +130,19 @@ def send_schema(force: false) end end - post_schema(schema, force) + begin + post_schema(schema, force) + rescue StandardError => e + # A well-formed secret can still be rejected by the server (wrong project, revoked + # secret, network down, ...). Same rule as an invalid format: block boot in + # production, but never in dev - warn and move on instead. Scoped to this call + # only, so a local bug (a broken customization, an unwritable schema_path, ...) + # still surfaces immediately instead of being logged as a generic warning. + raise e if Facades::Container.cache(:is_production) + + @logger.log('Warn', "[ForestAdmin] #{e.message}") + @logger.log('Warn', '[ForestAdmin] Schema sync failed, continuing without it.') + end end # Generates or loads the schema and writes it to file (in development mode). @@ -175,6 +190,40 @@ def write_schema_file(schema_path, schema) private + # Checked from send_schema rather than setup, so that schema-only mode (which never + # syncs to the server) never fails on a secret it doesn't actually need. + def secrets_format_valid? + errors = secret_format_errors + return true if errors.empty? + + if Facades::Container.cache(:is_production) + raise ForestAdminAgent::Http::Exceptions::ValidationError, errors.join(' ') + end + + # Don't block boot on a config mistake in dev: warn loudly and skip the schema + # sync instead, so the developer can still work on the rest of the app. + errors.each { |error| @logger.log('Warn', "[ForestAdmin] #{error}") } + @logger.log('Warn', '[ForestAdmin] Skipping schema sync until this is fixed.') + false + end + + def secret_format_errors + errors = [] + env_secret = @options.to_h[:env_secret] + + unless env_secret.is_a?(String) && env_secret.match?(ENV_SECRET_FORMAT) + errors << 'config.env_secret is invalid: it must be the 64-character hexadecimal secret from your ' \ + 'Forest Admin project settings.' + end + + auth_secret = @options.to_h[:auth_secret] + unless auth_secret.is_a?(String) + errors << 'config.auth_secret is invalid: it must be a string. Any long random value works.' + end + + errors + end + def container_replace(key, value) @container._container.delete(key.to_s) @container.register(key, value) @@ -211,6 +260,7 @@ def do_server_want_schema(hash) begin response = client.post('/forest/apimaps/hashcheck', { schemaFileHash: hash }.to_json) + client.raise_for_response!(response) body = JSON.parse(response.body) body['sendSchema'] rescue JSON::ParserError => e @@ -281,7 +331,8 @@ def log_schema_skip def send_schema_to_server(api_map) ForestAdminAgent::Facades::Container.logger.log('Info', 'schema was updated, sending new version') client = ForestAdminAgent::Http::ForestAdminApiRequester.new - client.post('/forest/apimaps', api_map.to_json) + response = client.post('/forest/apimaps', api_map.to_json) + client.raise_for_response!(response) rescue Faraday::Error => e status = e.response[:status] if e.response if status diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/http/forest_admin_api_requester.rb b/packages/forest_admin_agent/lib/forest_admin_agent/http/forest_admin_api_requester.rb index 522476743..3d2e2cbf9 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/http/forest_admin_api_requester.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/http/forest_admin_api_requester.rb @@ -41,33 +41,48 @@ def handle_response_error(error) ) end - if error.response[:status].zero? || error.response[:status] == 502 + raise_for_status(error.response[:status], cause: error) + end + + # Faraday does not raise on HTTP error statuses here (no raise_error middleware + # configured), so a response has to be checked explicitly to turn e.g. an + # invalid envSecret (404) into the same typed errors as handle_response_error. + def raise_for_response!(response) + return if response.success? + + raise_for_status(response.status, message: response.reason_phrase) + end + + private + + def raise_for_status(status, cause: nil, message: cause&.message) + if status.zero? || status == 502 raise BadGatewayError.new( 'Failed to reach ForestAdmin server. Are you online?', - details: { status: error.response[:status] }, - cause: error + details: { status: status }, + cause: cause ) end - if error.response[:status] == 404 + if status == 404 raise NotFoundError.new( 'ForestAdmin server failed to find the project related to the envSecret you configured. Can you check that you copied it properly in the Forest initialization?', - details: { status: error.response[:status] } + details: { status: status } ) end - if error.response[:status] == 503 + if status == 503 raise ServiceUnavailableError.new( 'Forest is in maintenance for a few minutes. We are upgrading your experience in the forest. We just need a few more minutes to get it right.', - details: { status: error.response[:status] }, - cause: error + details: { status: status }, + cause: cause ) end raise InternalServerError.new( 'An unexpected error occurred while contacting the ForestAdmin server. Please contact support@forestadmin.com for further investigations.', - details: { status: error.response[:status], message: error.message }, - cause: error + details: { status: status, message: message }, + cause: cause ) end end diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/builder/agent_factory_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/builder/agent_factory_spec.rb index fea917258..0382c327f 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/builder/agent_factory_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/builder/agent_factory_spec.rb @@ -24,6 +24,189 @@ module Builder expect(described_class.instance.container.resolve(:logger)).not_to be_nil expect(described_class.instance.container.resolve(:logger)).to be_instance_of Services::LoggerService end + + context 'when env_secret key is present but nil (e.g. an unconfigured sibling agent, like ' \ + 'ForestAdminRpcAgent bundled but never set up)' do + it 'sets @has_env_secret to false and skips validation entirely, without warning or raising' do + instance = described_class.instance + logger = instance_spy(Services::LoggerService) + allow(Services::LoggerService).to receive(:new).and_return(logger) + allow(Facades::Container).to receive(:cache).with(:skip_schema_update).and_return(false) + + expect do + instance.setup(auth_secret: nil, env_secret: nil, is_production: true) + end.not_to raise_error + + expect(instance.has_env_secret).to be false + + instance.send_schema + + expect(logger).not_to have_received(:log).with('Warn', anything) + end + end + + context 'with schema_only_mode enabled (offline schema generation, e.g. ' \ + 'rake forest_admin:schema:generate)' do + it 'never validates secrets, even a malformed env_secret in production' do + instance = described_class.instance + + expect do + instance.setup( + auth_secret: 'cba803d01a4d43b55010cab41fa1ea1f1f51a95e', + env_secret: 'not-a-valid-secret', + is_production: true + ) + end.not_to raise_error + + instance.schema_only_mode = true + allow(instance).to receive(:generate_schema_only) + + expect { instance.build }.not_to raise_error + expect(instance).to have_received(:generate_schema_only) + ensure + instance.schema_only_mode = false + end + end + end + + context 'when env_secret is present but malformed' do + let(:instance) { described_class.instance } + let(:valid_options) do + { + auth_secret: 'cba803d01a4d43b55010cab41fa1ea1f1f51a95e', + env_secret: '89719c6d8e2e2de2694c2f220fe2dbf02d5289487364daf1e4c6b13733ed0cdb', + skip_schema_update: false, + append_schema_path: nil + } + end + + context 'when running in production' do + let(:prod_options) { valid_options.merge(is_production: true) } + + it 'raises a ValidationError when env_secret is too short' do + instance.setup(prod_options.merge(env_secret: 'abc123')) + + expect { instance.send_schema }.to raise_error( + ForestAdminAgent::Http::Exceptions::ValidationError, /config\.env_secret is invalid/ + ) + end + + it 'raises a ValidationError when env_secret contains the variable name (a common copy-paste mistake)' do + instance.setup(prod_options.merge(env_secret: "FOREST_ENV_SECRET=#{valid_options[:env_secret]}")) + + expect { instance.send_schema }.to raise_error( + ForestAdminAgent::Http::Exceptions::ValidationError, /config\.env_secret is invalid/ + ) + end + + it 'raises a ValidationError when env_secret has uppercase characters' do + instance.setup(prod_options.merge(env_secret: valid_options[:env_secret].upcase)) + + expect { instance.send_schema }.to raise_error( + ForestAdminAgent::Http::Exceptions::ValidationError, /config\.env_secret is invalid/ + ) + end + + it 'raises a ValidationError when auth_secret is not a string' do + instance.setup(prod_options.merge(auth_secret: 42)) + + expect { instance.send_schema }.to raise_error( + ForestAdminAgent::Http::Exceptions::ValidationError, /config\.auth_secret is invalid/ + ) + end + + it 'does not raise when both secrets are well-formed' do + instance.setup(prod_options) + allow(instance).to receive_messages(generate_schema_file: { meta: {}, collections: [] }, post_schema: nil) + + expect { instance.send_schema }.not_to raise_error + end + end + + context 'when running outside production' do + let(:dev_options) { valid_options.merge(is_production: false) } + + it 'does not raise, warns instead, and skips the schema sync' do + logger = instance_spy(Services::LoggerService) + allow(Services::LoggerService).to receive(:new).and_return(logger) + instance.setup(dev_options.merge(env_secret: 'abc123')) + allow(instance).to receive(:generate_schema_file) + + expect { instance.send_schema }.not_to raise_error + + expect(logger).to have_received(:log).with('Warn', /config\.env_secret is invalid/) + expect(logger).to have_received(:log).with('Warn', /Skipping schema sync/) + expect(instance).not_to have_received(:generate_schema_file) + end + + it 'does not raise and does not warn when both secrets are well-formed' do + logger = instance_spy(Services::LoggerService) + allow(Services::LoggerService).to receive(:new).and_return(logger) + instance.setup(dev_options) + allow(instance).to receive_messages(generate_schema_file: { meta: {}, collections: [] }, post_schema: nil) + + expect { instance.send_schema }.not_to raise_error + + expect(logger).not_to have_received(:log).with('Warn', /is invalid/) + end + end + end + + context 'when the secret is well-formed but rejected by the server' do + let(:instance) { described_class.instance } + let(:valid_options) do + { + auth_secret: 'cba803d01a4d43b55010cab41fa1ea1f1f51a95e', + env_secret: '89719c6d8e2e2de2694c2f220fe2dbf02d5289487364daf1e4c6b13733ed0cdb', + skip_schema_update: false, + append_schema_path: nil + } + end + let(:not_found_error) do + ForestAdminAgent::Http::Exceptions::NotFoundError.new( + 'ForestAdmin server failed to find the project related to the envSecret you configured.' + ) + end + + context 'when running in production' do + it 'raises the error the server returned' do + instance.setup(valid_options.merge(is_production: true)) + allow(instance).to receive_messages(generate_schema_file: { meta: {}, collections: [] }) + allow(instance).to receive(:post_schema).and_raise(not_found_error) + + expect { instance.send_schema }.to raise_error(ForestAdminAgent::Http::Exceptions::NotFoundError) + end + end + + context 'when running outside production' do + it 'does not raise, warns instead, and continues without the schema' do + logger = instance_spy(Services::LoggerService) + allow(Services::LoggerService).to receive(:new).and_return(logger) + instance.setup(valid_options.merge(is_production: false)) + allow(instance).to receive_messages(generate_schema_file: { meta: {}, collections: [] }) + allow(instance).to receive(:post_schema).and_raise(not_found_error) + + expect { instance.send_schema }.not_to raise_error + + expect(logger).to have_received(:log).with('Warn', /failed to find the project/) + expect(logger).to have_received(:log).with('Warn', /Schema sync failed/) + end + end + end + + context 'when generate_schema_file raises a local error (e.g. a broken customization)' do + it 'still raises it outside production, instead of swallowing it as a Forest warning' do + instance = described_class.instance + instance.setup( + auth_secret: 'cba803d01a4d43b55010cab41fa1ea1f1f51a95e', + env_secret: '89719c6d8e2e2de2694c2f220fe2dbf02d5289487364daf1e4c6b13733ed0cdb', + skip_schema_update: false, + is_production: false + ) + allow(instance).to receive(:generate_schema_file).and_raise(ArgumentError, 'broken chart definition') + + expect { instance.send_schema }.to raise_error(ArgumentError, 'broken chart definition') + end end describe 'add_datasource' do @@ -313,7 +496,7 @@ module Builder expect(instance).to have_received(:post_schema).with(hash_including(collections: [{ name: 'Main' }, { name: 'Extra' }]), anything) end - it 'raises error if append_schema file cannot be loaded' do + it 'raises error if append_schema file cannot be loaded, regardless of environment' do instance = described_class.instance instance.instance_variable_set(:@has_env_secret, true) @@ -543,12 +726,15 @@ module Builder end it 'logs success message and posts schema when successful' do - allow(client).to receive(:post).with('/forest/apimaps', api_map.to_json) + response = instance_double(Faraday::Response) + allow(client).to receive(:post).with('/forest/apimaps', api_map.to_json).and_return(response) + allow(client).to receive(:raise_for_response!).with(response) instance.send(:send_schema_to_server, api_map) expect(logger).to have_received(:log).with('Info', 'schema was updated, sending new version') expect(client).to have_received(:post).with('/forest/apimaps', api_map.to_json) + expect(client).to have_received(:raise_for_response!).with(response) end context 'when error occurs with HTTP status' do @@ -602,6 +788,68 @@ module Builder end end end + + describe 'do_server_want_schema' do + let(:instance) { described_class.instance } + let(:client) { instance_double(ForestAdminAgent::Http::ForestAdminApiRequester) } + + before do + allow(ForestAdminAgent::Http::ForestAdminApiRequester).to receive(:new).and_return(client) + end + + it 'returns true when the server asks for the schema' do + response = instance_double(Faraday::Response, body: { sendSchema: true }.to_json) + allow(client).to receive(:post).and_return(response) + allow(client).to receive(:raise_for_response!).with(response) + + expect(instance.send(:do_server_want_schema, 'abc123')).to be true + end + + it 'returns false when the server already has this schema' do + response = instance_double(Faraday::Response, body: { sendSchema: false }.to_json) + allow(client).to receive(:post).and_return(response) + allow(client).to receive(:raise_for_response!).with(response) + + expect(instance.send(:do_server_want_schema, 'abc123')).to be false + end + + it 'propagates the error raised by raise_for_response! (e.g. an invalid envSecret)' do + response = instance_double(Faraday::Response, status: 404, body: '{"errors":[]}') + allow(client).to receive(:post).and_return(response) + allow(client).to receive(:raise_for_response!).with(response).and_raise( + ForestAdminAgent::Http::Exceptions::NotFoundError.new( + 'ForestAdmin server failed to find the project related to the envSecret you configured. ' \ + 'Can you check that you copied it properly in the Forest initialization?' + ) + ) + + expect do + instance.send(:do_server_want_schema, 'abc123') + end.to raise_error(ForestAdminAgent::Http::Exceptions::NotFoundError, /envSecret/) + end + + it 'raises InternalServerError when the response body is not valid JSON' do + response = instance_double(Faraday::Response, status: 200, body: 'not json') + allow(client).to receive(:post).and_return(response) + allow(client).to receive(:raise_for_response!).with(response) + + expect do + instance.send(:do_server_want_schema, 'abc123') + end.to raise_error(ForestAdminAgent::Http::Exceptions::InternalServerError, /Invalid JSON response/) + end + + it 'delegates to handle_response_error when the connection itself fails' do + error = Faraday::ConnectionFailed.new('Failed to open TCP connection') + allow(client).to receive(:post).and_raise(error) + allow(client).to receive(:handle_response_error).with(error).and_raise( + ForestAdminAgent::Http::Exceptions::BadGatewayError.new('Failed to reach ForestAdmin server. Are you online?') + ) + + expect do + instance.send(:do_server_want_schema, 'abc123') + end.to raise_error(ForestAdminAgent::Http::Exceptions::BadGatewayError) + end + end end end end diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/http/forest_admin_api_requester_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/http/forest_admin_api_requester_spec.rb index 6ed704f2b..aaf371ddd 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/http/forest_admin_api_requester_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/http/forest_admin_api_requester_spec.rb @@ -91,6 +91,72 @@ module Http 'An unexpected error occurred while contacting the ForestAdmin server. Please contact support@forestadmin.com for further investigations.' ) end + + it 'keeps the original error message in details for diagnostics' do + expect do + forest_admin_api_requester.handle_response_error(Faraday::ConnectionFailed.new('test', { status: 500 })) + end.to(raise_error { |error| expect(error.details[:message]).to eq('test') }) + end + end + + context 'when raise_for_response! is called' do + it 'does nothing when the response is successful' do + response = instance_double(Faraday::Response, success?: true) + + expect { forest_admin_api_requester.raise_for_response!(response) }.not_to raise_error + end + + it 'raises NotFoundError when the response status is 404 (e.g. an invalid envSecret)' do + response = instance_double(Faraday::Response, success?: false, status: 404, reason_phrase: 'Not Found') + + expect do + forest_admin_api_requester.raise_for_response!(response) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::NotFoundError, + 'ForestAdmin server failed to find the project related to the envSecret you configured. Can you check that you copied it properly in the Forest initialization?' + ) + end + + it 'raises BadGatewayError when the response status is 502' do + response = instance_double(Faraday::Response, success?: false, status: 502, reason_phrase: 'Bad Gateway') + + expect do + forest_admin_api_requester.raise_for_response!(response) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::BadGatewayError, + 'Failed to reach ForestAdmin server. Are you online?' + ) + end + + it 'raises ServiceUnavailableError when the response status is 503' do + response = instance_double(Faraday::Response, success?: false, status: 503, reason_phrase: 'Service Unavailable') + + expect do + forest_admin_api_requester.raise_for_response!(response) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::ServiceUnavailableError, + 'Forest is in maintenance for a few minutes. We are upgrading your experience in the forest. We just need a few more minutes to get it right.' + ) + end + + it 'raises InternalServerError for any other error status' do + response = instance_double(Faraday::Response, success?: false, status: 500, reason_phrase: 'Internal Server Error') + + expect do + forest_admin_api_requester.raise_for_response!(response) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::InternalServerError, + 'An unexpected error occurred while contacting the ForestAdmin server. Please contact support@forestadmin.com for further investigations.' + ) + end + + it 'keeps the response reason phrase in details for diagnostics' do + response = instance_double(Faraday::Response, success?: false, status: 500, reason_phrase: 'Internal Server Error') + + expect do + forest_admin_api_requester.raise_for_response!(response) + end.to(raise_error { |error| expect(error.details[:message]).to eq('Internal Server Error') }) + end end end end diff --git a/packages/forest_admin_rails/config/initializers/forest_admin_error_subscriber.rb b/packages/forest_admin_rails/config/initializers/forest_admin_error_subscriber.rb index b6413e2e7..3d758ceb2 100644 --- a/packages/forest_admin_rails/config/initializers/forest_admin_error_subscriber.rb +++ b/packages/forest_admin_rails/config/initializers/forest_admin_error_subscriber.rb @@ -1,7 +1,12 @@ class ForestAdminErrorSubscriber - def report(error, handled:, severity:, context:, source: nil) - return if ForestAdminAgent::Facades::Container.cache(:is_production) + SEVERITY_TO_LEVEL = { + error: 'Error', + warning: 'Warn', + info: 'Info' + }.freeze - ForestAdminAgent::Facades::Container.logger.log('Debug', error.full_message) + def report(error, handled:, severity:, context:, source: nil) + level = SEVERITY_TO_LEVEL.fetch(severity, 'Error') + ForestAdminAgent::Facades::Container.logger.log(level, "[ForestAdmin] #{error.full_message}") end end diff --git a/packages/forest_admin_rails/lib/forest_admin_rails/engine.rb b/packages/forest_admin_rails/lib/forest_admin_rails/engine.rb index ced010c7c..829667caf 100644 --- a/packages/forest_admin_rails/lib/forest_admin_rails/engine.rb +++ b/packages/forest_admin_rails/lib/forest_admin_rails/engine.rb @@ -29,7 +29,7 @@ class Engine < ::Rails::Engine end config.after_initialize do - Rails.error.handle(ForestAdminDatasourceToolkit::Exceptions::ForestException) do + Rails.error.handle(ForestAdminDatasourceToolkit::Exceptions::ForestException, severity: :error) do agent_factory = ForestAdminAgent::Builder::AgentFactory.instance agent_factory.setup(ForestAdminRails.config) load_configuration diff --git a/packages/forest_admin_rails/spec/config/initializers/forest_admin_error_subscriber_spec.rb b/packages/forest_admin_rails/spec/config/initializers/forest_admin_error_subscriber_spec.rb new file mode 100644 index 000000000..ffadfb69d --- /dev/null +++ b/packages/forest_admin_rails/spec/config/initializers/forest_admin_error_subscriber_spec.rb @@ -0,0 +1,41 @@ +require 'spec_helper' +require 'logger' + +require_relative '../../../config/initializers/forest_admin_error_subscriber' + +RSpec.describe ForestAdminErrorSubscriber do + subject(:subscriber) { described_class.new } + + let(:logger) { instance_double(Logger, log: nil) } + let(:error) { StandardError.new('envSecret invalid') } + + before do + logger_double = logger + container = Class.new + container.define_singleton_method(:logger) { logger_double } + stub_const('ForestAdminAgent::Facades::Container', container) + end + + it 'logs the error even when running in production' do + subscriber.report(error, handled: true, severity: :error, context: {}) + + expect(logger).to have_received(:log).with('Error', /envSecret invalid/) + end + + it 'maps each Rails error severity to the matching Forest logger level' do + subscriber.report(error, handled: true, severity: :error, context: {}) + expect(logger).to have_received(:log).with('Error', anything) + + subscriber.report(error, handled: true, severity: :warning, context: {}) + expect(logger).to have_received(:log).with('Warn', anything) + + subscriber.report(error, handled: true, severity: :info, context: {}) + expect(logger).to have_received(:log).with('Info', anything) + end + + it 'defaults to Error when given an unknown severity' do + subscriber.report(error, handled: true, severity: :unknown, context: {}) + + expect(logger).to have_received(:log).with('Error', anything) + end +end