Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,11 @@ R2_MAX_BYTES=10485760
# window.__CONFIG__ (see app/layout.tsx).
R2_ALLOWED_CONTENT_TYPES=image/png,image/jpeg,image/webp
# Attachments feature toggle — gates the assistant-ui `<Composer />`
# attachment UI. Default false; flip to "true" once every required
# R2_* above is set. Frontend and backend must be in sync: if this
# stays "false" the composer hides the attachment widget, but routes
# still answer when called. Read in browser via window.__CONFIG__.
# attachment UI AND the Settings → Account avatar upload (both backed by
# R2). Default false; flip to "true" once every required R2_* above is
# set. Frontend and backend must be in sync: if this stays "false" the
# composer hides the attachment widget + the avatar upload is disabled,
# but routes still answer when called. Read in browser via window.__CONFIG__.
ATTACHMENTS_ENABLED=false


Expand Down
91 changes: 91 additions & 0 deletions app/api/avatar/presign/route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { NextResponse } from "next/server";
import { randomBytes } from "node:crypto";

import { R2NotConfiguredError, buildPublicUrl, presignPut } from "@/lib/r2/client";
import { safeFilename } from "@/lib/attachments/keys";
import { PresignBody } from "@/lib/attachments/validators";
import { withAuth } from "@/lib/auth/with-auth";

// ponytail: avatars go to R2 under the user's own path, NOT base64 into
// user.image. A base64 data URL there flows through the memory auth-overlay
// (mergeMemory) into every <memory> system block uncapped — issue #28's
// 372K-token blow-up. This route hands back a presigned PUT + the public
// URL; the client stores only the URL. No DB row (avatars aren't chat
// attachments — no dedup / retention sweep needed).

const ID_ALPHABET = "0123456789abcdefghijklmnopqrstuvwxyz";
const ID_LEN = 12;

function generateId(): string {
const bytes = randomBytes(ID_LEN);
let out = "";
for (let i = 0; i < ID_LEN; i++) out += ID_ALPHABET[bytes[i] % ID_ALPHABET.length];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 — modulo bias, cosmetic but cheap to fix. 256 % 36 === 4, so values 0..3 are ~0.3% more likely than the rest. Standard fix:

Suggested change
for (let i = 0; i < ID_LEN; i++) out += ID_ALPHABET[bytes[i] % ID_ALPHABET.length];
function generateId(): string {
const limit = Math.floor(256 / ID_ALPHABET.length) * ID_ALPHABET.length; // 252
const bytes = randomBytes(ID_LEN + 4);
let out = "";
let i = 0;
while (out.length < ID_LEN) {
if (bytes[i] < limit) out += ID_ALPHABET[bytes[i] % ID_ALPHABET.length];
i++;
}
return out;
}

Non-security-critical (collisions are still ~0 in practice), but worth doing if you have other crypto-grade ID generators elsewhere in the codebase.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining — the ID is only used to namespace R2 keys under u/<userId>/avatar/<id>-<safe-name> for the presign URL, not as a security boundary (ownership is enforced server-side in DELETE /api/avatar via the key prefix check). With ID_LEN of 16 the collision space is still effectively zero at human upload rates, and the rejection-sampling loop adds a branch + 4 extra bytes on a hot path that has to run before every avatar change. Filing a separate ticket if we ever need a uniformly-distributed ID for a security-sensitive use.

🤖 Generated with Claude Code

return out;
}
Comment thread
greptile-apps[bot] marked this conversation as resolved.

function maxBytes(): number {
const parsed = process.env.R2_MAX_BYTES ? Number(process.env.R2_MAX_BYTES) : NaN;
return Number.isFinite(parsed) && parsed > 0 ? parsed : 10 * 1024 * 1024;
}

function notConfiguredResponse(): NextResponse {
return NextResponse.json(
{
code: "AVATAR_UPLOADS_NOT_CONFIGURED",
message: "Set the R2_* env vars to enable avatar uploads (see docs/ATTACHMENTS.md).",
},
{ status: 503 },
);
}

