Conversation
Signed-off-by: Kevin Jahns <kevin.jahns@protonmail.com>
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 33900402 | Triggered | Generic Password | 3bc372c | compose.yml | View secret |
🛠 Guidelines to remediate hardcoded secrets
-
Revoke and rotate the secret.
-
If possible, rewrite git history with
git commit --amendandgit push --force.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Contributor
|
Size Change: -3.67 kB (-0.08%) Total Size: 4.36 MB 📦 View Changed
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe collaboration service moves from Hocuspocus to YHub backed by Valkey and PostgreSQL. The former Y provider becomes converter-only. Backend collaboration APIs and websocket-specific document fields are removed. Frontend collaboration uses Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@env.d/production.dist/common`:
- Around line 9-10: Complete the production collaboration route: uncomment and
set COLLABORATION_WS_URL in env.d/production.dist/common at lines 9-10 to the
YHub /ws/docs endpoint; update src/helm/impress/values.yaml lines 53-79 and
templates/ingress_collaboration_ws.yaml so the collaboration ingress targets
YHub, or document the external override explicitly; update
documentation/system-requirements.md line 92 to describe the external port-3002
route and relevant YHub dependencies or sizing.
In `@src/backend/core/management/commands/clean_document.py`:
- Around line 3-10: Update clean_document to remove YHub PostgreSQL rows and
Redis room keys for every document in all_documents, including descendants,
before reporting success. Ensure failures or unavailable atomic cross-store
cleanup cause the command to fail or explicitly mark cleanup incomplete rather
than printing “Done.”
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-collaboration.spec.ts`:
- Around line 42-45: Restore authorization enforcement for active collaboration
sessions by implementing connection invalidation or per-update authorization
checks in the YHub collaboration flow; update
src/frontend/apps/e2e/__tests__/app-impress/doc-collaboration.spec.ts:42-45 to
remove the temporary TODO once enforcement works, and re-enable or replace the
skipped close/reconnect test at
src/frontend/apps/e2e/__tests__/app-impress/doc-collaboration.spec.ts:114-131
with coverage for the new behavior.
In
`@src/frontend/apps/impress/src/features/docs/doc-management/stores/useProviderStore.tsx`:
- Around line 110-123: Update destroyProvider and the useCollaboration cleanup
so the BlockNote collaboration extension is unbound from provider.doc before
provider.doc.destroy() runs. Ensure document destruction occurs from the
useCollaboration unmount cleanup after unbinding, while preserving provider
awareness and connection cleanup behavior.
In `@src/yhub-server/Dockerfile`:
- Around line 1-13: Update the yhub Dockerfile to create and select a dedicated
non-root user before CMD, ensuring the application runs without root privileges.
Add a HEALTHCHECK before CMD that probes the exposed port or service endpoint
and reports failure when the yhub process is unresponsive.
In `@src/yhub-server/server.js`:
- Around line 12-20: Validate REDIS and POSTGRES immediately after their
configuration constants are initialized, and fail startup with a clear error
identifying whichever required variable is missing before calling createYHub.
Preserve the existing defaults and startup flow when both values are present.
- Around line 42-64: Update readAuthInfo so only an explicit authentication
failure response, such as HTTP 401 from backendFetch, returns the cookie-derived
anon identity. Preserve propagation or distinct handling for network errors, 5xx
responses, and other non-authentication failures so they cannot silently become
anonymous users.
- Line 21: Update the Y_PROVIDER_API_KEY initialization to require an explicitly
configured secret rather than passing the hardcoded fallback to secret. Ensure
startup fails loudly when neither Y_PROVIDER_API_KEY nor Y_PROVIDER_API_KEY_FILE
is set, while preserving the X-Y-Provider-Key authentication flow.
- Around line 26-38: Update backendFetch to enforce a finite request timeout by
creating an AbortController, scheduling its abort, and passing its signal to
fetch; clear the timeout after fetch completes while preserving the existing
response validation and JSON return behavior.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8dfb28b1-7431-4a43-9667-9866e60dc88e
⛔ Files ignored due to path filters (2)
src/frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.locksrc/yhub-server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (58)
.gitignoreCHANGELOG.mdMakefilecompose-e2e.ymlcompose.ymldocker/files/yhub/initdb/01-yhub.sqldocumentation/system-requirements.mdenv.d/development/commonenv.d/development/common.e2eenv.d/production.dist/commonsrc/backend/core/management/commands/clean_document.pysrc/backend/core/services/collaboration_services.pysrc/backend/core/tests/documents/test_api_documents_can_edit.pysrc/backend/core/tests/documents/test_api_documents_content_update.pysrc/backend/core/tests/documents/test_api_documents_update.pysrc/backend/core/tests/external_api/test_external_api_documents_accesses.pysrc/backend/core/tests/external_api/test_external_api_documents_link_configuration.pysrc/backend/core/tests/test_services_collaboration_services.pysrc/backend/core/tests/test_tasks_access.pysrc/backend/impress/settings.pysrc/frontend/apps/e2e/.envsrc/frontend/apps/e2e/.env.examplesrc/frontend/apps/e2e/__tests__/app-impress/config.spec.tssrc/frontend/apps/e2e/__tests__/app-impress/doc-collaboration.spec.tssrc/frontend/apps/impress/package.jsonsrc/frontend/apps/impress/src/core/config/hooks/useCollaborationUrl.tsxsrc/frontend/apps/impress/src/features/docs/doc-comments/hooks/useComments.tssrc/frontend/apps/impress/src/features/docs/doc-editor/__tests__/DocEditor.spec.tsxsrc/frontend/apps/impress/src/features/docs/doc-editor/components/BlockNoteEditor.tsxsrc/frontend/apps/impress/src/features/docs/doc-editor/components/DocEditor.tsxsrc/frontend/apps/impress/src/features/docs/doc-editor/hook/useCollaboration.tsxsrc/frontend/apps/impress/src/features/docs/doc-editor/hook/useSaveDoc.tsxsrc/frontend/apps/impress/src/features/docs/doc-management/api/useDuplicateDoc.tsxsrc/frontend/apps/impress/src/features/docs/doc-management/api/useUpdateDoc.tsxsrc/frontend/apps/impress/src/features/docs/doc-management/stores/useProviderStore.tsxsrc/frontend/apps/impress/src/features/docs/doc-versioning/components/ModalConfirmationVersion.tsxsrc/frontend/apps/impress/src/features/right-panel/components/RightPanel.tsxsrc/frontend/apps/impress/src/stores/useBroadcastStore.tsxsrc/frontend/package.jsonsrc/frontend/servers/y-provider/__tests__/collaborationBackend.test.tssrc/frontend/servers/y-provider/__tests__/collaborationResetConnections.test.tssrc/frontend/servers/y-provider/__tests__/getDocumentConnectionInfoHandler.test.tssrc/frontend/servers/y-provider/__tests__/hocuspocusWS.test.tssrc/frontend/servers/y-provider/package.jsonsrc/frontend/servers/y-provider/src/api/collaborationBackend.tssrc/frontend/servers/y-provider/src/env.tssrc/frontend/servers/y-provider/src/handlers/collaborationResetConnectionsHandler.tssrc/frontend/servers/y-provider/src/handlers/collaborationWSHandler.tssrc/frontend/servers/y-provider/src/handlers/getDocumentConnectionInfoHandler.tssrc/frontend/servers/y-provider/src/handlers/index.tssrc/frontend/servers/y-provider/src/middlewares.tssrc/frontend/servers/y-provider/src/routes.tssrc/frontend/servers/y-provider/src/servers/appServer.tssrc/frontend/servers/y-provider/src/servers/hocuspocusServer.tssrc/frontend/servers/y-provider/src/servers/index.tssrc/yhub-server/Dockerfilesrc/yhub-server/package.jsonsrc/yhub-server/server.js
💤 Files with no reviewable changes (17)
- env.d/development/common.e2e
- src/frontend/servers/y-provider/src/servers/index.ts
- src/frontend/servers/y-provider/src/handlers/collaborationWSHandler.ts
- src/frontend/servers/y-provider/src/handlers/index.ts
- src/frontend/servers/y-provider/src/routes.ts
- src/backend/core/tests/external_api/test_external_api_documents_link_configuration.py
- src/frontend/servers/y-provider/src/handlers/collaborationResetConnectionsHandler.ts
- src/frontend/servers/y-provider/tests/collaborationBackend.test.ts
- src/frontend/servers/y-provider/tests/collaborationResetConnections.test.ts
- src/frontend/servers/y-provider/src/middlewares.ts
- src/frontend/servers/y-provider/tests/hocuspocusWS.test.ts
- src/frontend/servers/y-provider/src/env.ts
- src/frontend/servers/y-provider/src/api/collaborationBackend.ts
- src/frontend/servers/y-provider/src/servers/hocuspocusServer.ts
- src/backend/core/tests/test_tasks_access.py
- src/frontend/servers/y-provider/src/handlers/getDocumentConnectionInfoHandler.ts
- src/frontend/servers/y-provider/tests/getDocumentConnectionInfoHandler.test.ts
Comment on lines
+9
to
10
| # TODO(yhub): route is /ws/docs once prod ingress is migrated | ||
| #COLLABORATION_WS_URL=wss://${DOCS_HOST}/collaboration/ws/ No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate production deployment configuration and confirm that YHub is routed
# to /ws/docs and exposed to the frontend configuration.
rg -n -C 3 \
'COLLABORATION_WS_URL|YPROVIDER_HOST|yhub|/ws/docs|/collaboration/ws' \
.Repository: suitenumerique/docs
Length of output: 49692
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Helm collaboration ingress values"
sed -n '50,75p' src/helm/impress/values.yaml
echo
echo "## Helm charts mentioning yProvider / yhub / collaboration"
rg -n "yProvider|y-provider|yhub|yhub-service|ingressCollaborationWS|COLLABORATION_WS_URL|yHub|Y_HUB|YHUB" src/helm docs documentation CHANGELOG.md env.d src/backend src/frontend -g '*.yaml' -g '*.yml' -g '*.md' -g '*.py' -g '*.tsx' -g 'Dockerfile*' | head -200
echo
echo "## Frontend fallback logic context"
sed -n '1,35p' src/frontend/apps/impress/src/core/config/hooks/useCollaborationUrl.tsx
echo
echo "## Backend API config exposure"
sed -n '3060,3105p' src/backend/core/api/viewsets.pyRepository: suitenumerique/docs
Length of output: 28957
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Helm ingressCollaborationWS templates"
fd -a 'ingress.*collaboration.*|.*collaboration.*ingress.*' src/helm/impress/templates -x sh -c 'echo "--- $1"; sed -n "1,130p" "$1"' sh {} \;
echo
echo "## Helm chart yhub service/service references"
rg -n "yhub|yHub|3002|ws/docs|COLLABORATION_WS_URL|yProvider" src/helm src/helm/impress/templates src/helm/impress/templates/* -g '*.yaml' -g '*.yml' -g '*.tpl' -g '*.go'
echo
echo "## Deterministic config-fallback behavior probe"
node - <<'JS'
function frontendCollaborationUrl(config, windowHost) {
let backend = null; // backend config not modeled here; frontend gets COLLABORATION_WS_URL
if (backend && backend.COLLABORATION_WS_URL) return backend.COLLABORATION_WS_URL;
if (windowHost) return `wss://${windowHost}/ws/docs`;
return '';
}
const cases = [
[{COLLABORATION_WS_URL: undefined}, 'prod.example.com'],
[{COLLABORATION_WS_URL: ''}, 'prod.example.com'],
[{COLLABORATION_WS_URL: 'wss://prod.example.com/ws/docs'}, 'prod.example.com'],
];
for (const [config, host] of cases) {
console.log(JSON.stringify({config, host, result: frontendCollaborationUrl(config, host)}));
}
JSRepository: suitenumerique/docs
Length of output: 49302
Complete the production YHub route before release.
Default Helm values disable ingressCollaborationWS, but this chart still creates a /collaboration/ws/ Ingress that forwards to y-provider instead of YHub. The production env template also leaves COLLABORATION_WS_URL commented, so frontend clients can fall back to wss://${host}/ws/docs. If production is not using this chart or an equivalent custom route, production collaboration will fail unless COLLABORATION_WS_URL is provided externally.
env.d/production.dist/common#L9-L10: add the productionCOLLABORATION_WS_URL=wss://${DOCS_HOST}/ws/docsafter the ingress route is ready.src/helm/impress/values.yaml#L53-L79/src/helm/impress/templates/ingress_collaboration_ws.yaml: update the collaboration WS ingress/backend to YHub, or explicitly document when/where the route is overridden.documentation/system-requirements.md#L92-L92: update the row for port3002to mention the external route and any YHub sizing/dependency guidance if it belongs here.
📍 Affects 2 files
env.d/production.dist/common#L9-L10(this comment)documentation/system-requirements.md#L92-L92
🤖 Prompt for 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.
In `@env.d/production.dist/common` around lines 9 - 10, Complete the production
collaboration route: uncomment and set COLLABORATION_WS_URL in
env.d/production.dist/common at lines 9-10 to the YHub /ws/docs endpoint; update
src/helm/impress/values.yaml lines 53-79 and
templates/ingress_collaboration_ws.yaml so the collaboration ingress targets
YHub, or document the external override explicitly; update
documentation/system-requirements.md line 92 to describe the external port-3002
route and relevant YHub dependencies or sizing.
Comment on lines
+3
to
+10
| # TODO(yhub): this sandbox reset no longer erases the document content. It purges | ||
| # the S3 versions, but yhub durably retains the Yjs document in its own Postgres | ||
| # and re-serves it on the next websocket connect (CRDT merge with the empty | ||
| # seed resurrects the purged content). yhub has no delete API; until it grows | ||
| # one, the interim remediation is to run, against yhub's stores: | ||
| # DELETE FROM yhub_ydoc_v1 WHERE org='docs' AND docid='<document_id>'; | ||
| # and drop the `yhub:room:docs:<document_id>:*` redis keys. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make clean_document remove YHub state for every cleaned document.
The command deletes S3 content but leaves the root YHub document available for CRDT resurrection. It also deletes descendant Django records without deleting their YHub rows.
Before this command reports success, delete the YHub PostgreSQL rows and Redis room keys for all_documents. If atomic cross-store cleanup is not possible, fail the command or mark the cleanup incomplete instead of reporting Done.
🤖 Prompt for 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.
In `@src/backend/core/management/commands/clean_document.py` around lines 3 - 10,
Update clean_document to remove YHub PostgreSQL rows and Redis room keys for
every document in all_documents, including descendants, before reporting
success. Ensure failures or unavailable atomic cross-store cleanup cause the
command to fail or explicitly mark cleanup incomplete rather than printing
“Done.”
Comment on lines
+42
to
+45
| // TODO(yhub): re-add the close/reconnect check (the backend closed the | ||
| // connection when the doc visibility changed) once yhub exposes a kick | ||
| // API - `reset_connections` is currently a no-op so the server never | ||
| // closes the connection. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Restore authorization enforcement for active collaboration sessions.
YHub currently cannot invalidate an established connection after document authorization changes. The migration also skips the test that would detect this failure. Resolve both before release.
src/frontend/apps/e2e/__tests__/app-impress/doc-collaboration.spec.ts#L42-L45: implement connection invalidation or per-update authorization enforcement when access changes.src/frontend/apps/e2e/__tests__/app-impress/doc-collaboration.spec.ts#L114-L131: re-enable this test, or replace it with an integration test for the new enforcement mechanism.
📍 Affects 1 file
src/frontend/apps/e2e/__tests__/app-impress/doc-collaboration.spec.ts#L42-L45(this comment)src/frontend/apps/e2e/__tests__/app-impress/doc-collaboration.spec.ts#L114-L131
🤖 Prompt for 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.
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-collaboration.spec.ts` around
lines 42 - 45, Restore authorization enforcement for active collaboration
sessions by implementing connection invalidation or per-update authorization
checks in the YHub collaboration flow; update
src/frontend/apps/e2e/__tests__/app-impress/doc-collaboration.spec.ts:42-45 to
remove the temporary TODO once enforcement works, and re-enable or replace the
skipped close/reconnect test at
src/frontend/apps/e2e/__tests__/app-impress/doc-collaboration.spec.ts:114-131
with coverage for the new behavior.
Comment on lines
110
to
+123
| destroyProvider: () => { | ||
| clearTimeout(reconnectTimeout); | ||
| clearTimeout(lostConnectionTimeout); | ||
| const provider = get().provider; | ||
| if (provider) { | ||
| /** | ||
| * destroy() emits 'connection-close' synchronously before removing | ||
| * listeners, which re-arms lostConnectionTimeout: it must be cleared | ||
| * after, or a stale "connection lost" banner flashes on the next doc. | ||
| */ | ||
| provider.destroy(); | ||
| // y-websocket never destroys the awareness: its interval would leak | ||
| provider.awareness.destroy(); | ||
| provider.doc.destroy(); | ||
| } | ||
| clearTimeout(lostConnectionTimeout); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how useCollaboration and DocCoreEditor/BlockNoteEditor are composed in DocEditor.tsx
fd -a DocEditor.tsx src/frontend/apps/impress/src/features/docs/doc-editor
rg -n -B3 -A20 'useCollaboration\(' src/frontend/apps/impress/src/features/docs/doc-editor/components/DocEditor.tsxRepository: suitenumerique/docs
Length of output: 1104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DocEditor outline and relevant contents =="
ast-grep outline src/frontend/apps/impress/src/features/docs/doc-editor/components/DocEditor.tsx || true
wc -l src/frontend/apps/impress/src/features/docs/doc-editor/components/DocEditor.tsx
sed -n '1,260p' src/frontend/apps/impress/src/features/docs/doc-editor/components/DocEditor.tsx
echo
echo "== Collaboration hook =="
fd -a 'useCollaboration.tsx|useCollaboration.ts' src/frontend/apps/impress/src/features/docs/doc-management src/frontend/apps/impress/src/features/docs/doc-editor 2>/dev/null || true
rg -n -B5 -A80 'export function useCollaboration|export const useCollaboration|const useCollaboration|function useCollaboration|destroyProvider|useProviderStore' src/frontend/apps/impress/src/features/docs/doc-management src/frontend/apps/impress/src/features/docs/doc-editor 2>/dev/null || true
echo
echo "== DocCoreEditor and BlockNoteEditor composition details =="
fd -a 'BlockNoteEditor.tsx|DocCoreEditor.tsx|Editor.*\.tsx' src/frontend/apps/impress/src/features/docs 2>/dev/null || true
rg -n -B5 -A40 'DocCoreEditor|BlockNoteEditor|useSaveDoc\(|provider\.doc\.getXmlFragment|useProviderStore|getDocProvider|provider\.doc' src/frontend/apps/impress/src/features/docs/doc-editor src/frontend/apps/impress/src/features/docs/doc-management/store src/frontend/apps/impress/src/features/docs/doc-management/stores src/frontend/apps/impress/src/features/docs 2>/dev/null || trueRepository: suitenumerique/docs
Length of output: 50376
Destroy provider.doc only after unbinding the editor.
DocEditor renders DocCoreEditor after calling useCollaboration(doc.id), and DocEditor has no explicit cleanup before DocCoreEditor unmounts. React unmounts children before parent effects run, so the current useCollaboration cleanup does not guarantee this. Unbind the BlockNote collaboration extension from provider.doc first, then destroy it when useCollaboration unmounts.
🤖 Prompt for 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.
In
`@src/frontend/apps/impress/src/features/docs/doc-management/stores/useProviderStore.tsx`
around lines 110 - 123, Update destroyProvider and the useCollaboration cleanup
so the BlockNote collaboration extension is unbound from provider.doc before
provider.doc.destroy() runs. Ensure document destruction occurs from the
useCollaboration unmount cleanup after unbinding, while preserving provider
awareness and connection cleanup behavior.
Comment on lines
+1
to
+13
| # trixie for glibc >= 2.38 — uws prebuilt binaries reject bookworm's 2.36 | ||
| FROM node:22-trixie AS yhub | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| COPY package.json package-lock.json ./ | ||
| RUN npm ci --omit=dev | ||
|
|
||
| COPY server.js ./ | ||
|
|
||
| EXPOSE 3002 | ||
|
|
||
| CMD ["node", "server.js"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Add a non-root user and a HEALTHCHECK.
The image runs as root because no USER directive is set. Static analysis flags this (CKV_DOCKER_3). Add a dedicated non-root user before the CMD instruction to reduce the container's privilege footprint. Static analysis also flags a missing HEALTHCHECK (CKV_DOCKER_2). Add one so the orchestrator can detect an unresponsive yhub process.
🔒️ Proposed fix
COPY server.js ./
+RUN groupadd --gid 1000 yhub && useradd --uid 1000 --gid yhub --shell /bin/false yhub
+USER yhub
+
EXPOSE 3002
+HEALTHCHECK --interval=30s --timeout=3s CMD node -e "fetch('http://localhost:3002/').then(()=>process.exit(0)).catch(()=>process.exit(1))"
+
CMD ["node", "server.js"]🧰 Tools
🪛 Checkov (3.3.8)
[low] 1-13: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[low] 1-13: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🤖 Prompt for 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.
In `@src/yhub-server/Dockerfile` around lines 1 - 13, Update the yhub Dockerfile
to create and select a dedicated non-root user before CMD, ensuring the
application runs without root privileges. Add a HEALTHCHECK before CMD that
probes the exposed port or service endpoint and reports failure when the yhub
process is unresponsive.
Source: Linters/SAST tools
Comment on lines
+12
to
+20
| const PORT = Number(process.env.PORT || 3002); | ||
| const REDIS = process.env.REDIS; | ||
| const POSTGRES = process.env.POSTGRES; | ||
| const REDIS_PREFIX = process.env.REDIS_PREFIX || 'yhub'; | ||
| const COLLABORATION_BACKEND_BASE_URL = | ||
| process.env.COLLABORATION_BACKEND_BASE_URL || 'http://app-dev:8000'; | ||
| const allowedOrigins = ( | ||
| process.env.COLLABORATION_SERVER_ORIGIN || 'http://localhost:3000' | ||
| ).split(','); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Validate required configuration at startup.
REDIS and POSTGRES have no default and no explicit check before being passed into createYHub (lines 84-98). If either is unset, the failure surfaces deep inside @y/hub rather than as a clear startup error naming the missing variable.
Add an explicit check that fails fast with a clear message when REDIS or POSTGRES is missing.
🤖 Prompt for 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.
In `@src/yhub-server/server.js` around lines 12 - 20, Validate REDIS and POSTGRES
immediately after their configuration constants are initialized, and fail
startup with a clear error identifying whichever required variable is missing
before calling createYHub. Preserve the existing defaults and startup flow when
both values are present.
Signed-off-by: Kevin Jahns <kevin.jahns@protonmail.com>
We want to generate jwt token using the RS256 algotrithm. This token will be used for internal call with the yhub service.
The yhub service will need our public key in order to validate the jwt token we will used. We choose to expose a jwks endpoint as it is a standard wat to do this.
For now the only token we will need is ont with the admin claim set to True. To not repeat the creation of this token again and again, we created a dedicated method to issue this token in the JWTService class.
Thw private key needed to generate a jwt token will be mandatory. In order to ease the development we want to automate its generation
Since we use ruff, it is not needed anymore to check the presence of print statement, the rule T201 is already doing it in a more performant way.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@bin/generate-jwt-private-key.sh`:
- Around line 11-22: Set a restrictive umask before the mkdir and openssl
commands in the key-generation flow, ensuring the directory and private key are
created inaccessible to other users; retain the existing chmod 600 safeguard and
idempotent KEY_PATH check.
In `@src/backend/core/services/jwt_services.py`:
- Around line 84-86: Update the lifetime method to validate that
settings.JWT_TOKEN_LIFETIME is strictly greater than zero before returning it,
and reject zero or negative values so generate_token cannot create tokens with
an invalid exp relative to iat.
In `@src/backend/core/tests/test_api_jwks.py`:
- Around line 106-117: Update the JWKS rotation implementation exercised by
test_api_jwks_follows_the_key_rotation to retain and publish the retired public
key alongside the current key for at least JWT_TOKEN_LIFETIME. Extend the test
to create and preserve a token before changing JWT_PRIVATE_KEY, then verify that
token with the post-rotation JWKS while retaining the existing new-token
validation.
🪄 Autofix
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 29e129f4-04fd-4bfa-a317-f0f3c99106e1
⛔ Files ignored due to path filters (1)
src/backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/impress.ymlCHANGELOG.mdMakefileUPGRADE.mdbin/_config.shbin/generate-jwt-private-key.shcompose.ymldocumentation/env.mddocumentation/resource_server.mdenv.d/development/commonsrc/backend/core/api/viewsets.pysrc/backend/core/services/jwt_services.pysrc/backend/core/tests/test_api_jwks.pysrc/backend/core/tests/test_services_jwt_services.pysrc/backend/core/urls.pysrc/backend/impress/settings.pysrc/backend/pyproject.toml
💤 Files with no reviewable changes (1)
- .github/workflows/impress.yml
Comment on lines
+11
to
+22
| set -eo pipefail | ||
|
|
||
| REPO_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" | ||
| KEY_PATH="${REPO_DIR}/data/jwt/private.pem" | ||
|
|
||
| if [ -f "${KEY_PATH}" ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| mkdir -p "$(dirname "${KEY_PATH}")" | ||
| openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:2048 -out "${KEY_PATH}" 2>/dev/null | ||
| chmod 600 "${KEY_PATH}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Sensitive Data Exposure (CWE-732): Incorrect Permission Assignment for Critical Resource
Reachability: Internal
Create the private key with restrictive permissions.
If a shared development host uses a permissive umask and another user can traverse data/jwt, line 21 creates the private key before line 22 restricts it. A local attacker can read the key during that interval and mint JWTs accepted by yhub. Set umask 077 before creating the directory and key.
Proposed fix
set -eo pipefail
+umask 077
REPO_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"#!/usr/bin/env bash
set -euo pipefail
tmp_dir="$(mktemp -d)"
trap 'rm -rf "${tmp_dir}"' EXIT
(
umask 0022
openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:2048 \
-out "${tmp_dir}/private.pem" 2>/dev/null
stat -c 'created mode: %a' "${tmp_dir}/private.pem"
)🤖 Prompt for 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.
In `@bin/generate-jwt-private-key.sh` around lines 11 - 22, Set a restrictive
umask before the mkdir and openssl commands in the key-generation flow, ensuring
the directory and private key are created inaccessible to other users; retain
the existing chmod 600 safeguard and idempotent KEY_PATH check.
Comment on lines
+84
to
+86
| def lifetime(self): | ||
| """Return the token lifetime, in seconds.""" | ||
| return settings.JWT_TOKEN_LIFETIME |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-positive JWT lifetimes.
Line 86 returns zero or negative values without validation. generate_token then creates a token whose exp is at or before iat. yhub and external validators reject that token. Validate lifetime > 0 before use.
Proposed fix
def lifetime(self):
"""Return the token lifetime, in seconds."""
- return settings.JWT_TOKEN_LIFETIME
+ lifetime = settings.JWT_TOKEN_LIFETIME
+ if lifetime <= 0:
+ raise ConfigurationError("JWT_TOKEN_LIFETIME must be greater than zero.")
+ return lifetime🤖 Prompt for 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.
In `@src/backend/core/services/jwt_services.py` around lines 84 - 86, Update the
lifetime method to validate that settings.JWT_TOKEN_LIFETIME is strictly greater
than zero before returning it, and reject zero or negative values so
generate_token cannot create tokens with an invalid exp relative to iat.
Comment on lines
+106
to
+117
| def test_api_jwks_follows_the_key_rotation(jwt_settings): | ||
| """After a rotation, the JWKS validates the tokens signed with the new key.""" | ||
| first_jwks = APIClient().get("/api/v1.0/jwks").json() | ||
|
|
||
| jwt_settings.JWT_PRIVATE_KEY = OTHER_PRIVATE_KEY | ||
| token = JWTService().get_token({"sub": "user-id"}) | ||
| second_jwks = APIClient().get("/api/v1.0/jwks").json() | ||
|
|
||
| assert first_jwks != second_jwks | ||
|
|
||
| key = jwt.PyJWKSet.from_dict(second_jwks).keys[0] | ||
| assert jwt.decode(token, key, algorithms=["RS256"])["sub"] == "user-id" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep the retired public key available during key rotation.
The JWKS contains one key and replaces it immediately after rotation. Tokens signed before rotation remain valid for JWT_TOKEN_LIFETIME, but a consumer that refreshes this JWKS can no longer verify them.
Publish the current and retired public keys during an overlap period of at least the maximum token lifetime. Add a test that verifies a pre-rotation token with the post-rotation JWKS.
🤖 Prompt for 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.
In `@src/backend/core/tests/test_api_jwks.py` around lines 106 - 117, Update the
JWKS rotation implementation exercised by test_api_jwks_follows_the_key_rotation
to retain and publish the retired public key alongside the current key for at
least JWT_TOKEN_LIFETIME. Extend the test to create and preserve a token before
changing JWT_PRIVATE_KEY, then verify that token with the post-rotation JWKS
while retaining the existing new-token validation.
The CollaborationService was doing nothing since we started the migration to yhub, all the code using it is now removed. Also the `can-edit` endpoint and all the safeguard mechanism relying on the presence of other users connected to the websocket will not be used anymore, it will be possible to replace all of this with yhub, so all this code is also removed.
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 `@documentation/collaboration.md`:
- Around line 3-8: Update the collaboration paragraph to refer to the “yhub
service” instead of the “y-provider service,” while keeping the existing
COLLABORATION_BACKEND_BASE_URL and COLLABORATION_SERVER_ORIGIN configuration
details unchanged.
🪄 Autofix
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a0db8c39-75ab-47a1-9c93-f4897fd1486b
📒 Files selected for processing (26)
CHANGELOG.mddocumentation/collaboration.mddocumentation/env.mddocumentation/examples/helm/impress.values.yamlenv.d/development/commonenv.d/production.dist/yprovidersrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets.pysrc/backend/core/models.pysrc/backend/core/services/collaboration_services.pysrc/backend/core/tasks/access.pysrc/backend/core/tests/documents/test_api_document_accesses.pysrc/backend/core/tests/documents/test_api_documents_can_edit.pysrc/backend/core/tests/documents/test_api_documents_content_update.pysrc/backend/core/tests/documents/test_api_documents_link_configuration.pysrc/backend/core/tests/documents/test_api_documents_retrieve.pysrc/backend/core/tests/documents/test_api_documents_trashbin.pysrc/backend/core/tests/documents/test_api_documents_update.pysrc/backend/core/tests/external_api/test_external_api_documents_link_configuration.pysrc/backend/core/tests/test_api_config.pysrc/backend/core/tests/test_models_documents.pysrc/backend/core/tests/test_services_collaboration_services.pysrc/backend/core/tests/test_tasks_access.pysrc/backend/impress/settings.pysrc/helm/env.d/dev/values.impress.yaml.gotmplsrc/helm/env.d/feature/values.impress.yaml.gotmpl
💤 Files with no reviewable changes (18)
- src/helm/env.d/feature/values.impress.yaml.gotmpl
- env.d/production.dist/yprovider
- src/backend/core/tests/documents/test_api_documents_retrieve.py
- src/backend/core/models.py
- documentation/examples/helm/impress.values.yaml
- src/backend/core/tests/documents/test_api_documents_trashbin.py
- env.d/development/common
- src/backend/core/tasks/access.py
- documentation/env.md
- src/backend/core/tests/test_tasks_access.py
- src/backend/core/api/serializers.py
- src/backend/core/tests/documents/test_api_documents_can_edit.py
- src/backend/impress/settings.py
- src/helm/env.d/dev/values.impress.yaml.gotmpl
- src/backend/core/tests/test_models_documents.py
- src/backend/core/services/collaboration_services.py
- src/backend/core/tests/test_api_config.py
- src/backend/core/tests/test_services_collaboration_services.py
Comment on lines
+3
to
8
| By default with Docs, collaboration is enabled. To allow the collaboration between users, a connection to a websocket server is made (the y-provider service), you only have to configure the Django backend URL and the allowed origin in your y-provider service: | ||
|
|
||
| ```yaml | ||
| COLLABORATION_BACKEND_BASE_URL: https://{yourdocsdomain.tld} | ||
| COLLABORATION_SERVER_ORIGIN: https://{yourdocsdomain.tld} | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Collect the collaboration/yhub configuration variable names declared in the repository.
set -euo pipefail
fd -t f 'server.js|README.md' src/yhub-server --exec rg -n 'process\.env\.[A-Z_]+|COLLABORATION_[A-Z_]+' {}
fd -t f -g 'compose*.yml' . --exec rg -n 'yhub|COLLABORATION_[A-Z_]+' {}
rg -n 'COLLABORATION_[A-Z_]+' documentation env.dRepository: suitenumerique/docs
Length of output: 1499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant docs =="
sed -n '1,40p' documentation/collaboration.md
echo
echo "== changelog references =="
rg -n -C 3 'y-provider|yhub|COLLABORATION_SERVER_ORIGIN|COLLABORATION_BACKEND_BASE_URL|/collaboration/ws/' . -g '!node_modules' -g '!dist' -g '!build' | head -n 200
echo
echo "== documentation exact context =="
sed -n '1,12p' documentation/collaboration.mdRepository: suitenumerique/docs
Length of output: 12979
Update the collaboration docs to yhub.
Replace “y-provider service” with “yhub service” in this paragraph, because yhub-server/server.js reads these variables: COLLABORATION_BACKEND_BASE_URL and COLLABORATION_SERVER_ORIGIN (secret-file variants are supported for env var names such as Y_PROVIDER_API_KEY, but COLLABORATION_* can be set directly).
🤖 Prompt for 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.
In `@documentation/collaboration.md` around lines 3 - 8, Update the collaboration
paragraph to refer to the “yhub service” instead of the “y-provider service,”
while keeping the existing COLLABORATION_BACKEND_BASE_URL and
COLLABORATION_SERVER_ORIGIN configuration details unchanged.
Add POST /collaboration/reset-connections/v1/{org}/{docid} (optional
X-User-Id header) to yhub-server. It distributes yhub recheckAuth: every
server re-runs the access check per matching connection and closes only
those whose access actually changed (close code 4401), so unaffected
clients see no reconnect churn. The endpoint authenticates with the
RS256 admin JWT issued by JWTService, verified against the backend JWKS
(new jose dependency); the admin token acts as the "system" user and is
the only principal granted the reset-connections access purpose. The
backend does not trigger it on permission changes yet - that wiring
comes separately, now that CollaborationService is gone.
yhub is upgraded to 0.4.0 and serves every route under the
/collaboration/ prefix (server.apiPrefix): the websocket moves to
/collaboration/ws/v1/docs, and the built-in document apis are meant to
be publicly exposed alongside it, with reset-connections as the one
backend-internal exception.
Also harden websocket auth: fail closed when the backend errors (only a
genuine 401/403 falls back to the anonymous identity, so a signed-in
editor can never hide from a targeted recheck under an anon userid) and
tolerate small clock skew when verifying the cached admin token.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Kevin Jahns <kevin.jahns@protonmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
43-46: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBlock rollout until Helm routes collaboration to YHub.
The converter-only
lasuite/impress-y-providerimage removes the collaboration WebSocket. Existing Helm values therefore lose collaboration unless the chart routes/collaboration/to YHub. Update the chart and values before rollout, or add a deployment guard.🤖 Prompt for 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. In `@CHANGELOG.md` around lines 43 - 46, Block rollout of the converter-only y-provider image until the Helm chart routes /collaboration/ to YHub, updating the chart and values accordingly; alternatively, add a deployment guard that prevents configurations still routing collaboration to the y-provider image.
🤖 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 `@src/frontend/apps/impress/src/core/config/hooks/useCollaborationUrl.tsx`:
- Around line 12-16: Verify the runtime configuration wiring for
COLLABORATION_WS_URL in production and ensure it is always exposed to the
client; if it is not, add the required production ingress route for the fallback
used by the collaboration URL hook before release.
In `@src/yhub-server/README.md`:
- Around line 14-18: Use the exact collaboration route parameter name `{docid}`
consistently in the documentation. Update CHANGELOG.md lines 11-14 and 17-21 to
replace `{id}` with `{docid}` for the reset-connections and websocket routes;
src/yhub-server/README.md lines 14-18 requires no direct change and documents
the correct contract.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 43-46: Block rollout of the converter-only y-provider image until
the Helm chart routes /collaboration/ to YHub, updating the chart and values
accordingly; alternatively, add a deployment guard that prevents configurations
still routing collaboration to the y-provider image.
🪄 Autofix
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 00971b27-32ee-4038-811a-17ac39a1504e
⛔ Files ignored due to path filters (1)
src/yhub-server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
CHANGELOG.mdenv.d/development/commonsrc/frontend/apps/impress/src/core/config/hooks/useCollaborationUrl.tsxsrc/yhub-server/README.mdsrc/yhub-server/package.jsonsrc/yhub-server/server.js
Comment on lines
12
to
+16
| conf?.COLLABORATION_WS_URL || | ||
| (typeof window !== 'undefined' | ||
| ? `wss://${window.location.host}/collaboration/ws/` | ||
| : ''); | ||
|
|
||
| return `${base}?room=${room}`; | ||
| ? // TODO(yhub): no prod ingress route yet | ||
| `wss://${window.location.host}/collaboration/ws/v1/docs` | ||
| : '') |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
Verify the production fallback path.
The fallback uses /collaboration/ws/v1/docs, but the code states that production ingress does not provide this route. If COLLABORATION_WS_URL is absent from runtime configuration, collaboration cannot connect.
Confirm that production always exposes COLLABORATION_WS_URL to the client. Otherwise, add the ingress route before release.
🤖 Prompt for 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.
In `@src/frontend/apps/impress/src/core/config/hooks/useCollaborationUrl.tsx`
around lines 12 - 16, Verify the runtime configuration wiring for
COLLABORATION_WS_URL in production and ensure it is always exposed to the
client; if it is not, add the required production ingress route for the fallback
used by the collaboration URL hook before release.
Comment on lines
+14
to
+18
| - serves every route under the `/collaboration/` prefix | ||
| (`server.apiPrefix`), including the websocket sync route | ||
| `/collaboration/ws/v1/{org}/{docid}`, | ||
| - exposes `POST /collaboration/reset-connections/v1/{org}/{docid}` (optional | ||
| `X-User-Id` header), for the Django backend to re-check the authorization |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'reset-connections|ws/v1|apiPrefix' \
src/yhub-server/server.js src/yhub-server/README.md CHANGELOG.md src/backendRepository: suitenumerique/docs
Length of output: 7898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)server\.js$|(^|/)README\.md$|^CHANGELOG\.md$|src/backend' | sed -n '1,120p'
echo
echo "== reset-connections and ws route references =="
rg -n -C 3 'reset-connections|ws/v1|ws/v1\/docs|/collaboration/ws/v1|apiPrefix' \
CHANGELOG.md src/yhub-server src/backend compose.yml docker-compose*.yml -g '!node_modules' -g '!dist' || true
echo
echo "== exact server.js relevant section =="
sed -n '145,165p' src/yhub-server/server.jsRepository: suitenumerique/docs
Length of output: 14380
Use one exact collaboration route contract.
src/yhub-server/server.js registers POST /collaboration/reset-connections/v1/{org}/{docid} and GET /collaboration/ws/v1/{org}/{docid}. Update CHANGELOG.md#L11-L14 and CHANGELOG.md#L17-L21 to match that path shape instead of using the {id} path parameter in changelog docs.
📍 Affects 2 files
src/yhub-server/README.md#L14-L18(this comment)CHANGELOG.md#L11-L14CHANGELOG.md#L17-L21
🤖 Prompt for 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.
In `@src/yhub-server/README.md` around lines 14 - 18, Use the exact collaboration
route parameter name `{docid}` consistently in the documentation. Update
CHANGELOG.md lines 11-14 and 17-21 to replace `{id}` with `{docid}` for the
reset-connections and websocket routes; src/yhub-server/README.md lines 14-18
requires no direct change and documents the correct contract.
The Y_PROVIDER_API_KEY shared secret is replaced by a signed admin JWT when Django calls the y-provider conversion endpoint.
The /api/convert route no longer accepts the Y_PROVIDER_API_KEY shared secret. It now verifies the admin JWT signed by Django against the JWKS published on its /api/v1.0/jwks endpoint.
Y_PROVIDER_API_KEY is no longer used in the codebase, so we can remove it from the helm chart and the documentation. We adapt the documentation to use the new JWT conversion mechanism instead.
Add audience to the jwt, scoping the token to it prevents an admin JWT issued for another backend service from being replayed against y-provider.
Python cannot call yhub's built-in PATCH ydoc api because its body must
be lib0-any encoded - a lib0-specific binary framing with no
implementation outside javascript. The new endpoint
POST /collaboration/create-ydoc/v1/{org}/{docid} accepts the raw binary
Yjs update (pycrdt get_update() / Y.encodeStateAsUpdate output) as
application/octet-stream, so Django can seed a document's initial state
with a plain requests.post(url, data=raw_bytes) - needed by the
server-side creation flows (file import, create-for-owner, duplication,
template instantiation) whose yhub rooms currently stay empty until the
first browser connects.
Strict create semantics: 409 when the room already has content
(checked via getDoc, covering persisted state and uncompacted stream
messages; yhub has no atomic create, concurrent creates merge via CRDT
and never corrupt). The initial content is attributed to the optional
X-User-Id header, else to the caller's identity. Access uses the
default purpose, i.e. standard document write access like the built-in
ydoc routes: the admin JWT, or a user session with update ability.
Malformed updates map to 400 (the compute worker rejects them and the
pool replaces the thread), empty updates to 400, bodies over 10MiB to
413.
Gotcha worth noting: req.bytes() resolves to a Node Buffer, but yhub's
compute-task schema validates with lib0's exact-constructor Uint8Array
check, so the body is re-viewed as a plain Uint8Array before it is
handed to the compute pool.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Kevin Jahns <kevin.jahns@protonmail.com>
Address the findings of an adversarial review of the new endpoint: - Only the backend admin token may attribute content to another user via the X-User-Id header. The endpoint uses the default access purpose, so any editor with update ability can call it — honoring the header for them would let an editor forge the attribution history of the first revision (the websocket path likewise stamps the server-side identity). Regular callers now always author as themselves; verified: an editor session posting X-User-Id gets its own userid stamped. - Reject non-main ?branch= requests (400). Cookie users are main-only via getAccessType, but the admin token bypasses it and could seed an orphan (org, docid, branch) room no user-facing path reads — while dodging the branch-scoped 409 existence check. - Correct the concurrent-create comment: two racing creates merge as independently generated updates (fresh clientIDs), so the seeded content appears twice — user-visible duplication, not merely a doubly-attributed revision. Still accepted (Django creates each doc once and a duplicated seed is user-fixable), but the tradeoff is now stated accurately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Kevin Jahns <kevin.jahns@protonmail.com>
We will not block anymore the users not connected to the collaboration server from editing the document, we will have an HTTP fallback instead, so we can remove the "can-edit" mechanism and the related code.
COLLABORATION_SERVER_SECRET is no longer used in the codebase, so we can remove it from the codebase.
We remove the code related to the content GET and PATCH endpoints, as they are no longer used in the codebase. The yhub server will handle the content management directly, providing the content and managing the updates. This change simplifies the code and reduces the complexity of the frontend application. We will need to reimplement the saving mechanism in the service worker when we are offline. Let's wait that the service is fully developed on the yhub side before we implement this feature.
The Playwright MCP browser session writes snapshots and console logs into .playwright-mcp/ at the repository root while driving the app during development — keep them out of version control. Signed-off-by: Kevin Jahns <kevin.jahns@protonmail.com>
With SOFT_MIGRATION=true, the first access to a document yhub does not
know yet fetches the legacy snapshot from Django's S3 media bucket
({id}/file, UTF-8 base64 of a raw Yjs update), seeds the room through
the compute pool - attributed to "system" with a migration=s3 custom
attribution - and only then admits the connection, so the initial sync
always includes the seed. Now that the frontend no longer bootstraps
rooms client-side (content GET/PATCH removal), this is the only path
that brings legacy content into yhub; keep the flag on until a batch
backfill has migrated the full corpus.
A missing S3 object is the brand-new-document case and yields an empty
room; every real failure fails closed (opaque 401, y-websocket retries
with backoff). Existence is probed postgres-first (bare SELECT, then
the valkey stream, then the SELECT again to close the compaction
race). Guard rails: a per-docid verdict cache (poison objects cannot
sustain an S3 retry storm, transient errors expire in 15s, per-replica
seed backpressure denies once without caching), in-flight dedup, a
token-owned cross-replica valkey lock released by compare-and-delete,
a 10s S3 fetch timeout that also destroys a late-arriving response
stream, and the same 10MiB decoded cap as create-ydoc. Concurrent
seeds stay correct regardless: the frozen snapshots share one Yjs
lineage, so duplicates merge as CRDT no-ops.
Also reject non-lowercase docids (Django serializes UUIDs lowercase; a
case variant would open a parallel room and miss its S3 object) and
refuse to boot when AWS_S3_ENDPOINT_URL carries a path the minio
client cannot address. On AWS the read-only credentials must include
s3:ListBucket so a missing object surfaces as NoSuchKey rather than
AccessDenied - see the README for the full guarantees and ops notes.
Signed-off-by: Kevin Jahns <kevin.jahns@protonmail.com>
The hocuspocus-to-yhub migration entry claimed both the kick and get-connections APIs were deferred pending a yhub equivalent. The kick flow now has its server-side replacement (the reset-connections endpoint, backend wiring pending), and get-connections lost its only consumer when the can-edit mechanism was removed — it is dropped, not awaiting reimplementation. Signed-off-by: Kevin Jahns <kevin.jahns@protonmail.com>
yhub verified Django's RS256 admin JWT without checking "aud", so the y-converter token Django hands to the converter process was replayable here — and admin: true short-circuits getAccessType to "rw" on every document, plus the backend-internal reset-connections purpose and the X-User-Id attribution override. Require aud: "yhub", as y-provider already does for its own audience. Nothing in the backend calls yhub's admin endpoints yet, so no caller is affected. Signed-off-by: Kevin Jahns <kevin.jahns@protonmail.com>
Add POST /collaboration/migrate/v1/docs/{id}, which replays every S3 version
of a document's legacy `{id}/file` object into one gc:false Yjs document and
stores it as a single row at clock 0, crediting each version with its own S3
timestamp. Nothing existing is deleted and nothing goes on the stream, so the
next compaction merges that row like any other. The clock-0 insert is ON
CONFLICT DO NOTHING and migrated ids are kept in a valkey set, so the endpoint
is idempotent without a lock. The activity api then reports the same timeline
as the backend's /documents/{id}/versions/, instead of the single
migration-time change the lazy soft migration leaves behind.
That lazy seed now writes no insertAt/deleteAt. Persisted contentmaps are
merged rather than de-duplicated, so a seed timestamp would survive next to
the real per-version one on the same ids and the activity api would report
whichever the unordered row scan put last. A seed is not an editing event and
has no honest time to report.
Upgrade yhub to 0.5.0, where error codes encode retry semantics (4xx
permanent, 5xx and 429 retryable) and auth plugins may throw apiError(503). A
temporarily unreachable Django backend, JWKS endpoint or legacy S3 store is
now reported as 503 rather than denied like a permission failure, so clients
retry instead of giving up.
The legacy-store code moves out of server.js into migration.js, with the
shared *_FILE secret helper in env.js.
Signed-off-by: Kevin Jahns <kevin.jahns@protonmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Switch to yhub as the collaborative backend.
Note that a large chunk of this code was AI generated. I open this PR to work more closely together with others.
Remaining issues
(incomplete list)
General requirements
CI requirements
git commit --signoff(DCO compliance)git commit -S)<gitmoji>(type) title description## [Unreleased]section (if noticeable change)AI requirements