Bug fixes - #50
Open
bdurand wants to merge 12 commits into
Open
Conversation
Greptile SummaryThis PR is a sweeping set of bug fixes for the
Confidence Score: 4/5Safe to merge; all changes are targeted corrections backed by new tests. The fixes are well-reasoned and test coverage is thorough. Two minor observations: update_setting does not freeze the cache (inconsistent with every other write path, not currently exploitable), and development_mode? default changed from implicitly-development to implicitly-production when no env var is set. lib/super_settings/mini_i18n.rb (env-var default change) and lib/super_settings/local_cache.rb (update_setting missing .freeze) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
REQ["Incoming Request"] --> CALL["RackApplication#call\n(path prefix check)"]
CALL -->|"prefix matches or empty"| HANDLE["handle_request\n(SCRIPT_NAME + PATH_INFO resolution)"]
CALL -->|"no match + app"| DOWNSTREAM["Downstream App"]
CALL -->|"no match + no app"| N404["404 Not Found"]
HANDLE --> AUTH["check_authorization"]
AUTH -->|"401 + auth_url"| REDIRECT["302 to login"]
AUTH -->|"403"| F403["403 Forbidden\n(no redirect loop)"]
AUTH -->|"authorized"| ROUTE["Route"]
ROUTE -->|"POST /settings"| PP["post_params\n(JSON parse)"]
PP -->|"parse error or non-Hash"| B400A["400 Bad Request"]
PP -->|"valid Hash"| UPDATE["RestAPI.update"]
UPDATE -->|"save! raises InvalidRecordError"| FAIL["return false"]
UPDATE -->|"success"| S200A["200 OK"]
ROUTE -->|"GET /updated_since"| TP["parse time param"]
TP -->|"nil or ArgumentError"| B400B["400 Bad Request"]
TP -->|"valid Time"| SETTINGS["Setting.updated_since"]
SETTINGS --> S200B["200 OK"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
REQ["Incoming Request"] --> CALL["RackApplication#call\n(path prefix check)"]
CALL -->|"prefix matches or empty"| HANDLE["handle_request\n(SCRIPT_NAME + PATH_INFO resolution)"]
CALL -->|"no match + app"| DOWNSTREAM["Downstream App"]
CALL -->|"no match + no app"| N404["404 Not Found"]
HANDLE --> AUTH["check_authorization"]
AUTH -->|"401 + auth_url"| REDIRECT["302 to login"]
AUTH -->|"403"| F403["403 Forbidden\n(no redirect loop)"]
AUTH -->|"authorized"| ROUTE["Route"]
ROUTE -->|"POST /settings"| PP["post_params\n(JSON parse)"]
PP -->|"parse error or non-Hash"| B400A["400 Bad Request"]
PP -->|"valid Hash"| UPDATE["RestAPI.update"]
UPDATE -->|"save! raises InvalidRecordError"| FAIL["return false"]
UPDATE -->|"success"| S200A["200 OK"]
ROUTE -->|"GET /updated_since"| TP["parse time param"]
TP -->|"nil or ArgumentError"| B400B["400 Bad Request"]
TP -->|"valid Time"| SETTINGS["Setting.updated_since"]
SETTINGS --> S200B["200 OK"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughVersion 2.6.1 documents and implements fixes for web UI escaping, HTTP and Rack validation, configuration loading, cache concurrency, persistence failures, storage races, and runtime compatibility, with expanded regression coverage. Changes2.6.1 fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/super_settings/storage/s3_storage.rb (1)
118-132: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSynchronize
S3Storage#s3_bucketupdates.
@bucketand@bucket_hashcan still be published out of sync under concurrent reconfiguration. Guard the config snapshot and the bucket/hash assignment with a mutex, or freeze S3 config after boot if runtime changes are not intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/super_settings/storage/s3_storage.rb` around lines 118 - 132, Synchronize the configuration snapshot and publication of `@bucket` and `@bucket_hash` within S3Storage#s3_bucket using a mutex, ensuring concurrent callers cannot observe mismatched values. Keep bucket creation and hash assignment in the same protected critical section, while preserving the existing configuration-change check and option construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/super_settings/local_cache.rb`:
- Around line 92-94: Update merge_load and the refresh path so collection values
inserted into the shared cache are recursively frozen, or deep-copied before
return, matching the immutability contract enforced by set_cache_values. Ensure
arrays and nested collections cannot be mutated through returned references, and
add a regression spec covering an array added or updated by refresh.
In `@lib/super_settings/rack_application.rb`:
- Around line 239-242: Validate params["settings"] in the request handler before
calling SuperSettings::RestAPI.update: require an array whose every element is a
hash, and return the existing 400 Invalid request response for missing or
invalid values. Only invoke update with the validated settings payload.
In `@lib/super_settings/rest_api.rb`:
- Line 166: Update the history serialization around history_values so created_at
cannot reach the client as null, or coordinate the corresponding client handling
in the history modal script to skip date parsing when created_at is null.
Preserve valid timestamp formatting and ensure null history timestamps no longer
break modal rendering.
In `@lib/super_settings/storage/active_record_storage.rb`:
- Around line 106-133: The retry logic in ActiveRecordStorage#save! must roll
back each failed duplicate-key attempt before retrying. Wrap the attempt
containing the duplicate lookup and save operations in a requires_new
transaction (or move retry handling outside the surrounding
Setting.transaction), then add a regression test covering concurrent creation
and successful retry after ActiveRecord::RecordNotUnique.
In `@lib/super_settings/storage/json_storage.rb`:
- Around line 21-25: Update the save_all read-modify-write flow to coordinate
writers across processes, replacing the process-local SAVE_MUTEX protection with
backend-wide locking or conditional-write-and-retry semantics so concurrent
worker updates are merged without lost changes. Apply the same guarantee
throughout the persistence logic spanning the save_all implementation, and do
not rely on the existing Mutex as sufficient cross-process protection.
In `@lib/super_settings/storage/transaction.rb`:
- Around line 23-25: Update the transaction path around save_all so a false
result raises a dedicated persistence/storage exception rather than
Setting::InvalidRecordError. Ensure Setting.bulk_update does not classify this
exception as validation failure, and update RestAPI.update to translate it into
a structured 5xx/503 response instead of relying on setting.errors or returning
an empty 422.
---
Outside diff comments:
In `@lib/super_settings/storage/s3_storage.rb`:
- Around line 118-132: Synchronize the configuration snapshot and publication of
`@bucket` and `@bucket_hash` within S3Storage#s3_bucket using a mutex, ensuring
concurrent callers cannot observe mismatched values. Keep bucket creation and
hash assignment in the same protected critical section, while preserving the
existing configuration-change check and option construction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 974de693-819d-4d8a-b3f0-20c2c454de2f
📒 Files selected for processing (33)
CHANGELOG.mdVERSIONapp/helpers/super_settings/settings_helper.rblib/super_settings.rblib/super_settings/application.rblib/super_settings/application/helper.rblib/super_settings/application/scripts.jslib/super_settings/coerce.rblib/super_settings/configuration.rblib/super_settings/controller_actions.rblib/super_settings/engine.rblib/super_settings/http_client.rblib/super_settings/local_cache.rblib/super_settings/mini_i18n.rblib/super_settings/rack_application.rblib/super_settings/rest_api.rblib/super_settings/setting.rblib/super_settings/storage/active_record_storage.rblib/super_settings/storage/json_storage.rblib/super_settings/storage/mongodb_storage.rblib/super_settings/storage/redis_storage.rblib/super_settings/storage/s3_storage.rblib/super_settings/storage/test_storage.rblib/super_settings/storage/transaction.rbspec/spec_helper.rbspec/super_settings/coerce_spec.rbspec/super_settings/http_client_spec.rbspec/super_settings/local_cache_spec.rbspec/super_settings/rack_application_spec.rbspec/super_settings/setting_spec.rbspec/super_settings/storage/http_storage_spec.rbspec/super_settings/storage/transaction_spec.rbspec/super_settings_spec.rb
💤 Files with no reviewable changes (1)
- lib/super_settings/application.rb
- Freeze values inserted into LocalCache on refresh, cache miss, and direct update so callers cannot mutate the shared cache. - Return a 400 Bad Request from the Rack app when the settings update parameter is missing or is not an array of hashes. - Guard the web UI history view against history records with a null or unparseable timestamp. - Run each ActiveRecordStorage#save! attempt in a savepoint so a duplicate key failure can be rolled back and retried inside the surrounding transaction (previously left PostgreSQL transactions in an aborted state). - Raise a dedicated SuperSettings::Setting::PersistenceError when a storage engine fails to persist changes instead of reusing InvalidRecordError. - Synchronize construction of the cached S3 bucket so it is always published together with the configuration hash it was built from. - Document that JSON-payload based storage backends do not coordinate writes across processes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/super_settings/storage/s3_storage.rb`:
- Around line 124-137: Update the BUCKET_MUTEX.synchronize block in the bucket
construction flow to obtain one atomic snapshot of all Configuration fields
before computing config_hash or building options. Derive both `@bucket_hash` and
Aws::S3::Resource options, including the bucket name, endpoint, credentials, and
region, exclusively from that same snapshot so concurrent Configuration writes
cannot produce mixed values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9d3a2716-41f6-4497-984d-313314da7b34
📒 Files selected for processing (15)
CHANGELOG.mdlib/super_settings/application/scripts.jslib/super_settings/local_cache.rblib/super_settings/rack_application.rblib/super_settings/setting.rblib/super_settings/storage/active_record_storage.rblib/super_settings/storage/json_storage.rblib/super_settings/storage/s3_storage.rblib/super_settings/storage/transaction.rbspec/super_settings/local_cache_spec.rbspec/super_settings/rack_application_spec.rbspec/super_settings/setting_spec.rbspec/super_settings/storage/active_record_storage_spec.rbspec/super_settings/storage/http_storage_spec.rbspec/super_settings/storage/transaction_spec.rb
Owner
Author
|
Re the outside-diff comment on |
Building the bucket options and the configuration hash in separate read passes could cache a bucket built from mixed configuration values under the settled configuration hash after a concurrent reconfiguration. Both are now derived from the same read of the configuration attributes so a torn read cannot be cached permanently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security
Fixed
Setting.save!raising aNoMethodErrorin non-Rails applications that relied on the implicit ActiveRecord storage default. The transaction now resolves the storage class through the public accessor instead of the uninitialized instance variable.LocalCache#to_hreturning only the first element ofarraytype settings. It now returns the full array value.Setting#save!always updating theupdated_attimestamp (and triggering a write) even when nothing had changed, which caused unnecessary cache invalidation across processes.LocalCachewhere a value read on a cache miss could overwrite a fresher value written by a concurrent refresh.LocalCache#refreshnever picking up newly added settings if the cache had been loaded while the data store was empty.ActiveRecordStorage#save!that raised an unhandledActiveRecord::RecordNotUniquewhen the same key was created concurrently. The conflict is now retried and merged.HttpStoragesilently reporting success when the remote API rejected the changes.bulk_updatenow returnsfalseandsave!raises an error in this case.nilclient after a transient connection failure.MongoDBStorage.find_by_keyreturning records that reportedpersisted?asfalse.SuperSettings.authentication_urlwhen injected into the inline web UI JavaScript. URLs containing single quotes previously produced corrupted or invalid JavaScript.mapor Railsmount) without repeating the mount path in the constructor.HttpClientcorrupting base URLs that include a query string when appending the trailing path separator.HttpClientretrying non-idempotent POST requests after a connection error, which could apply an update twice./settings/updated_sinceendpoint returning a 500 (or a misleading empty success) when thetimeparameter was missing or unparseable. It now returns a 400 Bad Request.Coerce.booleanreturningtruefor a whitespace-only string.ActiveJob::Baseduring initialization.require "time"soTime.parsebased coercion works in non-Rails applications.Summary by CodeRabbit
Security
Bug Fixes
400responses for invalidtime, malformed JSON, and invalid update payloads.Chores