Harden input validation and add security documentation #243

New issue

Closed

opened 2026-06-17 00:08:59 +00:00 by ldraney · 3 comments

ldraney commented 2026-06-17 00:08:59 +00:00

Owner

Copy link

Type

Feature

Lineage

Discovered during PR #241 QA review (property picker for day detail page). Nits surfaced patterns that are consistent across the codebase, not regressions.

Repo

ldraney/landscaping-assistant

User Story

As an admin
I want controllers to gracefully reject invalid input and the team to have a shared understanding of the app's security posture
So that the app handles edge cases cleanly and we can reason about security as we grow

Context

PR #241 QA review surfaced 5 nits, several security-adjacent:

1. Defensive property_id lookup -- Controllers pass params[:property_id] directly to WorkQueueItem.new(property_id:). Non-existent IDs raise ActiveRecord::InvalidForeignKey (500 error). This pattern exists in DaysController#add_to_queue, WorkQueueItemsController#create, WorkQueueItemsController#mark_other, and WeeksController#toggle_assign. All are role-gated to admin, so low risk, but a 500 is never the right response for bad user input.

2. Unauthenticated request specs -- before_action :authenticate_user! in ApplicationController handles auth globally, but no spec verifies this. Every protected endpoint should have at least one spec confirming unauthenticated users get redirected to login.

3. No security documentation -- The app has auth docs (auth-strategy.md, keycloak-setup.md) but no unified security posture document. We need docs/security.md that maps: authentication model, authorization model (role hierarchy + require_role enforcement), attack surface, input validation patterns, CSRF, network security (Tailscale), and secrets management. Include mermaid diagrams for the auth flow and attack surface.

Dependency: This ticket depends on PR #241 being merged first. The DaysController#add_to_queue action is added by PR #241. The DaysController base (show action, route, view) was merged in PR #240 and exists on main.

File Targets

Files the agent should modify:

