Feature/login plugin#162
Open
kappsegla wants to merge 23 commits into
Open
Conversation
…ugin and GatewayRouter. Updated Bootstrap to integrate BasicAuth and adjusted RouterConfig for login endpoint.
… handling. Update BasicAuthPlugin to support dynamic realm rotation.
…ionAuthFilter`, `SessionStore`, and related plugins (`LogoutPlugin`, `SessionStatusPlugin`). Update tests for new session handling implementation.
…ionAuthFilter`, `SessionStore`, and related plugins (`LogoutPlugin`, `SessionStatusPlugin`). Update tests for new session handling implementation.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds server-side sessions, a global authentication filter, login/logout/status endpoints, route registration, frontend logout visibility, credential handling, and tests. ChangesSession Authentication Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant SessionAuthFilter
participant LoginPlugin
participant SessionStore
participant LogoutPlugin
Browser->>SessionAuthFilter: request protected path
SessionAuthFilter->>SessionStore: get(sid)
SessionStore-->>SessionAuthFilter: no session
SessionAuthFilter-->>Browser: 302 to /login
Browser->>LoginPlugin: POST credentials
LoginPlugin->>SessionStore: create(username)
SessionStore-->>LoginPlugin: Session
LoginPlugin-->>Browser: 302 with SID cookie
Browser->>SessionAuthFilter: request with SID cookie
SessionAuthFilter->>SessionStore: get(sid)
SessionStore-->>SessionAuthFilter: Session
SessionAuthFilter-->>Browser: continue request
Browser->>LogoutPlugin: GET or POST /logout
LogoutPlugin->>SessionStore: invalidate(sid)
LogoutPlugin-->>Browser: 302 with cleared cookie
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/Users (1)
1-2: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReplace the committed default login credentials.
LoginPluginloadsconfig/Usersby default, solunk:lunkbecomes a real credential unless every deployment overrides the path. If this is only a sample fixture, keep it out of the runtime fallback or replace it with clearly non-production data.🤖 Prompt for 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. In `@config/Users` around lines 1 - 2, Replace the committed default credentials in config/Users so LoginPlugin does not ship with a real fallback login. Update the default data used by LoginPlugin’s config loading path to remove lunk:lunk, and if this file is only a sample fixture, keep it out of the runtime fallback or replace it with clearly non-production placeholder credentials.
🧹 Nitpick comments (4)
src/main/java/org/juv25d/auth/SessionStore.java (1)
31-38: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCleanup runs synchronously on every session creation.
cleanupExpired()performs a full scan ofsessionson everycreate()call, so login latency grows with the number of stored sessions. Consider decoupling expiry cleanup from the hot path (e.g., a scheduled background sweep or a counter-triggered periodic sweep) instead of running it on every login.🤖 Prompt for 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. In `@src/main/java/org/juv25d/auth/SessionStore.java` around lines 31 - 38, SessionStore.create currently calls cleanupExpired() on every new session, putting a full sessions scan on the login hot path. Move the expiry sweep out of create by making cleanupExpired run on a background schedule or only after a periodic trigger/counter threshold, and keep create focused on creating and storing the Session. Use the SessionStore and cleanupExpired symbols to locate the change.src/test/java/org/juv25d/filter/SessionAuthFilterTest.java (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest leaves session in the shared singleton store.
SessionStoreis a process-wide singleton; the session created inauthenticatedRequest_passesThrough_andSetsHeadersis never invalidated, so it persists across tests/test classes sharing the JVM. Consider invalidating created sessions in@AfterEach.♻️ Proposed cleanup
+ `@AfterEach` + void tearDown() { + // invalidate any sessions created during the test + }Also applies to: 61-79
🤖 Prompt for 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. In `@src/test/java/org/juv25d/filter/SessionAuthFilterTest.java` around lines 22 - 27, The SessionAuthFilterTest setup leaves state behind in the process-wide SessionStore singleton, so sessions created by authenticatedRequest_passesThrough_andSetsHeaders can leak into later tests. Update SessionAuthFilterTest to track any created session IDs and invalidate them in an `@AfterEach` cleanup method, alongside any other test-created sessions, while keeping the existing setup() reset of SessionStore intact.src/test/java/org/juv25d/plugin/LoginPluginTest.java (1)
43-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding edge-case test coverage.
Current tests cover GET render, valid login, and invalid credentials. Missing: empty username/password (line 41-44 branch in LoginPlugin), unsupported HTTP method (405 branch), and missing/incorrect Content-Type on POST.
🤖 Prompt for 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. In `@src/test/java/org/juv25d/plugin/LoginPluginTest.java` around lines 43 - 101, Add edge-case coverage for LoginPlugin.handle by testing the branches not currently exercised: empty username/password input, unsupported HTTP methods that should return 405, and POST requests with missing or incorrect Content-Type. Extend LoginPluginTest with focused cases that call plugin.handle with these request variants and assert the expected status/body behavior so the login form and validation paths are fully covered.src/test/java/org/juv25d/plugin/LogoutPluginTest.java (1)
24-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a no-cookie/invalid-session logout test.
Only the happy path (valid session) is covered. Add a case where no
SIDcookie is sent (or it references an unknown session) to confirmhandle()still clears the cookie and redirects gracefully.🤖 Prompt for 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. In `@src/test/java/org/juv25d/plugin/LogoutPluginTest.java` around lines 24 - 53, Add a logout test that covers the no-cookie or invalid-session path in LogoutPluginTest, since only the valid-session flow is currently asserted. Create a request without an SID cookie, or with one that does not match any SessionStore entry, then call LogoutPlugin.handle and verify it still returns a 302 redirect to "/" and sets a clearing SID cookie with the expected attributes. Use the existing SessionStore, HttpRequest, HttpResponse, and LogoutPlugin symbols to keep the test aligned with the current happy-path setup.
🤖 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 `@src/main/java/org/juv25d/auth/SessionStore.java`:
- Around line 13-22: SessionStore is intended to be a singleton, but it is still
publicly instantiable because no constructor is declared. Add a private
constructor inside SessionStore so only getInstance() can create/access the
single shared instance, and keep the existing INSTANCE field as the sole source
of truth used by SessionAuthFilter, LoginPlugin, and LogoutPlugin.
In `@src/main/java/org/juv25d/Bootstrap.java`:
- Around line 33-39: Bootstrap currently relies on an unused RouterConfig DI
lookup to trigger route registration, which is easy to remove accidentally. In
the bootstrap flow that builds the Pipeline, replace the implicit
side-effect-only container.get(RouterConfig.class) with an explicit
initialization step on RouterConfig so the /login, /logout, /session/status, and
proxy routes are registered intentionally. Keep the base Router retrieval and
Pipeline construction tied to that explicit setup so the behavior remains
obvious and harder to break during refactors.
In `@src/main/java/org/juv25d/filter/SessionAuthFilter.java`:
- Around line 60-64: The authenticated response headers in SessionAuthFilter are
leaking user identity to clients, so remove the X-Authenticated and X-User
response header writes from the authenticated path and keep authentication state
exposed only through the existing /session/status flow. Update the doFilter
logic in SessionAuthFilter so it still allows the request through with
chain.doFilter(req, res) but no longer sets client-visible identity headers on
every authenticated response.
In `@src/main/java/org/juv25d/plugin/LoginPlugin.java`:
- Around line 46-51: The login check in LoginPlugin is comparing cleartext
passwords from loadUsers() directly against the submitted password, so update
the credential flow to store hashed passwords (for example via bcrypt or argon2)
and verify against the hash instead of using Objects.equals() on raw strings.
Keep the fix localized around loadUsers(), resolveUsersFilePath(), and the
username/password validation block so the login path performs a hash
verification with constant-time comparison for the final check.
- Around line 37-63: The LoginPlugin login handler accepts credentials without
any CSRF validation, so add a same-origin safeguard before creating the session.
Update the login flow in LoginPlugin to reject requests unless a valid CSRF
token is present or the request passes an Origin/Referer same-origin check, and
make sure the login form generated by renderLoginForm includes the token when
applicable. Apply the check in the credential-processing path that currently
uses parseForm, loadUsers, and SessionStore.create so the session cookie is only
issued for legitimate same-site submissions.
In `@src/main/java/org/juv25d/plugin/LogoutPlugin.java`:
- Around line 36-60: The readCookie logic in LogoutPlugin is duplicated across
session-related components, so move it into a shared utility such as CookieUtils
and have LogoutPlugin, SessionStatusPlugin, LoginPlugin, and SessionAuthFilter
call that common helper. Keep the same parsing and decoding behavior in the
extracted method, and update the UTF_8 reference to use the imported
StandardCharsets instead of the fully qualified name.
- Around line 18-34: The logout handler in LogoutPlugin.handle() currently
accepts any HTTP method, which allows CSRF via GET. Add an explicit method check
at the start of handle() so only POST is accepted, and reject other methods with
an appropriate non-success response before touching SessionStore or clearing the
SID cookie. Use the existing handle(HttpRequest, HttpResponse) entry point and
its req.method() access to enforce the restriction consistently with
LoginPlugin’s method-specific behavior.
---
Outside diff comments:
In `@config/Users`:
- Around line 1-2: Replace the committed default credentials in config/Users so
LoginPlugin does not ship with a real fallback login. Update the default data
used by LoginPlugin’s config loading path to remove lunk:lunk, and if this file
is only a sample fixture, keep it out of the runtime fallback or replace it with
clearly non-production placeholder credentials.
---
Nitpick comments:
In `@src/main/java/org/juv25d/auth/SessionStore.java`:
- Around line 31-38: SessionStore.create currently calls cleanupExpired() on
every new session, putting a full sessions scan on the login hot path. Move the
expiry sweep out of create by making cleanupExpired run on a background schedule
or only after a periodic trigger/counter threshold, and keep create focused on
creating and storing the Session. Use the SessionStore and cleanupExpired
symbols to locate the change.
In `@src/test/java/org/juv25d/filter/SessionAuthFilterTest.java`:
- Around line 22-27: The SessionAuthFilterTest setup leaves state behind in the
process-wide SessionStore singleton, so sessions created by
authenticatedRequest_passesThrough_andSetsHeaders can leak into later tests.
Update SessionAuthFilterTest to track any created session IDs and invalidate
them in an `@AfterEach` cleanup method, alongside any other test-created sessions,
while keeping the existing setup() reset of SessionStore intact.
In `@src/test/java/org/juv25d/plugin/LoginPluginTest.java`:
- Around line 43-101: Add edge-case coverage for LoginPlugin.handle by testing
the branches not currently exercised: empty username/password input, unsupported
HTTP methods that should return 405, and POST requests with missing or incorrect
Content-Type. Extend LoginPluginTest with focused cases that call plugin.handle
with these request variants and assert the expected status/body behavior so the
login form and validation paths are fully covered.
In `@src/test/java/org/juv25d/plugin/LogoutPluginTest.java`:
- Around line 24-53: Add a logout test that covers the no-cookie or
invalid-session path in LogoutPluginTest, since only the valid-session flow is
currently asserted. Create a request without an SID cookie, or with one that
does not match any SessionStore entry, then call LogoutPlugin.handle and verify
it still returns a 302 redirect to "/" and sets a clearing SID cookie with the
expected attributes. Use the existing SessionStore, HttpRequest, HttpResponse,
and LogoutPlugin symbols to keep the test aligned with the current happy-path
setup.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38a84309-4ed6-4b87-9d34-9c182997f2ac
📒 Files selected for processing (14)
config/Usersdocs/notes/pipeline-usage.mdsrc/main/java/org/juv25d/Bootstrap.javasrc/main/java/org/juv25d/auth/Session.javasrc/main/java/org/juv25d/auth/SessionStore.javasrc/main/java/org/juv25d/filter/SessionAuthFilter.javasrc/main/java/org/juv25d/plugin/LoginPlugin.javasrc/main/java/org/juv25d/plugin/LogoutPlugin.javasrc/main/java/org/juv25d/plugin/SessionStatusPlugin.javasrc/main/java/org/juv25d/router/RouterConfig.javasrc/main/resources/static/index.htmlsrc/test/java/org/juv25d/filter/SessionAuthFilterTest.javasrc/test/java/org/juv25d/plugin/LoginPluginTest.javasrc/test/java/org/juv25d/plugin/LogoutPluginTest.java
…iguration. Update `Bootstrap` to initialize routes using `RouterConfig`.
…date form rendering and tests for CSRF token validation.
…invalid methods. Update test to reflect change.
…fy absence of headers.
…Password` method for authentication and `hashPassword` utility for administrators. Update tests to reflect hashed password handling.
…. Refactor `SessionAuthFilter`, `LoginPlugin`, `SessionStatusPlugin`, and `LogoutPlugin` to use the new utility.
…. Refactor `SessionAuthFilter`, `LoginPlugin`, `SessionStatusPlugin`, and `LogoutPlugin` to use the new utility.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/juv25d/plugin/LoginPluginTest.java (1)
21-45: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore the previous system-property values.
setup()clears any existingusers.*properties, andcleanup()clears them again, permanently changing JVM-global state for other tests. Save the original values and restore them in@AfterEachto avoid order-dependent failures.Proposed fix
+ private String previousUsersFile; + private String previousUsersDir; + private String previousUsersFilename; + `@BeforeEach` void setup() throws IOException { + previousUsersFile = System.getProperty("users.file"); + previousUsersDir = System.getProperty("users.dir"); + previousUsersFilename = System.getProperty("users.filename"); System.clearProperty("users.file"); System.clearProperty("users.dir"); System.clearProperty("users.filename"); // ... } `@AfterEach` void cleanup() throws IOException { - System.clearProperty("users.file"); - System.clearProperty("users.dir"); - System.clearProperty("users.filename"); + restoreProperty("users.file", previousUsersFile); + restoreProperty("users.dir", previousUsersDir); + restoreProperty("users.filename", previousUsersFilename); // ... }🤖 Prompt for 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. In `@src/test/java/org/juv25d/plugin/LoginPluginTest.java` around lines 21 - 45, Update LoginPluginTest setup() and cleanup() to capture the original values of users.file, users.dir, and users.filename before clearing them, then restore those saved values in cleanup() instead of always clearing the properties. Preserve null values by removing properties that were originally unset, and continue deleting tempUsersFile as before.
🧹 Nitpick comments (1)
src/test/java/org/juv25d/plugin/LoginPluginTest.java (1)
68-115: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd negative CSRF and same-origin tests.
These tests only exercise valid CSRF tokens. Add cases for missing/mismatched CSRF tokens and cross-origin requests, asserting rejection and that no
SIDcookie is issued.🤖 Prompt for 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. In `@src/test/java/org/juv25d/plugin/LoginPluginTest.java` around lines 68 - 115, Add negative tests in LoginPluginTest alongside the existing postLogin tests for a missing CSRF token, a mismatched CSRF token, and a cross-origin request. Invoke LoginPlugin.handle for each case and assert the request is rejected, the response is not a successful login redirect, and the Set-Cookie header does not contain a SID cookie.
🤖 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 `@src/main/java/org/juv25d/plugin/LoginPlugin.java`:
- Around line 76-77: Update the password assignment in the login flow to
preserve the exact form value, removing the trim operation while retaining the
existing empty default. Keep username normalization unchanged and ensure the
unmodified password is passed to verification and hashPassword consistently.
- Around line 278-286: Update LoginPlugin.hashPassword to use a substantially
higher PBKDF2-HMAC-SHA256 iteration count and add a version identifier to the
returned hash format. Preserve the salt and derived-hash encoding, and ensure
the format clearly distinguishes new hashes so existing Users entries can be
migrated.
- Around line 218-229: Update isSameOrigin to parse the Origin and Referer
values as URIs, then compare their normalized host and port exactly against the
request Host authority. Remove the contains-based matching and ensure a missing
Host or invalid URI fails closed rather than matching every value.
---
Outside diff comments:
In `@src/test/java/org/juv25d/plugin/LoginPluginTest.java`:
- Around line 21-45: Update LoginPluginTest setup() and cleanup() to capture the
original values of users.file, users.dir, and users.filename before clearing
them, then restore those saved values in cleanup() instead of always clearing
the properties. Preserve null values by removing properties that were originally
unset, and continue deleting tempUsersFile as before.
---
Nitpick comments:
In `@src/test/java/org/juv25d/plugin/LoginPluginTest.java`:
- Around line 68-115: Add negative tests in LoginPluginTest alongside the
existing postLogin tests for a missing CSRF token, a mismatched CSRF token, and
a cross-origin request. Invoke LoginPlugin.handle for each case and assert the
request is rejected, the response is not a successful login redirect, and the
Set-Cookie header does not contain a SID cookie.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5be5072d-e362-45ad-8c47-029554c04f1a
📒 Files selected for processing (11)
src/main/java/org/juv25d/Bootstrap.javasrc/main/java/org/juv25d/auth/SessionStore.javasrc/main/java/org/juv25d/filter/SessionAuthFilter.javasrc/main/java/org/juv25d/plugin/LoginPlugin.javasrc/main/java/org/juv25d/plugin/LogoutPlugin.javasrc/main/java/org/juv25d/plugin/SessionStatusPlugin.javasrc/main/java/org/juv25d/router/RouterConfig.javasrc/main/java/org/juv25d/util/CookieUtils.javasrc/test/java/org/juv25d/filter/SessionAuthFilterTest.javasrc/test/java/org/juv25d/plugin/LoginPluginTest.javasrc/test/java/org/juv25d/plugin/LogoutPluginTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/juv25d/auth/SessionStore.java
- src/main/java/org/juv25d/router/RouterConfig.java
- src/test/java/org/juv25d/plugin/LogoutPluginTest.java
…t`, `Origin`, and `Referer` headers. Update tests to include `Host` and `Origin`.
…ks in `LoginPlugin`. Support case-insensitive username matching and add backwards compatibility to password verification. Upgrade PBKDF2 iteration count and introduce versioned password hashing format.
…LoginPlugin`, `LogoutPlugin`, and `SessionStatusPlugin`.
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/org/juv25d/plugin/LogoutPlugin.java (1)
19-33: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winLogout accepts GET → CSRF-via-GET.
Allowing GET lets a malicious page force-invalidate a victim's session with a simple
<img src="/logout">. Restrict logout to POST (or require the CSRF token) instead of accepting GET for convenience.🤖 Prompt for 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. In `@src/main/java/org/juv25d/plugin/LogoutPlugin.java` around lines 19 - 33, Update the method validation in LogoutPlugin to accept only POST requests, removing the GET allowance while preserving the existing 405 response for other methods. Keep the SID cookie lookup and SessionStore invalidation behavior unchanged for valid POST requests.src/main/java/org/juv25d/plugin/LoginPlugin.java (1)
50-58: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftCSRF protection is generated but never enforced.
A CSRF token is issued as a cookie and embedded as
_csrf, but the POST path never compares them, and the same-origin check was removed (Line 50). A cross-site page can auto-submit credentials and the 302 will set the victim'sSID, silently logging them into the attacker's account. Validate the submitted_csrfagainst theCSRF-TOKENcookie (constant-time) before creating the session, or reinstate anOrigin/Referersame-origin check that fails closed.🤖 Prompt for 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. In `@src/main/java/org/juv25d/plugin/LoginPlugin.java` around lines 50 - 58, Update the POST handling around parseForm in LoginPlugin to enforce CSRF protection before creating a session: retrieve the submitted _csrf value and CSRF-TOKEN cookie, compare them using a constant-time method, and reject the request when either is missing or mismatched. Alternatively, reinstate a same-origin Origin/Referer check that fails closed; ensure session creation and the login redirect occur only after validation succeeds.
🧹 Nitpick comments (1)
src/main/resources/static/index.html (1)
28-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrefer a POST form for logout.
This link invokes the state-changing logout handler through GET, allowing cross-site navigations or prefetches to invalidate a user’s session. Make logout POST-only with CSRF protection, or explicitly document this trade-off.
🤖 Prompt for 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. In `@src/main/resources/static/index.html` around lines 28 - 30, Update the nav-logout markup to submit logout through a POST form instead of the current GET link, and include the application’s established CSRF protection mechanism. Preserve the existing hidden styling and logout presentation while targeting the existing /logout handler.
🤖 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 `@config/Users`:
- Line 1: Replace the cleartext sample credential in config/Users at lines 1-1
with a PBKDF2 hash generated by LoginPlugin.hashPassword(...). In
src/main/java/org/juv25d/plugin/LoginPlugin.java at lines 229-236, remove the
plaintext fallback so verification accepts only pbkdf2:-formatted entries and
rejects cleartext passwords.
In `@src/main/java/org/juv25d/filter/SessionAuthFilter.java`:
- Around line 10-14: Clean up imports in SessionAuthFilter by removing the
unused URLDecoder, HashMap, and Map imports, retain StandardCharsets, and apply
the project’s Spotless formatting so import ordering and formatting pass CI.
---
Duplicate comments:
In `@src/main/java/org/juv25d/plugin/LoginPlugin.java`:
- Around line 50-58: Update the POST handling around parseForm in LoginPlugin to
enforce CSRF protection before creating a session: retrieve the submitted _csrf
value and CSRF-TOKEN cookie, compare them using a constant-time method, and
reject the request when either is missing or mismatched. Alternatively,
reinstate a same-origin Origin/Referer check that fails closed; ensure session
creation and the login redirect occur only after validation succeeds.
In `@src/main/java/org/juv25d/plugin/LogoutPlugin.java`:
- Around line 19-33: Update the method validation in LogoutPlugin to accept only
POST requests, removing the GET allowance while preserving the existing 405
response for other methods. Keep the SID cookie lookup and SessionStore
invalidation behavior unchanged for valid POST requests.
---
Nitpick comments:
In `@src/main/resources/static/index.html`:
- Around line 28-30: Update the nav-logout markup to submit logout through a
POST form instead of the current GET link, and include the application’s
established CSRF protection mechanism. Preserve the existing hidden styling and
logout presentation while targeting the existing /logout handler.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aec6d8cc-b187-400e-b045-3bd5a25e80b5
📒 Files selected for processing (15)
config/Usersdocs/notes/pipeline-usage.mdsrc/main/java/org/juv25d/Bootstrap.javasrc/main/java/org/juv25d/auth/Session.javasrc/main/java/org/juv25d/auth/SessionStore.javasrc/main/java/org/juv25d/filter/SessionAuthFilter.javasrc/main/java/org/juv25d/plugin/LoginPlugin.javasrc/main/java/org/juv25d/plugin/LogoutPlugin.javasrc/main/java/org/juv25d/plugin/SessionStatusPlugin.javasrc/main/java/org/juv25d/router/RouterConfig.javasrc/main/java/org/juv25d/util/CookieUtils.javasrc/main/resources/static/index.htmlsrc/test/java/org/juv25d/filter/SessionAuthFilterTest.javasrc/test/java/org/juv25d/plugin/LoginPluginTest.javasrc/test/java/org/juv25d/plugin/LogoutPluginTest.java
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/main/java/org/juv25d/plugin/LogoutPlugin.java (1)
18-28: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winLogout still reachable via GET (CSRF-via-GET).
This intentionally keeps GET enabled for logout, but that still means any cross-site page (
<img src="/logout">) can force-invalidate a victim's session without consent. This was previously flagged and remains open. If “logout via link” convenience is required, consider requiring POST only and using a JS-drivenfetch(..., {method:'POST'})from the nav link (as done for other authenticated actions) instead of a plain anchor tag.🤖 Prompt for 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. In `@src/main/java/org/juv25d/plugin/LogoutPlugin.java` around lines 18 - 28, Update LogoutPlugin.handle to accept POST only and reject GET requests with the existing 405 response path. Preserve the current response headers and body for unsupported methods, and move logout-link convenience to a client-side POST request rather than retaining GET access.src/main/java/org/juv25d/plugin/LoginPlugin.java (1)
33-58: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winCSRF protection is fully disabled despite the token scaffolding.
The CSRF token is generated, set as a cookie (line 36), and rendered into a hidden form field (line 129), but on POST the
_csrfvalue fromparseFormis never read or compared — the comments at lines 50 and 54 explicitly say the same-origin check and CSRF validation are disabled/removed. The previously-flaggedisSameOriginhelper no longer exists at all. This is a regression from an already-flagged critical issue: previously there was an imperfect same-origin check, now there is none, so any cross-site page can submit credentials to/loginand the resulting 302 will set the victim's session cookie (login CSRF / session fixation via forged submission).🔒 Suggested fix
Map<String, String> form = parseForm(req); - - // CSRF-validering är inaktiverad – värden används inte här. - + String csrfCookie = CookieUtils.readCookie(req, "CSRF-TOKEN"); + String csrfForm = form.getOrDefault("_csrf", ""); + if (csrfCookie == null || csrfForm.isEmpty() || !MessageDigest.isEqual( + csrfCookie.getBytes(StandardCharsets.UTF_8), csrfForm.getBytes(StandardCharsets.UTF_8))) { + res.setStatusCode(403); + res.setStatusText("Forbidden"); + renderLoginForm(res, "Invalid or missing CSRF token", generateCsrfToken()); + return; + } String username = form.getOrDefault("username", "").trim();Also applies to: 106-150, 204-208
🤖 Prompt for 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. In `@src/main/java/org/juv25d/plugin/LoginPlugin.java` around lines 33 - 58, Restore CSRF validation in LoginPlugin.handle: on POST, read the submitted _csrf value from parseForm and compare it with the CSRF token from the request cookie before processing credentials, rejecting missing or mismatched tokens with the existing bad-request/error response path. Ensure the GET-generated token remains available in the hidden form field and cookie, and remove the comments or flow that explicitly disable validation.config/Users (1)
1-1: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPlaintext credential handling remains unresolved across
config/UsersandLoginPlugin.java. The sample file still stores a cleartext password, and the verifier still accepts non-hashed entries; together they let plaintext credentials authenticate, bypassing the PBKDF2 hardening added elsewhere in this file.
config/Users#L1-L1: replacelunk:lunkwith a PBKDF2 hash produced byLoginPlugin.hashPassword(...).src/main/java/org/juv25d/plugin/LoginPlugin.java#L229-L236: remove the plaintext fallback branch so onlypbkdf2:-formatted entries verify, and reject cleartext.🤖 Prompt for 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. In `@config/Users` at line 1, Replace the cleartext credential in config/Users line 1 with a PBKDF2 hash generated by LoginPlugin.hashPassword(...). In src/main/java/org/juv25d/plugin/LoginPlugin.java lines 229-236, remove the plaintext verification fallback so only pbkdf2:-formatted entries are accepted and cleartext credentials are rejected.
🤖 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 `@src/main/java/org/juv25d/auth/SessionStore.java`:
- Around line 33-38: Move expired-session cleanup out of the create method’s
only trigger by scheduling cleanupExpired() through the application lifecycle,
using the existing SessionStore ownership boundaries. Ensure the scheduler is
started with the store/application and shut down cleanly, while preserving
immediate cleanup during create if desired. Add coverage proving sessions expire
and are removed without a subsequent login.
In `@src/main/java/org/juv25d/filter/SessionAuthFilter.java`:
- Around line 13-20: Correct the class Javadoc for the authentication filter:
remove the claim that valid sessions expose X-Authenticated and X-User headers,
and add /session/status to the documented public unauthenticated paths. Update
only the documentation near the filter declaration, preserving the
implementation behavior.
---
Duplicate comments:
In `@config/Users`:
- Line 1: Replace the cleartext credential in config/Users line 1 with a PBKDF2
hash generated by LoginPlugin.hashPassword(...). In
src/main/java/org/juv25d/plugin/LoginPlugin.java lines 229-236, remove the
plaintext verification fallback so only pbkdf2:-formatted entries are accepted
and cleartext credentials are rejected.
In `@src/main/java/org/juv25d/plugin/LoginPlugin.java`:
- Around line 33-58: Restore CSRF validation in LoginPlugin.handle: on POST,
read the submitted _csrf value from parseForm and compare it with the CSRF token
from the request cookie before processing credentials, rejecting missing or
mismatched tokens with the existing bad-request/error response path. Ensure the
GET-generated token remains available in the hidden form field and cookie, and
remove the comments or flow that explicitly disable validation.
In `@src/main/java/org/juv25d/plugin/LogoutPlugin.java`:
- Around line 18-28: Update LogoutPlugin.handle to accept POST only and reject
GET requests with the existing 405 response path. Preserve the current response
headers and body for unsupported methods, and move logout-link convenience to a
client-side POST request rather than retaining GET access.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c448ecf3-3b79-469e-ad98-067faa4be818
📒 Files selected for processing (15)
config/Usersdocs/notes/pipeline-usage.mdsrc/main/java/org/juv25d/Bootstrap.javasrc/main/java/org/juv25d/auth/Session.javasrc/main/java/org/juv25d/auth/SessionStore.javasrc/main/java/org/juv25d/filter/SessionAuthFilter.javasrc/main/java/org/juv25d/plugin/LoginPlugin.javasrc/main/java/org/juv25d/plugin/LogoutPlugin.javasrc/main/java/org/juv25d/plugin/SessionStatusPlugin.javasrc/main/java/org/juv25d/router/RouterConfig.javasrc/main/java/org/juv25d/util/CookieUtils.javasrc/main/resources/static/index.htmlsrc/test/java/org/juv25d/filter/SessionAuthFilterTest.javasrc/test/java/org/juv25d/plugin/LoginPluginTest.javasrc/test/java/org/juv25d/plugin/LogoutPluginTest.java
…n handling behavior.
… tests. Update application lifecycle to manage scheduler startup/shutdown.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/main/java/org/juv25d/auth/SessionStore.java`:
- Line 99: Update the session access and cleanup paths to serialize
expiry-check-plus-touch with expiry removal per session ID. In the method
handling session refresh (the get/touch flow), use the session map’s per-key
atomic operation so expiration validation and touch occur together; update
safeCleanup to use the same atomic per-key operation when removing expired
sessions, preserving existing behavior for missing or non-expired sessions.
In `@src/test/java/org/juv25d/auth/SessionStoreTest.java`:
- Around line 33-37: Update the cleanup test around store.get(id) to verify
scheduled eviction without triggering lazy eviction: poll the package-private
sessions membership directly until a deadline, asserting the entry is removed,
then optionally retain the get(id) null check. Use short polling intervals and a
bounded timeout to avoid a fixed sleep masking scheduler failures.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb411582-7023-436e-ade6-b417c8f33217
📒 Files selected for processing (5)
src/main/java/org/juv25d/App.javasrc/main/java/org/juv25d/auth/SessionStore.javasrc/main/java/org/juv25d/filter/SessionAuthFilter.javasrc/main/java/org/juv25d/plugin/LoginPlugin.javasrc/test/java/org/juv25d/auth/SessionStoreTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/juv25d/filter/SessionAuthFilter.java
- src/main/java/org/juv25d/plugin/LoginPlugin.java
…mproved thread safety.
… direct map access helper
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
2 participants
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.
Summary by CodeRabbit
SIDcookies, including background cleanup of idle sessions./login,/logout, and/session/statusendpoints./login(GET) or return401 Unauthorized(non-GET).SIDcookie.