Session staleness

[bourgeoa]

No description provided.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect identity transitions and authorization-cache freshness.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds session-transition detection and invalidates cached rdflib authorization metadata to prevent stale editability results.

Changes:

• Detects login, logout, refocus, and identity transitions.
• Emits sessionChange and flags authorization metadata.
• Adds compatibility wiring and transition/contract tests.

File summaries

File	Summary and final review note
test/transitions.test.ts	Tests session transition behavior.
test/rdflibEditableFlagContract.test.ts	Tests rdflib metadata behavior. Nit (1 vote): does not exercise a real fresh-response load path.
test/flagAuthorizationOnTransitions.test.ts	Tests invalidation wiring.
src/logic/solidLogic.ts	Connects session transitions to store invalidation.
src/authSession/transitions.ts	Detects identity transitions. Moderate (2 votes): may miss active-to-active WebID changes.
src/authSession/flagAuthorizationOnTransitions.ts	Flags authorization metadata. Moderate (1 vote): may not force a fresh response due to rdflib metadata lookup behavior.
src/authSession/events.ts	Adds the sessionChange event.
src/authSession/authSession.ts	Wires transitions and compatibility state. Critical (1 vote): restoring synthesized info can retain a stale identity across transitions.

Review details

Suppressed comments (2)

src/authSession/flagAuthorizationOnTransitions.ts:15

• After flagAuthorizationMetadata() this does not reliably lead to a fresh response. In rdflib 2.4.0, Fetcher.saveRequestMetadata() records link:requestedURI as a literal, but Fetcher.load() searches it with kb.sym(docuri); the lookup misses the flagged response, so it never sets force and an already-fetched document is returned from cache. Callers such as src/util/utilityLogic.ts:92-96 then still see editable() === undefined after a session transition. Please force/refresh the document or fix the metadata lookup, and cover that path rather than relying on manually inserted fresh metadata.

 * out-of-date. `fetcher.load()` clears the mark for a document and re-fetches
 * it with the current credentials, so editability answers definitively again
 * on the next load. Call sites that need the answer immediately use the async

test/rdflibEditableFlagContract.test.ts:42

• The “fresh response” path is not exercised here: this test never calls checkEditable() or fetcher.load(); it directly inserts a second request/response into the store. That bypasses the cache invalidation and literal-vs-named-node lookup used by production, so the test can pass while a real post-transition load still returns the stale out-of-date metadata. Set up a real Fetcher with a stubbed network response and assert the load/checkEditable result.

    // The next load records a fresh, authenticated response.
    const fresh = { request: sym('urn:x-auth-test:req-2'), response: sym('urn:x-auth-test:res-2') }
    store.add(fresh.request, LINK('requestedURI'), lit(doc), meta)
    store.add(fresh.request, LINK('response'), fresh.response, meta)
    store.add(fresh.response, HTTPH('wac-allow'), lit('user="read write"'), meta)
    store.add(fresh.response, HTTPH('accept-patch'), lit('text/n3'), meta)
    expect(updater.editable(doc)).toBe('N3PATCH')

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Three unresolved findings remain, including two critical stale-identity and authorization-cache issues.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved session-fetch and authorization-cache invalidation issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/authSession/authSession.ts:129

• The new isLoggedIn: false value is not sufficient for checkUser() to reject the cached WebID: SolidAuthnLogic.webIdFromSession() requires infoLoggedIn === false && rootLoggedIn === false && rootActive === false, but the actual session root has no isLoggedIn property, so rootLoggedIn is undefined and the function falls through to return this webId. After logout with a retained WebID, checkUser() can therefore still return the old identity; make the explicit inactive state win without requiring an absent root flag to be false.

