Dev#50
Merged
Merged
Conversation
the guild sidebar when present - fixed UI bug with horizontal scrollable channel panel
📝 WalkthroughWalkthroughAdds guild settings management: PATCH guild endpoint (name, logo) with permission checks and validation; presign endpoint for guild icon uploads to S3; frontend GuildSettingsDialog and wiring; announcement-channel send permission enforcement and "Decree" rebranding across UI and seeds. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant Dialog as Guild Settings Dialog
participant API as API Server
participant S3 as AWS S3
participant DB as Database
User->>Dialog: Choose icon file & edit name
Dialog->>Dialog: Validate file (MIME, ≤2MB) & create preview
alt Icon upload required
Dialog->>API: POST /uploads/guild-icon/presign (filename, contentType, size)
API->>API: Validate + assert guild update permission
API->>S3: Generate presigned PUT URL
S3-->>API: Return uploadUrl & fileUrl
API-->>Dialog: { uploadUrl, fileUrl }
Dialog->>S3: PUT file to uploadUrl
S3-->>Dialog: 200 OK
end
Dialog->>API: PATCH /guilds/{guildSlug} (name?, logo?)
API->>API: Assert guild update permission
API->>DB: Update guild row
DB-->>API: Updated guild
API-->>Dialog: { success: true, guild }
Dialog->>Dialog: Revoke preview, invalidate cache, close dialog
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/guilds/handlers.ts`:
- Around line 486-488: The handler currently copies body.logo directly into
updates.logo (which becomes schema.guild.logo), allowing arbitrary external
URLs; instead validate/reject or normalize the input: in the guild update
handler where updates is built (referencing updates and body.logo), only accept
a logo value that matches your storage prefix/URL pattern (e.g. startsWith your
STORAGE_PREFIX) or accept a server-issued object key and transform it into the
canonical storage URL/server key before setting updates.logo; if the value is
missing allow deletion/null, but return a 400 error when the caller supplies a
URL that is not from your upload pipeline. Ensure the validation logic runs
before persisting to schema.guild.logo.
In `@apps/api/src/routes/v1/guilds/schema.ts`:
- Around line 111-114: The schema updateGuildRequestSchema currently allows an
empty object because both name and logo are optional; update it to reject empty
payloads by adding a refinement on updateGuildRequestSchema that ensures at
least one of the fields (name or logo) is present (e.g., refine that data.name
!== undefined || data.logo !== undefined) and provide a clear error message like
"At least one field (name or logo) must be provided".
In `@apps/api/src/routes/v1/uploads/handlers.ts`:
- Around line 155-185: The guildIconPresign handler currently allows any
authenticated user to mint S3 presigned URLs; restrict it by validating a target
guild and ensuring the caller has edit rights (organization:update) for that
guild before creating the key and signing the URL. Update guildIconPresign to
accept/validate a guild identifier (e.g., from c.req.valid or route params),
load or authorize the guild (or call the existing permission helper) to confirm
the user has organization:update/edit permission on that guild, and only then
build the S3 Key (include the validated guild id) and return the presigned
uploadUrl/fileUrl; return a 403 if authorization fails.
In `@apps/realtime/src/services/messages.ts`:
- Around line 76-91: The presign upload flow currently skips the
announcement-channel send-permission check and can be abused; update the presign
handler (presign in uploads/handlers.ts) to mirror the check in createMessage:
when target channel.type === "announcement" and channel.guildId is present,
validate memberRole via isGuildRole and call guildAuthorityHasPermissions({
role: channel.memberRole as GuildRole, isOwner: channel.memberIsOwner }, {
announcement: ["send"] }); if the check fails, reject the presign request
(return a 403/error) to prevent users without announcement:send from obtaining
upload URLs.
In `@apps/web/src/routes/_authenticated/`$guildSlug/$channelId.tsx:
- Around line 336-337: Front-end: when canSendMessages is false, also disable
the route-level dropzone (in addition to the composer) so it refuses file
drops/uploads—check the same canSendMessages flag used for the composer and
early-return or set the dropzone's disabled prop/state. Back-end: add the
announcement-send permission check to the presign upload handler (the presign
function in uploads/handlers.ts) and return a 403 or error when the requester
lacks permission so presigns aren’t issued for blocked users. Ensure both checks
reference canSendMessages (frontend) and the same permission logic used for
posting decrees (backend).
🪄 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
Run ID: f29bd335-9884-41cc-9785-37eb52a6408b
📒 Files selected for processing (24)
apps/api/src/routes/v1/guilds/handlers.tsapps/api/src/routes/v1/guilds/index.tsapps/api/src/routes/v1/guilds/routes.tsapps/api/src/routes/v1/guilds/schema.tsapps/api/src/routes/v1/uploads/handlers.tsapps/api/src/routes/v1/uploads/index.tsapps/api/src/routes/v1/uploads/routes.tsapps/api/src/routes/v1/uploads/schema.tsapps/desktop/src-tauri/tauri.conf.jsonapps/realtime/src/services/messages.tsapps/web/src/components/chat/composer/message-input.tsxapps/web/src/components/guild/guild-settings-dialog.tsxapps/web/src/components/sidebar/channel-panel/channel-panel.tsxapps/web/src/components/sidebar/channel-panel/create-channel-dialog.tsxapps/web/src/components/sidebar/channel-panel/guild-header.tsxapps/web/src/components/sidebar/channel-panel/search-bar.tsxapps/web/src/components/sidebar/channel-panel/user-bar.tsxapps/web/src/components/sidebar/guild-bar/guild-bar.tsxapps/web/src/components/sidebar/index.tsxapps/web/src/lib/permissions.tsapps/web/src/routes/_authenticated.tsxapps/web/src/routes/_authenticated/$guildSlug/$channelId.tsxpackages/auth/src/lib/auth.tspackages/auth/src/lib/permissions.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/api/src/routes/v1/guilds/handlers.ts (1)
488-494:⚠️ Potential issue | 🟠 MajorRestrict logo URLs to the guild-icon upload prefix.
Checking only
env.S3_PUBLIC_URLis still too broad: it accepts any other public object under the bucket (attachments/...,avatars/...) and can also be fooled by lookalike raw prefixes. The guild-icon presign flow always emits.../guild-icons/${guild.id}/..., so validate against that exact prefix or callers can bypass the icon-specific MIME/size restrictions.🔒 Suggested change
+ const expectedLogoPrefix = `${env.S3_PUBLIC_URL.replace(/\/$/, "")}/guild-icons/${guild.id}/` + if ( body.logo && - !body.logo.startsWith(env.S3_PUBLIC_URL.replace(/\/$/, "")) + !body.logo.startsWith(expectedLogoPrefix) ) { throw new HTTPException(HttpStatusCodes.BAD_REQUEST, { message: "Invalid logo URL", }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/v1/guilds/handlers.ts` around lines 488 - 494, Validate that body.logo is under the exact guild-icon upload prefix rather than any public S3 URL: construct the exact prefix using env.S3_PUBLIC_URL (trim trailing slash) + "/guild-icons/" + guild.id + "/" and require body.logo.startsWith(thatPrefix); if it doesn't, throw the same HTTPException. Update the check near the body.logo validation in handlers.ts (where body.logo and env.S3_PUBLIC_URL are referenced) to use this exact prefix comparison and keep the existing error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/uploads/handlers.ts`:
- Around line 19-28: Change the two local relative imports to use the API
package alias so they follow the apps/api convention: replace the import of
types AvatarPresignRoute, GuildIconPresignRoute, PresignRoute from "./routes"
with "@/routes/v1/uploads/routes" and replace the constants MAX_AVATAR_SIZE,
MAX_GUILD_ICON_SIZE, PRESIGNED_URL_EXPIRY_SECONDS from "./schema" with
"@/routes/v1/uploads/schema"; keep the same imported symbols
(AvatarPresignRoute, GuildIconPresignRoute, PresignRoute, MAX_AVATAR_SIZE,
MAX_GUILD_ICON_SIZE, PRESIGNED_URL_EXPIRY_SECONDS) so tsc-alias can rewrite them
at build time.
- Around line 86-110: The announcement upload branch currently lets
unknown/invalid roles bypass checks because isGuildRole(member.role)
short-circuits; update the logic to reject unknown roles or reuse the existing
guard: fetch guildRecord as shown, then call assertGuildPermission(member,
guildRecord, { announcement: ["send"] }) (or, if you keep the manual check,
explicitly return 403 when isGuildRole(member.role) is false) so that invalid
member.role cannot issue a presign; reference isGuildRole, guildRecord,
member.role, guildAuthorityHasPermissions, and assertGuildPermission to locate
and update the code.
In `@apps/api/src/routes/v1/uploads/routes.ts`:
- Around line 11-18: The import of schema symbols (avatarPresignRequestSchema,
avatarPresignResponseSchema, guildIconPresignRequestSchema,
guildIconPresignResponseSchema, presignRequestSchema, presignResponseSchema)
should use the package alias instead of a relative path; update the import
source from "./schema" to "@/routes/v1/uploads/schema" so the file follows the
apps/api `@/`* alias convention and tsc-alias can rewrite paths at build time.
In `@apps/web/src/components/guild/guild-settings-dialog.tsx`:
- Around line 19-25: The local validation constants MAX_GUILD_ICON_BYTES,
ACCEPTED_IMAGE_TYPES and the hard-coded accept string in GuildSettingsDialog
should be removed and replaced by importing the canonical validation contract
from the shared uploads module (the same shared place as the existing shared
upload constants); update GuildSettingsDialog to import the MAX_GUILD_ICON_BYTES
(or equivalent), ACCEPTED_IMAGE_TYPES (or equivalent) and accept string from
that shared export, and use those imports wherever the dialog currently
references the local constants or accept value so the client and API share the
same rules.
---
Duplicate comments:
In `@apps/api/src/routes/v1/guilds/handlers.ts`:
- Around line 488-494: Validate that body.logo is under the exact guild-icon
upload prefix rather than any public S3 URL: construct the exact prefix using
env.S3_PUBLIC_URL (trim trailing slash) + "/guild-icons/" + guild.id + "/" and
require body.logo.startsWith(thatPrefix); if it doesn't, throw the same
HTTPException. Update the check near the body.logo validation in handlers.ts
(where body.logo and env.S3_PUBLIC_URL are referenced) to use this exact prefix
comparison and keep the existing error handling.
🪄 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
Run ID: 84be5eca-e542-451d-867e-e52d393500c3
📒 Files selected for processing (7)
apps/api/src/routes/v1/guilds/handlers.tsapps/api/src/routes/v1/guilds/schema.tsapps/api/src/routes/v1/uploads/handlers.tsapps/api/src/routes/v1/uploads/routes.tsapps/api/src/routes/v1/uploads/schema.tsapps/web/src/components/guild/guild-settings-dialog.tsxapps/web/src/routes/_authenticated/$guildSlug/$channelId.tsx
Comment on lines
+19
to
+28
| import type { | ||
| AvatarPresignRoute, | ||
| GuildIconPresignRoute, | ||
| PresignRoute, | ||
| } from "./routes" | ||
| import { | ||
| MAX_AVATAR_SIZE, | ||
| MAX_GUILD_ICON_SIZE, | ||
| PRESIGNED_URL_EXPIRY_SECONDS, | ||
| } from "./schema" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use @/* aliases for API-local imports.
Both ./routes and ./schema are relative imports inside apps/api. Converting them to @/routes/v1/uploads/routes and @/routes/v1/uploads/schema keeps this file aligned with the package import convention.
♻️ Suggested change
import type {
AvatarPresignRoute,
GuildIconPresignRoute,
PresignRoute,
-} from "./routes"
+} from "@/routes/v1/uploads/routes"
import {
MAX_AVATAR_SIZE,
MAX_GUILD_ICON_SIZE,
PRESIGNED_URL_EXPIRY_SECONDS,
-} from "./schema"
+} from "@/routes/v1/uploads/schema"As per coding guidelines, Use @/* path alias in apps/api for imports (e.g., import foo from "@/lib/foo"). tsc-alias will rewrite these to relative .js paths in the compiled output.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import type { | |
| AvatarPresignRoute, | |
| GuildIconPresignRoute, | |
| PresignRoute, | |
| } from "./routes" | |
| import { | |
| MAX_AVATAR_SIZE, | |
| MAX_GUILD_ICON_SIZE, | |
| PRESIGNED_URL_EXPIRY_SECONDS, | |
| } from "./schema" | |
| import type { | |
| AvatarPresignRoute, | |
| GuildIconPresignRoute, | |
| PresignRoute, | |
| } from "@/routes/v1/uploads/routes" | |
| import { | |
| MAX_AVATAR_SIZE, | |
| MAX_GUILD_ICON_SIZE, | |
| PRESIGNED_URL_EXPIRY_SECONDS, | |
| } from "@/routes/v1/uploads/schema" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/uploads/handlers.ts` around lines 19 - 28, Change the
two local relative imports to use the API package alias so they follow the
apps/api convention: replace the import of types AvatarPresignRoute,
GuildIconPresignRoute, PresignRoute from "./routes" with
"@/routes/v1/uploads/routes" and replace the constants MAX_AVATAR_SIZE,
MAX_GUILD_ICON_SIZE, PRESIGNED_URL_EXPIRY_SECONDS from "./schema" with
"@/routes/v1/uploads/schema"; keep the same imported symbols
(AvatarPresignRoute, GuildIconPresignRoute, PresignRoute, MAX_AVATAR_SIZE,
MAX_GUILD_ICON_SIZE, PRESIGNED_URL_EXPIRY_SECONDS) so tsc-alias can rewrite them
at build time.
Comment on lines
+86
to
+110
| // Block uploads in announcement channels for users without permission | ||
| if (ch.type === "announcement" && isGuildRole(member.role)) { | ||
| const guildRecord = await db | ||
| .select({ ownerId: guild.ownerId }) | ||
| .from(guild) | ||
| .where(eq(guild.id, ch.guildId)) | ||
| .limit(1) | ||
| .then((rows) => rows[0]) | ||
|
|
||
| if ( | ||
| !guildRecord || | ||
| !guildAuthorityHasPermissions( | ||
| { | ||
| role: member.role as GuildRole, | ||
| isOwner: guildRecord.ownerId === member.userId, | ||
| }, | ||
| { announcement: ["send"] } | ||
| ) | ||
| ) { | ||
| return c.json( | ||
| { success: false, message: "Forbidden" }, | ||
| HttpStatusCodes.FORBIDDEN | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't let unknown roles bypass announcement upload checks.
This branch fails open today: if member.role is not a recognized GuildRole, isGuildRole(member.role) short-circuits the whole announcement permission check and the presign is still issued. Reuse assertGuildPermission(member, guildRecord, { announcement: ["send"] }) or explicitly reject unknown roles so bad role data returns 403 instead of allowing uploads.
🛡️ Suggested change
- if (ch.type === "announcement" && isGuildRole(member.role)) {
+ if (ch.type === "announcement") {
const guildRecord = await db
.select({ ownerId: guild.ownerId })
.from(guild)
.where(eq(guild.id, ch.guildId))
.limit(1)
.then((rows) => rows[0])
- if (
- !guildRecord ||
- !guildAuthorityHasPermissions(
- {
- role: member.role as GuildRole,
- isOwner: guildRecord.ownerId === member.userId,
- },
- { announcement: ["send"] }
- )
- ) {
+ if (!guildRecord) {
return c.json(
{ success: false, message: "Forbidden" },
HttpStatusCodes.FORBIDDEN
)
}
+
+ assertGuildPermission(member, guildRecord, {
+ announcement: ["send"],
+ })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Block uploads in announcement channels for users without permission | |
| if (ch.type === "announcement" && isGuildRole(member.role)) { | |
| const guildRecord = await db | |
| .select({ ownerId: guild.ownerId }) | |
| .from(guild) | |
| .where(eq(guild.id, ch.guildId)) | |
| .limit(1) | |
| .then((rows) => rows[0]) | |
| if ( | |
| !guildRecord || | |
| !guildAuthorityHasPermissions( | |
| { | |
| role: member.role as GuildRole, | |
| isOwner: guildRecord.ownerId === member.userId, | |
| }, | |
| { announcement: ["send"] } | |
| ) | |
| ) { | |
| return c.json( | |
| { success: false, message: "Forbidden" }, | |
| HttpStatusCodes.FORBIDDEN | |
| ) | |
| } | |
| } | |
| // Block uploads in announcement channels for users without permission | |
| if (ch.type === "announcement") { | |
| const guildRecord = await db | |
| .select({ ownerId: guild.ownerId }) | |
| .from(guild) | |
| .where(eq(guild.id, ch.guildId)) | |
| .limit(1) | |
| .then((rows) => rows[0]) | |
| if (!guildRecord) { | |
| return c.json( | |
| { success: false, message: "Forbidden" }, | |
| HttpStatusCodes.FORBIDDEN | |
| ) | |
| } | |
| assertGuildPermission(member, guildRecord, { | |
| announcement: ["send"], | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/uploads/handlers.ts` around lines 86 - 110, The
announcement upload branch currently lets unknown/invalid roles bypass checks
because isGuildRole(member.role) short-circuits; update the logic to reject
unknown roles or reuse the existing guard: fetch guildRecord as shown, then call
assertGuildPermission(member, guildRecord, { announcement: ["send"] }) (or, if
you keep the manual check, explicitly return 403 when isGuildRole(member.role)
is false) so that invalid member.role cannot issue a presign; reference
isGuildRole, guildRecord, member.role, guildAuthorityHasPermissions, and
assertGuildPermission to locate and update the code.
Comment on lines
11
to
18
| import { | ||
| avatarPresignRequestSchema, | ||
| avatarPresignResponseSchema, | ||
| guildIconPresignRequestSchema, | ||
| guildIconPresignResponseSchema, | ||
| presignRequestSchema, | ||
| presignResponseSchema, | ||
| } from "./schema" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the API alias for this schema import.
./schema is still a relative import inside apps/api. Switch it to @/routes/v1/uploads/schema so this file follows the package-wide alias convention.
♻️ Suggested change
import {
avatarPresignRequestSchema,
avatarPresignResponseSchema,
guildIconPresignRequestSchema,
guildIconPresignResponseSchema,
presignRequestSchema,
presignResponseSchema,
-} from "./schema"
+} from "@/routes/v1/uploads/schema"As per coding guidelines, Use @/* path alias in apps/api for imports (e.g., import foo from "@/lib/foo"). tsc-alias will rewrite these to relative .js paths in the compiled output.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { | |
| avatarPresignRequestSchema, | |
| avatarPresignResponseSchema, | |
| guildIconPresignRequestSchema, | |
| guildIconPresignResponseSchema, | |
| presignRequestSchema, | |
| presignResponseSchema, | |
| } from "./schema" | |
| import { | |
| avatarPresignRequestSchema, | |
| avatarPresignResponseSchema, | |
| guildIconPresignRequestSchema, | |
| guildIconPresignResponseSchema, | |
| presignRequestSchema, | |
| presignResponseSchema, | |
| } from "@/routes/v1/uploads/schema" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/uploads/routes.ts` around lines 11 - 18, The import of
schema symbols (avatarPresignRequestSchema, avatarPresignResponseSchema,
guildIconPresignRequestSchema, guildIconPresignResponseSchema,
presignRequestSchema, presignResponseSchema) should use the package alias
instead of a relative path; update the import source from "./schema" to
"@/routes/v1/uploads/schema" so the file follows the apps/api `@/`* alias
convention and tsc-alias can rewrite paths at build time.
Comment on lines
+19
to
+25
| const MAX_GUILD_ICON_BYTES = 2 * 1024 * 1024 | ||
| const ACCEPTED_IMAGE_TYPES = [ | ||
| "image/jpeg", | ||
| "image/png", | ||
| "image/webp", | ||
| "image/svg+xml", | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Move the guild-icon validation contract into a shared module.
MAX_GUILD_ICON_BYTES, ACCEPTED_IMAGE_TYPES, and the hard-coded accept string duplicate the API rules in apps/api/src/routes/v1/uploads/schema.ts. Once one side changes, the dialog will either reject a valid file locally or let the user pick one the presign route refuses. This belongs in a shared package, similar to the existing shared upload constants.
Also applies to: 254-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/guild/guild-settings-dialog.tsx` around lines 19 -
25, The local validation constants MAX_GUILD_ICON_BYTES, ACCEPTED_IMAGE_TYPES
and the hard-coded accept string in GuildSettingsDialog should be removed and
replaced by importing the canonical validation contract from the shared uploads
module (the same shared place as the existing shared upload constants); update
GuildSettingsDialog to import the MAX_GUILD_ICON_BYTES (or equivalent),
ACCEPTED_IMAGE_TYPES (or equivalent) and accept string from that shared export,
and use those imports wherever the dialog currently references the local
constants or accept value so the client and API share the same rules.
upload permissions
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api/src/routes/v1/uploads/handlers.ts (1)
19-28: 🛠️ Refactor suggestion | 🟠 MajorSwitch these API-local imports to
@/aliases.Lines 19-28 still use relative imports inside
apps/api, which is inconsistent with the API package build convention.♻️ Suggested change
import type { AvatarPresignRoute, GuildIconPresignRoute, PresignRoute, -} from "./routes" +} from "@/routes/v1/uploads/routes" import { MAX_AVATAR_SIZE, MAX_GUILD_ICON_SIZE, PRESIGNED_URL_EXPIRY_SECONDS, -} from "./schema" +} from "@/routes/v1/uploads/schema"As per coding guidelines,
Use@/* path alias in apps/api for imports (e.g., import foo from "@/lib/foo"). tsc-alias will rewrite these to relative .js paths in the compiled output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/v1/uploads/handlers.ts` around lines 19 - 28, Update the local imports in this file to use the API package path alias instead of relative paths: replace the import of types (AvatarPresignRoute, GuildIconPresignRoute, PresignRoute) and constants (MAX_AVATAR_SIZE, MAX_GUILD_ICON_SIZE, PRESIGNED_URL_EXPIRY_SECONDS) so they come from "@/routes" and "@/schema" respectively; ensure the imported symbol names remain unchanged so tsc-alias can rewrite them correctly during build.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/guilds/handlers.ts`:
- Around line 488-493: The current check compares the raw body.logo string with
guildIconPrefix causing paths containing "../" to bypass validation; update the
validation in the handler to parse and normalize the incoming logo URL (use the
URL constructor) and compare the normalized origin+pathname (ensure
env.S3_PUBLIC_URL is normalized without trailing slash and the pathname is
compared after URL pathname normalization) against guildIconPrefix; if URL
parsing fails or the normalized URL does not start with the expected prefix
(constructed with guild.id), throw the existing
HTTPException(HttpStatusCodes.BAD_REQUEST, { message: "Invalid logo URL" }).
---
Duplicate comments:
In `@apps/api/src/routes/v1/uploads/handlers.ts`:
- Around line 19-28: Update the local imports in this file to use the API
package path alias instead of relative paths: replace the import of types
(AvatarPresignRoute, GuildIconPresignRoute, PresignRoute) and constants
(MAX_AVATAR_SIZE, MAX_GUILD_ICON_SIZE, PRESIGNED_URL_EXPIRY_SECONDS) so they
come from "@/routes" and "@/schema" respectively; ensure the imported symbol
names remain unchanged so tsc-alias can rewrite them correctly during build.
🪄 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
Run ID: 1ef1de06-aa3d-47eb-b243-f10bc6551580
📒 Files selected for processing (2)
apps/api/src/routes/v1/guilds/handlers.tsapps/api/src/routes/v1/uploads/handlers.ts
Comment on lines
+488
to
+493
| const guildIconPrefix = `${env.S3_PUBLIC_URL.replace(/\/$/, "")}/guild-icons/${guild.id}/` | ||
| if (body.logo && !body.logo.startsWith(guildIconPrefix)) { | ||
| throw new HTTPException(HttpStatusCodes.BAD_REQUEST, { | ||
| message: "Invalid logo URL", | ||
| }) | ||
| } |
There was a problem hiding this comment.
Normalize the logo URL before enforcing the guild prefix.
Lines 488-489 compare the raw string with startsWith(). A value like .../guild-icons/<guildId>/../other/object.png still passes this check, but URL parsers normalize it to a different path. Validate the normalized URL instead of the raw input.
🛡️ Proposed fix
- const guildIconPrefix = `${env.S3_PUBLIC_URL.replace(/\/$/, "")}/guild-icons/${guild.id}/`
- if (body.logo && !body.logo.startsWith(guildIconPrefix)) {
- throw new HTTPException(HttpStatusCodes.BAD_REQUEST, {
- message: "Invalid logo URL",
- })
- }
+ const publicBaseUrl = new URL(env.S3_PUBLIC_URL.replace(/\/?$/, "/"))
+ const guildIconPrefix = new URL(
+ `guild-icons/${guild.id}/`,
+ publicBaseUrl
+ ).href
+ if (body.logo) {
+ const logoUrl = new URL(body.logo)
+ if (!logoUrl.href.startsWith(guildIconPrefix)) {
+ throw new HTTPException(HttpStatusCodes.BAD_REQUEST, {
+ message: "Invalid logo URL",
+ })
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const guildIconPrefix = `${env.S3_PUBLIC_URL.replace(/\/$/, "")}/guild-icons/${guild.id}/` | |
| if (body.logo && !body.logo.startsWith(guildIconPrefix)) { | |
| throw new HTTPException(HttpStatusCodes.BAD_REQUEST, { | |
| message: "Invalid logo URL", | |
| }) | |
| } | |
| const publicBaseUrl = new URL(env.S3_PUBLIC_URL.replace(/\/?$/, "/")) | |
| const guildIconPrefix = new URL( | |
| `guild-icons/${guild.id}/`, | |
| publicBaseUrl | |
| ).href | |
| if (body.logo) { | |
| const logoUrl = new URL(body.logo) | |
| if (!logoUrl.href.startsWith(guildIconPrefix)) { | |
| throw new HTTPException(HttpStatusCodes.BAD_REQUEST, { | |
| message: "Invalid logo URL", | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/guilds/handlers.ts` around lines 488 - 493, The
current check compares the raw body.logo string with guildIconPrefix causing
paths containing "../" to bypass validation; update the validation in the
handler to parse and normalize the incoming logo URL (use the URL constructor)
and compare the normalized origin+pathname (ensure env.S3_PUBLIC_URL is
normalized without trailing slash and the pathname is compared after URL
pathname normalization) against guildIconPrefix; if URL parsing fails or the
normalized URL does not start with the expected prefix (constructed with
guild.id), throw the existing HTTPException(HttpStatusCodes.BAD_REQUEST, {
message: "Invalid logo URL" }).
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
1 participant
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.
Pull Request Summary: Dev (PR #50)
Overview
This PR implements guild settings management (update name & icon) with presigned icon uploads and tightens posting permissions for announcement channels (user-facing "decree"). It adds backend endpoints and schemas, corresponding frontend UI to edit guild settings and presign/upload icons, and permission checks in realtime and upload flows so only roles with announcement.send may post in announcement channels.
Key Changes
Backend API Endpoints
Permission System
Frontend Components & UX
Schemas, Types & Validation
Miscellaneous
Code Quality Observations
✅ Strengths
Scope & Review Effort
Confidence Score: 4/5
Well-structured and consistent with existing patterns; types and server-side permission checks are sound. Lacks automated tests and a couple small auth/clarity items to confirm before merging.