fix(oauth): preserve TTL on refresh, age via updated_at, reject fractional floats

Addresses Oracle second-pass review findings on dd3c25f9:

- _refresh_access_token: when the refresh response omits expires_in,
  preserve the prior TTL by computing (expires_at - updated_at) from the
  existing record instead of clearing expires_at to NULL. Clearing it
  caused proactive refresh to stop after the first such response, since
  _is_token_expired returns False for NULL. Falls back to NULL only when
  the token had no prior expiry (genuine no-expiry providers like GitHub
  OAuth Apps). New _preserve_prior_ttl() module-level helper handles
  timezone-naive timestamps and non-positive deltas.

- cleanup_expired_tokens: NULL-expiry rows now age via updated_at, not
  created_at. store_tokens advances updated_at on re-authorization, so
  using created_at would delete tokens that were re-authorized recently
  but originally created more than max_age_days ago.

- parse_expires_in: tighten validation against int() truncation. Previously
  expires_in=-0.5 would silently pass as 0 because int(-0.5) == 0 (Python
  truncates toward zero). Now: sign-check the original numeric before
  conversion, and reject non-integer floats (RFC 6749 §5.1 specifies
  integer seconds). Order matters - sign check fires first since 'negative'
  is the more fundamental violation.

Tests:
- test_refresh_without_expires_in_clears_expiry replaced by:
  test_refresh_without_expires_in_preserves_prior_ttl (asserts new TTL
  ~= prior TTL after refresh) and
  test_refresh_without_expires_in_no_prior_ttl_stays_none.
- test_cleanup_expired_tokens_targets_null_expires_at now asserts the
  query uses updated_at AND does NOT use created_at.
- test_parse_expires_in_negative_raises extended with -0.5 and -3600.7.
- New test_parse_expires_in_non_integer_float_raises covers 3600.5, 0.5,
  3600.7.

Signed-off-by: Jonathan Springer <jps@s390x.com>

Author: jonpspri

mcpgateway/services/oauth_manager.py

@@ -1711,6 +1711,13 @@ def parse_expires_in(token_response: Dict[str, Any]) -> Optional[int]:
     # Reject bools (True/False are int subclasses in Python) and any non-scalar types.
     if isinstance(raw, bool) or not isinstance(raw, (int, float, str)):
         raise OAuthError(f"Invalid expires_in from OAuth provider: {raw!r}")
+    # Sign-check the original numeric BEFORE int() truncation, otherwise int(-0.5) == 0
+    # would bypass the negative check.
+    if isinstance(raw, (int, float)) and raw < 0:
+        raise OAuthError(f"Invalid expires_in from OAuth provider (negative): {raw}")
+    # RFC 6749 §5.1 specifies integer seconds; reject non-integral floats explicitly.
+    if isinstance(raw, float) and not raw.is_integer():
+        raise OAuthError(f"Invalid expires_in from OAuth provider (non-integer): {raw}")
     try:
         value = int(raw)
     except (TypeError, ValueError) as exc:

mcpgateway/services/token_storage_service.py

@@ -29,6 +29,48 @@
 logger = logging.getLogger(__name__)
 
 
