feat(server): add a self-hostable projects API and collaboration relay - #1690
Merged
Conversation
Sharing, accounts, and live collaboration could only ever run on share.geolibre.app and Cloudflare, so a self-hoster running the Docker image got the map and the sidecar but none of it. Write the projects and identity contract down, add a FastAPI reference implementation of it, lift the collab session logic out of the Durable Object into a shared core, and host that core from a plain Node relay as well. One conformance suite runs against both relays so their permission behavior cannot drift. Closes #1685 Closes #1686
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR adds a self-hostable FastAPI projects API, a SQLite-backed Node collaboration relay, shared collaboration contracts and validation, Docker Compose deployment, documentation, and CI coverage. ChangesProjects and identity API
Self-hosted collaboration relay
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Contributor
🔍 Cloudflare PR preview
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a documented, self-hostable “projects + identity” HTTP API and a Docker-friendly collaboration relay, while extracting the shared collaboration session policy/validation logic into a transport-agnostic core package so the Cloudflare Worker and Node relay stay behaviorally consistent.
Changes:
- Introduces
backend/geolibre_server_api(FastAPI) as a reference implementation of the projects/identity contract and documents the contract indocs/server-api.md. - Extracts collaboration session logic into
@geolibre/collab-coreand updates the Cloudflare Worker to consume it; addsworkers/collab-node(Node + SQLite) as a self-hostable relay. - Adds
docker-compose.ymland updates docs/CI to support running/testing the new services.
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| workers/collab/src/session.ts | Switches Worker session logic to shared @geolibre/collab-core helpers/constants. |
| workers/collab/src/protocol.ts | Re-exports the wire protocol from @geolibre/collab-core. |
| workers/collab/src/comment-validate.ts | Re-exports comment validators from @geolibre/collab-core. |
| workers/collab/package.json | Adds @geolibre/collab-core dependency for the Worker. |
| workers/collab-node/tsconfig.json | Adds TypeScript config for the Node relay workspace. |
| workers/collab-node/test/relay.test.ts | Adds Node relay integration tests over real HTTP/WebSocket flows. |
| workers/collab-node/src/store.ts | Implements SQLite persistence for sessions/snapshots/chat. |
| workers/collab-node/src/server.ts | Implements HTTP + WebSocket Node relay using @geolibre/collab-core. |
| workers/collab-node/README.md | Documents running/configuring the Node relay. |
| workers/collab-node/package.json | Defines build/test/start scripts and deps for the Node relay. |
| workers/collab-node/Dockerfile | Provides a container build/runtime for the Node relay. |
| tests/collab-core-conformance.test.ts | Adds shared policy conformance tests to prevent host drift. |
| packages/collab-core/tsconfig.json | Adds TypeScript config for the extracted collaboration core package. |
| packages/collab-core/src/session.ts | Defines shared constants + permission/override/sanitization helpers. |
| packages/collab-core/src/protocol.ts | Houses the shared wire protocol types. |
| packages/collab-core/src/index.ts | Barrel export for @geolibre/collab-core. |
| packages/collab-core/src/comment-validate.ts | Houses shared comment-mutation validators and limits. |
| packages/collab-core/package.json | Adds the new private workspace package and export map. |
| package.json | Extends test:worker to include Node relay typecheck + tests. |
| package-lock.json | Records workspace links and dependency updates for the new packages. |
| docs/server-api.md | Documents the v1 projects/identity API contract for compatible servers. |
| docs/getting-started.md | Updates self-hosting guidance to use the new Compose setup and server docs. |
| docs/collaboration.md | Documents the two relay hosts and how to run/test the Node relay. |
| Dockerfile | Ensures the web build includes the new collab-core workspace metadata. |
| docker-compose.yml | Adds Compose stack for web + projects server + collab relay + Postgres. |
| backend/geolibre_server_api/tests/test_api.py | Adds API tests for accounts/tokens/projects/visibility/thumbnails/forks/errors. |
| backend/geolibre_server_api/README.md | Documents running/configuring the FastAPI reference server. |
| backend/geolibre_server_api/pyproject.toml | Defines the Python package, extras, and CLI entry point. |
| backend/geolibre_server_api/geolibre_server_api/main.py | Implements the FastAPI reference server (auth, projects, storage, thumbnails). |
| backend/geolibre_server_api/geolibre_server_api/init.py | Exposes create_app for testing/embedding. |
| backend/geolibre_server_api/Dockerfile | Adds container build/runtime for the projects/identity server. |
| backend/geolibre_server_api/.gitignore | Ignores local DB/data artifacts for the new server package. |
| backend/geolibre_server_api/.dockerignore | Excludes dev/test artifacts from the server image build context. |
| .gitignore | Ignores the default local SQLite DB for the reference server at repo root. |
| .github/workflows/ci.yml | Installs/tests the new server package in CI and caches its pyproject deps. |
| .dockerignore | Allows workers/collab-node to be included in the root Docker build context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ### `DELETE /api/auth/token` | ||
|
|
||
| Revokes the presented Bearer token. Response: `204`. |
Member
Author
There was a problem hiding this comment.
Not reproducible -- grep -n '\*\{3,\}' docs/server-api.md returns nothing, and line 90 is "Revokes the presented Bearer token. Response: 204." There is no literal ****** in the file. Leaving this open in case you can point at the rendered text you saw.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
I also read through |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 34
🤖 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 @.dockerignore:
- Around line 11-13: Update the .dockerignore rules after the
workers/collab-node re-includes to exclude its build artifacts and dependencies,
specifically dist, node_modules, and data beneath workers/collab-node, while
preserving inclusion of the source needed by COPY workers/collab-node
workers/collab-node.
In `@backend/geolibre_server_api/geolibre_server_api/main.py`:
- Around line 601-602: Replace the non-atomic counter updates in
backend/geolibre_server_api/geolibre_server_api/main.py at lines 601-602 and
691-693 with SQL-side increments using sqlalchemy.update and the Project column
expressions; import update from sqlalchemy. In the fork-count path, update the
row identified by source.id, and in the view-count path, perform the increment
only after the raw_response object read succeeds so missing projects are not
counted.
- Around line 447-449: The GET /api/users/me response must use the full account
contract. In backend/geolibre_server_api/geolibre_server_api/main.py lines
447-449, update get_current_user to return account_json(account); in
docs/server-api.md lines 92-98, retain and verify the documented id, username,
and createdAt shape matches account_json; in
backend/geolibre_server_api/tests/test_api.py line 55, assert the returned
user’s username field instead of exact equality with a single-field object.
- Around line 562-571: Update the content-update flow around the project version
allocation to serialize concurrent requests: lock the project row before
calculating the next version, derive it from the maximum existing Version.number
rather than len(project.versions), and ensure the storage write cannot overwrite
an already allocated version key. Handle any remaining IntegrityError with the
established transaction rollback/retry path so the losing request does not
return an unhandled 500.
- Around line 301-304: Update the Token model to add an expires_at column,
populate it when tokens are created using the intended lifetime, and update the
authorization lookup around session.get(Token, token_digest(...)) to reject
tokens whose expiry has passed with the existing 401 response. Ensure expired
tokens cannot authenticate while preserving valid-token behavior.
- Around line 544-550: Update the partial-update handling around the visibility
and tags branches to reject explicit null values with HTTP 422 before assignment
or validation. Ensure tags is validated only when non-null and visibility is
never assigned None, while preserving existing behavior for valid values and
omitted fields.
- Around line 426-431: Protect the unauthenticated login flow in login with rate
limiting before password verification, enforcing both per-client-IP and
per-username limits through the existing middleware or reverse-proxy
configuration. Ensure requests exceeding either limit receive HTTP 429, while
preserving the current authentication behavior for allowed requests.
- Line 43: Update USERNAME_RE to require usernames to be 3–39 characters, while
preserving the existing lowercase alphanumeric and hyphen placement rules.
Ensure validation rejects one- and two-character usernames consistently with the
error text and documented limits.
- Around line 644-646: Update the thumbnail upload handler and the JSON content
routes to consume request bodies incrementally rather than using request.body()
or parsing the full payload before validation. Track accumulated bytes while
iterating the request stream, immediately raise HTTPException(413) once
max_thumbnail_bytes or max_project_bytes is exceeded, and pass only the bounded
collected data to existing parsing logic such as parse_content.
- Around line 267-274: Update the CORS origin parsing near app.add_middleware so
a wildcard origin is never combined with explicit origins: reject configurations
containing "*" alongside other values, or normalize them to wildcard-only with
allow_credentials disabled. Ensure allow_credentials is true only when the final
origins list contains no wildcard.
- Around line 105-107: Update the Credentials model to enforce maximum lengths
for both username and password, using appropriate bounded values before
password_hash processes authentication input. Keep the existing required string
validation and token endpoint behavior unchanged for inputs within those limits.
- Around line 254-256: Enable SQLite foreign-key enforcement in the engine setup
around create_engine by registering a connection-level connect event that
executes PRAGMA foreign_keys=ON for each SQLite connection. Preserve the
existing check_same_thread handling and leave non-SQLite engine behavior
unchanged.
- Around line 712-718: Remove the module-level app instance created by
create_app() and update run() to launch "geolibre_server_api.main:create_app"
with factory=True, preserving the existing host and port settings so app
construction occurs only when uvicorn starts.
In `@backend/geolibre_server_api/tests/test_api.py`:
- Around line 71-82: Add unlisted-project coverage to
test_project_crud_visibility_listing_and_raw_views by creating an unlisted
project, asserting it is excluded from the public and another user's
get_user_projects listings, and verifying its rawJsonUrl is accessible without
authentication.
- Around line 9-20: Update the client fixture to construct a FileStorage rooted
at tmp_path / "objects" and pass that instance to create_app via its storage
parameter, removing the post-construction root reassignment and mkdir calls.
Keep the existing app URL and database setup unchanged so the fixture is
independent of storage-related environment variables.
In `@docs/server-api.md`:
- Around line 170-177: Update the GET /api/projects query-parameter
documentation to include the supported mine=true option for requesting the
authenticated user’s projects, and clarify the public-listing Authorization
statement so it does not contradict this explicit mine behavior.
In `@packages/collab-core/src/protocol.ts`:
- Around line 1-7: Update the header comment in the shared protocol module to
identify it as the single relay-side definition used by the Cloudflare worker,
Node relay, and conformance suite, rather than a worker-side copy. Preserve the
note about the frontend parallel definition and its concrete GeoLibreProject
type, and mention that workers/collab/src/protocol.ts re-exports this module.
In `@packages/collab-core/src/session.ts`:
- Around line 16-20: Centralize the duplicated HEX_COLOR_RE and finite helpers
in a new unexported internal module. In packages/collab-core/src/session.ts
lines 16-20 and packages/collab-core/src/comment-validate.ts lines 51-55, remove
the local declarations and import both helpers from the shared module, such as
./internal/validate, without changing the package’s public exports.
- Around line 121-123: Update sanitizeDisplayName to trim string inputs before
applying the 60-character limit and "Guest" fallback, matching validateAuthor’s
empty-name handling. Ensure whitespace-only values resolve to "Guest" and
nonblank names retain their trimmed, capped value.
In `@packages/collab-core/tsconfig.json`:
- Around line 2-11: Update the package tsconfig to extend the repository’s
shared tsconfig.base.json using the correct relative path, then remove locally
duplicated compiler options such as target, module, lib, and strict while
preserving package-specific settings like noEmit and include.
In `@tests/collab-core-conformance.test.ts`:
- Around line 73-79: Extend the conformance test around authorizeSnapshot to
pass and verify an explicit non-default maxBytes limit, ensuring values at the
limit are accepted and values above it return the "too-large" decision. Also
import sanitizeView and add assertions that a non-array center becomes null
while a four-element finite bbox is preserved.
In `@workers/collab-node/Dockerfile`:
- Around line 11-19: Harden the runtime stage around the node_modules copy by
retaining only production dependencies while preserving the
`@geolibre/collab-core` workspace link, or bundle ws and remove the runtime
dependency copy. Add a non-root runtime user and switch to it before CMD, then
add a Docker HEALTHCHECK targeting the existing GET /health endpoint.
In `@workers/collab-node/src/server.ts`:
- Around line 156-158: Remove the empty conditional block checking raw,
Array.isArray(raw), ArrayBuffer, and ArrayBuffer.isView from the surrounding
handler, leaving binary-frame handling exclusively at the event site.
- Around line 159-171: Validate the result of JSON.parse in the message handling
flow before assigning or using it as ClientMessage, rejecting null and other
non-object payloads with the existing bad-message response and return path.
Update the message-listener call to handleMessage around the visible call site
so handler exceptions are caught and cannot terminate the relay process.
- Around line 496-506: Update the close method to give each peer socket a short
grace period after initiating the graceful close handshake, then forcibly
terminate any socket still open before awaiting server.close(). Ensure the
fallback cleanup is scheduled for every peer and does not interfere with sockets
that close normally.
- Around line 436-441: Update the request body handling around the raw, data,
and end listeners to register an "error" listener on the IncomingMessage and
safely handle client-abort errors. When accumulated data reaches or exceeds 16
KiB, destroy the request immediately instead of continuing to consume input,
while preserving normal end processing for bodies within the limit.
- Around line 473-490: Add per-connection ping/pong heartbeat handling around
the upgrade callback: track whether the socket answered the previous ping,
periodically ping each peer and terminate unresponsive sockets, and register the
socket with the heartbeat mechanism before returning. Clear the heartbeat
interval when the socket closes, while preserving the existing message, close,
and error handling in the wss.handleUpgrade callback.
- Around line 435-460: Add a store-level sweep method that deletes sessions
whose updated_at exceeds idleTtlMs and are not currently live, then invoke it at
startup and on a recurring interval from createRelay. Track the interval and
clear sweepTimer in close(), while preserving existing closePeer cleanup
behavior.
In `@workers/collab-node/src/store.ts`:
- Around line 82-95: Update saveSnapshot to use one prepared UPDATE statement
with RETURNING rev, pass the snapshot, timestamp, and id parameters, and call
StatementSync.get() to read and return the updated revision atomically instead
of issuing a separate SELECT.
In `@workers/collab-node/test/relay.test.ts`:
- Around line 154-166: Update the “restores the latest snapshot and revision
from SQLite after restart” test to build its temporary directory from
os.tmpdir() instead of hardcoding /tmp. Replace the 25 ms sleep after host.send
with an observable synchronization event by connecting a second session client
and awaiting its received snapshot broadcast before closing the host and
cleaning up.
In `@workers/collab/package.json`:
- Around line 10-12: Update the `@geolibre/collab-core` dependency in
workers/collab/package.json from the exact 0.0.0 version to the
workspace-standard wildcard specifier, preserving the existing dependency entry
and aligning it with other internal `@geolibre/`* references.
In `@workers/collab/src/session.ts`:
- Around line 452-461: Duplicate socket-and-attachment collection exists in both
handlers. In workers/collab/src/session.ts lines 452-461 and 493-502, add a
private attachedSockets() helper that deserializes and filters live socket
attachments while preserving the same object instances, then replace both inline
expressions with calls to that helper.
- Around line 510-517: Remove the unused socketByClientId helper from the
session module, since target lookup now uses socketsWithAttachments.find(...)
and no callers remain. Leave the active target lookup and serialization flow
unchanged.
- Line 83: Update SocketAttachment so it only extends SessionParticipant while
retaining its Cloudflare-specific documentation, and remove the redeclared
shared members clientId, displayName, color, role, editOverride, lastChatTs, and
lastCommentTs. Afterward, remove CollaborationRole from the imports if it is no
longer referenced.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: a9ef32b3-318e-4c51-af20-e45a3b7a9f42
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.dockerignore.github/workflows/ci.yml.gitignoreDockerfilebackend/geolibre_server_api/.dockerignorebackend/geolibre_server_api/.gitignorebackend/geolibre_server_api/Dockerfilebackend/geolibre_server_api/README.mdbackend/geolibre_server_api/geolibre_server_api/__init__.pybackend/geolibre_server_api/geolibre_server_api/main.pybackend/geolibre_server_api/pyproject.tomlbackend/geolibre_server_api/tests/test_api.pydocker-compose.ymldocs/collaboration.mddocs/getting-started.mddocs/server-api.mdpackage.jsonpackages/collab-core/package.jsonpackages/collab-core/src/comment-validate.tspackages/collab-core/src/index.tspackages/collab-core/src/protocol.tspackages/collab-core/src/session.tspackages/collab-core/tsconfig.jsontests/collab-core-conformance.test.tsworkers/collab-node/Dockerfileworkers/collab-node/README.mdworkers/collab-node/package.jsonworkers/collab-node/src/server.tsworkers/collab-node/src/store.tsworkers/collab-node/test/relay.test.tsworkers/collab-node/tsconfig.jsonworkers/collab/package.jsonworkers/collab/src/comment-validate.tsworkers/collab/src/protocol.tsworkers/collab/src/session.ts
Contributor
🔍 GitHub Pages PR preview
|
Three defects only a real `docker compose up` surfaces. The entrypoint embeds its runtime-config program in a single-quoted `python -c`, so the apostrophes in `'/'.join(...)` closed that argument and every web container died at boot on a SyntaxError. The server image created only /data, so a named volume mounted at /data/objects came up root-owned and the unprivileged user could not write any project. Forking required a request body despite documenting a private default, so a bodyless "fork this" answered 422.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Everything else I checked — the collab-core extraction correctness (Cloudflare Worker |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@backend/geolibre_server_api/Dockerfile`:
- Around line 12-14: Update the container startup flow around the geolibre user
and /data/objects setup to repair ownership of existing mounted
geolibre-projects volumes, not only during image build. Add an upgrade migration
or privileged initialization step that chowns /data/objects to geolibre:geolibre
before switching to the unprivileged user, while preserving normal startup for
newly created volumes.
In `@backend/geolibre_server_api/geolibre_server_api/main.py`:
- Around line 587-590: Update the body parameter in fork_project to default to
None instead of constructing ForkRequest() at definition time, then instantiate
ForkRequest() inside the handler when body is None so bodyless requests retain
their existing defaults.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: c2feb837-5e70-472b-bf98-4e0057e77165
📒 Files selected for processing (4)
backend/geolibre_server_api/Dockerfilebackend/geolibre_server_api/geolibre_server_api/main.pybackend/geolibre_server_api/tests/test_api.pydocker/entrypoint.sh
Relay, crash and correctness:
- Reject non-object JSON before reading .type. `null` is valid JSON, so one
frame from any client threw inside the ws listener and killed the process,
taking every other session with it. Reproduced against the container, which
Docker then restarted. handleMessage is also wrapped defensively.
- Answer presence frames before the per-message SQLite read. It is a synchronous
SELECT * over the snapshot and chat blobs, and this relay hosts every session
in one process, so a cursor burst added latency to unrelated sessions.
- Add an error listener to the session-create request stream and destroy the
request past the size cap, rather than reading on forever.
- Sweep sessions that were created but never joined; only the socket-close path
deleted rows, so an unjoined code lived in SQLite forever.
- Add a ping/pong heartbeat, so a connection dropped without a close frame stops
pinning the roster and blocking idle cleanup, and terminate sockets that never
finish the closing handshake instead of hanging shutdown.
- Make saveSnapshot atomic with UPDATE ... RETURNING rev.
- Trim in sanitizeDisplayName: a whitespace-only name is truthy and reached the
roster, participant broadcasts, and chat author fields.
Server correctness and hardening:
- USERNAME_RE accepted a 1-character name; the middle group was optional.
- Bound username and password length. Both endpoints taking them are
unauthenticated and feed scrypt, so size drove CPU and memory.
- Never pair a wildcard CORS origin with allow_credentials.
- Enable SQLite foreign keys, so deleting an account no longer strands tokens.
- Reject explicit null visibility/tags in PATCH, which returned 500 not 422.
- Increment views and forkCount in SQL; the contract promises atomicity.
- Allocate version numbers from max(number) with retry, so concurrent content
updates cannot collide on a key and overwrite each other.
- Stream thumbnail uploads instead of buffering the whole body before the check.
- Build the app in a uvicorn factory; importing the module created a stray
database and storage directory, including under pytest.
- Return the documented account shape from /api/users/me.
Packaging and docs:
- Run the relay container as non-root with a healthcheck, and document the
one-time chown for volumes created by an earlier root-owned image.
- Re-exclude collab-node build artifacts after the .dockerignore negations.
- Extend tsconfig.base.json, use the workspace "*" dependency convention, drop
the duplicated SocketAttachment members and the dead socketByClientId, and
share HEX_COLOR_RE/finite from one internal module.
- Document `mine=true`, correct the users/{username}/projects auth description
to the filtered-200 behavior the code and its tests implement, and state
plainly that rate limiting and token expiry are left to the operator.
- Cover unlisted visibility, explicit-null PATCH, username length, the
configurable snapshot ceiling, and sanitizeView; drop the fixed sleep and the
hardcoded /tmp path from the relay test.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@workers/collab-node/README.md`:
- Line 26: Change the “Volume ownership” heading in the README from level three
to level two, using ## so the document heading hierarchy satisfies MD001.
In `@workers/collab-node/src/server.ts`:
- Around line 488-499: Update the request data handler to measure the UTF-8 byte
length of raw plus the current chunk before appending it. If the combined size
exceeds 16,384 bytes, mark the request aborted, destroy it, and return the
existing 413 response; otherwise append the chunk and continue. Use the existing
request.on("data") flow and raw accumulator.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: d9bc0ca3-9625-45be-9b52-6663afe64d3f
📒 Files selected for processing (20)
.dockerignorebackend/geolibre_server_api/README.mdbackend/geolibre_server_api/geolibre_server_api/main.pybackend/geolibre_server_api/tests/test_api.pydocs/server-api.mdpackages/collab-core/src/comment-validate.tspackages/collab-core/src/index.tspackages/collab-core/src/internal/validate.tspackages/collab-core/src/protocol.tspackages/collab-core/src/session.tspackages/collab-core/tsconfig.jsontests/collab-core-conformance.test.tsworkers/collab-node/Dockerfileworkers/collab-node/README.mdworkers/collab-node/package.jsonworkers/collab-node/src/server.tsworkers/collab-node/src/store.tsworkers/collab-node/test/relay.test.tsworkers/collab/package.jsonworkers/collab/src/session.ts
- Reject an oversized POST /sessions body on the declared Content-Length before registering the body handlers, so a request claiming a large body cannot hold the connection open while it uploads slowly. The running byte count stays for clients that omit or understate the header, and both paths now share one MAX_SESSION_BODY_BYTES constant. - Add the integration test that was asked for. It writes the request over a raw socket, because fetch refuses to send fewer bytes than it declared, and that mismatch is exactly the case under test; without the guard it times out waiting for a response instead of getting an immediate 413. This was an outside-diff-range comment in the review body rather than an inline thread, which is why it was missed in the earlier rounds.
Member
Author
|
@coderabbitai full review The outside-diff-range comment about rejecting an oversized |
Contributor
|
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedFull review finished. |
giswqs
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
August 4, 2026 03:31
All findings from this review are addressed and every inline thread is resolved (60 threads across four rounds; see fc7592c, da41e9b, b498b70, 7e6a52e and 7d1c7c6). This includes the outside-diff-range item about rejecting an oversized POST /sessions body on the declared Content-Length, which is fixed in fc7592c with an integration test.
Dismissing because the repository ruleset sets dismiss_stale_reviews_on_push: false, so this review persists across pushes, and re-reviews are submitted as COMMENTED rather than APPROVED, which cannot clear it. Two threads remain intentionally open and are noted in the PR description: implementing token expiry, and npm prune --omit=dev in the relay image.
Contributor
|
All findings have been posted as inline comments. Now writing the final summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Everything else checked out: visibility enforcement ( |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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 `@backend/geolibre_server_api/Dockerfile`:
- Around line 12-15: Pin the geolibre user and group IDs in the Dockerfile’s
user-creation command to 1000, matching the documented volume ownership command
in README.md. Keep the existing home-directory creation, /data ownership, and
USER geolibre behavior unchanged.
In `@backend/geolibre_server_api/geolibre_server_api/main.py`:
- Around line 471-481: Update the account-creation flow around the existing
username check and session.commit to catch the database IntegrityError caused by
a concurrent username insert, roll back the session, and raise
HTTPException(409, "username already exists"). Preserve the current pre-check
and successful account/token response, following the existing create_project
conflict-handling pattern.
- Around line 292-317: Move the CORSMiddleware registration block below the
limit_body middleware definition so CORSMiddleware remains the outermost
wrapper. Preserve the existing body-size rejection logic and CORS configuration,
ensuring early 413 responses receive the appropriate CORS headers.
In `@backend/geolibre_server_api/README.md`:
- Around line 11-23: Update the Configuration section in the README to document
the GEOLIBRE_HOST and GEOLIBRE_PORT environment variables read by run(),
including their purpose for configuring the bind address and port and their
defaults if defined by the implementation.
In `@docker-compose.yml`:
- Line 30: Update the GEOLIBRE_DATABASE_URL definition and the other referenced
Postgres password configuration to require POSTGRES_PASSWORD without a geolibre
fallback. Update the self-hosting instructions in docs/getting-started.md to
state that POSTGRES_PASSWORD must be set.
In `@docs/getting-started.md`:
- Around line 319-325: Update the production Compose guidance for the
geolibre-server and geolibre-collab services so their published ports bind only
to 127.0.0.1 or are removed, while preserving internal service connectivity.
Document that only the web/TLS proxy should be externally reachable and clarify
this alongside the existing public URL and password deployment instructions.
In `@docs/server-api.md`:
- Around line 235-240: Update the POST /api/projects/{id}/forks documentation to
state that the request body is optional, including that omitting it defaults
visibility to private. Preserve the existing documented body form, response,
authentication, and forkCount behavior.
In `@packages/collab-core/src/comment-validate.ts`:
- Around line 152-158: Update validateComment’s inline reply processing to track
reply IDs already accepted and skip subsequent replies with the same id before
pushing them into replies. Preserve the existing MAX_REPLIES_PER_COMMENT cap and
validateReply filtering, ensuring only the first valid occurrence of each reply
ID is retained.
In `@tests/collab-core-conformance.test.ts`:
- Around line 48-51: Update the override-specific test case around
participantCanEdit and authorizeSnapshot to capture the authorizeSnapshot result
and assert both that ok is false and that message equals the view-only override
text “The host has set you to view-only.”.
In `@workers/collab-node/Dockerfile`:
- Around line 14-20: Update the Dockerfile COPY directives for node_modules,
package.json, and dist to use --chown=node:node, then change the RUN command to
create /data and chown only /data. Remove the recursive chown of /app so copied
files are not rewritten into an additional layer.
In `@workers/collab-node/src/store.ts`:
- Line 3: Update the workers/collab-node package manifest to add an engines.node
constraint of >=22.13, ensuring the package requires the minimum Node version
supported by the node:sqlite import in store.ts.
- Around line 66-80: Update the get method’s stored mode mapping to reuse the
shared normalizeMode function from `@geolibre/collab-core` instead of the inline
ternary, preserving the same normalized mode contract used by the relay in
server.ts.
In `@workers/collab/src/session.ts`:
- Around line 465-481: Update the comments around setParticipantOverride and the
subsequent target lookup to remove claims about local type guarding,
strict-boolean coercion, and disconnect races that no longer apply. Revise the
target-handling logic to reuse the participant/attachment pair identified by
setParticipantOverride rather than re-scanning socketsWithAttachments with find,
preserving the existing no-change early return and target processing 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fd2593d0-a2f6-4363-b52c-bcb977b3de83
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
.dockerignore.github/workflows/ci.yml.gitignoreDockerfilebackend/geolibre_server_api/.dockerignorebackend/geolibre_server_api/.gitignorebackend/geolibre_server_api/Dockerfilebackend/geolibre_server_api/README.mdbackend/geolibre_server_api/geolibre_server_api/__init__.pybackend/geolibre_server_api/geolibre_server_api/main.pybackend/geolibre_server_api/pyproject.tomlbackend/geolibre_server_api/tests/test_api.pydocker-compose.ymldocker/entrypoint.shdocs/collaboration.mddocs/getting-started.mddocs/server-api.mdpackage.jsonpackages/collab-core/package.jsonpackages/collab-core/src/comment-validate.tspackages/collab-core/src/index.tspackages/collab-core/src/internal/validate.tspackages/collab-core/src/protocol.tspackages/collab-core/src/session.tspackages/collab-core/tsconfig.jsontests/collab-core-conformance.test.tsworkers/collab-node/Dockerfileworkers/collab-node/README.mdworkers/collab-node/package.jsonworkers/collab-node/src/server.tsworkers/collab-node/src/store.tsworkers/collab-node/test/relay.test.tsworkers/collab-node/tsconfig.jsonworkers/collab/package.jsonworkers/collab/src/comment-validate.tsworkers/collab/src/protocol.tsworkers/collab/src/session.ts
- Translate an IntegrityError on account creation into the documented 409. The uniqueness check and the commit are not atomic, and no IntegrityError handler is registered, so the loser of a race escaped as a raw 500. - Eager-load owner and versions on the two listing queries. project_json reads both lazily, so a page fired one query per row; measured 28 statements for 25 projects before, 4 after. - Hash on a dummy password when the account is not found, so login costs the same either way. Short-circuiting skipped scrypt for an unknown username, which enumerates accounts by response time regardless of a request-count limiter. - Handle SIGTERM and SIGINT in the relay entrypoint. Nothing outside the tests called close(), so docker stop killed the process outright and the graceful shutdown path never ran in production. - Hoist the stored-chat validators into @geolibre/collab-core and use them in both relays. The Node store only JSON-parsed the column, so a corrupt record would reach joiners and crash a client on coordinate.lat.toFixed; the Worker had guarded this alone. Covered in the conformance suite. - Warn that POSTGRES_PASSWORD is substituted into a DSN verbatim. Verified the two characters that actually break: "@" reparses the host, and "%" silently percent-decodes to a different password. "/" and ":" are fine, so the note names only what genuinely fails.
Comment on lines
+319
to
+322
| @app.middleware("http") | ||
| async def limit_body(request: Request, call_next): | ||
| declared = request.headers.get("content-length") | ||
| if declared and declared.isdigit() and int(declared) > body_ceiling: |
Contributor
There was a problem hiding this comment.
Security (medium confidence): this guard only inspects the content-length header. A client sending Transfer-Encoding: chunked (or an HTTP/2 request, which never sets content-length) has no declared length, so limit_body is skipped entirely and the request falls straight through to route handling — where the JSON body is still fully materialized by Starlette/Pydantic before any per-route size check runs. This applies to the unauthenticated /api/accounts and /api/auth/token endpoints too, so it's a memory-exhaustion vector against anonymous, scrypt-backed endpoints that this middleware was specifically added to close. The per-route checks presumably still bound the stored value, but not the parsing cost this middleware exists to avoid.
Member
Author
There was a problem hiding this comment.
Partially addressed in ab4571c -- accurate, and I had flagged the same caveat when adding the guard. Closing it properly needs a streaming body reader in the middleware, which is a bigger change than I want to land in a review pass, so instead the limitation is now stated in the code comment and in the "leaves to the operator" section of docs/server-api.md, which tells operators to cap request size at the proxy. Leaving open for a maintainer to decide whether the streaming reader belongs in this PR.
| | { ok: false; code: "forbidden" | "too-large"; message: string }; | ||
|
|
||
| /** Shared authorization/size gate used by every relay implementation. */ | ||
| export function authorizeSnapshot( |
Contributor
There was a problem hiding this comment.
Performance/DoS (medium confidence): authorizeSnapshot enforces only a byte-size ceiling with no rate limit, unlike chat and comment mutations which both throttle to one accepted frame per MIN_CHAT_INTERVAL_MS/MIN_COMMENT_INTERVAL_MS (250ms). Any participant with edit permission can send up to ~1MB snapshot frames back-to-back; each one triggers a storage write plus a full broadcast fan-out to every connected peer on both relays, with no server-side throttle bounding that cost. Worth a deliberate decision (even if "not for this PR") rather than an oversight, since it's shared code both relays inherit.
Member
Author
There was a problem hiding this comment.
Not changed, flagging for a decision -- you are right that snapshots have no throttle while chat and comment mutations both do, and that the cost is a storage write plus a full fan-out per frame. I would rather not pick an interval unilaterally: unlike chat, snapshots are the live-editing path, so too coarse a throttle degrades the feature it exists to protect, and the right value probably belongs with the client's own send cadence. Leaving open for a maintainer.
| @@ -0,0 +1,163 @@ | |||
| import assert from "node:assert/strict"; | |||
Contributor
There was a problem hiding this comment.
Test coverage (quality, high confidence): this suite only imports and calls pure functions directly from @geolibre/collab-core; it never instantiates or drives the Worker's CollabSession Durable Object or the Node relay's handleMessage/createRelay. npm run test:worker also only typechecks the Worker package rather than running it. So a regression in either relay's own wiring of the shared functions — or in logic that was never actually extracted into collab-core (see the comment-mutation duplication flagged in workers/collab-node/src/server.ts) — passes this suite with zero signal. The PR description's claim that this suite prevents the two implementations from "drifting into subtly different strictness" is true only for the logic that actually lives in @geolibre/collab-core, not for logic still duplicated per relay. Consider adding at least one end-to-end case per relay (spin up createRelay/CollabSession and drive real messages) so wiring drift is caught too.
Member
Author
There was a problem hiding this comment.
Partly accurate, leaving open -- the Node relay is driven end to end: workers/collab-node/test/relay.test.ts calls createRelay, opens real WebSockets and exercises join/snapshot/mode/override/comment-mutation/persistence-across-restart, and it caught two real bugs this round. The gap you are describing is specifically the Worker: npm run test:worker only typechecks it, so CollabSession has no runtime coverage and its wiring of the shared functions is unverified. That is a fair criticism and I have softened the PR description accordingly. Adding a miniflare-backed Worker suite is worth doing but is more than a review pass; leaving open for a maintainer.
Contributor
|
All six inline comments posted successfully. Now the final summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Verified as correctly fixed (claims made in the PR description that held up under direct inspection): the |
- Register CORSMiddleware after limit_body so it stays outermost. Starlette wraps in reverse registration order, so the 413 was returned without CORS headers and a browser could not read the documented error body. Confirmed before and after, and pinned in the test. - Pin the geolibre uid/gid to 1000, which the documented volume-repair chown assumes; the base image merely happened to allocate it. - Require POSTGRES_PASSWORD instead of committing a shared default for the account that owns all project metadata, and document that Postgres only applies it at volume initialization, so rotating it later needs ALTER USER or a fresh volume. - Document binding the projects server and relay to loopback behind a proxy, with an override file; pointing the browser URLs at a proxy does not stop direct access to those listeners. - Deduplicate reply ids in validateComment. The relay's reply action already skipped a duplicate id, leaving inline replies as the one path that could persist two replies sharing an id. - Use COPY --chown in the relay image instead of a recursive chown of /app, which rewrote node_modules into a second layer. - Declare engines.node >=22.13, below which node:sqlite is flag-gated. - Reuse normalizeMode for the stored mode rather than an inline ternary that would silently downgrade a future third mode. - Document GEOLIBRE_HOST and GEOLIBRE_PORT, and that the fork request body is optional. - Pin the override-specific refusal message in the conformance suite, and correct two comments in the Worker that described code the extraction removed.
- Filter non-object entries out of project.comments and target.replies in the Node relay. Snapshot content is opaque, so a client could plant null with an ordinary snapshot frame and every subsequent .id read threw, leaving commenting permanently broken for that session while the Worker tolerated the same data. Reproduced against the container, then covered in the relay suite. - Attach an error listener to the raw upgrade socket before writing to it. A net.Socket with no listener throws on the next TCP error, which is uncaught and stops the relay, and this is the pre-auth path most exposed to arbitrary traffic. - Retry the slug and version allocations on OperationalError too. On the SQLite default a concurrent writer surfaces as "database is locked" rather than IntegrityError, which the loops did not catch. - Add a catch-all exception handler returning the documented JSON error shape. Only HTTPException and RequestValidationError were registered, so anything else escaped as a plain-text 500. - Document that the Content-Length guard sees only a declared length, so a chunked or HTTP/2 request is still parsed in full, and that request size should also be capped at the proxy.
giswqs
dismissed
coderabbitai[bot]’s stale review
August 4, 2026 04:05
All 12 inline comments from this review are addressed and resolved in ee5544b (CORS middleware ordering, uid/gid pinning, required POSTGRES_PASSWORD, loopback port guidance, reply-id dedupe, COPY --chown, engines.node, normalizeMode reuse, GEOLIBRE_HOST/PORT and fork-body docs, the override-message assertion, and the stale Worker comments). Two later rounds of findings were also fixed in ab4571c.
Dismissing because the repository ruleset sets dismiss_stale_reviews_on_push: false, so this review persists across pushes, and re-reviews arrive as COMMENTED rather than APPROVED, which cannot clear it. Five threads remain intentionally open and are noted in the PR description: a streaming body reader for chunked requests, a snapshot rate limit, runtime coverage for the Worker, npm prune in the relay image, and one Copilot report of text that is not in the file.
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
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.
Summary
docs/server-api.md, so a compatible server can be written from the spec rather than by reading the client, and addsbackend/geolibre_server_api, a FastAPI reference implementation covering accounts, API tokens, projects with private/unlisted/public visibility, filesystem-backed storage, thumbnails, views, versions, and forks.@geolibre/collab-corepackage, and addsworkers/collab-node, a plain Node WebSocket relay backed by SQLite that hosts the same core. The Worker stays as the hosted deployment.docker-compose.ymlwiring the web container, the projects server, and the collab relay together, so a self-hoster gets sharing, accounts, and live sessions without a Cloudflare account.tests/collab-core-conformance.test.ts) that both relays must pass, covering the permission cases specifically, so the two implementations cannot drift into subtly different strictness for the logic that actually lives in@geolibre/collab-core. The Node relay is additionally driven end to end over real WebSockets byworkers/collab-node/test/relay.test.ts; the Worker is only typechecked, so its wiring of the shared functions has no runtime coverage yet. The snapshot cap stays configurable and defaults to the same value on both.Closes #1685
Closes #1686
Closes #1692
Fixes found by running the stack
Three defects that only a real
docker compose upsurfaces, each with the unit tests passing:docker/entrypoint.shembeds its runtime-config program in a single-quotedpython -c, so the apostrophes in'/'.join(...)closed that shell argument. Python received{/.join(...)}and died on a SyntaxError. Because-ccompiles as a unit this hit every web container at boot, not just a misconfigured one, andset -eturned it into a crashloop. This is [Bug]: Container fails to start using latest GeoLibre docker image #1692, reported independently against the published image: reproduced byte for byte on the reporter's digestsha256:b6bf7e69, and the image built from this branch starts and serves200under their exactdocker runcommand./data, so the named volume mounted at/data/objectscame up root-owned and the unprivileged user could not write any project. A named volume inherits image ownership only when the exact path already exists.privatedefault, so a bodyless "fork this project" answered 422. Covered by a new regression test that fails without the fix.Test plan
npm run test:worker(typechecks every worker, runs the Node relay suite: 4 passing)node --import tsx --test tests/collab-core-conformance.test.ts(6 passing, both relay hosts)pytest backend/geolibre_server_api/tests(4 passing)npm run test:frontend:coverage(5035 passing, coverage above the 78/78/63 floors)npm run lintandnpm run buildcleanpre-commit run --files <changed>cleandocker compose upend to end, all four services healthy:privateby default, and the slug collision resolves to-2while the sourceforkCountincrements.max-age=3600on immutable version URLs,max-age=60on the mutable latest URL,private, no-storeon private projects, matching the spec.AuthorizationandContent-Type; a foreign origin gets no allow header.view-only, a per-participant override restoring edit,set-modeandset-participant-modegated to the host, an oversized snapshot answered withtoo-largeand an intact socket, chat fan-out, and a late joiner restoring both snapshot and chat history. The relay assigns its ownclientIdrather than trusting the client's.docker compose restartof the relay via SQLite on the mounted volume.Summary by CodeRabbit
Review feedback
Addressed 58 inline comments from Copilot, the Claude review bot, and CodeRabbit across three rounds. Highlights, all verified against the running stack rather than only the unit tests:
JSON.parseaccepts the literalnull, so reading.typeoff it threw inside thewslistener as an uncaught exception. Confirmed by sending it to the container, which Docker then restarted, dropping every live session on that instance.comment-mutation: the Node relay checked permissions before validating the payload, so a malformed frame from a view-only guest answeredforbiddenwhere the Worker answersbad-message, and it persisted the mutated project with no size ceiling. Nothing exercised that path; it now has coverage.USERNAME_RE, an explicit JSONnullin PATCH returned 500 instead of 422,viewsandforkCountwere non-atomic despite the contract promising otherwise, and concurrent content updates could collide on a version number and overwrite each other.Two threads are intentionally left open for a maintainer: implementing token expiry (documented rather than built, since it needs a migration and a refresh story), and
npm prune --omit=devin the relay image (risky against the linked@geolibre/collab-coreworkspace package). One Copilot comment reporting a literal******indocs/server-api.mdis a false positive; there is no such text in the file.