Harden input validation and add security documentation #244

Merged

ldraney merged 2 commits from 243-harden-input-validation-and-add-security into main 2026-06-17 07:32:12 +00:00

Conversation 2 Commits 2 Files changed 10 +381 -3

ldraney commented 2026-06-17 03:49:01 +00:00

Owner

Copy link

Summary

Harden property_id validation across 4 controllers to prevent 500 errors from invalid foreign keys, add unauthenticated request specs for all protected controllers, and document the app's security posture.

Changes

• app/controllers/application_controller.rb -- Add shared find_property_or_reject helper for property ID lookup
• app/controllers/days_controller.rb -- Guard add_to_queue against non-existent property_id (returns 422)
• app/controllers/work_queue_items_controller.rb -- Guard create (when no client_name) and mark_other against non-existent property_id (returns 422)
• app/controllers/weeks_controller.rb -- Extend toggle_assign guard to reject non-existent positive property IDs (returns 422)
• spec/requests/days_spec.rb -- Add invalid property_id specs and unauthenticated access spec
• spec/requests/work_queue_items_spec.rb -- Add invalid property_id specs for create and mark_other, unauthenticated access spec
• spec/requests/weeks_spec.rb -- Add invalid property_id spec for toggle_assign, unauthenticated access spec
• spec/requests/properties_spec.rb -- Add unauthenticated access spec
• spec/requests/uploads_spec.rb -- Add unauthenticated access spec
• docs/security.md -- New security posture document with auth model, role hierarchy, attack surface diagram, input validation patterns, CSRF, secrets management, and network security

Test Plan

• All 276 request specs pass (0 failures)
• docker compose run --rm -e RAILS_ENV=test web bundle exec rspec spec/requests/
• Invalid property_id specs verify 422 responses for all 4 guarded actions
• Unauthenticated specs verify redirect to /login for days, work_queue_items, weeks, properties, uploads (crew/profile/join_crew already had coverage)

Review Checklist

• Controller guards return 422 with flash for invalid property_id
• Shared helper is opt-in, not global before_action
• Unauthenticated specs cover all 8 protected controllers
• Security doc reflects actual posture, not aspirational
• Existing tests still pass
• No unrelated changes

Related

Closes #243

Related Notes

• Depends on PR #241 (merged -- branch includes those changes via merge)
• Discovered during PR #241 QA review

## Summary Harden property_id validation across 4 controllers to prevent 500 errors from invalid foreign keys, add unauthenticated request specs for all protected controllers, and document the app's security posture. ## Changes - `app/controllers/application_controller.rb` -- Add shared `find_property_or_reject` helper for property ID lookup - `app/controllers/days_controller.rb` -- Guard `add_to_queue` against non-existent property_id (returns 422) - `app/controllers/work_queue_items_controller.rb` -- Guard `create` (when no client_name) and `mark_other` against non-existent property_id (returns 422) - `app/controllers/weeks_controller.rb` -- Extend `toggle_assign` guard to reject non-existent positive property IDs (returns 422) - `spec/requests/days_spec.rb` -- Add invalid property_id specs and unauthenticated access spec - `spec/requests/work_queue_items_spec.rb` -- Add invalid property_id specs for create and mark_other, unauthenticated access spec - `spec/requests/weeks_spec.rb` -- Add invalid property_id spec for toggle_assign, unauthenticated access spec - `spec/requests/properties_spec.rb` -- Add unauthenticated access spec - `spec/requests/uploads_spec.rb` -- Add unauthenticated access spec - `docs/security.md` -- New security posture document with auth model, role hierarchy, attack surface diagram, input validation patterns, CSRF, secrets management, and network security ## Test Plan - All 276 request specs pass (0 failures) - `docker compose run --rm -e RAILS_ENV=test web bundle exec rspec spec/requests/` - Invalid property_id specs verify 422 responses for all 4 guarded actions - Unauthenticated specs verify redirect to /login for days, work_queue_items, weeks, properties, uploads (crew/profile/join_crew already had coverage) ## Review Checklist - [x] Controller guards return 422 with flash for invalid property_id - [x] Shared helper is opt-in, not global before_action - [x] Unauthenticated specs cover all 8 protected controllers - [x] Security doc reflects actual posture, not aspirational - [x] Existing tests still pass - [x] No unrelated changes ## Related Closes #243 ## Related Notes - Depends on PR #241 (merged -- branch includes those changes via merge) - Discovered during PR #241 QA review

