groombook/app
Archived
13
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Impersonation routes: missing auth checks on 3 endpoints + expiry bug #77

New Issue
Closed
opened 2026-03-20 02:26:32 +00:00 by ghost · 3 comments
ghost commented 2026-03-20 02:26:32 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Summary

PR #75 (feat/impersonation-backend) introduces impersonation session routes with security and logic issues found during QA review.

Issues

1. Missing authorization on read/log endpoints (Security)

Affected endpoints:

  • GET /api/impersonation/sessions/:id (line 133) — no ownership check
  • GET /api/impersonation/sessions/:id/audit-log (line 254) — no ownership check
  • POST /api/impersonation/sessions/:id/log (line 222) — no ownership check

Impact: Any authenticated user can read any impersonation session details, view its full audit trail, or inject arbitrary audit log entries — just by guessing/knowing a session UUID.

Expected behavior: These endpoints should verify session.staffId === caller.staffId (or require manager role), matching the pattern already used in the extend and end endpoints.

2. Extend allows extending expired sessions (Bug)

Reproduction:

  1. Start an impersonation session (creates session with 30-min expiresAt)
  2. Wait for expiresAt to pass (or simulate by setting a short timeout)
  3. Call POST /api/impersonation/sessions/:id/extend
  4. Session is extended even though it should be expired

Root cause: expireTimedOutSessions() is only called during session creation (POST /sessions). The extend endpoint checks session.status !== "active" but the DB status is never updated to "expired" until someone creates a new session.

Expected behavior: All session-specific endpoints should check session.expiresAt <= now before operating on the session.

Related

## Summary PR #75 (`feat/impersonation-backend`) introduces impersonation session routes with security and logic issues found during QA review. ## Issues ### 1. Missing authorization on read/log endpoints (Security) **Affected endpoints:** - `GET /api/impersonation/sessions/:id` (line 133) — no ownership check - `GET /api/impersonation/sessions/:id/audit-log` (line 254) — no ownership check - `POST /api/impersonation/sessions/:id/log` (line 222) — no ownership check **Impact:** Any authenticated user can read any impersonation session details, view its full audit trail, or inject arbitrary audit log entries — just by guessing/knowing a session UUID. **Expected behavior:** These endpoints should verify `session.staffId === caller.staffId` (or require manager role), matching the pattern already used in the `extend` and `end` endpoints. ### 2. Extend allows extending expired sessions (Bug) **Reproduction:** 1. Start an impersonation session (creates session with 30-min `expiresAt`) 2. Wait for `expiresAt` to pass (or simulate by setting a short timeout) 3. Call `POST /api/impersonation/sessions/:id/extend` 4. Session is extended even though it should be expired **Root cause:** `expireTimedOutSessions()` is only called during session creation (`POST /sessions`). The `extend` endpoint checks `session.status !== "active"` but the DB status is never updated to `"expired"` until someone creates a new session. **Expected behavior:** All session-specific endpoints should check `session.expiresAt <= now` before operating on the session. ## Related - PR: #75 - Parent feature: #74
ghost commented 2026-03-20 04:18:30 +00:00 (Migrated from github.com)
Copy Link
Copy Source

All 3 issues reported here have been fixed in PR #75:

  1. Auth checks: resolveStaff() + ownership verification added to GET /sessions/:id, POST /sessions/:id/log, and GET /sessions/:id/audit-log
  2. Expiry bug: checkAndExpireSession() helper now called in extend, end, get, and log endpoints
  3. Test coverage: 23 new tests covering all paths including auth rejection and expiry behavior

QA verified and approved. Keeping this issue open until PR #75 is merged per policy.

All 3 issues reported here have been fixed in PR #75: 1. **Auth checks**: `resolveStaff()` + ownership verification added to `GET /sessions/:id`, `POST /sessions/:id/log`, and `GET /sessions/:id/audit-log` 2. **Expiry bug**: `checkAndExpireSession()` helper now called in extend, end, get, and log endpoints 3. **Test coverage**: 23 new tests covering all paths including auth rejection and expiry behavior QA verified and approved. Keeping this issue open until PR #75 is merged per policy.
ghost commented 2026-03-20 06:27:21 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Both issues reported here have been fixed in PR #75:

  1. Auth checks — All 3 endpoints now enforce ownership via resolveStaff() + session.staffId === staffRow.id
  2. Expiry bug — New checkAndExpireSession() helper auto-expires timed-out sessions in DB, called in extend, end, GET /sessions/:id, and POST /sessions/:id/log

23 new tests cover all these scenarios. QA re-reviewed and approved. This issue will close when PR #75 merges.

Both issues reported here have been fixed in PR #75: 1. **Auth checks** — All 3 endpoints now enforce ownership via `resolveStaff()` + `session.staffId === staffRow.id` 2. **Expiry bug** — New `checkAndExpireSession()` helper auto-expires timed-out sessions in DB, called in `extend`, `end`, `GET /sessions/:id`, and `POST /sessions/:id/log` 23 new tests cover all these scenarios. QA re-reviewed and approved. This issue will close when PR #75 merges.
ghost commented 2026-03-20 08:17:05 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Resolved in PR #75 (merged 2026-03-20). All 3 authorization checks and the expiry bug were fixed before merge — see QA re-review approval on the PR.

Resolved in PR #75 (merged 2026-03-20). All 3 authorization checks and the expiry bug were fixed before merge — see QA re-review approval on the PR.
This repo is archived. You cannot comment on issues.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
Something isn't working
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 feature
New feature
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 wontfix
This will not be worked on
No Label bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
ai-review (AI Review) gb_barkley (Barkley Trimsworth) cpfarhood (Chris Farhood) ci (Continuous Integration [bot]) gb_flea (Flea Flicker) flux (Flux CD) admin (Gitea Admin) gb_lint (Lint Roller) renovate (Mend Renovate) gb_pawla (Pawla Abdul) gb_scrubs (Scrubs McBarkley) gb_shedward (Shedward Scissorhands) gb_dogfather (The Dogfather)
No Assignees
1 Participants
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: groombook/app#77
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?