190 lines
10 KiB
Markdown
190 lines
10 KiB
Markdown
# Technical Concerns
|
||
|
||
**Analysis Date:** 2026-05-05
|
||
**Revised:** 2026-05-05 — verified call sites; demoted false positives (see CHANGELOG at bottom)
|
||
|
||
Prioritized list of issues found during codebase analysis.
|
||
|
||
---
|
||
|
||
## CRITICAL — Security / Data Loss Risk
|
||
|
||
### C1. Race condition on booking creation — no atomic lock
|
||
|
||
**Files:** `includes/class-availability.php` (`is_available`), `api/class-rest-controller.php` (`create_booking` lines 326–362)
|
||
|
||
Flow: `is_available()` → `Booking::create()` → `mark_as_booked()` — three separate non-transactional operations. Two concurrent POST requests for the same yacht and overlapping dates will both pass the check before either writes. The `UNIQUE KEY yacht_date (yacht_id, date)` will reject the second `mark_as_booked()` iteration silently, but both `yacht_booking` posts are already created. Admin sees two confirmed bookings for the same dates.
|
||
|
||
**Fix:** Wrap in a DB transaction with `SELECT ... FOR UPDATE` on availability rows, or insert a sentinel lock row first and check the result before proceeding.
|
||
|
||
---
|
||
|
||
### C2. OAuth credentials stored unencrypted in `wp_options`
|
||
|
||
**File:** `integrations/google-calendar/class-oauth-handler.php` lines 185–187, 213–215
|
||
|
||
`yacht_booking_gcal_credentials` (client_id, client_secret) and `yacht_booking_gcal_tokens` (access_token, refresh_token) stored as plaintext serialized arrays. Any plugin or user with DB read access can extract a working Google Calendar refresh token. If the database is compromised (via SQL injection elsewhere on the WordPress install or hosting breach), the Google Calendar account is fully exposed.
|
||
|
||
**Fix:** Encrypt with WordPress secret keys (`AUTH_KEY` etc.) before storing; decrypt on read.
|
||
|
||
---
|
||
|
||
## HIGH — Reliability
|
||
|
||
### H1. GCal API HTTP errors silently ignored
|
||
|
||
**Files:** `integrations/google-calendar/class-gcal-service.php`, `class-oauth-handler.php`
|
||
|
||
Every API call checks `is_wp_error()` but not the HTTP response code. A `401`, `403`, `429`, or `500` from Google is treated as success — `json_decode` runs, expected fields like `$body['id']` or `$body['access_token']` are absent, function returns `false`. All errors only logged when `WP_DEBUG === true`. In production: complete silence.
|
||
|
||
---
|
||
|
||
### H2. Token refresh has no concurrency protection
|
||
|
||
**File:** `integrations/google-calendar/class-oauth-handler.php` lines 107–128
|
||
|
||
Simultaneous WP-Cron events both finding the token expired will both call `refresh_access_token()`. Google may invalidate the first new token when the second refresh request arrives. Result: permanently broken GCal connection requiring manual reauth.
|
||
|
||
**Fix:** Wrap refresh in a transient-based lock (`set_transient('gcal_refresh_lock', 1, 60)`), or use `wp_cache_add()` semantics.
|
||
|
||
---
|
||
|
||
### H3. `register_cron_actions()` may not be called — cron hooks fire with no registered callbacks
|
||
|
||
**File:** `integrations/google-calendar/class-sync-controller.php` lines 141–153
|
||
|
||
The constructor registers `on_booking_created` etc., but `register_cron_actions()` (which registers the `yacht_booking_sync_to_gcal` action itself) is a separate static method. Need to verify it is called on every `init` / `plugins_loaded` request. If not, scheduled single events silently fire with no handler.
|
||
|
||
---
|
||
|
||
### H4. Single calendar for all yachts — GCal events apply to every yacht
|
||
|
||
**File:** `integrations/google-calendar/class-gcal-service.php` lines 196–272
|
||
|
||
`get_calendar_id()` returns one shared option (`yacht_booking_gcal_calendar_id`). `pull_from_gcal()` calls `sync_from_gcal($yacht->ID)` for each yacht but each call fetches all events from the same calendar and blocks that yacht for all those dates. Multi-yacht sites will incorrectly block all yachts for every GCal event.
|
||
|
||
---
|
||
|
||
### H5. `tmp-fix-polish.php` in project root reveals dev paths
|
||
|
||
**File:** `tmp-fix-polish.php`
|
||
|
||
Contains only one line: a path string to another local project (`c:\visual studio code\projekty\jachty.pagedev.pl\tmp-fix-polish.php`). No PHP tag, no code. If deployed via FTP, the raw path string is publicly accessible at the URL. Should be deleted and excluded from deployment.
|
||
|
||
---
|
||
|
||
## MEDIUM — Maintainability / Correctness
|
||
|
||
### M1. Date validation missing on booking creation
|
||
|
||
**File:** `api/class-rest-controller.php` lines 116–122
|
||
|
||
`start_date`/`end_date` use only `sanitize_text_field` — no `validate_callback`. An invalid date string like `"not-a-date"` reaches `new \DateTime()` in `Availability::count_days()` without try/catch, producing an uncaught fatal error visible to the public. The `/availability` endpoint validates dates correctly; `/bookings` does not.
|
||
|
||
**Fix:** Add `validate_callback` checking `\DateTime::createFromFormat('Y-m-d', $param) !== false`.
|
||
|
||
---
|
||
|
||
### M2. No pagination on `get_yachts` and `get_bookings`
|
||
|
||
**File:** `api/class-rest-controller.php` lines 222–245, 429–462
|
||
|
||
Both endpoints use `posts_per_page => -1`. Will degrade progressively as bookings accumulate.
|
||
|
||
---
|
||
|
||
### M3. N+1 query pattern in booking and yacht lists
|
||
|
||
**File:** `api/class-rest-controller.php` lines 442–458
|
||
|
||
For each booking: `get_post($yacht_id)` called individually inside loop. For each yacht: three separate `get_post_meta()` calls per row. With 20 bookings: 60+ queries per response.
|
||
|
||
---
|
||
|
||
### M4. iCal importer ignores `STATUS:CANCELLED` events
|
||
|
||
**File:** `integrations/ical/class-ical-import.php` `parse_ics()` method
|
||
|
||
Parser reads UID, SUMMARY, DTSTART, DTEND but ignores STATUS. Cancelled events in the iCal feed are imported as blocks. The GCal service correctly skips `status: cancelled` events (line 347 of `class-gcal-service.php`) — parity needed in iCal importer.
|
||
|
||
---
|
||
|
||
### M5. `mark_as_blocked()` passes `null` booking_id with `%d` format
|
||
|
||
**File:** `includes/class-availability.php` lines 98–109
|
||
|
||
`$wpdb->replace()` receives `null` with `%d` format specifier. In older MySQL/MariaDB this may insert `0` instead of `NULL`, making it impossible to distinguish a blocked row from one linked to booking ID 0.
|
||
|
||
---
|
||
|
||
### M6. OAuth redirect URI hardcoded to production domain
|
||
|
||
**File:** `integrations/google-calendar/class-oauth-handler.php` lines 40, 72
|
||
|
||
`$redirect_uri = 'https://jachty.pagedev.pl/wp-admin/...'` — literal string, not `admin_url()`. OAuth fails completely on staging or local dev. Comment acknowledges this is intentional but makes testing and domain migration fragile.
|
||
|
||
---
|
||
|
||
## LOW — Cleanup / Polish
|
||
|
||
### L1. Dead code: `Availability::mark_as_available()` has bug but is never called
|
||
|
||
**File:** `includes/class-availability.php` lines 123–149
|
||
|
||
The function ignores its `$start_date`/`$end_date` parameters and runs `$wpdb->delete()` filtered only by `yacht_id` — which would wipe the entire availability table for that yacht.
|
||
|
||
**Verification:** `grep` across the plugin shows zero call sites. All code that handles cancellation/deletion uses `clear_booking_availability($booking_id)` (in `Admin`, `Rest_Controller`, `GCal_Service`, `ICal_Import`), which correctly filters by `booking_id`.
|
||
|
||
**Risk:** Trap for a future developer who reads the method name and decides to use it. Recommend either deleting the function or replacing its body with the commented-out correct implementation that already lives below it (lines 137–146).
|
||
|
||
---
|
||
|
||
### L2. Dead form field: `yacht_booking_nonce` from `wp_nonce_field` is never read
|
||
|
||
**Files:** `frontend/class-shortcode.php:329`, `frontend/class-calendar-widget.php:429`
|
||
|
||
The booking form renders `wp_nonce_field('yacht_booking_submit', 'yacht_booking_nonce')` as a hidden input, but `calendar.js` (lines 336–343) builds `formData` manually from named fields and never includes it. The actual nonce travels via `X-WP-Nonce` header from `yachtBookingData.nonce` (a `wp_create_nonce('wp_rest')` value), which matches what `Rest_Controller::create_booking()` verifies.
|
||
|
||
**Risk:** Code rot — looks like it's doing something but isn't. Either remove the `wp_nonce_field` call, or wire JS to send it as a fallback for the no-JS case (currently there is no no-JS fallback anyway).
|
||
|
||
---
|
||
|
||
### L3. Logging disabled in production
|
||
|
||
All `error_log()` calls gated on `WP_DEBUG === true`. GCal sync errors, token refresh failures, and iCal import problems are silently discarded in production. Consider a persistent log table or option-based error queue surfaced in admin.
|
||
|
||
---
|
||
|
||
### L4. No incremental DB migration path
|
||
|
||
**File:** `includes/class-installer.php`
|
||
|
||
No `upgrade()` method runs when `yacht_booking_version` in `wp_options` differs from `YACHT_BOOKING_VERSION`. `dbDelta()` adds missing columns but won't remove old ones or modify existing column types. Future schema changes risk silent failures on existing installs.
|
||
|
||
---
|
||
|
||
### L5. `yacht_booking` CPT exposed to default WP REST API
|
||
|
||
**File:** `includes/class-booking.php` line 48
|
||
|
||
`show_in_rest => true` registers the CPT at `/wp/v2/yacht_booking`. The plugin's own `/yacht-booking/v1/` namespace is the intended REST surface. Set `show_in_rest => false` unless there is a reason for the default WP exposure.
|
||
|
||
---
|
||
|
||
### L6. `Booking::create()` does not validate end > start
|
||
|
||
**File:** `includes/class-booking.php` lines 229–230
|
||
|
||
A booking with `end_date` before `start_date` is stored without error. `get_date_range()` while-loop exits immediately, no availability rows are created, and the booking is invisible in the calendar but exists as a CPT post.
|
||
|
||
---
|
||
|
||
## CHANGELOG
|
||
|
||
**2026-05-05 (revision):** Subagent audit produced two false positives that were initially listed as CRITICAL. Verified by tracing call sites:
|
||
|
||
- **Old C1** (`mark_as_available` deletes all rows) → demoted to **L1**. The function is never called anywhere in the plugin; all cancellation paths use the correct `clear_booking_availability($booking_id)`. Remains a trap for future devs but no production impact.
|
||
- **Old C3** (POST /bookings nonce mismatch) → demoted to **L2**. The actual auth path (JS → `X-WP-Nonce` header → `wp_verify_nonce(..., 'wp_rest')`) is fully consistent. The `wp_nonce_field('yacht_booking_submit', ...)` in shortcode/widget renders a hidden input that JS never reads — dead form field, not a security gap.
|
||
|
||
Race condition (old C2 → C1) and unencrypted OAuth tokens (old C4 → C2) remain valid CRITICAL items.
|