Public links for spaces and folders#1906
Conversation
|
Paragon Review Skipped Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review. Please visit https://app.paragon.run to finish your review. |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| [], | ||
| ); | ||
|
|
||
| const url = `${webUrl}/c/${collectionId ?? ""}`; |
There was a problem hiding this comment.
Consider encoding the id when building the canonical link (keeps URLs valid if the id ever includes reserved chars, and matches the revalidatePath usage elsewhere).
| const url = `${webUrl}/c/${collectionId ?? ""}`; | |
| const url = `${webUrl}/c/${collectionId ? encodeURIComponent(collectionId) : ""}`; |
|
|
||
| try { | ||
| const headersList = await headers(); | ||
| const request = new Request("https://cap.so/api/collection-password", { |
There was a problem hiding this comment.
Minor: the URL here is only used to construct a Request object for checkRateLimit, but hard-coding cap.so is a bit confusing for self-hosted deployments.
| const request = new Request("https://cap.so/api/collection-password", { | |
| const request = new Request("https://example.invalid/api/collection-password", { |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export async function setVerifiedPasswordCookie(passwordHash: string) { | ||
| (await cookies()).set("x-cap-password", await encrypt(passwordHash), { | ||
| httpOnly: true, | ||
| secure: process.env.NODE_ENV === "production", | ||
| sameSite: "lax", | ||
| path: "/", | ||
| }); | ||
| } |
There was a problem hiding this comment.
x-cap-password cookie shared across all resources
The cookie is set on path: "/" and holds exactly one encrypted hash. Verifying a collection password overwrites any previously stored video-password hash, and vice versa. A user who unlocks a password-protected video share (/s/) and then verifies a collection password (/c/) will silently lose access to the video and need to re-enter its password. The same happens in reverse. The cookie value is also read by videoPasswordPredicate in public-collections.ts, so the cross-resource collision is live today. Consider scoping cookies by resource ID (e.g. x-cap-password-${id}) or using a Map-style cookie value.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/lib/password-cookie.ts
Line: 11-18
Comment:
**Single `x-cap-password` cookie shared across all resources**
The cookie is set on `path: "/"` and holds exactly one encrypted hash. Verifying a collection password overwrites any previously stored video-password hash, and vice versa. A user who unlocks a password-protected video share (`/s/`) and then verifies a collection password (`/c/`) will silently lose access to the video and need to re-enter its password. The same happens in reverse. The cookie value is also read by `videoPasswordPredicate` in `public-collections.ts`, so the cross-resource collision is live today. Consider scoping cookies by resource ID (e.g. `x-cap-password-${id}`) or using a `Map`-style cookie value.
How can I resolve this? If you propose a fix, please make it concise.| const resolvePublicCollection = cache( | ||
| async (collectionId: string): Promise<PublicCollection | null> => { | ||
| // Fetched by primary key, then gated in `resolvePublicCollectionCandidate` | ||
| // so the public/tombstone policy lives in one tested place. | ||
| const [folderRow] = await db() | ||
| .select({ | ||
| id: folders.id, | ||
| name: folders.name, | ||
| color: folders.color, | ||
| public: folders.public, | ||
| settings: folders.settings, | ||
| spaceId: folders.spaceId, | ||
| organizationId: folders.organizationId, | ||
| organizationName: organizations.name, | ||
| organizationTombstoneAt: organizations.tombstoneAt, | ||
| allowedEmailDomain: organizations.allowedEmailDomain, | ||
| organizationIconUrl: organizations.iconUrl, | ||
| ownerStripeSubscriptionStatus: users.stripeSubscriptionStatus, | ||
| ownerThirdPartyStripeSubscriptionId: | ||
| users.thirdPartyStripeSubscriptionId, | ||
| passwordHash: spaces.password, | ||
| }) | ||
| .from(folders) | ||
| .innerJoin(organizations, eq(folders.organizationId, organizations.id)) | ||
| .innerJoin(users, eq(organizations.ownerId, users.id)) | ||
| .leftJoin(spaces, eq(folders.spaceId, spaces.id)) | ||
| .where(eq(folders.id, collectionId as Folder.FolderId)) | ||
| .limit(1); | ||
|
|
||
| const folder = resolvePublicCollectionCandidate( | ||
| folderRow ? { kind: "folder" as const, ...folderRow } : null, | ||
| null, | ||
| ); | ||
|
|
||
| if (folder) { | ||
| const publicPage = resolveEffectivePublicPage( | ||
| userIsPro({ | ||
| stripeSubscriptionStatus: folder.ownerStripeSubscriptionStatus, | ||
| thirdPartyStripeSubscriptionId: | ||
| folder.ownerThirdPartyStripeSubscriptionId, | ||
| }), | ||
| folder.settings?.publicPage, | ||
| ); | ||
| const icons = await resolveIconUrls({ | ||
| ...publicPageIconKeys(publicPage, { | ||
| organizationIconUrl: folder.organizationIconUrl, | ||
| }), | ||
| }); | ||
|
|
||
| return { | ||
| id: folder.id, | ||
| kind: "folder", | ||
| name: folder.name, | ||
| color: folder.color, | ||
| description: null, | ||
| spaceId: folder.spaceId, | ||
| organizationId: folder.organizationId, | ||
| organizationName: folder.organizationName, | ||
| allowedEmailDomain: folder.allowedEmailDomain, | ||
| passwordHash: folder.passwordHash, | ||
| publicPage, | ||
| ...icons, | ||
| }; | ||
| } | ||
|
|
||
| const [spaceRow] = await db() | ||
| .select({ | ||
| id: spaces.id, | ||
| name: spaces.name, | ||
| description: spaces.description, | ||
| public: spaces.public, | ||
| settings: spaces.settings, | ||
| organizationId: spaces.organizationId, | ||
| organizationName: organizations.name, | ||
| organizationTombstoneAt: organizations.tombstoneAt, | ||
| allowedEmailDomain: organizations.allowedEmailDomain, | ||
| organizationIconUrl: organizations.iconUrl, | ||
| ownerStripeSubscriptionStatus: users.stripeSubscriptionStatus, | ||
| ownerThirdPartyStripeSubscriptionId: | ||
| users.thirdPartyStripeSubscriptionId, | ||
| passwordHash: spaces.password, | ||
| }) | ||
| .from(spaces) | ||
| .innerJoin(organizations, eq(spaces.organizationId, organizations.id)) | ||
| .innerJoin(users, eq(organizations.ownerId, users.id)) | ||
| .where(eq(spaces.id, collectionId as Space.SpaceIdOrOrganisationId)) | ||
| .limit(1); | ||
|
|
||
| const space = resolvePublicCollectionCandidate( | ||
| null, | ||
| spaceRow ? { kind: "space" as const, ...spaceRow } : null, | ||
| ); | ||
|
|
||
| if (!space) return null; | ||
|
|
||
| const publicPage = resolveEffectivePublicPage( | ||
| userIsPro({ | ||
| stripeSubscriptionStatus: space.ownerStripeSubscriptionStatus, | ||
| thirdPartyStripeSubscriptionId: | ||
| space.ownerThirdPartyStripeSubscriptionId, | ||
| }), | ||
| space.settings?.publicPage, | ||
| ); | ||
| const icons = await resolveIconUrls({ | ||
| ...publicPageIconKeys(publicPage, { | ||
| organizationIconUrl: space.organizationIconUrl, | ||
| }), | ||
| }); | ||
|
|
||
| return { | ||
| id: space.id, | ||
| kind: "space", | ||
| name: space.name, | ||
| color: null, | ||
| description: space.description, | ||
| spaceId: space.id, | ||
| organizationId: space.organizationId, | ||
| organizationName: space.organizationName, | ||
| allowedEmailDomain: space.allowedEmailDomain, | ||
| passwordHash: space.passwordHash, | ||
| publicPage, | ||
| ...icons, | ||
| }; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
resolvePublicCollection always queries folders first, then—only when the folder lookup returns nothing or a private row—queries spaces. For every space collection page render, the folder lookup is wasted, adding a serial DB round-trip. The function is memoized with React cache() so within one request it only fires once, but the first call still pays the double-query cost. Consider running both lookups in parallel with Promise.all and returning whichever resolves to a public candidate, or at minimum add a union/single-query approach.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/lib/public-collections.ts
Line: 275-399
Comment:
**Sequential DB round-trips for space collection IDs**
`resolvePublicCollection` always queries `folders` first, then—only when the folder lookup returns nothing or a private row—queries `spaces`. For every space collection page render, the folder lookup is wasted, adding a serial DB round-trip. The function is memoized with React `cache()` so within one request it only fires once, but the first call still pays the double-query cost. Consider running both lookups in parallel with `Promise.all` and returning whichever resolves to a public candidate, or at minimum add a union/single-query approach.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| } catch (error) { | ||
| console.error("Error verifying collection password:", error); | ||
| return { success: false, error: "Failed to verify password" }; | ||
| } |
There was a problem hiding this comment.
console.error fires on every wrong-password attempt
All throws inside the try block—including the expected "Invalid password" path—flow through the catch, which calls console.error. Every failed password attempt (including normal typos) will produce a server error log entry, making this noise rather than signal. The expected failure paths (!passwordHash, !valid) should either be handled before the try/catch or logged at a lower level (e.g. console.info), while only truly unexpected errors hit console.error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/collections/password.ts
Line: 64-67
Comment:
**`console.error` fires on every wrong-password attempt**
All throws inside the `try` block—including the expected `"Invalid password"` path—flow through the `catch`, which calls `console.error`. Every failed password attempt (including normal typos) will produce a server error log entry, making this noise rather than signal. The expected failure paths (`!passwordHash`, `!valid`) should either be handled before the try/catch or logged at a lower level (e.g. `console.info`), while only truly unexpected errors hit `console.error`.
How can I resolve this? If you propose a fix, please make it concise.| const [[space], access] = await Promise.all([ | ||
| db() | ||
| .select({ | ||
| organizationId: spaces.organizationId, | ||
| public: spaces.public, | ||
| }) | ||
| .from(spaces) | ||
| .where(eq(spaces.id, id)) | ||
| .limit(1), | ||
| requireSpaceManager(user.id, id).catch(() => null), | ||
| ]); |
There was a problem hiding this comment.
.catch(() => null) swallows unexpected errors as "Unauthorized"
Any error thrown by requireSpaceManager—including DB connection failures or unexpected runtime exceptions—is silently converted to null and then returned as { success: false, error: "Unauthorized" }. Real errors become invisible, which makes production debugging difficult. Narrowing the catch to only the expected denial error allows genuine failures to propagate.
| const [[space], access] = await Promise.all([ | |
| db() | |
| .select({ | |
| organizationId: spaces.organizationId, | |
| public: spaces.public, | |
| }) | |
| .from(spaces) | |
| .where(eq(spaces.id, id)) | |
| .limit(1), | |
| requireSpaceManager(user.id, id).catch(() => null), | |
| ]); | |
| const [spaceRows, access] = await Promise.all([ | |
| db() | |
| .select({ | |
| organizationId: spaces.organizationId, | |
| public: spaces.public, | |
| }) | |
| .from(spaces) | |
| .where(eq(spaces.id, id)) | |
| .limit(1), | |
| requireSpaceManager(user.id, id).catch((err) => { | |
| if (err instanceof Error && err.name === "PolicyDeniedError") return null; | |
| throw err; | |
| }), | |
| ]); | |
| const [space] = spaceRows; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/collections/visibility.ts
Line: 53-63
Comment:
**Broad `.catch(() => null)` swallows unexpected errors as "Unauthorized"**
Any error thrown by `requireSpaceManager`—including DB connection failures or unexpected runtime exceptions—is silently converted to `null` and then returned as `{ success: false, error: "Unauthorized" }`. Real errors become invisible, which makes production debugging difficult. Narrowing the catch to only the expected denial error allows genuine failures to propagate.
```suggestion
const [spaceRows, access] = await Promise.all([
db()
.select({
organizationId: spaces.organizationId,
public: spaces.public,
})
.from(spaces)
.where(eq(spaces.id, id))
.limit(1),
requireSpaceManager(user.id, id).catch((err) => {
if (err instanceof Error && err.name === "PolicyDeniedError") return null;
throw err;
}),
]);
const [space] = spaceRows;
```
How can I resolve this? If you propose a fix, please make it concise.- Store every verified share password in the x-cap-password cookie (encrypted JSON array, legacy single-hash cookies still accepted) so unlocking a collection no longer evicts a verified video password and vice versa; VideoPasswordAttachment now carries all verified hashes - Run the folder and space lookups in resolvePublicCollection in parallel instead of sequentially - Return expected password failures (typos, removed passwords) without logging so console.error stays reserved for genuine errors - Replace requireSpaceManager(...).catch(() => null) with getSpaceAccess so unexpected failures propagate instead of reading as "Unauthorized"
|
please re-review the pr @greptileai |
| password: string, | ||
| ) { | ||
| try { | ||
| if (!collectionId || typeof password !== "string") { |
There was a problem hiding this comment.
Small hardening: since server actions are callable with untrusted input, it might be worth bounding password length to avoid worst-case PBKDF2 CPU/memory on a single request.
| if (!collectionId || typeof password !== "string") { | |
| if (!collectionId || typeof password !== "string" || password.length > 1024) { | |
| return { success: false, error: "Failed to verify password" }; | |
| } |
1 participant
Adds public collection pages at /c/[id] for spaces and folders. Org members can publish a space or folder, share a link, and visitors get a branded grid of caps with optional password protection and page customization.
Greptile Summary
This PR adds public collection pages at
/c/[id]for spaces and folders, allowing org members to publish a space or folder with an optional password, email-domain restriction, custom branding, and a configurable grid/list layout. The feature is gated on the org owner's Pro plan./c/[id]): Parallel DB lookups for folder and space candidates, server-side access control (password / email restriction), paginated video grid, and branded header; previously flagged sequential DB queries are now resolved withPromise.all.password-cookie.tsnow stores an encrypted JSON array of hashes (up to 10) instead of a single hash, eliminating the cross-resource collision issue raised in the prior review.visibility,password,logo) and backendFolders.updateextension all enforce manager-level access and Pro checks before writing; un-publishing is always allowed without a Pro check.Confidence Score: 5/5
The change is safe to merge; authorization is enforced server-side on all write paths and the previously flagged issues have been addressed.
All three issues from the prior review (password cookie collision, sequential DB queries, console.error on bad passwords) are resolved. Server-side Pro and permission gates cover every write path, settings patches are schema-validated, updates are atomic via JSON_MERGE_PATCH, and password verification is rate-limited. The only new findings are style-level concerns.
Folder.tsx and FoldersDropdown.tsx — the Make public dropdown item is shown to all users who can see a folder card, not just those who can manage it.
Important Files Changed
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(web): address review feedback on pub..." | Re-trigger Greptile