pal-e-docs

Review: Harden input validation and add security documentation

review-1476-2026-06-16 Review

review ready
Project

Verdict: APPROVED

Re-review after NEEDS_REFINEMENT on prior pass. The issue body has been updated to address all previous findings. Scope is solid, file targets verified, dependency documented. Ready for agent implementation.

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- PR #241 QA review origin
  • [x] Repo -- ldraney/landscaping-assistant
  • [x] User Story -- admin role, input validation + security posture
  • [x] Context -- detailed background with 3 nit categories
  • [x] File Targets -- modify list, create list, and "should NOT touch" list all present
  • [x] Feature Flag -- none (correct for security hardening)
  • [x] Acceptance Criteria -- 7 criteria, all verifiable
  • [x] Test Expectations -- 11 specific specs with run command
  • [x] Constraints -- 5 constraints including pattern guidance
  • [x] Checklist -- present
  • [x] Related -- project, PRs, parent issue referenced

Traceability

  • [x] story:security label -- present on board item
  • [ ] story note -- MISSING: no "security" entry in project-landscaping-assistant user-stories table. Acceptable: this is foundational/cross-cutting security work, not a user-facing feature story. The user story in the issue body is well-formed. Not blocking.
  • [x] arch:rails-app label -- present on board item
  • [ ] arch note -- MISSING: no arch-rails-app note in pal-e-docs. This is a pre-existing gap across the entire board (all rails-app items use this label). Not blocking for this ticket specifically.
  • [x] Forgejo issue -- ldraney/landscaping-assistant#243, state: open

File Targets

  • [x] app/controllers/days_controller.rb -- exists at HEAD (commit 40044da, PR #240). Does NOT yet have add_to_queue action; that comes from PR #241 (still open, not merged). Issue correctly documents this dependency.
  • [x] app/controllers/work_queue_items_controller.rb -- verified: create (line 65) and mark_other (line 183) both pass params[:property_id] directly to WorkQueueItem without existence check.
  • [x] app/controllers/weeks_controller.rb -- verified: toggle_assign (line 45) guards property_id <= 0 (line 49) but does not guard non-existent positive IDs. WorkQueueItem.create! at line 65 would raise ActiveRecord::InvalidForeignKey.
  • [x] app/controllers/application_controller.rb -- verified: before_action :authenticate_user! at line 13. Shared helper target is reasonable.
  • [x] spec/requests/work_queue_items_spec.rb -- exists, no unauthenticated tests currently.
  • [x] spec/requests/weeks_spec.rb -- exists, no unauthenticated tests currently.
  • [x] spec/requests/properties_spec.rb -- exists, no unauthenticated tests currently (role_access_spec covers /properties redirect but per-controller coverage is the goal here).
  • [x] spec/requests/crew_spec.rb -- exists, has 2 mentions of unauthenticated/redirect.
  • [x] spec/requests/uploads_spec.rb -- exists, no unauthenticated tests currently.
  • [x] spec/requests/days_spec.rb -- exists at HEAD (PR #240). Depends on PR #241 for add_to_queue spec.
  • [x] docs/security.md -- does not yet exist (correctly listed as "create").
  • [x] "Should NOT touch" list -- properties_controller.rb (uses Property.find, correct 404), sessions_controller.rb (skips auth), client_errors_controller.rb (skips auth), property_picker_controller.js (client-side). All verified accurate.

Repo Placement

OK. Issue is filed on ldraney/landscaping-assistant. All file targets are within this repo. No cross-repo work needed.

Dependencies

  • PR #241 (property picker for day detail page) -- not yet merged, state: open. DaysController#add_to_queue and spec/requests/days_spec.rb changes depend on this. Issue correctly documents this dependency. Agent should handle DaysController changes conditionally or this ticket should wait for PR #241 merge.
  • PR #240 (commit 40044da) -- already merged to main. DaysController base (show action) exists at HEAD. Previous reviewer incorrectly flagged this as phantom; confirmed present.
  • No board-level blockers found. Item #1476 is in backlog, no other items explicitly depend on or block it.

Acceptance Criteria

All 7 AC are verifiable by an agent:
  • AC 1-4: property_id validation returns 422 -- testable with request specs using non-existent IDs
  • AC 5: unauthenticated redirect specs -- testable with raw HTTP requests without session
  • AC 6: docs/security.md content requirements -- verifiable by file read
  • AC 7: existing tests pass -- verifiable by running test suite
AC 1 depends on PR #241. The remaining 6 AC can be implemented independently.

Blast Radius

  • Missing controllers in unauthenticated spec list: join_crew_controller.rb and profile_controller.rb are protected (inherit authenticate_user! from ApplicationController, no skip_before_action), but are NOT listed as unauthenticated spec targets. However, this is a minor scope gap -- the issue's stated goal is "at least one spec per protected controller," and these two controllers have existing spec files (join_crew_spec.rb, profile_spec.rb) that also lack unauthenticated coverage. Not blocking since the issue focuses on the controllers surfaced by the PR #241 QA review, but worth noting for the implementing agent.
  • role_access_spec.rb overlap: This file already covers unauthenticated redirects for /today, /properties, /weeks, /crew. The ticket's per-controller approach is complementary (defense in depth), not redundant. No conflict.
  • WeeksController#toggle_assign line 65: WorkQueueItem.create!(property_id: property_id, ...) with a non-existent property_id will raise ActiveRecord::InvalidForeignKey (500 error). The guard at line 49 only catches <= 0. Confirmed the ticket's diagnosis is correct.

Decomposition Assessment

7 AC, 11 test expectations, all in one repo. File targets: 5 controllers to modify + 6 spec files to modify + 1 doc to create = 12 files total. However, the property_id guard is a repetitive pattern (same fix applied 4 times), and the unauthenticated specs are boilerplate. The security doc is standalone writing. Estimated agent time: 4-5 minutes. Borderline but fits in a single pass at 3 points. No decomposition needed.

Recommendations

No blocking issues. Minor notes for the implementing agent:
  • [NOTE] DaysController changes (AC 1) depend on PR #241 being merged first. If PR #241 is not merged when work begins, implement all other AC first and leave AC 1 for a follow-up or wait.
  • [NOTE] Consider adding unauthenticated specs for join_crew_controller.rb and profile_controller.rb while in the neighborhood -- they are protected controllers with no unauthenticated coverage. Optional scope expansion.
  • [NOTE] The story:security entry and arch-rails-app note are pre-existing gaps across the board, not specific to this ticket. Can be addressed separately.