Allow provide session max life #3567
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6888a61ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if time.Since(session.LastActive).Hours() > 7*24 { | ||
| // more than week old unused session | ||
| if time.Since(session.LastActive).Hours() > float64(util.Config.Auth.MaxSessionLifeHours) { |
There was a problem hiding this comment.
If auth.max_session_life_hours is not set in config/env, MaxSessionLifeHours remains 0 because loadDefaultsToObject doesn’t descend into pointer-to-struct fields (like AuthConfig), so the new default tag never applies. With a zero value, time.Since(...).Hours() > 0 becomes true for essentially every existing session, and this code expires sessions immediately on the next request. This is a regression for installs that don’t set the new field (including the default setup flow which doesn’t write an auth section). Consider explicitly handling a zero value here or ensuring defaults are applied to pointer fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This pull request adds configurable session maximum lifetime to the authentication system. Previously, sessions were hardcoded to expire after 7 days (168 hours) of inactivity. This change allows administrators to customize the session lifetime via configuration (JSON file or environment variable).
Changes:
- Added
MaxSessionLifeHoursconfiguration field toAuthConfigwith a default of 168 hours (7 days) - Updated session expiration logic in both
api/auth.goandservices/session_svc.goto use the configurable value - Updated dependencies in the
pro/module (golang.org/x/crypto and golang.org/x/sys)
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| util/config_auth.go | Added new MaxSessionLifeHours field to AuthConfig struct with default value and environment variable support |
| services/session_svc.go | Replaced hardcoded 7*24 hours with configurable util.Config.Auth.MaxSessionLifeHours |
| api/auth.go | Replaced hardcoded 7*24 hours with configurable util.Config.Auth.MaxSessionLifeHours |
| pro/go.mod | Updated golang.org/x/crypto to v0.46.0 and golang.org/x/sys to v0.39.0 |
| pro/go.sum | Added checksums for updated dependencies |
| Email *EmailAuthConfig `json:"email,omitempty"` | ||
|
|
||
| // MaxSessionLifeHours defines the maximum lifetime of a session in hours. | ||
| // After this period of inactivity, the session will expire. |
There was a problem hiding this comment.
The documentation is misleading. It states "After this period of inactivity, the session will expire" but the implementation actually uses the total time since the last active timestamp, not a rolling window of inactivity. The comment should clarify that this is the maximum time since the session was last active, not a period of continuous inactivity.
| // After this period of inactivity, the session will expire. | |
| // When the time elapsed since the session was last active exceeds this value, the session will expire. |
|
|
||
| if time.Since(session.LastActive).Hours() > 7*24 { | ||
| // more than week old unused session | ||
| if time.Since(session.LastActive).Hours() > float64(util.Config.Auth.MaxSessionLifeHours) { |
There was a problem hiding this comment.
Potential nil pointer dereference. If util.Config.Auth is nil, this code will panic when trying to access MaxSessionLifeHours. The Auth field in ConfigType is defined as a pointer (*AuthConfig) and is not initialized in NewConfigType(). Consider adding a nil check or ensuring Auth is initialized with default values during config initialization.
| if time.Since(session.LastActive).Hours() > float64(util.Config.Auth.MaxSessionLifeHours) { | |
| if util.Config.Auth != nil && time.Since(session.LastActive).Hours() > float64(util.Config.Auth.MaxSessionLifeHours) { |
|
|
||
| if time.Since(session.LastActive).Hours() > 7*24 { | ||
| // more than week old unused session | ||
| if time.Since(session.LastActive).Hours() > float64(util.Config.Auth.MaxSessionLifeHours) { |
There was a problem hiding this comment.
Potential nil pointer dereference. If util.Config.Auth is nil, this code will panic when trying to access MaxSessionLifeHours. The Auth field in ConfigType is defined as a pointer (*AuthConfig) and is not initialized in NewConfigType(). Consider adding a nil check or ensuring Auth is initialized with default values during config initialization.
| if time.Since(session.LastActive).Hours() > float64(util.Config.Auth.MaxSessionLifeHours) { | |
| if util.Config.Auth != nil && util.Config.Auth.MaxSessionLifeHours > 0 && | |
| time.Since(session.LastActive).Hours() > float64(util.Config.Auth.MaxSessionLifeHours) { |
| // MaxSessionLifeHours defines the maximum lifetime of a session in hours. | ||
| // After this period of inactivity, the session will expire. | ||
| // Default is 168 hours (7 days). | ||
| MaxSessionLifeHours int `json:"max_session_life_hours,omitempty" default:"168" env:"SEMAPHORE_MAX_SESSION_LIFE_HOURS"` |
There was a problem hiding this comment.
Missing input validation for MaxSessionLifeHours. Zero or negative values would cause sessions to never expire or behave unexpectedly. Consider adding a validation rule in the struct tag (similar to other fields like Port and MaxParallelTasks in config.go) to ensure the value is positive, or add validation logic in the validateConfig function.
| MaxSessionLifeHours int `json:"max_session_life_hours,omitempty" default:"168" env:"SEMAPHORE_MAX_SESSION_LIFE_HOURS"` | |
| MaxSessionLifeHours int `json:"max_session_life_hours,omitempty" default:"168" env:"SEMAPHORE_MAX_SESSION_LIFE_HOURS" validate:"gt=0"` |
| golang.org/x/exp v0.0.0-20250620022241-b7579e27df2b // indirect | ||
| golang.org/x/net v0.47.0 // indirect | ||
| golang.org/x/sys v0.38.0 // indirect | ||
| golang.org/x/sys v0.39.0 // indirect |
There was a problem hiding this comment.
This dependency update appears unrelated to the session lifetime configuration feature. Consider separating dependency updates into a different pull request for better change isolation and easier review.
| golang.org/x/sys v0.39.0 // indirect |
No description provided.