feat(session): serve session expiry from VIPER 2, not the legacy CFM - #304
Open
rlorenzo wants to merge 1 commit into
Open
feat(session): serve session expiry from VIPER 2, not the legacy CFM#304rlorenzo wants to merge 1 commit into
rlorenzo wants to merge 1 commit into
Conversation
rlorenzo
force-pushed
the
feature/session-timeout-endpoint
branch
from
August 7, 2026 19:17
95891e8 to
b893056
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
📝 WalkthroughWalkthroughThe change adds a read-only session-timeout API, exposes its status model, and updates the session-timeout view to poll the API and format expiry times. Reflection tests verify that the controller does not extend sessions through inherited bases or filters. ChangesSession timeout API and display
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/Controllers/SessionTimeoutController.cs`:
- Around line 66-70: Replace the filtered generic catch in the session-timeout
handling with separate catch blocks for SqlException and
InvalidOperationException. In both blocks, preserve the existing Logger.Error
call and Status fallback response.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a02c865c-16f7-4431-9a5e-4f20891d1ebd
📒 Files selected for processing (6)
test/Controllers/SessionTimeoutControllerTests.csweb/Classes/SessionTimeoutStatus.csweb/Classes/Utilities/SessionTimeoutService.csweb/Controllers/SessionTimeoutController.csweb/Views/Shared/Components/SessionTimeout/Default.cshtmlweb/Views/Shared/Components/SessionTimeout/SessionTimeout.cs
rlorenzo
force-pushed
the
feature/session-timeout-endpoint
branch
from
August 7, 2026 21:29
b893056 to
d056d7b
Compare
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## fix/session-refresh-pathbase #304 +/- ##
================================================================
- Coverage 41.88% 41.85% -0.03%
================================================================
Files 986 987 +1
Lines 49488 49518 +30
Branches 5843 5843
================================================================
Hits 20726 20726
- Misses 27852 27882 +30
Partials 910 910
Flags with carried forward coverage won't be shown. Click here to find out more.
|
rlorenzo
force-pushed
the
feature/session-timeout-endpoint
branch
from
August 7, 2026 23:47
d056d7b to
6e97020
Compare
Copilot stopped reviewing on behalf of
rlorenzo due to an error
August 8, 2026 02:17
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Replaces the legacy ColdFusion session-timeout polling endpoint with a first-party ASP.NET Core API, and updates the session-timeout UI to use it while avoiding polling for anonymous visitors.
Changes:
- Add
/api/sessionTimeoutcontroller returning session timeout status derived from the auth cookie and DB record. - Update the session timeout component and client script to poll the new API and improve warning reset/formatting.
- Remove the old
SessionTimeoutCheckmodel and add a small test to prevent inheriting session-extending controller bases/filters.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| web/Views/Shared/Components/SessionTimeout/SessionTimeout.cs | Stops rendering the timeout UI for anonymous users to avoid false “expired” dialogs. |
| web/Views/Shared/Components/SessionTimeout/Default.cshtml | Switches polling to the new API, adds reset behavior, and centralizes time formatting. |
| web/Models/SessionTimeoutCheck.cs | Removes legacy model no longer used by the new polling contract. |
| web/Controllers/SessionTimeoutController.cs | Introduces the new read-only polling endpoint and ensures no-store caching. |
| web/Classes/Utilities/SessionTimeoutService.cs | Exposes timeout duration for reuse by the new controller. |
| web/Classes/SessionTimeoutStatus.cs | Adds the JSON contract returned to the session-timeout dialog. |
| test/Controllers/SessionTimeoutControllerTests.cs | Adds regression tests to ensure the polling controller doesn’t extend sessions via base types/filters. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
rlorenzo
force-pushed
the
feature/session-timeout-endpoint
branch
3 times, most recently
from
August 8, 2026 17:35
633ad99 to
3ffa268
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/Controllers/SessionTimeoutControllerTests.cs`:
- Around line 14-31: Add execution tests for
SessionTimeoutController.GetSessionTimeout covering a stored expiry, an
authenticated user without a stored row, an anonymous user, and each fallback
exception path. Assert both SecondsUntilTimeout and the returned offset
timestamp contract, using the existing test setup and collaborators rather than
only reflection-based checks.
In `@web/Controllers/SessionTimeoutController.cs`:
- Line 52: Update SessionTimeoutController.GetSessionTimeout() to use a
read-only SessionTimeouts query with AsNoTracking(), filtering by LoginId and
Service, instead of calling SessionTimeoutService.GetSessionTimeout() and its
Find-based path.
In `@web/Views/Shared/Components/SessionTimeout/Default.cshtml`:
- Around line 47-51: Update the secondsUntilTimeout boundary in the session
timeout polling condition to use <= 300 instead of < 300, ensuring the warning
and 15-second pre-expiry polling path also run when the API returns exactly 300
seconds.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ad6355cf-63f9-459e-91c1-76b73e3f1971
📒 Files selected for processing (6)
.review-pr-ignored-304test/Controllers/SessionTimeoutControllerTests.csweb/Classes/Utilities/SessionTimeoutService.csweb/Controllers/SessionTimeoutController.csweb/Models/SessionTimeoutCheck.csweb/Views/Shared/Components/SessionTimeout/Default.cshtml
💤 Files with no reviewable changes (1)
- web/Models/SessionTimeoutCheck.cs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
web/Controllers/SessionTimeoutController.cs:56
secondsUntilTimeoutcan become negative if the storedSessionTimeoutDateTimeis already in the past (clock skew, delayed poll, stale DB row). Returning a negative value is surprising for the client contract and can lead to inconsistent UI behavior; clamp it to 0 on the server.
return Status(record.SessionTimeoutDateTime,
(int)(record.SessionTimeoutDateTime - DateTime.Now).TotalSeconds);
rlorenzo
force-pushed
the
feature/session-timeout-endpoint
branch
from
August 8, 2026 18:08
3ffa268 to
f959924
Compare
rlorenzo
force-pushed
the
feature/session-timeout-endpoint
branch
from
August 10, 2026 03:27
f959924 to
4c01b44
Compare
- Keep the endpoint read-only by deriving from ControllerBase, since ApiController and AreaController both write a fresh expiry on every action and would stop the session ever timing out - Read the user from the auth cookie, replacing the legacy endpoint's unauthenticated loginID query parameter - Tell an authenticated user with no row apart from a dead session, which legacy could not do and so reported as expired - Skip the poll for anonymous visitors, who would otherwise be told on public pages that a session they never had has expired - Stand the warning down when the session is extended elsewhere, and render midnight as 12 AM rather than 0 AM - Drop the ViewComponent's hardcoded localhost and double slash, now that the poll is same-origin - Delete the unused SessionTimeoutCheck model, which was added as this endpoint's DTO in 2023 and never wired up
rlorenzo
force-pushed
the
feature/session-timeout-endpoint
branch
from
August 10, 2026 05:56
4c01b44 to
f8e26f4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the cross-origin session-expiry poll to legacy VIPER 1 with a read-only VIPER 2 endpoint.
Why
seconds_until_timeout_v2.cfmtakesloginIDas an unauthenticated query string parameter, so anyone can read anyone's session expiry. The new endpoint takes no parameters and derives the user from the auth cookie.localhost:7157-7159purely to work in dev. Same-origin removes the need.The one thing to check carefully
The endpoint must never extend the session. Both controller base classes in this app write a fresh expiry on every action:
ApiControllervia[ApiSessionUpdateFilter](ApiSessionUpdateFilter.cs:13)AreaControllervia itsOnActionExecutionAsyncoverride (AreaController.cs:20)So
SessionTimeoutControllerdeliberately derives from plainControllerBase. If someone "tidies" it ontoApiControllerlater, polling would renew the session every 5 minutes and sessions would never expire. InheritingApiControllerwould also wrap the payload in theApiResponseenvelope and break the JSON contract the dialog reads.That invariant is now pinned by
test/Controllers/SessionTimeoutControllerTests.cs, which fails if the base class changes.Decisions
<cfcatch>, so a DB blip cannot strand a user behind a warning dialog they cannot dismiss.AreaController,ApiSessionUpdateFilterandRefreshSession, so a page served by a plainController(CMSController, for instance) leaves a perfectly valid session with no row at all. Legacy had no auth context, could not tell that apart from a dead session, and reported "expired" to a user who was fine. With the cookie we can: an authenticated user with no row gets a full window, an anonymous caller still gets 0. This weakens nothing, because the row drives only this advisory dialog and is not enforced server-side.The JSON contract (
sessionTimeoutDateTime,secondsUntilTimeout) is unchanged, so the client shape is untouched.Also here
datetimewritten fromDateTime.Now, so it is local wall-clock; without an offset a client in another timezone reads it as its own and displays the wrong expiry.no-storeon a per-user, time-sensitive GET.secondsUntilTimeoutclimbed back above 300 the poll skipped the whole branch and never clearedshowSessionTimeoutWarning, so a user who kept working in a second tab was left staring at a stale "your session will expire at ..." that would never go away.SessionTimeout.cscleanup: the hardcodedhttp://localhost/, the double slash, and theloginIdin the query string are all gone with the URL. It no longer needs to be async.Minor behavioural note
Moving the poll same-origin means it now reaches .NET carrying the auth cookie, and cookie auth uses sliding expiration (
ExpireTimeSpan12h atProgram.cs:146,SlidingExpirationleft unset so it defaults to true). The old cross-origin ColdFusion poll could not touch that cookie.The practical effect is small. Sliding expiration only reissues the cookie after 6h have elapsed, and polling stops once the session expires, so an abandoned tab goes quiet roughly 30 minutes after the last real activity and never survives to the renewal point. Keeping the poll running to 6h would require activity every ~29.5 minutes to keep the
SessionTimeoutrow alive, and that activity is itself same-origin .NET traffic already renewing the cookie. So the poll can extend cookie life by at most ~30 minutes beyond what ordinary navigation already does. Recording it for the record rather than flagging it as a blocker.Not done
The SPA (
VueApp/src/components/SessionTimeout.vue) still polls the legacy CFM with?id=<loginId>, so the CFM cannot be decommissioned yet.VueApp/was out of scope for this work. Migrating it is the natural follow-up and would close the unauthenticated-read hole for the SPA too.Verification
npm run lint -- --fix <changed files>npm run verify:buildnpm run test:backendNot verifiable locally: local dev has no PathBase, so the
/2behaviour of@Url.Content("~/api/sessionTimeout")needs a TEST deploy to confirm, as does the end-to-end poll against a real database.