fix(security): enforce strict revocable sessions on every JWT transport - #414
fix(security): enforce strict revocable sessions on every JWT transport#414seonghobae wants to merge 45 commits into
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthrough세션 JWT 검증을 공통 함수로 통합하고 URL 토큰 경로에 Changes세션 JWT 폐기 검증
MS Project XML 파서
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant URLTokenEndpoint
participant verifySessionJwt
participant Database
Client->>URLTokenEndpoint: JWT 요청
URLTokenEndpoint->>verifySessionJwt: 세션 JWT 검증
verifySessionJwt->>Database: token_version 조회
Database-->>verifySessionJwt: 현재 세션 버전
verifySessionJwt-->>URLTokenEndpoint: 인증 성공 또는 실패
URLTokenEndpoint-->>Client: HTTP 200 또는 401
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/app.mjs`:
- Around line 32-35: Update verifySessionJwt to require the payload.tv claim to
be present and an integer before querying or comparing the user token version;
reject missing, null, false, and other non-integer values, then compare the
validated claim directly with user.token_version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e60b929a-645c-43d7-a11e-153b03953950
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.github/workflows/fuzz.yml.github/workflows/server-tests.yml.jules/sentinel.md.jules/verification-session-revocation.mdCHANGELOG.mdcloud-sync.jspackage.jsonserver/app.mjstests/api/session-revocation.test.mjstests/unit/msproject.test.mjs
Pull request was converted to draft
83fb51f to
6c3f91b
Compare
6c3f91b to
382d683
Compare
|
Current-head maintainer verification for |
server/app.mjs에서 첨부파일(attachments)의 PENDING 상태를 동기화할 때, 기존 for...of 루프 내부에서 jobStatus를 순차적으로 await 하던 로직을 Promise.all(rows.map(...))을 사용하도록 변경했습니다. 이를 통해 첨부파일이 여러 개일 경우 발생하는 네트워크 호출 병목을 효과적으로 줄이고 응답 지연을 방지합니다.
server/app.mjs에서 첨부파일(attachments)의 PENDING 상태를 동기화할 때, 기존 for...of 루프 내부에서 jobStatus를 순차적으로 await 하던 로직을 Promise.all(rows.map(...))을 사용하도록 변경했습니다. 이를 통해 첨부파일이 여러 개일 경우 발생하는 네트워크 호출 병목을 효과적으로 줄이고 응답 지연을 방지합니다. 추가로 CI Trivy 스캔에서 발견된 hono 패키지의 취약점(CVE-2026-69207)을 해결하기 위해 버전을 4.12.32에서 4.13.0으로 업데이트했습니다.
Address CodeRabbit feedback: unbounded Promise.all over all pending attachments could exceed Clearfolio connection/rate limits. Filter to PENDING/RUNNING rows and process in chunks of 5, preserving best-effort stale-status handling. Also revise the .jules/bolt.md guidance to require bounded concurrency for external calls. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pull request was converted to draft
f11761b to
1756da0
Compare
1756da0 to
02c8d84
Compare
|
Superseded by clean replacement #436. The replacement is based directly on #432 exact head |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Enforce one fail-closed, database-backed session boundary for every ScopeWeave endpoint that accepts a session JWT:
/api/projects/:id/calendar.ics?token=...;/api/projects/:id/stream?token=...;/api/projects/:id/attachments/:attachment_id/view?token=....signTokenrefuses to mint a session unless the subject, token version, and lifetime are bounded safe integers.verifyTokenauthenticates the compact signature before parsing claims, requires a signedHS256/JWTheader, requires an object payload with a positive safe-integer subject and future safe-integer expiry, requires a non-negative safe-integertv, requires the user to exist, and comparestvexactly withusers.token_versionuntil the canonical database-object migration in #433.Missing, null, Boolean, string, fractional, negative, unsafe, expired, forged, wrong-header, malformed-payload, nonexistent-user, and stale tokens fail before tenant or resource lookup.
Security impact
Before this change, URL-token session JWTs could remain usable until expiration after
logout-allincrementedtoken_version. Route-local coercion could also treat malformed token-version claims as version zero. Missing expiry and weakly typed subject claims were not rejected centrally. Consolidating the contract removes transport-specific omissions and prevents internal callers from minting malformed sessions.This is the revocation prerequisite for #413, which will replace full session JWTs in URLs with narrowly scoped ephemeral grants and independently revocable calendar subscription secrets.
Regression coverage
tests/api/session-revocation.test.mjsproves signer claim validation, two-device authentication across bearer/calendar/SSE/attachment-view transports, malformed/forged/expired/stale token rejection, nonexistent-user rejection before resource lookup, logout-all revocation, replacement-token continuity, and authentication-before-resource-lookup ordering.All shipped authentication primitives and helpers have beginner-readable JSDoc. The regression is wired into
test:apiand the c8 coverage gate.Standards traceability
docs/doctoring/session-revocation.mdrecords the shared invariant, regression contract, modular authentication boundary, and APA 7th references to RFC 7519, RFC 6750, RFC 8725, and RFC 9700. The implementation follows pinned-algorithm, strict-claim, and fail-closed session semantics without claiming that full session JWT query transport is an acceptable long-term design.Stacked verification and merge order
This PR is intentionally Draft and currently targets #432's exact branch. Its head is a focused session-security layer on top of #432, so review and test evidence remain isolated.
After #432 merges:
develop;Protections must not be bypassed.