fix: add limit to recovery session

[staaldraad]

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

A session would be deemed a recovery session indefinitely long.

What is the new behavior?

This time boxes the recovery to 15 minutes.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Cache: Disabled due to Reviews > Disable Cache setting

Disabled knowledge base sources:

• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 203ff85 and 61a08e8.

📒 Files selected for processing (1)

• internal/api/user_test.go

---

📝 Walkthrough

Summary by CodeRabbit

• Improvements

  • Recovery sessions (OTP and magic link) now require creation within 15 minutes to allow password updates without the current password; after 15 minutes current-password verification is required.
  • Error message clarified to "A valid current password required when setting new password."

• Tests

  • Added tests for fresh vs. stale recovery sessions, including OTP recovery behavior and staleness handling.

Walkthrough

Recovery sessions for password updates are now valid only if the session's AMR claims include "otp" or "magiclink" and the session was created within the last 15 minutes. If the session is older, the API requires and validates the current password before allowing a password change. The error message for a missing or incorrect current password was updated to "A valid current password required when setting new password." Tests were added/updated to cover fresh and stale OTP recovery session scenarios.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant API
    participant SessionStore
    participant UserDB

    Client->>API: POST /user/updatePassword (may omit current_password)
    API->>SessionStore: fetch session by token
    SessionStore-->>API: session {amr_claims, created_at}
    alt session.amr contains "otp" or "magiclink" AND created_at within 15m
        API->>UserDB: update password (no current password required)
        UserDB-->>API: success
        API-->>Client: 200 OK
    else otherwise
        API->>API: require & validate current_password
        alt current_password valid
            API->>UserDB: update password
            UserDB-->>API: success
            API-->>Client: 200 OK
        else invalid or missing
            API-->>Client: 400/401 "A valid current password required when setting new password."
        end
    end

Loading

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/api/user.go`:
- Around line 175-184: The code dereferences session (session.CreatedAt and
session.AMRClaims) without checking for nil, causing a panic when
UpdatePasswordRequireReauthentication is false and
UpdatePasswordRequireCurrentPassword is true; update the block that checks
session.CreatedAt and iterates session.AMRClaims to first verify session != nil
(and return/skip the recovery session logic if nil), ensuring you only access
session.CreatedAt and call claim.GetAuthenticationMethod() when session is
non-nil; adjust the logic around UpdatePasswordRequireReauthentication /
UpdatePasswordRequireCurrentPassword and the isRecoverySession variable so
behavior remains the same when session is present.

---

ℹ️ Review info

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Cache: Disabled due to Reviews > Disable Cache setting

Disabled knowledge base sources:

• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ab7c9f9 and 93722eb.

📒 Files selected for processing (2)

• internal/api/user.go
• internal/api/user_test.go

[coderabbitai]

♻️ Duplicate comments (1)

internal/api/user.go (1)

175-176: ⚠️ Potential issue | 🔴 Critical

Nil pointer dereference when session is nil.

If UpdatePasswordRequireReauthentication is false but UpdatePasswordRequireCurrentPassword is true, and session is nil, accessing session.CreatedAt on line 175 will cause a panic. The nil check at line 154 only protects the reauthentication block, not this code path.

Proposed fix

 				// it is only a recovery session if it was recently created
-				if time.Now().Before(session.CreatedAt.Add(15*time.Minute)) {
+				if session != nil && time.Now().Before(session.CreatedAt.Add(15*time.Minute)) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/api/user.go` around lines 175 - 176, The code dereferences session
(session.CreatedAt and session.AMRClaims) without ensuring session != nil when
UpdatePasswordRequireReauthentication is false but
UpdatePasswordRequireCurrentPassword is true; add a nil guard for session before
using it (or early-return/force reauthentication when session == nil) so the
block that checks time.Now().Before(session.CreatedAt.Add(15*time.Minute)) and
iterates session.AMRClaims only runs if session != nil; update the logic around
UpdatePasswordRequireReauthentication / UpdatePasswordRequireCurrentPassword to
consistently handle the nil session case in the functions/methods referencing
session.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@internal/api/user.go`:
- Around line 175-176: The code dereferences session (session.CreatedAt and
session.AMRClaims) without ensuring session != nil when
UpdatePasswordRequireReauthentication is false but
UpdatePasswordRequireCurrentPassword is true; add a nil guard for session before
using it (or early-return/force reauthentication when session == nil) so the
block that checks time.Now().Before(session.CreatedAt.Add(15*time.Minute)) and
iterates session.AMRClaims only runs if session != nil; update the logic around
UpdatePasswordRequireReauthentication / UpdatePasswordRequireCurrentPassword to
consistently handle the nil session case in the functions/methods referencing
session.

---

ℹ️ Review info

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Cache: Disabled due to Reviews > Disable Cache setting

Disabled knowledge base sources:

• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 93722eb and 2aa7002.

📒 Files selected for processing (1)

• internal/api/user.go