You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The BackupTokenAuthenticatable concern is a solid baseline (timing-safe secure_compare, bytesize pre-check, fails closed when env is unset). Before relying on it in production, a few things to button up:
Enforce token strength. The code accepts any non-empty BACKUP_API_TOKEN. A short or guessable value is brute-forceable. Document a minimum (e.g. 32 random bytes, SecureRandom.hex(32)) in the deploy notes and/or add a length check on boot.
Rate limiting. No throttling on POST /api/v1/backups. Auth failures return fast so it's not a DB-dump DoS, but unauthenticated request floods still hit the app. Consider Rack::Attack or an ingress-level limit.
Audit log. Currently no record of who triggered a backup or when. Add a log line (timestamp, source IP, success/failure) so incidents are traceable.
Header logging. Rails filters params by default, not headers. Verify X-Backup-Token doesn't end up in Rails logs, ingress/proxy logs, or APM traces. If it can, add the header to the filter list.
HTTPS. The concern doesn't enforce TLS itself — it relies on force_ssl / ingress. Confirm production terminates only via HTTPS for this route.
Replay / rotation. A captured token works forever until rotated. Either commit to a rotation cadence in ops docs, or upgrade to HMAC + timestamp if higher assurance is needed later.
None of these are blockers for shipping behind a strong token + HTTPS, but worth tracking before this endpoint sees real traffic.
Follow-up to the security checklist. Grouped roughly by impact.
Scope vs original request
Coverage This PR cleanly solves the Nov 22 Slack ask ("get a backup file onto our side"). It does not yet cover Justyna's Jan 8 follow-up, which is the operational one: a usable file for offline trip prep with locations (incl. coordinates) and assigned people, refreshed roughly monthly, that ops can open if the app is down before a trip. Right now we ship raw per-table CSVs for the whole DB as "surowy zrzut" — hard to actually work from. Suggest adding a second service like Backups::TripPrepExport that emits a single joined xlsx (location | coords | assigned people).
Operational
Sync work in the controllerBackupsController#create runs sequential Drive uploads run inline in the request. On a real DB (~30 tables × one upload each) this will likely take 30–90s and tie up a Puma worker, with risk of ALB / ingress timeouts as the DB grows. Suggest: keep the endpoint as a manual trigger (e.g. a button in the admin panel for ad-hoc backups) and add a rake backup task that wraps the same services, scheduled via the simplest AWS option (ECS Scheduled Task, EventBridge cron, or plain crontab on the host).
No retention / cleanup. Drive folder grows unbounded — every run creates a new backup_<timestamp>_UTC/ with no expiration. Decide a policy (keep last N, or delete older than X days/months) and either implement it in the service or document it as an ops responsibility.
Data fidelity
Raw connection.execute bypasses ActiveRecord.DumpAllTables reads tables via SELECT *, so values come out as raw DB representations — enums as their stored form, JSON columns as serialized text, encrypted attributes as ciphertext, NULL indistinguishable from empty string in CSV. Fine if the intent is a forensic snapshot, but it won't round-trip cleanly back into AR models on restore. If the disaster-recovery file from item 1 needs to be human-readable or programmatically restorable, prefer Model.find_each with as_json / explicit serialization.
Memory (informational)
DumpAllTables loads every table fully into memory (a hash of CSV strings), then UploadToGoogleDrive holds the whole hash while uploading sequentially. Likely moot once the scope shifts to dumping only what Justyna actually needs (locations, people, animals) per the first point, but flagging in case the all-tables path stays. Cheap fix if it does: yield per-table → upload → release, instead of building the full hash first.
Tests
Backups::UploadToGoogleDrive has no dedicated spec. It's stubbed at the request-spec level and DumpAllTables has its own unit test, but the upload service's logic — env-var guard, subfolder naming, content type, iteration — is untested. Add a spec with a fake session double covering at least the GOOGLE_DRIVE_BACKUP_FOLDER_ID-missing case and the happy path.
Minor / nits
Time.now.utc → Time.current. In UploadToGoogleDrive#call, Rails convention is Time.current (or Time.zone.now) so the app's configured zone is respected. Practically identical here since the format string is _UTC-suffixed anyway, but matches the rest of the codebase.
Route is singular.resource :backup, only: :create → POST /api/v1/backup. Conceptually each call creates one of many backups, so resources :backups, only: :create → POST /api/v1/backups reads more naturally and matches the plural convention used elsewhere in the API. Bikeshed-grade.
Google::DriveSession config path is relative to CWD.File.read("google_drive_client_config.json.erb") resolves against the working directory, not Rails.root. Works for bin/dev and most invocations, but fragile if the service is ever called from a rake task run in a different CWD, a console started in a subdirectory, or a background runner. One-line fix: File.read(Rails.root.join("google_drive_client_config.json.erb")).
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
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.
Description
Please include a short summary of the changes.
Please include relevant motivation and context.
Add a screenshot and/or video of interface changes if any.
Checklist BEFORE requesting a review
standardrbfor ruby code (runstandardb --fix).yarn prettier app/assets app/javascript --check)Relevant task
Trello card (link)