• app/controllers/days_controller.rb -- add property lookup guard in add_to_queue (added by PR #241, must be merged first)
• app/controllers/work_queue_items_controller.rb -- add property lookup guard in create and mark_other
• app/controllers/weeks_controller.rb -- add property lookup guard in toggle_assign (guards <= 0 but not non-existent positive IDs)
• app/controllers/application_controller.rb -- optional shared find_property_or_reject helper
• spec/requests/days_spec.rb -- add unauthenticated request spec and invalid property_id spec
• spec/requests/work_queue_items_spec.rb -- add unauthenticated request spec and invalid property_id spec
• spec/requests/weeks_spec.rb -- add unauthenticated request spec and invalid property_id spec
• spec/requests/properties_spec.rb -- add unauthenticated request spec
• spec/requests/crew_spec.rb -- add unauthenticated request spec
• spec/requests/uploads_spec.rb -- add unauthenticated request spec
• spec/requests/profile_spec.rb -- add unauthenticated request spec
• spec/requests/join_crew_spec.rb -- add unauthenticated request spec

Files the agent should create:

• docs/security.md -- security posture document with mermaid diagrams

Files the agent should NOT touch:

• app/javascript/controllers/property_picker_controller.js -- client-side, not relevant
• app/controllers/properties_controller.rb -- uses Property.find(params[:id]) which already returns 404 via Rails rescue, this is correct behavior
• app/controllers/sessions_controller.rb -- intentionally skips auth (skip_before_action :authenticate_user!)
• app/controllers/client_errors_controller.rb -- intentionally skips auth

Feature Flag

none

Acceptance Criteria

• DaysController#add_to_queue returns 422 with flash when property_id does not exist (depends on PR #241)
• WorkQueueItemsController#create returns 422 with flash when property_id does not exist (and no client_name)
• WorkQueueItemsController#mark_other returns 422 with flash when property_id does not exist
• WeeksController#toggle_assign returns 422 or appropriate error when property_id does not exist
• At least one spec per protected controller verifies unauthenticated users get redirected to login (8 controllers: days, work_queue_items, weeks, properties, crew, uploads, profile, join_crew)
• docs/security.md exists with: auth model, authorization model, attack surface mermaid diagram, input validation section, network security section
• Existing tests still pass

Test Expectations

• Spec: POST /days/:date/add with non-existent property_id returns 422 (depends on PR #241)
• Spec: POST /today with non-existent property_id returns 422
• Spec: POST /today/mark_other with non-existent property_id returns 422
• Spec: POST /weeks/toggle_assign with non-existent property_id returns 422
• Spec: GET /days/:date without auth redirects to login
• Spec: GET /today without auth redirects to login
• Spec: GET /properties without auth redirects to login
• Spec: GET /crew without auth redirects to login
• Spec: GET /weeks without auth redirects to login
• Spec: GET /uploads without auth redirects to login
• Spec: GET /profile without auth redirects to login
• Spec: GET /join-crew without auth redirects to login
• Run command: docker compose run --rm -e RAILS_ENV=test web bundle exec rspec spec/requests/

Constraints

• Follow the existing respond_to pattern with turbo_stream flash + HTML redirect for error responses
• The shared helper (if added) should be opt-in, not a global before_action -- controllers that use Property.find(params[:id]) already get correct 404 behavior
• Security doc should reflect the ACTUAL posture, not aspirational -- document what exists and note gaps honestly
• Mermaid diagrams should cover: (1) auth flow (browser -> Keycloak -> Rails session), (2) role hierarchy and tab access, (3) network topology (Tailscale funnel -> k3s -> app)
• WeeksController#toggle_assign already guards property_id <= 0 -- extend to also guard non-existent positive IDs

Checklist

• PR opened
• Tests pass
• No unrelated changes

Related

• project-landscaping-assistant -- project this affects
• PR #241 -- the review that surfaced these nits (must be merged before DaysController changes)
• PR #240 -- merged PR that added DaysController base
• #237 -- parent feature (day detail property picker)

### Type Feature ### Lineage Discovered during PR #241 QA review (property picker for day detail page). Nits surfaced patterns that are consistent across the codebase, not regressions. ### Repo `ldraney/landscaping-assistant` ### User Story As an admin I want controllers to gracefully reject invalid input and the team to have a shared understanding of the app's security posture So that the app handles edge cases cleanly and we can reason about security as we grow ### Context PR #241 QA review surfaced 5 nits, several security-adjacent: 1. **Defensive `property_id` lookup** -- Controllers pass `params[:property_id]` directly to `WorkQueueItem.new(property_id:)`. Non-existent IDs raise `ActiveRecord::InvalidForeignKey` (500 error). This pattern exists in `DaysController#add_to_queue`, `WorkQueueItemsController#create`, `WorkQueueItemsController#mark_other`, and `WeeksController#toggle_assign`. All are role-gated to admin, so low risk, but a 500 is never the right response for bad user input. 2. **Unauthenticated request specs** -- `before_action :authenticate_user!` in `ApplicationController` handles auth globally, but no spec verifies this. Every protected endpoint should have at least one spec confirming unauthenticated users get redirected to login. 3. **No security documentation** -- The app has auth docs (`auth-strategy.md`, `keycloak-setup.md`) but no unified security posture document. We need `docs/security.md` that maps: authentication model, authorization model (role hierarchy + `require_role` enforcement), attack surface, input validation patterns, CSRF, network security (Tailscale), and secrets management. Include mermaid diagrams for the auth flow and attack surface. **Dependency:** This ticket depends on PR #241 being merged first. The `DaysController#add_to_queue` action is added by PR #241. The `DaysController` base (show action, route, view) was merged in PR #240 and exists on main. ### File Targets Files the agent should modify: - `app/controllers/days_controller.rb` -- add property lookup guard in `add_to_queue` (added by PR #241, must be merged first) - `app/controllers/work_queue_items_controller.rb` -- add property lookup guard in `create` and `mark_other` - `app/controllers/weeks_controller.rb` -- add property lookup guard in `toggle_assign` (guards `<= 0` but not non-existent positive IDs) - `app/controllers/application_controller.rb` -- optional shared `find_property_or_reject` helper - `spec/requests/days_spec.rb` -- add unauthenticated request spec and invalid property_id spec - `spec/requests/work_queue_items_spec.rb` -- add unauthenticated request spec and invalid property_id spec - `spec/requests/weeks_spec.rb` -- add unauthenticated request spec and invalid property_id spec - `spec/requests/properties_spec.rb` -- add unauthenticated request spec - `spec/requests/crew_spec.rb` -- add unauthenticated request spec - `spec/requests/uploads_spec.rb` -- add unauthenticated request spec - `spec/requests/profile_spec.rb` -- add unauthenticated request spec - `spec/requests/join_crew_spec.rb` -- add unauthenticated request spec Files the agent should create: - `docs/security.md` -- security posture document with mermaid diagrams Files the agent should NOT touch: - `app/javascript/controllers/property_picker_controller.js` -- client-side, not relevant - `app/controllers/properties_controller.rb` -- uses `Property.find(params[:id])` which already returns 404 via Rails rescue, this is correct behavior - `app/controllers/sessions_controller.rb` -- intentionally skips auth (`skip_before_action :authenticate_user!`) - `app/controllers/client_errors_controller.rb` -- intentionally skips auth ### Feature Flag none ### Acceptance Criteria - [ ] `DaysController#add_to_queue` returns 422 with flash when `property_id` does not exist (depends on PR #241) - [ ] `WorkQueueItemsController#create` returns 422 with flash when `property_id` does not exist (and no `client_name`) - [ ] `WorkQueueItemsController#mark_other` returns 422 with flash when `property_id` does not exist - [ ] `WeeksController#toggle_assign` returns 422 or appropriate error when `property_id` does not exist - [ ] At least one spec per protected controller verifies unauthenticated users get redirected to login (8 controllers: days, work_queue_items, weeks, properties, crew, uploads, profile, join_crew) - [ ] `docs/security.md` exists with: auth model, authorization model, attack surface mermaid diagram, input validation section, network security section - [ ] Existing tests still pass ### Test Expectations - [ ] Spec: POST `/days/:date/add` with non-existent `property_id` returns 422 (depends on PR #241) - [ ] Spec: POST `/today` with non-existent `property_id` returns 422 - [ ] Spec: POST `/today/mark_other` with non-existent `property_id` returns 422 - [ ] Spec: POST `/weeks/toggle_assign` with non-existent `property_id` returns 422 - [ ] Spec: GET `/days/:date` without auth redirects to login - [ ] Spec: GET `/today` without auth redirects to login - [ ] Spec: GET `/properties` without auth redirects to login - [ ] Spec: GET `/crew` without auth redirects to login - [ ] Spec: GET `/weeks` without auth redirects to login - [ ] Spec: GET `/uploads` without auth redirects to login - [ ] Spec: GET `/profile` without auth redirects to login - [ ] Spec: GET `/join-crew` without auth redirects to login - Run command: `docker compose run --rm -e RAILS_ENV=test web bundle exec rspec spec/requests/` ### Constraints - Follow the existing `respond_to` pattern with turbo_stream flash + HTML redirect for error responses - The shared helper (if added) should be opt-in, not a global before_action -- controllers that use `Property.find(params[:id])` already get correct 404 behavior - Security doc should reflect the ACTUAL posture, not aspirational -- document what exists and note gaps honestly - Mermaid diagrams should cover: (1) auth flow (browser -> Keycloak -> Rails session), (2) role hierarchy and tab access, (3) network topology (Tailscale funnel -> k3s -> app) - `WeeksController#toggle_assign` already guards `property_id <= 0` -- extend to also guard non-existent positive IDs ### Checklist - [ ] PR opened - [ ] Tests pass - [ ] No unrelated changes ### Related - `project-landscaping-assistant` -- project this affects - PR #241 -- the review that surfaced these nits (must be merged before DaysController changes) - PR #240 -- merged PR that added DaysController base - #237 -- parent feature (day detail property picker)

ldraney commented 2026-06-17 00:12:51 +00:00

Author

Owner

Copy link

Scope Review: NEEDS_REFINEMENT

Review note: review-1476-2026-06-16
Issue references a non-existent DaysController and /days/ routes -- all "today" functionality lives in WorkQueueItemsController at /today. Multiple AC and test expectations target phantom routes.

[BODY] fixes required:

• Remove all references to DaysController, days_controller.rb, spec/requests/days_spec.rb -- none exist
• Remove AC 1 (DaysController#add_to_queue returns 422) -- controller/action does not exist
• Remove test expectations for POST /days/:date/add and GET /days/:date -- routes do not exist
• Consider adding WeeksController#toggle_assign to scope -- same params[:property_id] vulnerability (current guard only rejects <=0, not non-existent IDs)

[SCOPE] items:

• Create user story entry "security" on project-landscaping-assistant user-stories section
• Create architecture note arch-rails-app (used by many board items)

## Scope Review: NEEDS_REFINEMENT Review note: `review-1476-2026-06-16` Issue references a non-existent `DaysController` and `/days/` routes -- all "today" functionality lives in `WorkQueueItemsController` at `/today`. Multiple AC and test expectations target phantom routes. **[BODY] fixes required:** - Remove all references to `DaysController`, `days_controller.rb`, `spec/requests/days_spec.rb` -- none exist - Remove AC 1 (`DaysController#add_to_queue` returns 422) -- controller/action does not exist - Remove test expectations for `POST /days/:date/add` and `GET /days/:date` -- routes do not exist - Consider adding `WeeksController#toggle_assign` to scope -- same `params[:property_id]` vulnerability (current guard only rejects <=0, not non-existent IDs) **[SCOPE] items:** - Create user story entry "security" on project-landscaping-assistant user-stories section - Create architecture note `arch-rails-app` (used by many board items)

ldraney commented 2026-06-17 00:14:25 +00:00

Author

Owner

Copy link

Scope refinement after review-1476-2026-06-16

Changes to issue body based on scope review findings:

1. Added WeeksController#toggle_assign to file targets, acceptance criteria, and test expectations. Reviewer correctly identified this as a blast radius miss -- same property_id vulnerability (guards <= 0 but not non-existent positive IDs).

2. Added all protected controllers to unauthenticated spec targets. Original scope only listed days and work_queue_items. Now includes: days, work_queue_items, weeks, properties, crew, uploads.

3. Added PR #241 dependency note. The DaysController#add_to_queue action is added by PR #241 (not yet merged). Clarified that DaysController base exists on main (merged in PR #240).

4. Clarified reviewer false positive. The reviewer flagged DaysController and days_spec.rb as phantom files. These DO exist on main -- DaysController was merged in PR #240 (commit 40044da). The add_to_queue action is from PR #241 which must merge first.

5. Added sessions_controller.rb and client_errors_controller.rb to "should NOT touch" list since they intentionally skip auth.

## Scope refinement after review-1476-2026-06-16 Changes to issue body based on scope review findings: 1. **Added `WeeksController#toggle_assign`** to file targets, acceptance criteria, and test expectations. Reviewer correctly identified this as a blast radius miss -- same `property_id` vulnerability (guards `<= 0` but not non-existent positive IDs). 2. **Added all protected controllers** to unauthenticated spec targets. Original scope only listed days and work_queue_items. Now includes: days, work_queue_items, weeks, properties, crew, uploads. 3. **Added PR #241 dependency note.** The `DaysController#add_to_queue` action is added by PR #241 (not yet merged). Clarified that `DaysController` base exists on main (merged in PR #240). 4. **Clarified reviewer false positive.** The reviewer flagged `DaysController` and `days_spec.rb` as phantom files. These DO exist on main -- `DaysController` was merged in PR #240 (commit 40044da). The `add_to_queue` action is from PR #241 which must merge first. 5. **Added `sessions_controller.rb` and `client_errors_controller.rb`** to "should NOT touch" list since they intentionally skip auth.

ldraney commented 2026-06-17 00:18:11 +00:00

Author

Owner

Copy link

Scope Review: APPROVED

Review note: review-1476-2026-06-16

Re-review after NEEDS_REFINEMENT. All previous findings addressed in the updated issue body. Template complete, file targets verified against codebase, PR #241 dependency correctly documented.

Non-blocking notes for implementing agent:

• DaysController add_to_queue depends on PR #241 (still open) -- implement remaining 6 AC first if #241 is not merged
• join_crew_controller.rb and profile_controller.rb are protected but not listed as unauthenticated spec targets (optional scope expansion)
• story:security user story entry and arch-rails-app architecture note are pre-existing gaps, not specific to this ticket

## Scope Review: APPROVED Review note: `review-1476-2026-06-16` Re-review after NEEDS_REFINEMENT. All previous findings addressed in the updated issue body. Template complete, file targets verified against codebase, PR #241 dependency correctly documented. **Non-blocking notes for implementing agent:** - DaysController `add_to_queue` depends on PR #241 (still open) -- implement remaining 6 AC first if #241 is not merged - `join_crew_controller.rb` and `profile_controller.rb` are protected but not listed as unauthenticated spec targets (optional scope expansion) - `story:security` user story entry and `arch-rails-app` architecture note are pre-existing gaps, not specific to this ticket

ldraney referenced this issue from a commit 2026-06-17 03:48:37 +00:00

Harden input validation and add security documentation

ldraney referenced this issue from a pull request that will close it, 2026-06-17 03:49:01 +00:00

Harden input validation and add security documentation #244

ldraney referenced this issue 2026-06-17 04:29:23 +00:00

Harden input validation and add security documentation #244

ldraney closed this issue 2026-06-17 07:32:12 +00:00

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

docs/update-app-architecture

revert-213

move-other-crew-bottom-remove-schedule

feature/today-view-improvements

154-enable-assume-ssl

revert-145-person-icon

docs/user-stories-auth

docs/update-infra-pipeline-post-incident

fix-cache-step-blocking-deploy

fix-kaniko-registry-pull

deactivate-from-today

feature/quick-address-entry

No results found.

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/landscaping-assistant#243

No description provided.