export function legacySessionInfo (session: SessionLike): { webId?: string; isLoggedIn?: boolean } {
  return {
    webId: session.webId,
    isLoggedIn: sessionIsActive(session)

src/authSession/authSession.ts:129

• Although this derived value now reports isLoggedIn: false, SolidAuthnLogic.currentUser() still ends its decision with || Boolean(this.fallbackWebId) and does not clear that fallback on logout/session transitions. Once checkUser() has cached identity A there, a logout can still make currentUser() return A despite the inactive session; invalidate or gate the fallback when the session explicitly reports inactive.

export function legacySessionInfo (session: SessionLike): { webId?: string; isLoggedIn?: boolean } {
  return {
    webId: session.webId,
    isLoggedIn: sessionIsActive(session)

src/authSession/flagAuthorizationOnTransitions.ts:81

• The pinned rdflib 2.4.0 fetcher.refresh() is callback-based and returns void, so awaiting this call resumes before the refresh response records new authorization metadata. The helper can therefore read editable() while it is still undefined; its test misses this because the fake refresh returns a Promise. Promisify the refresh callback and only read editability after that callback completes.

    try {
      await refresh(doc)

• Files reviewed: 10/10 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical session-state and authorization-refresh issues, plus a moderate cache-refresh documentation issue, remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/authn/SolidAuthnLogic.ts:294

• Returning null for an explicitly inactive session now sends resolveCurrentUser() into the NSS cookie fallback at SolidAuthnLogic.ts:195-198. After a real logout, the uvdsl session clears its local state but does not clear the server cookie, so probeNssCookieBackedWebId() can recover the same WebID and checkUser() immediately re-establishes the logged-out identity. Skip that fallback for an explicit logout (while retaining it for a fresh/unknown session), and add a regression test for the cookie-backed case.

    if (infoLoggedIn === false || rootLoggedIn === false || rootActive === false) {

src/logic/solidLogic.ts:33

• With the pinned rdflib 2.4.0, fetcher.load() does not clear the flagged request for an already-loaded document; editable() remains undefined until refreshDocumentAuthorization() calls refresh(). This comment therefore promises a repair that the wiring alone does not provide and can mislead future call sites. Describe the force-refresh requirement instead.

    // Whose credentials a request would carry changed: mark every recorded
    // response out-of-date so editability re-answers per document on its next
    // load instead of reporting the previous identity's access. See
    // flagAuthorizationOnTransitions.ts.
    flagAuthorizationOnSessionTransitions(store, session)

• Files reviewed: 14/14 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical stale-authorization and cached-data issues can permit incorrect identity or editability decisions.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/authSession/flagAuthorizationOnTransitions.ts:79

• This generation counter is module-global even though transition handlers are registered per store/session. If two createSolidLogic instances use different sessions, a transition in session B makes an in-flight refresh for session A look overtaken and can force retries or an undefined result despite A never changing. Scope the generation to the relevant store/session (or pass a per-session generation into the refresh helper).

// Every observed identity transition is counted, so an in-flight
// authorization refresh can tell whether the response it recorded still
// belongs to the identity that asked for it — see
// refreshDocumentAuthorization().
let authorizationGeneration = 0

src/logic/solidLogic.ts:32

• This comment says editability is repaired by the document's next load(), but the pinned rdflib 2.4.0 path deliberately does not refetch a flagged, already-loaded document. The actual repair is the explicit refreshDocumentAuthorization() used by decision points, so this wording can mislead future callers into relying on load() and reintroducing the stale-authorization bug.

    // Whose credentials a request would carry changed: mark every recorded
    // response out-of-date so editability re-answers per document on its next
    // load instead of reporting the previous identity's access. See
    // flagAuthorizationOnTransitions.ts.

src/util/utilityLogic.ts:137

• The same stale-data bypass exists here: after a flagged load(), the store.any(...) result on the preceding line can be a link from the previous identity, so this new refresh is skipped and that private link is returned. Force/validate a fresh document before reading cached triples, rather than only repairing editability when no result exists.

    const editable = store.updater.editable(doc) ??
      await refreshDocumentAuthorization(store, doc)

• Files reviewed: 14/14 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved authorization-cache safety and session-fallback issues require fixes before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

src/authSession/flagAuthorizationOnTransitions.ts:130

• forceRefresh() resolves even when the refresh callback reports failure or the refresh call throws, and this line then returns whatever editable() has cached. When flagging failed, that value is explicitly from the previous identity, so ensureDocumentAuthorization() can treat a failed repair as a definitive permission and allow stale data/writes. Carry refresh success through this helper and return undefined on failure instead of reading the old authorization result.

      return store.updater?.editable?.(doc)

src/authSession/flagAuthorizationOnTransitions.ts:150

• refreshDocumentAuthorization() can return undefined when every refresh attempt is overtaken, but this helper discards that result and still resolves. The utility callers then execute store.any(...) and may return the old cached link before the later editable() check, so the documented fail-closed behavior is not achieved. Propagate the unknown result or throw before callers consume cached triples, while preserving a definitive false (read-only) result.

  if (state.refreshRequired || store.updater?.editable?.(doc) === undefined) {
    await refreshDocumentAuthorization(store, doc)

src/authn/SolidAuthnLogic.ts:54

• This guard also clears the NSS cookie-backed fallback that resolveCurrentUser() intentionally sets when the OIDC session has no active client state (probeNssCookieBackedWebId() at SolidAuthnLogic.ts:194-202). After a successful cookie probe, a later currentUser() still sees sessionExplicitlyInactive(), drops fallbackWebId, and returns null, so the fallback login cannot remain usable. Distinguish a remembered pre-logout fallback from a freshly probed cookie-backed identity, or clear the fallback only on an observed logout transition.

    if (sessionExplicitlyInactive(sessionAny)) {
      // A logout that leaves the WebID cached must not keep answering for the
      // previous user: drop the remembered fallback and report logged out.
      this.fallbackWebId = null
      return offlineTestID() // null unless testing

src/util/utilityLogic.ts:134

• The same fail-open path exists here: ensureDocumentAuthorization() may return after an unsuccessful or overtaken refresh, yet store.any still consumes the cached graph. That can return a link loaded under the previous identity; do not read triples unless the authorization repair reports a current result.

    await ensureDocumentAuthorization(store, doc)
    const result = store.any(subject, predicate, null, doc)

• Files reviewed: 14/14 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved authorization-generation and identity-transition issues could leave stale session data.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/authSession/transitions.ts:60

• note() runs on every visible refocus, not only when classifySessionTransition reports a change. If the page starts in the partial-logout state { isActive: false, webId: A }, this expression returns true for an unchanged snapshot solely because !next.isActive is true, so every refocus emits identityReplaced and can repeatedly trigger the consumer's reload. Gate the inactive transition on prev.isActive while retaining the WebID-difference case.

  return next.webId !== prev.webId || !next.isActive

src/authn/SolidAuthnLogic.ts:213

• The NSS cookie fallback is a second identity source, but changing these fields emits no session event. Because the OIDC session remains inactive and has no WebID, the transition watcher cannot observe anonymous→cookie-user, cookie-user A→B, or cookie logout; the shared RDF store and authorization metadata therefore remain associated with the prior identity and reload consumers receive no identityReplaced signal. Treat changes to the cookie-backed fallback as an identity transition (and wire that event through invalidation/reload).

    if (webId) {
      this.fallbackWebId = webId
      this.cookieBackedFallback = cookieBacked

src/util/utilityLogic.ts:140

• The same in-flight-load race exists here: a response begun before a session transition can be recorded after the invalidation and appear definitive to ensureDocumentAuthorization(). The subsequent store.any can therefore read the old-identity graph without a force refresh; include this load in the generation check/retry rather than relying only on the later helper.

    await store.fetcher.load(doc)
    if (!(await ensureDocumentAuthorization(store, doc))) {
      const msg = `followOrCreateLinkWithContentOnCreate: cannot establish the authorization of ${doc.value}`
      debug.warn(msg)
      throw new NotEditableError(msg)

• Files reviewed: 15/15 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Cross-tab refocus synchronization and duplicate fallback notifications remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/authSession/transitions.ts:181

• On refocus this only calls note(), which snapshots the current tab's in-memory session; it never synchronizes from the shared session database. The local-dev path constructs SessionCore with IndexedDB (src/authSession/session.ts:177-189), and that instance does not receive another tab's login/logout, so after a cross-tab change previous and next remain identical and no transition is emitted. Trigger a restore/synchronization or add a cross-tab signal before comparing snapshots.

      if (doc.visibilityState === 'visible') note()

• Files reviewed: 15/15 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved cross-tab resynchronization, logout detection, and duplicate transition signals remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/authSession/authSession.ts:110

• sessionHasCrossTabPush() is a static choice made during construction, but SolidAuthnLogic.resolveCurrentUser() already documents that a WebSession can construct successfully and later have a worker whose restore() hangs and times out. In that failure mode this branch still disables the visibility resync, so a cross-tab login/logout cannot update the local snapshot and no transition is emitted. The worker-health/timeout path needs to disable this optimization or otherwise retain a bounded refocus resync.

const resyncSession = sessionHasCrossTabPush()
  ? undefined
  : () => {

src/authSession/session.ts:184

• crossTabPush is set to true solely because new WebSession() returned. A SharedWorker can be constructed successfully and then fail to load or connect asynchronously; this codebase already documents that such a worker can leave restore() pending. In that case authSession.ts passes no resync callback, so visibility changes never re-read the session and cross-tab login/logout transitions remain invisible. Detect worker readiness/failure and switch to a guarded resync path instead of treating constructor success as proof that push events are available.

// Whether the chosen session receives another tab's identity change as a
// pushed event (WebSession + SharedWorker) or must be re-read on refocus
// (SessionCore + IndexedDB). See watchSessionTransitions().
let crossTabPush = true

/** For consumers that must re-read the session when a tab regains focus. */
export function sessionHasCrossTabPush (): boolean {
  return crossTabPush

src/authn/SolidAuthnLogic.ts:310

• This branch treats every non-null previous fallback as an established cookie identity, even when previousCookieBacked is false. For an OIDC user B followed by logout while a different cookie user A is available, the watcher already emits identityReplaced for B → inactive, then checkUser() reaches this branch and emits a second replacement for B → A; a reload consumer can therefore reload twice. The inverse cookie → OIDC path also duplicates the watcher’s sessionChange. Coordinate fallback reporting with the raw-session transition (and add integration coverage) so each transition signal is emitted once.

    if (previousFallback !== null) {
      // An established cookie-backed identity was replaced or cleared.
      events.emit('sessionChange')
      events.emit('identityReplaced')

• Files reviewed: 16/16 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved moderate issues affect restore resynchronization, fallback identity refresh, and duplicate transition events.

Review details

Suppressed comments (4)

src/authSession/authSession.ts:124

• If restore() takes longer than 2 seconds, this race resolves as 'changed' and syncThenNote() compares the current snapshot, but the eventual restore outcome is discarded. A slow cross-tab logout can therefore reject with No session to restore after the timeout without emitting logout/identityReplaced, leaving this tab on the old identity until another visibility event; keep a continuation for the timed-out restore (with duplicate suppression) or otherwise do not discard its result.

  return Promise.race([
    restored,
    new Promise<'changed'>((resolve) => setTimeout(() => resolve('changed'), RESYNC_TIMEOUT_MS))

src/authSession/authSession.ts:127

• The visibility resync only re-reads _session; it never re-runs the NSS cookie probe or updates SolidAuthnLogic.fallbackWebId, which is changed only by checkUser(). If this tab is using cookie-backed identity A and another tab logs out or switches identity while it is backgrounded, the raw OIDC session can remain inactive/unchanged, so refocus emits no replacement and currentUser() can continue returning cached A. Revalidate the cookie-backed source on refocus (and route the result through reportFallbackIdentityChange) before treating the snapshot as current.

watchSessionTransitions(_session as unknown as SessionLike, (event) => events.emit(event), undefined, resyncSession)

src/authSession/events.ts:17

• The event documentation now lists logout and sessionChange but omits the newly supported identityReplaced event, even though it is emitted by this watcher and is the event consumed by the exported reload helper. Include it in this public compatibility-layer event list so consumers can discover the new subscription.

 * and by the transition watcher in authSession.ts (logout, sessionChange).

src/authSession/transitions.ts:60

• After an active A→inactive A transition, watchSessionTransitions stores { isActive: false, webId: A } as previous after already emitting identityReplaced. A subsequent clear (A→undefined) or login as B still satisfies next.webId !== prev.webId here, so identityReplaced is emitted a second time. Require the previous snapshot to be active, or otherwise track that the replacement was already reported, before emitting another replacement.

  if (prev.webId === undefined) return false
  if (next.webId !== prev.webId) return true

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Lite