* 🕰️ fix: Guard `expires_in` So a Token Response Cannot Outlive Its Credential
RFC 6749 §5.1 makes `expires_in` only RECOMMENDED, so a token response may legally omit it.
Four sites derived a lifetime from the raw field, where `undefined * 1000` is `NaN`.
`NaN` is not a short TTL, it is no TTL. `@keyv/redis` writes the key without `PX` because
`NaN` is falsy, so the entry is stored in Redis with no expiration at all; the in-memory
backend embeds `expires: NaN` and every check compares with `>`, always false against `NaN`.
The namespace default does not stand in either, since Keyv applies it with `??=` and `NaN` is
neither `null` nor `undefined`. The exchanged access token was therefore cached permanently at
`openidStrategy.js` and `GraphApiService.js`, and once it genuinely expired the poisoned entry
kept being served with no path to eviction.
The same omission is sharper in `ActionService.js`, where `new Date(NaN).toISOString()` throws
`RangeError: Invalid time value`. Both call sites are inside a `try`, so the failure surfaces as
a generic "Failed to authenticate OAuth tool" that names nothing, and the refresh site falls
through to `requestLogin()` on every attempt, looping with no exit.
The rule had six hand-written homes and three of them were wrong, so it now has one. A new
`packages/api/src/oauth/expiry.ts` normalizes `expires_in` to a positive finite number of
seconds or nothing, and exposes the two shapes callers actually need: a cache TTL that falls
back rather than returning `NaN`, and an absolute expiry that is absent rather than Invalid.
The four unguarded sites adopt it, and the two ad-hoc guards in `openidStrategy.js` and
`OboTokenService.js` are consolidated onto it.
`createHandleOAuthToken` is folded in as well. Its guard already handled `null` and unparseable
strings but admitted `NaN`, since `typeof NaN === 'number'` satisfied its first branch.
The `mcp/oauth` sites are deliberately left alone: `tokens.ts` guards on truthiness and carries
richer logic that reads a JWT access token's own expiry when the response omits one, and the
file is being reworked in #13901.
Closes#15318Closes#15319
* 🕰️ fix: Address `expires_in` Guard Review Round 1
Preserve an explicitly elapsed lifetime instead of collapsing it into "unknown". `expires_in: 0`
is the provider stating the credential is already dead, which is information; treating it as
absent handed it the one-hour fallback in `createHandleOAuthToken` and dropped the expiry
entirely in `ActionService`, so a credential declared expired could be used and retained for up
to an hour. Both sites preserved that value before this branch, so the collapse was a regression
introduced here.
`normalizeExpiresIn` now returns any finite number, positive or not, and reports `undefined` only
for a lifetime that is genuinely unusable. `getTokenExpiresAt` therefore yields a past timestamp
for an elapsed lifetime, so callers refresh rather than guess.
Cache TTLs cannot pass such a value through raw: Keyv reads a TTL of exactly `0` as "no expiry",
turning a dead credential into the immortal entry this module exists to prevent. `getTokenCacheTtlMs`
floors an elapsed lifetime at one millisecond, which expires immediately without ever writing an
entry that outlives its credential.
Parse numeric strings with `Number` rather than `parseInt`, which truncates a complete value such
as `"3.6e3"` to `3` and would expire an hour-long credential after three seconds, re-exchanging
against the identity provider on every request. An empty or blank string is rejected rather than
read as zero, since `Number('')` is `0`.
* 🕰️ fix: Bound `expires_in` to Lifetimes a Date Can Represent
Parsing the complete numeric string last round made an overflow reachable that `parseInt` had
been masking. `parseInt('1e13', 10)` was `1`; `Number('1e13')` is `1e13`, and `1e13` seconds is
1e16 ms, past the ECMAScript time value range of ±8.64e15. Every derived timestamp was therefore
an Invalid Date whose `toISOString()` throws `RangeError: Invalid time value` — the exact failure
this branch exists to remove, reintroduced by its own fix. The token model derives the same way
at `packages/data-schemas/src/methods/token.ts:19`, so storage and authentication would fail with it.
A lifetime is now reported as unusable unless it can still produce a valid `Date`. The bound is
the time value range halved, leaving room for the `Date.now()` every derived timestamp adds. At
roughly 137,000 years it rejects nothing a provider could mean: a one-year refresh token and even
a hundred-year lifetime still pass through untouched, while `1e13`, `Number.MAX_SAFE_INTEGER` and
`1e300` take the caller's fallback instead of poisoning a timestamp.
The invariant tests now carry the overflow shapes rather than a fixed list of small ones, since a
guard that only sees the inputs its author imagined is how the previous round's regression got in.
* feat: implement optimized Entra group sync with auto-creation
## Changes
### MUST FIX (Critical Issues) - RESOLVED
1. **BUG FIX: Prevent unintended user removal from existing groups**
- ISSUE: db.syncUserEntraGroups() was called with only missing groups, causing removal
from all existing Entra groups (full bidirectional sync behavior)
- SOLUTION: Replaced with db.upsertGroupByExternalId() for each missing group followed
by single bulkUpdateGroups() to add memberships (race-safe, idempotent)
- BENEFIT: User memberships correctly maintained for mix of existing + new groups
2. **JSDoc @throws contradiction**
- ISSUE: JSDoc declared function throws, but implementation catches all errors
- SOLUTION: Removed @throws from JSDoc - function is best-effort
- BENEFIT: Prevents unnecessary try/catch in caller code
3. **Missing test for group creation flow**
- ISSUE: Auto-creating missing Entra groups had no test coverage
- SOLUTION: Added regression test for mix of existing + new groups scenario
- BENEFIT: Prevents future regressions on critical path
### SHOULD FIX (Important Improvements) - RESOLVED
4. **E11000 race condition handling**
- SOLUTION: Upserts are idempotent and race-safe by design
- BENEFIT: Concurrent logins no longer race each other
5. **Direct Mongoose access instead of db layer**
- SOLUTION: Added findGroupsByExternalIds() helper to userGroup.ts
- BENEFIT: Centralized data access, easier to add tenant scoping
6. **Serial DB round-trips on login path**
- ISSUE: 40+ queries for user with 20 new groups
- SOLUTION: Promise.all() for parallel upserts + single bulkUpdate
- BENEFIT: ~10x performance improvement
7. **Graph API 429/503 throttling unhandled**
- SOLUTION: Retry logic with exponential backoff (1s, 2s delays)
- BENEFIT: Temporary API issues no longer cause permanent membership loss
8. **Sequential batch requests slow**
- ISSUE: 200 groups = 10 batches × 200ms = ~2s sequential
- SOLUTION: Promise.all() with concurrency limit (5 parallel batches)
- BENEFIT: ~400ms total time
## Minor Fixes
- Removed dead code check
- PII removal: user._id instead of user.email in logs
- ES6 shorthand fixes
- Style consistency (blank lines)
- Projection optimization
## Verification
✅ npm run build - success
✅ npm run test:api - 61/61 passing (+ new regression test)
✅ npm run lint - no errors
✅ All feedback from danny-avila resolved
* docs: better JSDoc for the syncUserEntraGroupMemberships method
---------
Co-authored-by: Airam Hernández Hernández <airam.hernandez@intelequia.com>
WIP: pre-granular-permissions commit
feat: Add category and support contact fields to Agent schema and UI components
Revert "feat: Add category and support contact fields to Agent schema and UI components"
This reverts commit c43a52b4c9.
Fix: Update import for renderHook in useAgentCategories.spec.tsx
fix: Update icon rendering in AgentCategoryDisplay tests to use empty spans
refactor: Improve category synchronization logic and clean up AgentConfig component
refactor: Remove unused UI flow translations from translation.json
feat: agent marketplace features
🔐 feat: Granular Role-based Permissions + Entra ID Group Discovery (#7804)