ldraney added 1 commit 2026-06-17 03:49:01 +00:00

Harden input validation and add security documentation ... 580bd84b29

Closes #243

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-17 03:51:18 +00:00

Author

Owner

Copy link

PR #244 Review

DOMAIN REVIEW

Stack: Ruby on Rails 8.1, RSpec request specs, Hotwire/Turbo Stream, Markdown documentation.

Controller hardening (4 controllers, 1 shared helper)

The find_property_or_reject helper in ApplicationController is clean -- it does one thing (Property.find_by(id:)) and returns nil on miss, leaving rejection handling to each caller. This is the right call vs. a global before_action because only 4 specific actions need this guard, and each has different response formats (Turbo Stream vs. HTML redirect vs. bare head).

All four guarded actions are the correct set. I verified via grep that no other controller accepts property_id from params. The PropertiesController correctly uses Property.find(params[:id]) which raises RecordNotFound (Rails 404) -- appropriate since the property IS the resource there, not a foreign key.

Response format consistency:

• DaysController#add_to_queue: Turbo Stream 422 + HTML redirect -- correct, matches the existing "Already in queue" pattern in the same action.
• WorkQueueItemsController#create: Turbo Stream 422 + HTML redirect -- correct, matches existing error pattern.
• WorkQueueItemsController#mark_other: Turbo Stream 422 + HTML redirect -- correct, same pattern.
• WeeksController#toggle_assign: Bare head(:unprocessable_entity) -- correct, this action already uses head(:bad_request) for property_id <= 0 and head(:created)/head(:no_content) for success. JSON-only endpoint, no flash needed.

DRY observation: The Turbo Stream error response block is repeated 3 times across DaysController and WorkQueueItemsController (lines in diff: days_controller +24..+29, work_queue_items +76..+81, +197..+202). All three render the same turbo_stream.append("flash-messages", partial: "shared/flash", ...) pattern with a 422 status. This is an existing pattern in the codebase (the "Already in queue" error in DaysController uses it too), so it is consistent rather than novel duplication. Not a blocker -- extracting a shared render_flash_error(message, redirect_path) helper would be a future cleanup, not a gate for this PR.

Test coverage:

• Invalid property_id specs cover all 4 guarded actions: DaysController#add_to_queue (Turbo Stream + HTML), WorkQueueItemsController#create (Turbo Stream), WorkQueueItemsController#mark_other (Turbo Stream), WeeksController#toggle_assign (JSON). Good.
• The WorkQueueItemsController#create spec also verifies the client_name bypass path still works (creates a new property instead of looking up by ID). This is a smart edge case test.
• Unauthenticated access specs added for 5 controllers (Days, WorkQueueItems, Weeks, Properties, Uploads). PR body notes that Crew, Profile, and JoinCrew already had coverage. This completes the set.

Security document (docs/security.md, 247 lines):

• Auth flow diagram accurately reflects Auth Code + PKCE with server-side token exchange. Consistent with docs/auth-strategy.md and docs/keycloak-setup.md.
• Controller Role Map matches actual require_role declarations. I verified PropertiesController (lead, admin, super_admin, except: [:show, :new, :edit, :create, :update]), DaysController (admin, super_admin), WeeksController (admin, super_admin), WorkQueueItemsController (member, lead, admin, super_admin, except: :index). All accurate.
• Attack surface diagram correctly shows Tailscale Funnel as the only public ingress. CNPG, Keycloak, and Solid Queue are shown as internal.
• Known gaps section is honest: no rate limiting, no network policies, no explicit param allowlisting on some actions. This is documenting actual posture, not aspirational state -- exactly right.
• Session security section correctly notes tokens are used once during callback and discarded. No tokens in session storage.

BLOCKERS

None.

NITS