+def _preserve_prior_ttl(token_record: OAuthToken) -> Optional[int]:
+    """Compute the token's prior TTL in seconds, or ``None`` if not derivable.
+
+    Used when an OAuth refresh response omits ``expires_in`` but the token
+    previously had a finite lifetime - the gateway preserves the original
+    issuance TTL by computing ``expires_at - updated_at`` from the existing
+    record. Returns ``None`` when either timestamp is missing or the difference
+    is non-positive (clock skew or already-expired records).
+
+    Args:
+        token_record: Existing OAuth token row, before the refresh applies.
+
+    Returns:
+        Positive integer seconds of prior TTL, or ``None``.
+
+    Examples:
+        >>> from types import SimpleNamespace
+        >>> from datetime import datetime, timedelta, timezone
+        >>> issued = datetime(2026, 1, 1, tzinfo=timezone.utc)
+        >>> rec = SimpleNamespace(expires_at=issued + timedelta(hours=1), updated_at=issued)
+        >>> _preserve_prior_ttl(rec)
+        3600
+        >>> _preserve_prior_ttl(SimpleNamespace(expires_at=None, updated_at=issued)) is None
+        True
+        >>> _preserve_prior_ttl(SimpleNamespace(expires_at=issued, updated_at=issued + timedelta(hours=1))) is None
+        True
+    """
+    prev_expires_at = token_record.expires_at
+    prev_updated_at = token_record.updated_at
+    if prev_expires_at is None or prev_updated_at is None:
+        return None
+    # Normalize naive timestamps to UTC for the subtraction.
+    if prev_expires_at.tzinfo is None:
+        prev_expires_at = prev_expires_at.replace(tzinfo=timezone.utc)
+    if prev_updated_at.tzinfo is None:
+        prev_updated_at = prev_updated_at.replace(tzinfo=timezone.utc)
+    prev_ttl = int((prev_expires_at - prev_updated_at).total_seconds())
+    if prev_ttl <= 0:
+        return None
+    return prev_ttl
+
+
 class TokenStorageService:
     """Manages OAuth token storage and retrieval.
 
@@ -330,15 +372,30 @@ def normalize_resource(url: str, *, preserve_query: bool = False) -> str | None:
             # Update the token record
             token_record.access_token = encrypted_access
             token_record.refresh_token = encrypted_refresh
+            now = datetime.now(timezone.utc)
             if expires_in is not None:
-                token_record.expires_at = datetime.now(timezone.utc) + timedelta(seconds=expires_in)
+                token_record.expires_at = now + timedelta(seconds=expires_in)
             else:
-                logger.info(
-                    "No expires_in on refresh response for gateway %s; clearing local expiry",
-                    SecurityValidator.sanitize_log_message(token_record.gateway_id),
-                )
-                token_record.expires_at = None
-            token_record.updated_at = datetime.now(timezone.utc)
+                # Refresh response omitted expires_in. If the token previously had a finite
+                # expiry, preserve the prior TTL (expires_at - updated_at) so proactive
+                # refresh keeps working - clearing it outright would cause _is_token_expired
+                # to return False forever and stop the refresh loop. If there was no prior
+                # expiry, leave it as None (provider-level "no known lifetime").
+                preserved_ttl = _preserve_prior_ttl(token_record)
+                if preserved_ttl is not None:
+                    logger.info(
+                        "No expires_in on refresh response for gateway %s; preserving prior TTL of %d seconds",
+                        SecurityValidator.sanitize_log_message(token_record.gateway_id),
+                        preserved_ttl,
+                    )
+                    token_record.expires_at = now + timedelta(seconds=preserved_ttl)
+                else:
+                    logger.info(
+                        "No expires_in on refresh response for gateway %s; no prior TTL to preserve",
+                        SecurityValidator.sanitize_log_message(token_record.gateway_id),
+                    )
+                    token_record.expires_at = None
+            token_record.updated_at = now
 
             self.db.commit()
             logger.info(f"Successfully refreshed token for gateway {token_record.gateway_id}, user {token_record.app_user_email}")
@@ -500,14 +557,18 @@ async def cleanup_expired_tokens(self, max_age_days: int = 30) -> int:
         1. Tokens whose ``expires_at`` is older than the cutoff (the original
            "expired more than N days ago" behaviour).
         2. Tokens with ``expires_at IS NULL`` (provider omitted ``expires_in``)
-           whose ``created_at`` is older than the cutoff. ``NULL < <cutoff>``
+           whose ``updated_at`` is older than the cutoff. ``NULL < <cutoff>``
            evaluates to ``NULL`` in SQL three-valued logic, so without this
-           branch those rows would never age out.
+           branch those rows would never age out. ``updated_at`` (rather than
+           ``created_at``) is the right freshness signal because
+           ``store_tokens`` advances it on re-authorization, so a recently
+           re-authorized token isn't deleted just because its original row was
+           old.
 
         Args:
             max_age_days: Maximum age of tokens to keep, measured from
                 ``expires_at`` for tokens with a known expiry and from
-                ``created_at`` for tokens with no provider-supplied expiry.
+                ``updated_at`` for tokens with no provider-supplied expiry.
 
         Returns:
             Number of tokens cleaned up
@@ -526,7 +587,7 @@ async def cleanup_expired_tokens(self, max_age_days: int = 30) -> int:
 
             stale_filter = or_(
                 OAuthToken.expires_at < cutoff_date,
-                and_(OAuthToken.expires_at.is_(None), OAuthToken.created_at < cutoff_date),
+                and_(OAuthToken.expires_at.is_(None), OAuthToken.updated_at < cutoff_date),
             )
             result = self.db.execute(delete(OAuthToken).where(stale_filter))
             count = result.rowcount

tests/unit/mcpgateway/services/test_oauth_manager.py

@@ -69,8 +69,9 @@ def test_parse_expires_in_explicit_null_returns_none():
     assert parse_expires_in({"expires_in": None}) is None
 
 
-@pytest.mark.parametrize("bad_value", [-1, -3600, "-1"])
+@pytest.mark.parametrize("bad_value", [-1, -3600, "-1", -0.5, -3600.7])
 def test_parse_expires_in_negative_raises(bad_value):
+    """Negative numerics raise even when int() would silently truncate (-0.5 -> 0)."""
     with pytest.raises(OAuthError, match="negative"):
         parse_expires_in({"expires_in": bad_value})
 
@@ -81,6 +82,13 @@ def test_parse_expires_in_garbage_string_raises(bad_value):
         parse_expires_in({"expires_in": bad_value})
 
 
+@pytest.mark.parametrize("bad_value", [3600.5, 0.5, 3600.7])
+def test_parse_expires_in_non_integer_float_raises(bad_value):
+    """RFC 6749 §5.1 specifies integer seconds; non-integer floats are rejected."""
+    with pytest.raises(OAuthError, match="non-integer"):
+        parse_expires_in({"expires_in": bad_value})
+
+
 @pytest.mark.parametrize("bad_value", [True, False, [3600], {"seconds": 3600}, object()])
 def test_parse_expires_in_non_scalar_raises(bad_value):
     with pytest.raises(OAuthError, match="Invalid expires_in"):

tests/unit/mcpgateway/services/test_token_storage_service.py

@@ -339,15 +339,44 @@ async def test_refresh_success(service, mock_db):
 
 
 @pytest.mark.asyncio
-async def test_refresh_without_expires_in_clears_expiry(service, mock_db):
+async def test_refresh_without_expires_in_preserves_prior_ttl(service, mock_db):
+    """Refresh response missing expires_in: preserve the prior TTL so proactive refresh keeps working."""
     gw = MagicMock(oauth_config={"token_url": "https://token", "client_id": "cid"}, url="https://gw.com")
     mock_db.query.return_value.filter.return_value.first.return_value = gw
     mock_oauth_manager = MagicMock()
     mock_oauth_manager.refresh_token = AsyncMock(return_value={"access_token": "new_access", "refresh_token": "new_refresh"})
+
+    # Token issued 100 seconds ago with a 1-hour TTL.
     record = _make_token_record()
+    issued = datetime.now(tz=timezone.utc) - timedelta(seconds=100)
+    record.updated_at = issued
+    record.expires_at = issued + timedelta(seconds=3600)
+
+    with patch("mcpgateway.services.oauth_manager.OAuthManager", return_value=mock_oauth_manager):
+        result = await service._refresh_access_token(record)
+
+    assert result == "new_access"
+    # Prior TTL preserved: new expires_at should be ~3600s after the refresh moment (now).
     assert record.expires_at is not None
+    delta = (record.expires_at - record.updated_at).total_seconds()
+    assert 3599 <= delta <= 3601
+
+
+@pytest.mark.asyncio
+async def test_refresh_without_expires_in_no_prior_ttl_stays_none(service, mock_db):
+    """Refresh response missing expires_in AND no prior TTL: expires_at stays None."""
+    gw = MagicMock(oauth_config={"token_url": "https://token", "client_id": "cid"}, url="https://gw.com")
+    mock_db.query.return_value.filter.return_value.first.return_value = gw
+    mock_oauth_manager = MagicMock()
+    mock_oauth_manager.refresh_token = AsyncMock(return_value={"access_token": "new_access", "refresh_token": "new_refresh"})
+
+    # Token had no prior expiry (e.g. GitHub OAuth Apps).
+    record = _make_token_record()
+    record.expires_at = None
+
     with patch("mcpgateway.services.oauth_manager.OAuthManager", return_value=mock_oauth_manager):
         result = await service._refresh_access_token(record)
+
     assert result == "new_access"
     assert record.expires_at is None
 
@@ -568,7 +597,10 @@ async def test_cleanup_expired_tokens_targets_null_expires_at(service, mock_db):
     delete_stmt = mock_db.execute.call_args.args[0]
     rendered = str(delete_stmt.compile(compile_kwargs={"literal_binds": True})).lower()
     assert "expires_at is null" in rendered
-    assert "created_at" in rendered
+    # NULL-expires_at rows must be aged out by updated_at (re-auth advances it),
+    # not created_at (which would delete recently re-authorized tokens).
+    assert "updated_at" in rendered
+    assert "created_at" not in rendered
 
 
 # ---------- token_type validation in get_user_token ----------