🍪 fix: Validate Shared-File Cookie Auth Against the Live Refresh Session#13908
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: ce98bc9fac
ℹ️ 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".
| return null; | ||
| } | ||
|
|
||
| const session = await findSession({ userId, refreshToken: token }); |
There was a problem hiding this comment.
In strict tenant-isolation deployments, image/anchor requests to shared-file routes reach this fallback without a bearer token, so optionalJwtAuth has not established tenant ALS before optionalShareFileAuth runs. Session is tenant-isolated (packages/data-schemas/src/models/session.ts) and its query middleware throws when TENANT_ISOLATION_STRICT=true with no tenant context, so this new findSession call is caught, leaves req.user unset, and valid refresh-token viewers are rejected by canAccessSharedLink for non-public shared files. Wrap the session lookup in runAsSystem (as the user lookup below already does) or otherwise establish the tenant context first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 01fd69134 by wrapping the refresh-token findSession lookup in runAsSystem, matching the user lookup that already had system tenant context. I also updated optionalShareFileAuth.spec.js to assert the session lookup path enters runAsSystem for both live and revoked refresh-token sessions.
Verified with:
npm --prefix api test -- optionalShareFileAuth.spec.js --runInBandgit diff --check
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
danny-avila
changed the title
…ion (danny-avila#13908) * fix: validate shared file cookie sessions * fix: run shared file session lookup as system
…ion (danny-avila#13908) * fix: validate shared file cookie sessions * fix: run shared file session lookup as system
1 participant
Motivation
Description
api/server/middleware/optionalShareFileAuth.jsto verify that arefreshTokencookie resolves to a live session viafindSessionbefore settingreq.user, and rename/clarify helpers (verifySignedUserId,getRefreshTokenUserId,getOpenIdUserId).openid_user_idfallback so it only applies when an active server-side OpenID session refresh token matches the request, preventing the signed cookie alone from granting access.canAccessSharedLinkon failure) and load the resolved user withgetUserByIdunderrunAsSystemto respect tenant isolation.api/server/middleware/optionalShareFileAuth.spec.jsto cover live-session acceptance, revoked-session rejection, active OpenID fallback acceptance, and stale OpenID rejection.Testing
npm --prefix api test -- optionalShareFileAuth.spec.js --runInBand, and all tests passed (8 passed).git diff --check(no issues reported) to validate formatting/whitespace.Codex Task