1. Method naming: find_property_or_reject does not actually reject -- it returns nil and leaves rejection to the caller. A name like find_property or find_property_by_id would be more accurate. The current name implies the method handles the rejection itself (like Rails' find_or_create_by! pattern). Very minor -- the docstring clarifies intent.

2. HTML format assertion on days invalid property_id test: The spec asserts expect(response).to redirect_to(day_path(date: tuesday)) but does not verify the flash alert message. The Turbo Stream path checks for "Property not found" in the body, but the HTML path only checks the redirect target. Consider adding follow_redirect! and checking for the alert, or at minimum asserting response.status is 302. Low risk since the controller code clearly sets alert: "Property not found.".

3. Hardcoded test ID 999999: All invalid property_id specs use 999999 as the non-existent ID. This works but is brittle if the test database ever auto-increments that high (unlikely but theoretically possible). A more robust pattern would be Property.maximum(:id).to_i + 1 or 0. Not worth changing -- just noting.

4. Security doc: Keycloak H2 reference: The attack surface diagram shows H2 Database (PVC) for Keycloak. If Keycloak was migrated to PostgreSQL (common for production), this should be updated. If it genuinely runs H2 in production, the doc is accurate but worth a comment noting the operational risk.

SOP COMPLIANCE

• PR body has: Summary, Changes, Test Plan, Related -- all present and detailed
• Review Checklist included with all items checked
• No secrets, .env files, or credentials committed
• No unnecessary file changes (all 10 files are directly related to the issue scope)
• Commit message is descriptive (PR title matches issue title)
• Test plan includes specific run command (docker compose run --rm -e RAILS_ENV=test web bundle exec rspec spec/requests/)
• PR body references parent issue (Closes #243)
• PR body notes dependency on merged PR #241

PROCESS OBSERVATIONS

• Deployment risk: Low. Controller changes add early-return guards before existing logic -- no change to happy-path behavior. Security doc is additive documentation only.
• Change failure risk: Low. All new code paths return 422 on invalid input, which is the correct HTTP semantics for "valid request format, invalid data." No database migrations.
• Documentation impact: The docs/security.md file should be linked from the main CLAUDE.md docs table. Currently the table of contents in CLAUDE.md does not reference it. This can be a follow-up.
• Coverage completeness: This PR closes a real gap discovered during PR #241 review -- foreign key violations causing 500s in production. The fix is proportional to the problem.

VERDICT: APPROVED

## PR #244 Review ### DOMAIN REVIEW **Stack:** Ruby on Rails 8.1, RSpec request specs, Hotwire/Turbo Stream, Markdown documentation. **Controller hardening (4 controllers, 1 shared helper)** The `find_property_or_reject` helper in `ApplicationController` is clean -- it does one thing (`Property.find_by(id:)`) and returns nil on miss, leaving rejection handling to each caller. This is the right call vs. a global `before_action` because only 4 specific actions need this guard, and each has different response formats (Turbo Stream vs. HTML redirect vs. bare `head`). All four guarded actions are the correct set. I verified via grep that no other controller accepts `property_id` from params. The `PropertiesController` correctly uses `Property.find(params[:id])` which raises `RecordNotFound` (Rails 404) -- appropriate since the property IS the resource there, not a foreign key. **Response format consistency:** - `DaysController#add_to_queue`: Turbo Stream 422 + HTML redirect -- correct, matches the existing "Already in queue" pattern in the same action. - `WorkQueueItemsController#create`: Turbo Stream 422 + HTML redirect -- correct, matches existing error pattern. - `WorkQueueItemsController#mark_other`: Turbo Stream 422 + HTML redirect -- correct, same pattern. - `WeeksController#toggle_assign`: Bare `head(:unprocessable_entity)` -- correct, this action already uses `head(:bad_request)` for `property_id <= 0` and `head(:created)`/`head(:no_content)` for success. JSON-only endpoint, no flash needed. **DRY observation:** The Turbo Stream error response block is repeated 3 times across `DaysController` and `WorkQueueItemsController` (lines in diff: days_controller +24..+29, work_queue_items +76..+81, +197..+202). All three render the same `turbo_stream.append("flash-messages", partial: "shared/flash", ...)` pattern with a 422 status. This is an existing pattern in the codebase (the "Already in queue" error in `DaysController` uses it too), so it is consistent rather than novel duplication. Not a blocker -- extracting a shared `render_flash_error(message, redirect_path)` helper would be a future cleanup, not a gate for this PR. **Test coverage:** - Invalid `property_id` specs cover all 4 guarded actions: `DaysController#add_to_queue` (Turbo Stream + HTML), `WorkQueueItemsController#create` (Turbo Stream), `WorkQueueItemsController#mark_other` (Turbo Stream), `WeeksController#toggle_assign` (JSON). Good. - The `WorkQueueItemsController#create` spec also verifies the `client_name` bypass path still works (creates a new property instead of looking up by ID). This is a smart edge case test. - Unauthenticated access specs added for 5 controllers (Days, WorkQueueItems, Weeks, Properties, Uploads). PR body notes that Crew, Profile, and JoinCrew already had coverage. This completes the set. **Security document (docs/security.md, 247 lines):** - Auth flow diagram accurately reflects Auth Code + PKCE with server-side token exchange. Consistent with `docs/auth-strategy.md` and `docs/keycloak-setup.md`. - Controller Role Map matches actual `require_role` declarations. I verified `PropertiesController` (`lead, admin, super_admin, except: [:show, :new, :edit, :create, :update]`), `DaysController` (`admin, super_admin`), `WeeksController` (`admin, super_admin`), `WorkQueueItemsController` (`member, lead, admin, super_admin, except: :index`). All accurate. - Attack surface diagram correctly shows Tailscale Funnel as the only public ingress. CNPG, Keycloak, and Solid Queue are shown as internal. - Known gaps section is honest: no rate limiting, no network policies, no explicit param allowlisting on some actions. This is documenting actual posture, not aspirational state -- exactly right. - Session security section correctly notes tokens are used once during callback and discarded. No tokens in session storage. ### BLOCKERS None. ### NITS 1. **Method naming:** `find_property_or_reject` does not actually reject -- it returns nil and leaves rejection to the caller. A name like `find_property` or `find_property_by_id` would be more accurate. The current name implies the method handles the rejection itself (like Rails' `find_or_create_by!` pattern). Very minor -- the docstring clarifies intent. 2. **HTML format assertion on days invalid property_id test:** The spec asserts `expect(response).to redirect_to(day_path(date: tuesday))` but does not verify the flash alert message. The Turbo Stream path checks for "Property not found" in the body, but the HTML path only checks the redirect target. Consider adding `follow_redirect!` and checking for the alert, or at minimum asserting `response.status` is 302. Low risk since the controller code clearly sets `alert: "Property not found."`. 3. **Hardcoded test ID `999999`:** All invalid property_id specs use `999999` as the non-existent ID. This works but is brittle if the test database ever auto-increments that high (unlikely but theoretically possible). A more robust pattern would be `Property.maximum(:id).to_i + 1` or `0`. Not worth changing -- just noting. 4. **Security doc: Keycloak H2 reference:** The attack surface diagram shows `H2 Database (PVC)` for Keycloak. If Keycloak was migrated to PostgreSQL (common for production), this should be updated. If it genuinely runs H2 in production, the doc is accurate but worth a comment noting the operational risk. ### SOP COMPLIANCE - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and detailed - [x] Review Checklist included with all items checked - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes (all 10 files are directly related to the issue scope) - [x] Commit message is descriptive (PR title matches issue title) - [x] Test plan includes specific run command (`docker compose run --rm -e RAILS_ENV=test web bundle exec rspec spec/requests/`) - [x] PR body references parent issue (`Closes #243`) - [x] PR body notes dependency on merged PR #241 ### PROCESS OBSERVATIONS - **Deployment risk:** Low. Controller changes add early-return guards before existing logic -- no change to happy-path behavior. Security doc is additive documentation only. - **Change failure risk:** Low. All new code paths return 422 on invalid input, which is the correct HTTP semantics for "valid request format, invalid data." No database migrations. - **Documentation impact:** The `docs/security.md` file should be linked from the main `CLAUDE.md` docs table. Currently the table of contents in `CLAUDE.md` does not reference it. This can be a follow-up. - **Coverage completeness:** This PR closes a real gap discovered during PR #241 review -- foreign key violations causing 500s in production. The fix is proportional to the problem. ### VERDICT: APPROVED

ldraney commented 2026-06-17 04:29:23 +00:00

Author

Owner

Copy link

PR #244 Review

Title: Harden input validation and add security documentation
Parent Issue: #243
Scope: 10 files changed, +381 / -3

---

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, Hotwire/Turbo Streams, RSpec request specs, Mermaid diagrams in documentation.

Controller Changes (Input Validation)

The shared find_property_or_reject helper in ApplicationController is well-designed:

• It uses find_by (returns nil) rather than find (raises), which is the correct pattern for validating a foreign key vs. finding the primary resource.
• It is opt-in (explicit call), not a global before_action, avoiding unintended side effects on controllers that do not need it.
• The naming is slightly misleading -- find_property_or_reject does not reject; it only looks up the property. The rejection logic lives in each calling controller. A more accurate name would be find_property or find_property_by_id. This is a nit, not a blocker.

All 4 identified user-input paths for property_id are now guarded:

• DaysController#add_to_queue -- returns 422 with flash (Turbo Stream) or redirect (HTML)
• WorkQueueItemsController#create (non-client_name path) -- returns 422 with flash or redirect
• WorkQueueItemsController#mark_other -- returns 422 with flash or redirect
• WeeksController#toggle_assign -- returns 422 via head(:unprocessable_entity)

The error handling in WeeksController#toggle_assign is correctly simpler (just head :unprocessable_entity) because that endpoint is JSON-only. The three Turbo Stream endpoints all render flash messages, which is consistent with the existing error patterns in those controllers.

DRY Observation (non-blocking)

The respond_to block for invalid property_id is repeated three times across DaysController#add_to_queue, WorkQueueItemsController#create, and WorkQueueItemsController#mark_other. The pattern is identical each time:

respond_to do |format|
  format.turbo_stream do
    render turbo_stream: turbo_stream.append("flash-messages",
      partial: "shared/flash", locals: { message: "..." }), status: :unprocessable_entity
  end
  format.html { redirect_to ..., alert: "..." }
end

This is the same pattern already used for other error responses in these controllers (e.g., duplicate queue item, save failure). Extracting it into a shared render_validation_error(message, redirect_path:) helper would reduce repetition, but since this pattern is already established across the codebase (not introduced by this PR), this is not a blocker. It is a future improvement opportunity.

Test Coverage

Strong coverage. The PR adds:

• Invalid property_id specs for all 4 guarded actions (Turbo Stream and HTML formats tested for days and work_queue_items; JSON-only for weeks)
• Unauthenticated access specs for 5 controllers (days, properties, uploads, weeks, work_queue_items), completing coverage for all 8 protected controllers (crew, profile, join_crew already had coverage per the PR body)
• A negative test (does not reject when client_name is provided) that confirms the create path still works when creating a property inline -- this is an important regression guard

One observation: the days_spec.rb invalid property_id test for HTML format asserts redirect_to(day_path(date: tuesday)) but does not verify the flash alert message. This is minor since the Turbo Stream test already validates the message text.

Security Documentation

The docs/security.md is thorough and reflects the actual posture rather than aspirational goals. Specific strengths:

• Documents the "Known Gaps" section honestly (no rate limiting, no request body size limits, no explicit parameter allowlisting)
• Correctly explains why PropertiesController does NOT need the find_property_or_reject guard (it uses Property.find for the primary resource, which raises RecordNotFound / 404)
• Mermaid diagrams for auth flow, role hierarchy, and attack surface are well-structured
• Secrets management table is complete and references the actual provisioning tools (Terraform, Salt, CNPG operator)
• Controller role map is accurate based on my reading of the existing controller files

Minor doc nit: The "Controller Role Map" references DaysController as requiring "admin, super_admin" roles, which matches the require_role :admin, :super_admin on line 2 of days_controller.rb. Verified correct.

---

BLOCKERS

None identified.

• No unvalidated user input: All 4 property_id input paths are now guarded.
• No secrets committed: No credentials, API keys, or .env files in the diff.
• Test coverage present: All new validation paths have corresponding specs.
• No DRY violation in auth/security paths: The find_property_or_reject helper is centralized; the respond_to blocks are duplicated but follow the existing pattern and are not auth logic.

---

NITS

1. find_property_or_reject naming: The method only finds, it does not reject. Consider find_property for accuracy. The "or_reject" suffix implies the method itself handles the rejection, but callers must do that. Low priority.

2. HTML format test for days invalid property_id: The test asserts redirect but does not verify the alert: "Property not found." flash. Consider adding follow_redirect! and checking for the message text, or asserting expect(flash[:alert]).to eq("Property not found.").

3. Magic number 999999: Used in all invalid property_id specs. Consider extracting to a shared let(:nonexistent_id) { 999999 } or using a comment. Very minor.

4. Security doc line length: Some lines in the Mermaid diagrams are long. Not a functional issue but may render oddly in narrow viewports.

---

SOP COMPLIANCE

• PR body has: Summary, Changes, Test Plan, Related -- all present and detailed
• Review Checklist included and populated
• No secrets, .env files, or credentials committed
• No unnecessary file changes (all 10 files are directly related to the issue scope)
• Commit message is descriptive (PR title matches issue title)
• Tests pass (PR body claims 276 specs, 0 failures)

---

PROCESS OBSERVATIONS

• Change failure risk: Low. The validation guards only add early-return paths for invalid input; they do not modify the happy path. The find_by call adds one SELECT query per guarded action, but only when the action is invoked -- no impact on index/show performance.
• Documentation quality: The docs/security.md file is a meaningful operational artifact. It should be added to the docs table of contents in CLAUDE.md.
• Deployment frequency impact: None. This is a hardening PR with no migrations or infrastructure changes.
• Origin: Discovered during QA review of PR #241, which is a healthy pattern -- QA review surfacing follow-up work items.

---

VERDICT: APPROVED

## PR #244 Review **Title:** Harden input validation and add security documentation **Parent Issue:** #243 **Scope:** 10 files changed, +381 / -3 --- ### DOMAIN REVIEW **Tech stack:** Ruby on Rails 8.1, Hotwire/Turbo Streams, RSpec request specs, Mermaid diagrams in documentation. **Controller Changes (Input Validation)** The shared `find_property_or_reject` helper in `ApplicationController` is well-designed: - It uses `find_by` (returns nil) rather than `find` (raises), which is the correct pattern for validating a foreign key vs. finding the primary resource. - It is opt-in (explicit call), not a global `before_action`, avoiding unintended side effects on controllers that do not need it. - The naming is slightly misleading -- `find_property_or_reject` does not reject; it only looks up the property. The rejection logic lives in each calling controller. A more accurate name would be `find_property` or `find_property_by_id`. This is a nit, not a blocker. All 4 identified user-input paths for `property_id` are now guarded: - `DaysController#add_to_queue` -- returns 422 with flash (Turbo Stream) or redirect (HTML) - `WorkQueueItemsController#create` (non-client_name path) -- returns 422 with flash or redirect - `WorkQueueItemsController#mark_other` -- returns 422 with flash or redirect - `WeeksController#toggle_assign` -- returns 422 via `head(:unprocessable_entity)` The error handling in `WeeksController#toggle_assign` is correctly simpler (just `head :unprocessable_entity`) because that endpoint is JSON-only. The three Turbo Stream endpoints all render flash messages, which is consistent with the existing error patterns in those controllers. **DRY Observation (non-blocking)** The respond_to block for invalid property_id is repeated three times across `DaysController#add_to_queue`, `WorkQueueItemsController#create`, and `WorkQueueItemsController#mark_other`. The pattern is identical each time: ```ruby respond_to do |format| format.turbo_stream do render turbo_stream: turbo_stream.append("flash-messages", partial: "shared/flash", locals: { message: "..." }), status: :unprocessable_entity end format.html { redirect_to ..., alert: "..." } end ``` This is the same pattern already used for other error responses in these controllers (e.g., duplicate queue item, save failure). Extracting it into a shared `render_validation_error(message, redirect_path:)` helper would reduce repetition, but since this pattern is already established across the codebase (not introduced by this PR), this is not a blocker. It is a future improvement opportunity. **Test Coverage** Strong coverage. The PR adds: - Invalid `property_id` specs for all 4 guarded actions (Turbo Stream and HTML formats tested for days and work_queue_items; JSON-only for weeks) - Unauthenticated access specs for 5 controllers (days, properties, uploads, weeks, work_queue_items), completing coverage for all 8 protected controllers (crew, profile, join_crew already had coverage per the PR body) - A negative test (`does not reject when client_name is provided`) that confirms the `create` path still works when creating a property inline -- this is an important regression guard One observation: the `days_spec.rb` invalid property_id test for HTML format asserts `redirect_to(day_path(date: tuesday))` but does not verify the flash alert message. This is minor since the Turbo Stream test already validates the message text. **Security Documentation** The `docs/security.md` is thorough and reflects the actual posture rather than aspirational goals. Specific strengths: - Documents the "Known Gaps" section honestly (no rate limiting, no request body size limits, no explicit parameter allowlisting) - Correctly explains why `PropertiesController` does NOT need the `find_property_or_reject` guard (it uses `Property.find` for the primary resource, which raises RecordNotFound / 404) - Mermaid diagrams for auth flow, role hierarchy, and attack surface are well-structured - Secrets management table is complete and references the actual provisioning tools (Terraform, Salt, CNPG operator) - Controller role map is accurate based on my reading of the existing controller files Minor doc nit: The "Controller Role Map" references `DaysController` as requiring "admin, super_admin" roles, which matches the `require_role :admin, :super_admin` on line 2 of `days_controller.rb`. Verified correct. --- ### BLOCKERS None identified. - **No unvalidated user input**: All 4 `property_id` input paths are now guarded. - **No secrets committed**: No credentials, API keys, or `.env` files in the diff. - **Test coverage present**: All new validation paths have corresponding specs. - **No DRY violation in auth/security paths**: The `find_property_or_reject` helper is centralized; the respond_to blocks are duplicated but follow the existing pattern and are not auth logic. --- ### NITS 1. **`find_property_or_reject` naming**: The method only finds, it does not reject. Consider `find_property` for accuracy. The "or_reject" suffix implies the method itself handles the rejection, but callers must do that. Low priority. 2. **HTML format test for days invalid property_id**: The test asserts redirect but does not verify the `alert: "Property not found."` flash. Consider adding `follow_redirect!` and checking for the message text, or asserting `expect(flash[:alert]).to eq("Property not found.")`. 3. **Magic number 999999**: Used in all invalid property_id specs. Consider extracting to a shared `let(:nonexistent_id) { 999999 }` or using a comment. Very minor. 4. **Security doc line length**: Some lines in the Mermaid diagrams are long. Not a functional issue but may render oddly in narrow viewports. --- ### SOP COMPLIANCE - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and detailed - [x] Review Checklist included and populated - [x] No secrets, `.env` files, or credentials committed - [x] No unnecessary file changes (all 10 files are directly related to the issue scope) - [x] Commit message is descriptive (PR title matches issue title) - [x] Tests pass (PR body claims 276 specs, 0 failures) --- ### PROCESS OBSERVATIONS - **Change failure risk**: Low. The validation guards only add early-return paths for invalid input; they do not modify the happy path. The `find_by` call adds one SELECT query per guarded action, but only when the action is invoked -- no impact on index/show performance. - **Documentation quality**: The `docs/security.md` file is a meaningful operational artifact. It should be added to the docs table of contents in `CLAUDE.md`. - **Deployment frequency impact**: None. This is a hardening PR with no migrations or infrastructure changes. - **Origin**: Discovered during QA review of PR #241, which is a healthy pattern -- QA review surfacing follow-up work items. --- ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-17 07:30:27 +00:00

Merge branch 'main' into 243-harden-input-validation-and-add-security e30186fadf

ldraney merged commit 263b7e7a4b into main 2026-06-17 07:32:12 +00:00

ldraney deleted branch 243-harden-input-validation-and-add-security 2026-06-17 07:32:12 +00:00

ldraney referenced this pull request from a commit 2026-06-17 07:32:14 +00:00

Harden input validation and add security documentation (#244)

Sign in to join this conversation.

Reviewers

No reviewers

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!244

No description provided.