Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: session refresh works as intended now#5330

Open
isXander wants to merge 3 commits intomodrinth:main from
isXander:fix/5324
Open

fix: session refresh works as intended now #5330
isXander wants to merge 3 commits intomodrinth:main from
isXander:fix/5324

Conversation

@isXander
Copy link
Contributor

@isXander isXander commented Feb 8, 2026
edited
Loading

Fixes #5324

What's fixed

  1. The /session/refresh failed with expired sessions given, but the frontend only attempted to refresh after a failed fetch. The route fails because it attempts to get the current user from the auth token, but this fails because the session has expired.
  2. If a session was refreshed successfully, the refresh_expires DB column would be reset to 60 days, not retain the timestamp from the original session, making this functionality completely useless.
  3. /session/refresh accepted any authorization token, not just mra_ session tokens, and would issue a new session.

How's it fixed

  • struct SessionBuilder has two new fields, expires: Option<DateTime<Utc>> and refresh_expires.
    • When None, database defaults are used
  • Add param to get_user_record_from_bearer_token to ignore session expiry
    • Any usages have been set to false
  • New get_user_from_bearer_token function that transforms DBUser -> User
  • routes/internal/session.rs/issue_session function now accepts refresh_expires: Option<DateTime<Utc>> as a new parameter, feeds it into the SessionBuilder
  • session/refresh route now feeds in the expired session's refresh_expires
  • Prevented session/refresh route from accepting Authorization tokens that are not mra_

Questions to you

In an attempt to not duplicate the defaults set by the database table migration
(https://github.com/modrinth/code/blob/main/apps/labrinth/migrations/20230628180115_kill-ory.sql#L32-L33), a slightly over-complicated match expression was used to only insert those values when they are Some.
https://github.com/isXander/modrinth/blob/76c63148152b7591b7c93688175bed8e8da4b8aa/apps/labrinth/src/database/models/session_item.rs#L66-L111

If instead you do not mind some duplication, this would make this function significantly simpler.

@isXander isXander marked this pull request as draft February 8, 2026 14:16
@IMB11 IMB11 added the backend Involves work from the backend team label Feb 8, 2026
@IMB11 IMB11 requested a review from a team February 8, 2026 14:18
@IMB11 IMB11 added the 📂 Under review [Triage] Is being reviewed by Modrinth Staff for future roadmap consideration. label Feb 8, 2026
Copy link
Contributor Author

isXander commented Feb 8, 2026

Just confirmed locally that this fix does indeed work!

@isXander isXander marked this pull request as ready for review February 8, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

backend Involves work from the backend team 📂 Under review [Triage] Is being reviewed by Modrinth Staff for future roadmap consideration.

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

session/refresh fails when session has expired

2 participants

AltStyle によって変換されたページ (->オリジナル) /