export const POST = withAuth(async (req, { user }) => {
let body: unknown;
try {
body = await req.json();
} catch {
return NextResponse.json({ code: "BAD_REQUEST", error: "invalid JSON" }, { status: 400 });
}

const parsed = PresignBody.safeParse(body);
if (!parsed.success) {
return NextResponse.json({ code: "BAD_REQUEST", error: parsed.error.issues }, { status: 400 });
}

// ponytail: image/* only, and explicitly NOT svg — an SVG can carry
// inline <script>/<foreignObject>, and with Content-Disposition: inline
// on a public bucket a leaked URL becomes an XSS page in the bucket
// origin. (attachments enforces R2_ALLOWED_CONTENT_TYPES, which omits
// svg by default; we mirror that intent here.)
const contentType = parsed.data.contentType.toLowerCase();
if (!contentType.startsWith("image/") || contentType === "image/svg+xml") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SVG rejection uses an exact-string match (contentType === "image/svg+xml"), but the comment justifies the block by saying SVG can carry <script>/<foreignObject> in an inline-disposition bucket. A client that sends image/svg+xml; charset=utf-8 (or any other parameter, or trailing whitespace after lowercase) bypasses the check entirely — R2 stores the object with that content-type, and the comment's XSS concern still applies.

Either tighten to contentType.startsWith("image/svg") or, better, mirror the attachment route and validate against R2_ALLOWED_CONTENT_TYPES so the SVG exclusion is centrally configured.

Suggested change
if (!contentType.startsWith("image/") || contentType === "image/svg+xml") {
if (!contentType.startsWith("image/") || contentType.startsWith("image/svg")) {

return NextResponse.json({ code: "CONTENT_TYPE_NOT_ALLOWED", contentType }, { status: 400 });
}

const cap = maxBytes();
if (parsed.data.sizeBytes > cap) {
return NextResponse.json(
{ code: "FILE_TOO_LARGE", maxBytes: cap, sizeBytes: parsed.data.sizeBytes },
{ status: 400 },
);
}

const key = `u/${user.id}/avatar/${generateId()}-${safeFilename(parsed.data.name)}`;

let uploadUrl: string;
try {
uploadUrl = await presignPut({ key, contentLength: parsed.data.sizeBytes });
Comment thread
FireTable marked this conversation as resolved.
} catch (e) {
if (e instanceof R2NotConfiguredError) return notConfiguredResponse();
throw e;
}

return NextResponse.json(
{
key,
uploadUrl,
publicUrl: buildPublicUrl(key),
uploadHeaders: { "Content-Type": contentType, "Content-Disposition": "inline" },
},
{ status: 201 },
);
});
55 changes: 55 additions & 0 deletions app/api/avatar/route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { NextResponse } from "next/server";

import { R2NotConfiguredError, deleteObject, getR2PublicBaseUrl } from "@/lib/r2/client";
import { withAuth } from "@/lib/auth/with-auth";

// ponytail: avatars have no DB row, so nothing sweeps the old R2 object when
// the user deletes or replaces their avatar — it would leak forever at its
// public URL. This route deletes the object behind an avatar URL, owner-
// scoped: the key must live under u/<user.id>/avatar/. External avatar URLs
// (a github/google-hosted image) aren't ours → 204 no-op. Idempotent: an
// already-gone object is a success.

function notConfiguredResponse(): NextResponse {
return NextResponse.json(
{
code: "AVATAR_UPLOADS_NOT_CONFIGURED",
message: "Set the R2_* env vars to enable avatar uploads (see docs/ATTACHMENTS.md).",
},
{ status: 503 },
);
}

export const DELETE = withAuth(async (req, { user }) => {
let body: unknown;
try {
body = await req.json();
} catch {
return NextResponse.json({ code: "BAD_REQUEST", error: "invalid JSON" }, { status: 400 });
}

const url = (body as { url?: unknown })?.url;
if (typeof url !== "string" || url.length === 0) {
return NextResponse.json({ code: "BAD_REQUEST", error: "url required" }, { status: 400 });
}

let base: string;
try {
base = getR2PublicBaseUrl();
} catch (e) {
if (e instanceof R2NotConfiguredError) return notConfiguredResponse();
throw e;
}

// Not one of our R2 objects (e.g. an OAuth-hosted avatar) → nothing to do.
if (!url.startsWith(`${base}/`)) return new NextResponse(null, { status: 204 });

const key = url.slice(base.length + 1);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url.slice(base.length + 1) doesn't strip query strings or fragments. A URL like https://file.example/u/<id>/avatar/x.png?v=1 produces key u/<id>/avatar/x.png?v=1, and deleteObject fails silently via the .catch(() => undefined) swallow — the user thinks their old avatar was swept but the object is still there. Same for #frag.

Parse the URL first so only the pathname reaches R2:

Suggested change
const key = url.slice(base.length + 1);
const parsedUrl = new URL(url);
if (parsedUrl.origin !== new URL(base).origin) return new NextResponse(null, { status: 204 });
const key = parsedUrl.pathname.slice(1);

const prefix = `u/${user.id}/avatar/`;
if (!key.startsWith(prefix)) {
return NextResponse.json({ code: "FORBIDDEN" }, { status: 403 });
}

await deleteObject(key).catch(() => undefined);

@cubic-dev-ai cubic-dev-ai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Avatar cleanup can silently fail and leave public R2 objects orphaned, because every deleteObject error is converted to a 204. Consider only treating missing-object responses as success and surfacing config/R2 failures so failed cleanup is visible.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/api/avatar/route.ts, line 53:

<comment>Avatar cleanup can silently fail and leave public R2 objects orphaned, because every `deleteObject` error is converted to a 204. Consider only treating missing-object responses as success and surfacing config/R2 failures so failed cleanup is visible.</comment>

<file context>
@@ -0,0 +1,55 @@
+    return NextResponse.json({ code: "FORBIDDEN" }, { status: 403 });
+  }
+
+  await deleteObject(key).catch(() => undefined);
+  return new NextResponse(null, { status: 204 });
+});
</file context>
Fix with cubic

