Quality: Invalid SESSION_MAX_AGE_SECS is silently ignored, causing wrong session lifetime#15367
Conversation
… wrong session lifetime `max_age()` catches `ValueError` when `SESSION_MAX_AGE_SECS` is malformed, logs it, and silently falls back to 30 days. This hides configuration mistakes and can materially change authentication behavior (sessions lasting far longer or shorter than intended) without failing fast. Affected files: auth_utils.py Signed-off-by: Nguyen Van Nam <nam.nv205106@gmail.com>
cjllanwarne
left a comment
cjllanwarne
left a comment
There was a problem hiding this comment.
This looks good to me, thanks for the contribution
There was a problem hiding this comment.
Pull request overview
This PR hardens session lifetime configuration parsing in gear by failing fast when SESSION_MAX_AGE_SECS is malformed or non-positive, avoiding silently falling back to a default that can change authentication behavior.
Changes:
- Raise a
ValueErrorwhenSESSION_MAX_AGE_SECScannot be parsed as an integer. - Validate
SESSION_MAX_AGE_SECSis> 0instead of accepting zero/negative values. - Remove exception logging + fallback behavior in favor of explicit failure.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| MAX_AGE_SECS = int(s) | ||
| except ValueError: | ||
| logging.exception("Unable to interpret SESSION_MAX_AGE_SECS as an integer.") | ||
| except ValueError as e: | ||
| raise ValueError(f"Invalid SESSION_MAX_AGE_SECS={s!r}; expected integer seconds") from e | ||
| if MAX_AGE_SECS <= 0: | ||
| raise ValueError('SESSION_MAX_AGE_SECS must be > 0') |
There was a problem hiding this comment.
MAX_AGE_SECS is assigned before the <= 0 validation. If SESSION_MAX_AGE_SECS is 0/negative, this function raises, but the global remains set to an invalid value; if the exception is caught and max_age() is called again, it will skip re-initialization and return the invalid cached value. Consider validating in a local variable first (or resetting MAX_AGE_SECS back to None before raising) so the cache can’t be left in a bad state.
| def max_age(): | ||
| global MAX_AGE_SECS | ||
| if MAX_AGE_SECS is None: | ||
| if (s := os.getenv('SESSION_MAX_AGE_SECS')) is not None: | ||
| try: | ||
| MAX_AGE_SECS = int(s) | ||
| except ValueError: | ||
| logging.exception("Unable to interpret SESSION_MAX_AGE_SECS as an integer.") | ||
| except ValueError as e: | ||
| raise ValueError(f"Invalid SESSION_MAX_AGE_SECS={s!r}; expected integer seconds") from e | ||
| if MAX_AGE_SECS <= 0: | ||
| raise ValueError('SESSION_MAX_AGE_SECS must be > 0') |
There was a problem hiding this comment.
The updated behavior (raising on malformed/non-positive SESSION_MAX_AGE_SECS) is security/behavior-sensitive and currently untested. There are existing pytest-based tests for other gear utilities (e.g. batch/test/test_time_limited_max_size_cache.py), so it would be good to add unit tests that cover: valid override, malformed value raises, non-positive value raises, and that failures don’t poison the global cache for subsequent calls.
✨ Code Quality
Problem
max_age()catchesValueErrorwhenSESSION_MAX_AGE_SECSis malformed, logs it, and silently falls back to 30 days. This hides configuration mistakes and can materially change authentication behavior (sessions lasting far longer or shorter than intended) without failing fast.Severity:
mediumFile:
gear/gear/auth_utils.pySolution
Fail fast on invalid configuration instead of defaulting:
try:
MAX_AGE_SECS = int(s)
except ValueError as e:
raise ValueError(f"Invalid SESSION_MAX_AGE_SECS={s!r}; expected integer seconds") from e
Optionally also validate positivity:
if MAX_AGE_SECS <= 0:
raise ValueError("SESSION_MAX_AGE_SECS must be > 0")
Changes
gear/gear/auth_utils.py(modified)Change Description
Fixes #<issue_number> (delete if N/A)
Brief description and justification of what this PR is doing.
Security Assessment
Delete all except the correct answer:
Impact Rating
Delete all except the correct answer:
Impact Description
Replace this content with a description of the impact of the change:
Appsec Review
Closes #15366