fix(security): harden SSRF, upgrades, bind policy, and mutation UX - #201
Conversation
Address the comprehensive code-review findings: validate redirects before following them, reject arbitrary HTTP update sources, require signed remote upgrades, fail closed on non-loopback binds outside containers, reject URL credentials consistently, apply default body limits, drop unsafe restore bypass from HTTP, and make frontend mutations confirm before success UX. Co-authored-by: Robert Goniszewski <goniszewski@users.noreply.github.com>
Prefer category_id-only list/search params in hook tests, and use a concrete ApiUpdateCheckResult shape to avoid deep DTO instantiation. Co-authored-by: Robert Goniszewski <goniszewski@users.noreply.github.com>
Preserve a "valid URL" phrase in the dialog error so existing journey coverage continues to match the safer http(s)/no-credentials check. Co-authored-by: Robert Goniszewski <goniszewski@users.noreply.github.com>
- update/service: thread allowPrivateHosts into the real-fetch path so the CLI's local test sources (e.g. --source http://127.0.0.1:9999) are not blocked by the new per-hop redirect guard; private hosts stay rejected by default on the HTTP route - pipeline/fetcher: send the current Grimoire/<version> user-agent (was the stale LittleImp/0.0 string pointing at the old little-imp repo URL) - tests: cover private-source checkForUpdates behavior and the fetcher UA Co-authored-by: Robert Goniszewski <robertgoniszewski@outlook.com> Signed-off-by: Robert Goniszewski <robertgoniszewski@outlook.com>
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
Pull request overview
This PR hardens Grimoire’s local-first security posture across the daemon (SSRF defenses, update/upgrade authenticity, bind policy, URL validation, body limits) while aligning the frontend UX with “await daemon confirmation” mutation behavior and safer external-link handling.
Changes:
- Daemon: add centralized public URL parsing/redaction, manual redirect validation for outbound fetches, stricter update/upgrade source + signature requirements, and loopback bind enforcement (with container opt-ins).
- Frontend: route-level error boundary, safer external URL handling (no credential URLs), centralized API calls, and mutation flows that wait for confirmation before success UI.
- Docs: regenerated API contract/OpenAPI/API.md plus task report updates for the security hardening work.
Reviewed changes
Copilot reviewed 50 out of 51 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/Settings.tsx | Uses api.ts helper for AI connection testing |
| src/pages/Index.tsx | Awaits add/delete operations before success toasts; category move uses IDs |
| src/lib/safe-url.ts | Adds frontend helper to validate safe http(s) URLs without credentials |
| src/lib/bookmarklet.ts | Injects bookmarklet values via JSON.stringify to prevent JS breakouts |
| src/lib/bookmarklet.test.ts | Updates tests for bookmarklet escaping behavior |
| src/lib/bookmark-open.ts | Blocks opening unsafe external URLs |
| src/lib/api.ts | Centralizes update-check, AI test, and export download logic |
| src/hooks/use-update-check.ts | Routes update check through api.ts helper |
| src/hooks/use-bookmarks.ts | Makes add/delete async, broadens invalidations, prefers category_id |
| src/hooks/use-bookmarks.test.ts | Updates expectations to avoid sending both category and category_id |
| src/hooks/use-app-lock.ts | Clarifies lock is UI-only, not daemon authentication |
| src/components/ExportMenu.tsx | Uses centralized export download helper |
| src/components/ErrorBoundary.tsx | Adds route-level error boundary component |
| src/components/BookmarkDetailContent.tsx | Sanitizes markdown links + disables unsafe open actions; safer date formatting |
| src/components/BookmarkCard.tsx | Surfaces an error toast when a bookmark URL can’t be opened safely |
| src/components/AddBookmarkDialog.tsx | Validates URLs (http/s, no credentials) and adds submitting state |
| src/App.tsx | Wraps route tree with ErrorBoundary |
| docs/task-reports/index.html | Adds latest task report card for security hardening |
| docs/task-reports/2026/index.html | Updates July summary to include security hardening |
| docs/task-reports/2026/07/index.html | Adds security hardening report card in July index |
| docs/task-reports/2026/07/2026-07-22-security-hardening-review-fixes/index.html | New security hardening task report page |
| docs/task-reports/2026/07/2026-07-22-security-hardening-review-fixes/assets/01-hardening-map.svg | New SVG asset for the task report |
| docs/security-boundaries.md | Updates update-source boundary language to reflect new constraints |
| docs/openapi.json | Adds credential_url classification; removes unsafe restore option |
| docs/backup-design.md | Removes HTTP restore “unsafe no checksum” option from design docs |
| docs/api-contract.json | Adds credential_url classification; removes unsafe restore option |
| API.md | Removes allow_unsafe_no_checksum; documents new import classification |
| Dockerfile | Sets LITTLEIMP_IN_CONTAINER=1 for container bind-policy detection |
| docker-compose.yml | Sets LITTLEIMP_IN_CONTAINER=1 for container bind-policy detection |
| daemon/src/update/upgrade.ts | Enforces https base URL, signature requirements, fingerprint pinning, size caps |
| daemon/src/update/service.ts | Adds credential checks, safer redirect-following for update checks |
| daemon/src/test/pipeline/fetch.test.ts | Adds redirect/credential/user-agent tests for fetch hardening |
| daemon/src/test/lib/public-url.test.ts | New tests for shared public URL parsing + redaction |
| daemon/src/test/lib/bind-host.test.ts | New tests for bind-host policy |
| daemon/src/test/integration/updates.test.ts | Tightens HTTP update-check source behavior; adds allowPrivateHosts test |
| daemon/src/test/cli-update.test.ts | Adds signature-required behavior + allow-unsigned escape hatch coverage |
| daemon/src/server.ts | Applies default JSON body limits for mutating routes |
| daemon/src/routes/updates.ts | Rejects arbitrary source query param on HTTP update check |
| daemon/src/routes/import.ts | Adds credential_url classification and warning mapping |
| daemon/src/routes/capture.ts | Switches capture URL validation to shared public-url helper |
| daemon/src/routes/bookmarks.ts | Switches bookmark URL validation + log redaction; handles create races |
| daemon/src/routes/backup.ts | Removes HTTP restore checksum bypass behavior |
| daemon/src/pipeline/fetcher.ts | Uses safe redirect-following; validates URLs/credentials; updates UA string |
| daemon/src/mcp/server.ts | Uses shared public URL validation for MCP URL inputs |
| daemon/src/lib/safe-fetch.ts | New safe-fetch helper with manual redirect validation |
| daemon/src/lib/public-url.ts | New shared public http(s) URL validation + redaction |
| daemon/src/lib/bind-host.ts | New bind-host policy for loopback-only defaults |
| daemon/src/index.ts | Enforces bind-host policy at daemon start |
| daemon/src/import/netscape-parser.ts | Adds credential URL skipping reason during import parsing |
| daemon/src/cli.ts | Adds --allow-unsigned and improved signature messaging |
| daemon/src/api/contract.ts | Updates schema enums and removes unsafe restore input field |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| export function isRedirectStatus(status: number): boolean { | ||
| return status >= 300 && status < 400; | ||
| } |
| const { url, title } = body as { url: string; title?: unknown }; | ||
|
|
||
| if (!isValidUrl(url)) { | ||
| return problem(c, 422, "Unprocessable Entity", "Invalid URL - must be http or https"); | ||
| const parsedUrl = parsePublicHttpUrl(url); | ||
| if (!parsedUrl.ok) { | ||
| return problem(c, 422, "Unprocessable Entity", urlValidationDetail(url)); | ||
| } |
| void Promise.all(ids.map((id) => store.deleteBookmark(id))) | ||
| .then(() => { | ||
| const count = deletedBookmarks.length; | ||
| toast({ | ||
| title: `${count} bookmark${count !== 1 ? "s" : ""} deleted`, | ||
| action: ( | ||
| <ToastAction | ||
| altText="Undo delete" | ||
| onClick={() => deletedBookmarks.forEach((b) => void store.restoreBookmark(b.id))} | ||
| > | ||
| Undo | ||
| </ToastAction> | ||
| ), | ||
| }); | ||
| exitSelectionMode(); | ||
| setBulkDeleteOpen(false); | ||
| }) | ||
| .catch((err) => { | ||
| toast({ | ||
| title: "Could not delete bookmarks", | ||
| description: err instanceof Error ? err.message : "Unknown error", | ||
| variant: "destructive", | ||
| }); | ||
| }); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df3b87452b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const combined = `${outputToString(result.stderr)}\n${outputToString(result.stdout)}`; | ||
| const found = [...combined.matchAll(/([A-F0-9]{40}|[A-F0-9]{64})/gi)].map((m) => | ||
| m[1].toUpperCase() | ||
| ); | ||
| const matched = found.some((fp) => allowedFingerprints.includes(fp)); |
There was a problem hiding this comment.
When LITTLEIMP_UPGRADE_SIGNING_KEY_FINGERPRINTS is configured, this scans all human-readable GPG output for any 40- or 64-digit hex string, including attacker-controlled user IDs printed in a “Good signature” diagnostic. A valid signature from the wrong imported key can therefore satisfy the allowlist if its user ID contains an allowed fingerprint. GnuPG 2.4.4 exposes --status-fd; parse the VALIDSIG fingerprint from that machine-readable output instead.
Useful? React with 👍 / 👎.
| if (c.req.query("source") != null && c.req.query("source") !== "") { | ||
| return problem( | ||
| c, | ||
| 422, | ||
| "Unprocessable Entity", | ||
| "The source query parameter is not accepted on the HTTP update check. Configure LITTLEIMP_UPDATE_SOURCE or use the CLI." |
There was a problem hiding this comment.
Any client generated from the current contract can still send the documented optional source query parameter, but this branch now unconditionally returns 422 for it. daemon/src/api/contract.ts:1792-1795, docs/api-contract.json, and generated API.md still advertise that parameter, so update the contract and regenerate the documentation alongside this route change.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| }); | ||
| toast({ title: `Moved ${selectedIds.size} bookmark${selectedIds.size !== 1 ? "s" : ""} to "${category}"` }); | ||
| exitSelectionMode(); | ||
| void Promise.all(ids.map((id) => store.deleteBookmark(id))) |
There was a problem hiding this comment.
If one selected bookmark deletion fails after other requests have already succeeded—for example, a stale ID returns 404—Promise.all rejects even though the successful requests are not rolled back. The catch path then reports only failure and never exposes the promised Undo action, leaving some bookmarks moved to Trash with no indication of which ones changed. Collect individual results or restore the successful subset before reporting the operation as failed.
Useful? React with 👍 / 👎.
| const host = hostname.replace(/^\[|\]$/g, "").toLowerCase(); | ||
| return host === "localhost" || host === "127.0.0.1" || host === "::1"; | ||
| } |
There was a problem hiding this comment.
Native configurations using valid loopback spellings such as HOST=127.0.0.2 or expanded IPv6 HOST=0:0:0:0:0:0:0:1 now fail startup with BindHostError, even though these addresses do not widen daemon exposure. Classify loopback IPs by address range rather than accepting only the two canonical literals.
Useful? React with 👍 / 👎.
Summary
Implements the comprehensive code-review hardening items for Grimoire’s critical paths while staying inside the local-first / loopback threat model.
Daemon / security
safe-fetch) so private redirect targets are never contactedGET /updates/checkrejects arbitrarysourcequery params (default/env source only)LITTLEIMP_UPGRADE_SIGNING_KEY_FINGERPRINTS, and download size caps;--allow-unsigned/LITTLEIMP_ALLOW_UNSIGNED_UPGRADE=1is an explicit escape hatchLITTLEIMP_ALLOW_NON_LOOPBACK_BIND=1is set, with a loud warningallow_unsafe_no_checksumFrontend
category_idover namewindow.open/hrefErrorBoundaryJSON.stringifytestAiConnection/ export download / update-check go throughapi.tsDocs
docs/task-reports/2026/07/2026-07-22-security-hardening-review-fixes/Test plan
npm run check(lint, type-check, frontend + daemon tests, docs:api:check, build)npm run test:e2e(25 passed; revalidated the invalid-URL dialog assertion after copy tweak)