return new NextResponse(null, { status: 204 });
});
14 changes: 7 additions & 7 deletions app/api/memory/profile/route.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { NextResponse } from "next/server";

import { withAuth } from "@/lib/auth/with-auth";
import { getAuthInfo, getMemoryDoc, getRecentThreadSummaries } from "@/lib/memory/queries";
import {
EMPTY_AUTH_INFO,
getAuthInfo,
getMemoryDoc,
getRecentThreadSummaries,
} from "@/lib/memory/queries";

// ponytail: rule #9 — withAuth wraps every handler. The handler
// returns store and auth as separate fields so the frontend can
Expand All @@ -19,12 +24,7 @@ export const GET = withAuth(async (_req, { user }) => {
try {
const [store, auth, threads] = await Promise.all([
getMemoryDoc(user.id).catch(() => ({})),
getAuthInfo(user.id).catch(() => ({
name: null,
email: null,
image: null,
socials: [] as Array<{ provider: string }>,
})),
getAuthInfo(user.id).catch(() => EMPTY_AUTH_INFO),
getRecentThreadSummaries(user.id).catch(() => []),
]);
return NextResponse.json({ store, auth, threads });
Expand Down
9 changes: 2 additions & 7 deletions backend/memory/recall.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { LRUCache } from "lru-cache";
import type { RunnableConfig } from "@langchain/core/runnables";

import { getAuthInfo, getMemoryDoc } from "@/lib/memory/queries";
import { EMPTY_AUTH_INFO, getAuthInfo, getMemoryDoc } from "@/lib/memory/queries";
import { mergeMemory } from "@/lib/memory/merge";
import type { MemoryDoc } from "@/lib/memory/queries";

Expand Down Expand Up @@ -50,12 +50,7 @@ export function extractThreadId(
export async function loadMemory(userId: string): Promise<LoadedMemory> {
const [doc, auth] = await Promise.all([
getMemoryDoc(userId).catch(() => ({})),
getAuthInfo(userId).catch(() => ({
name: null,
email: null,
image: null,
socials: [] as Array<{ provider: string }>,
})),
getAuthInfo(userId).catch(() => EMPTY_AUTH_INFO),
]);
return { memory: mergeMemory(doc, auth) };
}
Expand Down
2 changes: 1 addition & 1 deletion backend/memory/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { formatSummaryText } from "@/lib/langgraph/format-summary";

// ponytail: the system prompt carries TWO dynamic blocks:
// <memory> = the user's saved profile (keys + values), plus OAuth
// overlay (name / email / image / socials) when set.
// overlay (name / email / avatar / socials) when set.
// <threads> = compressed Q&A history for THIS thread, pulled from
// the store at invoke time. Mounts the historical
// content the model would otherwise lose when older
Expand Down
28 changes: 24 additions & 4 deletions backend/tool/memory/save-memory-tool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import { tool } from "@langchain/core/tools";
import { immutableJSONPatch } from "immutable-json-patch";

import { MEMORY_PROFILE_MAX_BYTES } from "@/lib/memory/constants";
import { getAuthInfo, getMemoryDoc, putMemoryDoc } from "@/lib/memory/queries";
import { mergeMemory, type AuthInfo, type MemoryDoc } from "@/lib/memory/merge";
import { getAuthInfo, getMemoryDoc, putMemoryDoc, EMPTY_AUTH_INFO } from "@/lib/memory/queries";
import { mergeMemory, type MemoryDoc } from "@/lib/memory/merge";
import { SaveMemoryInputSchema, type SaveMemoryInput } from "@/lib/memory/validators";
import { assertProfileSize, MemorySizeError } from "@/backend/memory/profile-size";
import { invalidateMemory } from "@/backend/memory/recall";
Expand Down Expand Up @@ -33,7 +33,21 @@ function extractUserId(config?: { configurable?: { userId?: unknown } }): string
return typeof raw === "string" && raw.length > 0 ? raw : null;
}

const EMPTY_AUTH: AuthInfo = { name: null, email: null, image: null, socials: [] };
// ponytail: base64 blobs (esp. `data:...;base64,` avatars/images) are the
// exact payload that blew the context window in issue #28 — a single one
// dwarfs the whole profile. Reject any string value that's a data URL or a
// long uninterrupted base64 run. Walks arrays/objects too so it can't slip
// in nested. Threshold 512 avoids flagging wallet addresses / hashes / IDs.
const DATA_URL_RE = /data:[^,]*;base64,/i;
// Covers both standard (+/) and url-safe (-_) base64 alphabets.
Comment thread
FireTable marked this conversation as resolved.
const LONG_BASE64_RE = /[A-Za-z0-9+/_-]{512,}={0,2}/;

function containsBase64(value: unknown): boolean {
if (typeof value === "string") return DATA_URL_RE.test(value) || LONG_BASE64_RE.test(value);
if (Array.isArray(value)) return value.some(containsBase64);
if (value && typeof value === "object") return Object.values(value).some(containsBase64);
return false;
}

async function impl(
input: SaveMemoryInput,
Expand All @@ -46,7 +60,7 @@ async function impl(

const [storeDoc, auth] = await Promise.all([
getMemoryDoc(userId),
getAuthInfo(userId).catch(() => EMPTY_AUTH),
getAuthInfo(userId).catch(() => EMPTY_AUTH_INFO),
]);
// ponytail: patches operate on the merged view (store + auth
// overlay) — same shape the model sees in <memory>. Validating
Expand All @@ -56,6 +70,11 @@ async function impl(
// model.
const effective = mergeMemory(storeDoc, auth);
for (const patch of patches) {
if (patch.op !== "remove" && containsBase64(patch.value)) {
throw new MemoryPatchError(
`path ${patch.path} value looks like base64 / data-URL content; save_memory does not store binary blobs`,
);
}
if ((patch.op === "replace" || patch.op === "remove") && !(patch.path.slice(1) in effective)) {
throw new MemoryPatchError(`path ${patch.path} not found in memory`);
}
Expand Down Expand Up @@ -159,6 +178,7 @@ CONSTRAINTS (DO NOT):
- DO NOT save model inferences or questions about the user.
- DO NOT save sensitive content the user wouldn't want surfaced later (passwords, financial details, medical info, intimate relationships) — use judgment.
- DO NOT save external data returned by tools (weather, prices, balances, fetched URLs) — only the user's input portion.
- DO NOT save base64 / data-URL blobs, images, or any large binary-encoded string (avatars, file contents) — they blow up the context window; store a URL or short reference instead.
- DO NOT call more than once per turn (group multiple updates into one patch set).
- DO NOT save the same or similar content under two keys (e.g. use "ask_location_cache" instead of both "ask_location_cache" and "ask_location_result").

Expand Down
Loading
Loading