Open
Conversation
Contributor
📝 WalkthroughWalkthroughRenames avatar/banner fields across profile-editor (DB, types, services, client), adds URL download+upload to file-manager for webhooks, introduces public file preview endpoint and isPublic file flag, updates profile-editor port/base URL (3006→3007), adds marketplace entry and ProfessionalProfile JSON Schema, and extends dev-sandbox identities with displayName. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FileManagerAPI as File Manager API
participant FileService
participant DB as DB/Storage
Client->>FileManagerAPI: GET /api/public/files/:id
FileManagerAPI->>FileService: getFileByIdPublic(id)
FileService->>DB: Query file by id where isPublic = true
DB-->>FileService: file record / null
alt file record has file.url
FileManagerAPI-->>Client: 302 Redirect to file.url
else file record has file.data
FileManagerAPI-->>Client: 200 File bytes (Content-Type, Content-Disposition, Content-Length, Cache-Control: public, max-age=3600)
else not found or soft-deleted
FileManagerAPI-->>Client: 404 { error: "File not found" }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
platforms/pictique/api/src/controllers/WebhookController.ts (1)
44-48: Add logging for skipped unmapped schemas; consider validating mapping before acquiring lock.Unmapped webhooks currently return early with no log trace, making production debugging harder. While locks auto-expire after 15 seconds, moving the validation before lock acquisition keeps intent clear and avoids temporary lock contention for invalid payloads.
♻️ Suggested improvement
const mapping = Object.values(this.adapter.mapping).find( (m) => m.schemaId === schemaId ); if (!mapping) { + console.warn( + `[WebhookController] Skipping unmapped schemaId=${schemaId}, globalId=${globalId}` + ); return res.status(200).send(); } + + this.adapter.addToLockedIds(globalId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/pictique/api/src/controllers/WebhookController.ts` around lines 44 - 48, In WebhookController, move the mapping validation to occur before calling this.adapter.addToLockedIds(globalId) and add a clear log entry when mapping is missing; specifically, check the existence of mapping (the variable referenced in the snippet) at the top of the handler and if absent call the controller/logger (e.g., use this.logger or resourced logger used elsewhere in WebhookController) to record the unmapped schema and incoming metadata, then return res.status(200).send() without acquiring the lock to avoid unnecessary lock contention and make skipped webhooks observable.platforms/profile-editor/api/src/utils/file-proxy.ts (1)
36-46: Consider SSRF mitigations for URL fetching.This function downloads from arbitrary URLs provided in webhook data. While the timeout is set (15s), consider adding:
- URL scheme validation (allow only
https://)- Blocklist for internal/private IP ranges to prevent SSRF attacks
🛡️ Example validation
} else { + // Basic SSRF mitigation + const parsed = new URL(url); + if (!['http:', 'https:'].includes(parsed.protocol)) { + console.warn('Rejected non-HTTP URL:', url); + return null; + } const response = await axios.get(url, { responseType: "arraybuffer", timeout: 15_000, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/utils/file-proxy.ts` around lines 36 - 46, The download branch that calls axios.get with the variable url must validate the target before fetching to mitigate SSRF: ensure the URL has an allowed scheme (only "https"), parse the hostname and resolve it to an IP and block private/internal ranges (e.g., 10.0.0.0/8, 127.0.0.0/8, 169.254.0.0/16, 172.16.0.0/12, 192.168.0.0/16, ::1, fd00::/8, fe80::/10) and deny fetches if matched; perform these checks immediately before the axios.get call in file-proxy.ts (the block that sets responseType and timeout), and throw/return an error if validation fails. Also ensure any error path logs or surfaces the reason and does not proceed to Buffer.from(response.data) or header parsing when blocked.platforms/profile-editor/client/src/lib/utils/file-manager.ts (1)
4-4: Prefer a single source of truth for Profile Editor base URL.
API_BASEduplicates fallback logic already present inapiClient; this can drift again on future port/env changes.♻️ Suggested refactor
-const API_BASE = () => PUBLIC_PROFILE_EDITOR_BASE_URL || 'http://localhost:3007'; +const API_BASE = () => apiClient.defaults.baseURL ?? PUBLIC_PROFILE_EDITOR_BASE_URL ?? 'http://localhost:3007';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/utils/file-manager.ts` at line 4, API_BASE in file-manager duplicates the base URL fallback logic; remove the local API_BASE function and consume the single source from the existing apiClient module instead (e.g., import and use the exported base-url/getBaseUrl/API_BASE symbol from apiClient). Update any usage in file-manager.ts to reference that imported symbol so the fallback logic and env precedence are maintained centrally in apiClient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 95: Remove the surrounding quotes from the PUBLIC_PROFILE_EDITOR_BASE_URL
entry in the .env example to satisfy dotenv-linter's QuoteCharacter rule; locate
the PUBLIC_PROFILE_EDITOR_BASE_URL="http://localhost:3007" line and change it to
an unquoted value so it reads
PUBLIC_PROFILE_EDITOR_BASE_URL=http://localhost:3007.
In `@infrastructure/dev-sandbox/src/routes/`+page.svelte:
- Around line 472-475: The current update uses selectedIndex which can be stale;
instead locate the provisioned identity by its stable key (e.g., keyId or w3id)
and update that entry immutably: find the identity in the identities array whose
key matches the provisioned key from the provisioning result, create a
shallow-copied array with that element replaced to include profile.displayName,
assign identities to the new array and call saveIdentities(newArray); update the
code around selectedIndex, identities, saveIdentities and use the provisioned
identity key (keyId/w3id) and profile.displayName to perform the backfill.
In `@platforms/file-manager/api/src/controllers/FileController.ts`:
- Around line 574-577: The publicPreview endpoint currently calls
FileService.getFileByIdPublic which exposes any non-deleted file without auth;
update publicPreview to enforce access control by either calling a service
method that verifies requester permissions (e.g.,
FileService.getFileByIdWithAccess or a new FileService.checkAccessForFile) or by
validating the returned file's visibility/owner and the requester's auth (check
file.deleted, file.visibility/permissions and req.user) and return 403/404 when
access is not allowed; reference FileController.publicPreview and
FileService.getFileByIdPublic when making the change so the controller only
serves files that pass the access check.
- Around line 570-600: In publicPreview (controller method publicPreview calling
fileService.getFileByIdPublic) add a null check for file.data before using the
legacy fallback: if file.url is falsy and file.data is null, return an
appropriate error response (e.g., 404 or 410 with a JSON error) and log the
condition; otherwise continue to set headers and send file.data. Ensure the
check references the File entity's nullable data field so you don't send null to
res.send and avoid downstream errors.
In `@platforms/file-manager/api/src/index.ts`:
- Line 83: The public GET route app.get("/api/public/files/:id",
fileController.publicPreview) delegates to FileService.getFileByIdPublic which
currently lacks access control; update the service
(FileService.getFileByIdPublic) to enforce access rules by either (A) adding an
isPublic boolean to the File entity and having getFileByIdPublic verify
file.isPublic before returning it, or (B) restricting the endpoint/service to
only allow specific kinds of files (e.g., only profile avatars/banners) by
checking file.type or file.context in getFileByIdPublic and returning 403/404
for disallowed files; ensure the controller still calls the secured service
method and return appropriate error codes when access is denied.
In `@platforms/file-manager/api/src/services/FileService.ts`:
- Around line 117-125: The getFileByIdPublic method currently returns any
non-deleted file by id; add a boolean visibility flag to the File entity (e.g.,
isPublic: boolean, default false), update the repository query in
getFileByIdPublic to include where: { id, isPublic: true } (or use
fileRepository.findOne({ where: { id, isPublic: true } })) and ensure any
existing creation/update flows (e.g., File constructor, save/update handlers)
set isPublic appropriately; also add a migration to persist the new isPublic
column and update any tests that assume public access.
In `@platforms/profile-editor/api/src/controllers/WebhookController.ts`:
- Around line 81-90: Before calling downloadUrlAndUploadToFileManager for
rawBody.data.avatarUrl or bannerUrl, validate and reject unsafe URLs to prevent
SSRF: add an isUrlAllowed(url) helper that parses the URL (new URL()), rejects
non-http/https schemes (e.g., file://), rejects hostnames like "localhost" or
literal loopback/Private/Link-local IP ranges (127.0.0.0/8, 10.0.0.0/8,
172.16.0.0/12, 192.168.0.0/16, ::1, fc00::/7, fe80::/10) and resolves DNS names
to check resolved IPs before fetching; call isUrlAllowed(...) in the
WebhookController around downloadUrlAndUploadToFileManager and skip/log and
return null for disallowed URLs so userData.avatar/banner are not set from
unsafe sources.
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts`:
- Around line 320-337: The code currently builds patch by cloning the full
existing payload (variable patch = { ...existing }) and then sends it with acl:
["*"] in UPDATE_MUTATION/CREATE_MUTATION, which unintentionally makes the entire
user envelope public; instead, modify EVaultProfileService.ts to only send the
minimal fields that need to be public (e.g., avatarUrl and bannerUrl) or create
a separate public envelope for media, and do not overwrite or set acl to ["*"]
on the entire USER_ONTOLOGY payload—preserve the existing ACL when updating the
main user record (use existing.acl if needed) or explicitly set acl only on the
new public envelope; update the branches that reference patch, UPDATE_MUTATION,
CREATE_MUTATION, userNode, existing, and eName accordingly so only intended
fields become world-readable.
---
Nitpick comments:
In `@platforms/pictique/api/src/controllers/WebhookController.ts`:
- Around line 44-48: In WebhookController, move the mapping validation to occur
before calling this.adapter.addToLockedIds(globalId) and add a clear log entry
when mapping is missing; specifically, check the existence of mapping (the
variable referenced in the snippet) at the top of the handler and if absent call
the controller/logger (e.g., use this.logger or resourced logger used elsewhere
in WebhookController) to record the unmapped schema and incoming metadata, then
return res.status(200).send() without acquiring the lock to avoid unnecessary
lock contention and make skipped webhooks observable.
In `@platforms/profile-editor/api/src/utils/file-proxy.ts`:
- Around line 36-46: The download branch that calls axios.get with the variable
url must validate the target before fetching to mitigate SSRF: ensure the URL
has an allowed scheme (only "https"), parse the hostname and resolve it to an IP
and block private/internal ranges (e.g., 10.0.0.0/8, 127.0.0.0/8,
169.254.0.0/16, 172.16.0.0/12, 192.168.0.0/16, ::1, fd00::/8, fe80::/10) and
deny fetches if matched; perform these checks immediately before the axios.get
call in file-proxy.ts (the block that sets responseType and timeout), and
throw/return an error if validation fails. Also ensure any error path logs or
surfaces the reason and does not proceed to Buffer.from(response.data) or header
parsing when blocked.
In `@platforms/profile-editor/client/src/lib/utils/file-manager.ts`:
- Line 4: API_BASE in file-manager duplicates the base URL fallback logic;
remove the local API_BASE function and consume the single source from the
existing apiClient module instead (e.g., import and use the exported
base-url/getBaseUrl/API_BASE symbol from apiClient). Update any usage in
file-manager.ts to reference that imported symbol so the fallback logic and env
precedence are maintained centrally in apiClient.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f57c6f50-44d9-4a9a-8d3e-2eb99e284c04
📒 Files selected for processing (26)
.env.exampleinfrastructure/dev-sandbox/src/routes/+page.svelteplatforms/file-manager/api/src/controllers/FileController.tsplatforms/file-manager/api/src/index.tsplatforms/file-manager/api/src/services/FileService.tsplatforms/marketplace/client/client/src/data/apps.jsonplatforms/marketplace/client/client/src/pages/app-detail.tsxplatforms/pictique/api/src/controllers/WebhookController.tsplatforms/profile-editor/api/src/controllers/ProfileController.tsplatforms/profile-editor/api/src/controllers/WebhookController.tsplatforms/profile-editor/api/src/database/entities/User.tsplatforms/profile-editor/api/src/database/migrations/1775600000000-RenameAvatarBannerColumns.tsplatforms/profile-editor/api/src/index.tsplatforms/profile-editor/api/src/services/EVaultProfileService.tsplatforms/profile-editor/api/src/services/EVaultSyncService.tsplatforms/profile-editor/api/src/services/UserSearchService.tsplatforms/profile-editor/api/src/types/profile.tsplatforms/profile-editor/api/src/utils/file-proxy.tsplatforms/profile-editor/api/src/web3adapter/mappings/professional-profile.mapping.jsonplatforms/profile-editor/api/src/web3adapter/mappings/user.mapping.jsonplatforms/profile-editor/client/src/lib/components/profile/ProfileHeader.svelteplatforms/profile-editor/client/src/lib/stores/discovery.tsplatforms/profile-editor/client/src/lib/stores/profile.tsplatforms/profile-editor/client/src/lib/utils/axios.tsplatforms/profile-editor/client/src/lib/utils/file-manager.tsservices/ontology/schemas/professionalProfile.json
💤 Files with no reviewable changes (2)
- platforms/profile-editor/api/src/web3adapter/mappings/user.mapping.json
- platforms/profile-editor/api/src/web3adapter/mappings/professional-profile.mapping.json
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
platforms/profile-editor/client/src/lib/components/profile/ProfileHeader.svelte (2)
50-64: Revoke blob URLs to prevent memory leaks.
URL.createObjectURL()allocates browser memory that persists until explicitly revoked or page unload. Repeated uploads will accumulate unreleased blob URLs.♻️ Proposed fix for handleAvatarUpload
uploadingAvatar = true; try { const result = await uploadFile(file); await updateProfile({ avatar: result.id }); + URL.revokeObjectURL(avatarPreview!); avatarPreview = null; toast.success('Avatar updated'); } catch { toast.error('Failed to upload avatar'); + URL.revokeObjectURL(avatarPreview!); avatarPreview = null; } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/components/profile/ProfileHeader.svelte` around lines 50 - 64, The blob URL created with URL.createObjectURL(file) (assigned to avatarPreview) must be revoked to avoid leaks: before assigning a new avatarPreview revoke any previous URL (if avatarPreview) using URL.revokeObjectURL(avatarPreview), and after the upload completes or fails ensure you revoke the created blob URL when you clear avatarPreview (in both the catch and finally paths) so that neither successful nor failed uploads leave unreleased blob URLs; update the logic around avatarPreview in the upload flow (the code that calls uploadFile and updateProfile — e.g., the handleAvatarUpload / upload handler where avatarPreview, uploadingAvatar, uploadFile, updateProfile are used) to perform these revocations.
72-86: Same blob URL leak in banner upload.Apply the same
URL.revokeObjectURL()fix here.♻️ Proposed fix for handleBannerUpload
uploadingBanner = true; try { const result = await uploadFile(file); await updateProfile({ banner: result.id }); + URL.revokeObjectURL(bannerPreview!); bannerPreview = null; toast.success('Banner updated'); } catch { toast.error('Failed to upload banner'); + URL.revokeObjectURL(bannerPreview!); bannerPreview = null; } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/client/src/lib/components/profile/ProfileHeader.svelte` around lines 72 - 86, The banner upload flow creates a blob URL via URL.createObjectURL(file) and never revokes it, leaking blobs; update the handler around the uploadFile/updateProfile logic (the code that sets bannerPreview and calls uploadFile and updateProfile) to call URL.revokeObjectURL(bannerPreview) when the blob URL is no longer needed (both on success and on error/finally) while ensuring you don't revoke if bannerPreview is null; reference the bannerPreview variable and use URL.revokeObjectURL alongside the existing uploadFile and updateProfile calls so the preview is cleaned up in all paths.platforms/profile-editor/api/src/utils/file-proxy.ts (1)
34-35: Edge case: MIME subtypes with suffixes produce odd extensions.For MIME types like
image/svg+xml, the extension becomessvg+xml. Consider stripping suffixes:-const ext = mimeType.split("/")[1] || "bin"; +const ext = (mimeType.split("/")[1] || "bin").split("+")[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/utils/file-proxy.ts` around lines 34 - 35, The current logic in file-proxy.ts uses mimeType.split("/")[1] to build ext and sets filename = `upload.${ext}`, which yields suffixes like "svg+xml"; update the code that computes ext to strip any MIME suffix after a "+" (e.g., take the substring before "+") and fall back to "bin" if empty, so ext becomes "svg" for "image/svg+xml" and filename remains `upload.${ext}`; ensure you reference the existing mimeType, ext, and filename variables when making this change.platforms/profile-editor/api/src/services/EVaultProfileService.ts (1)
290-298: UnusedUserclass in destructuring.The
Userclass is destructured on line 293 but never used within this block (it was needed in previous iterations forrepo.create). The patternrepo.create({ ename: eName })works without it sincerepois already typed.- const { repo, user: localUser, User } = await getLocalUser(eName); + const { repo, user: localUser } = await getLocalUser(eName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts` around lines 290 - 298, The destructuring pulls a unused `User` symbol from getLocalUser; update the call to remove `User` from the destructuring (e.g., change "const { repo, user: localUser, User } = await getLocalUser(eName);" to "const { repo, user: localUser } = await getLocalUser(eName);") so the block uses only `repo`, `localUser`, and `u` for creating/saving the avatar/banner fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts`:
- Around line 129-134: The getLocalUser function uses a dynamic import of
AppDataSource and immediately calls AppDataSource.getRepository, which can throw
if the TypeORM data source is not initialized; update getLocalUser to check
AppDataSource.isInitialized (or equivalent initialization flag/method) after
import and either await AppDataSource.initialize() or throw/return a clear error
if not initialized, then proceed to call AppDataSource.getRepository(User) and
repo.findOneBy({ ename: eName }); ensure you reference the getLocalUser function
and AppDataSource and handle initialization before using getRepository.
In `@platforms/profile-editor/api/src/utils/file-proxy.ts`:
- Around line 20-46: The downloadUrlAndUploadToFileManager function currently
fetches arbitrary URLs; add strict URL validation before any network request:
ensure the URL uses only http or https scheme, parse the hostname and resolve it
to IP(s) (e.g., via dns.lookup or dns.promises.lookup) and reject if any
resolved IP is in private/loopback/169.254/Link-local ranges or is localhost,
and optionally reject if hostname is a bare IP literal in those ranges; also
defend against DNS rebinding by re-resolving the hostname immediately before the
axios.get and comparing IPs, and throw/reject early with a clear error instead
of calling axios for disallowed addresses. Use the function name
downloadUrlAndUploadToFileManager and the axios.get call site to locate where to
insert these checks.
---
Nitpick comments:
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts`:
- Around line 290-298: The destructuring pulls a unused `User` symbol from
getLocalUser; update the call to remove `User` from the destructuring (e.g.,
change "const { repo, user: localUser, User } = await getLocalUser(eName);" to
"const { repo, user: localUser } = await getLocalUser(eName);") so the block
uses only `repo`, `localUser`, and `u` for creating/saving the avatar/banner
fields.
In `@platforms/profile-editor/api/src/utils/file-proxy.ts`:
- Around line 34-35: The current logic in file-proxy.ts uses
mimeType.split("/")[1] to build ext and sets filename = `upload.${ext}`, which
yields suffixes like "svg+xml"; update the code that computes ext to strip any
MIME suffix after a "+" (e.g., take the substring before "+") and fall back to
"bin" if empty, so ext becomes "svg" for "image/svg+xml" and filename remains
`upload.${ext}`; ensure you reference the existing mimeType, ext, and filename
variables when making this change.
In
`@platforms/profile-editor/client/src/lib/components/profile/ProfileHeader.svelte`:
- Around line 50-64: The blob URL created with URL.createObjectURL(file)
(assigned to avatarPreview) must be revoked to avoid leaks: before assigning a
new avatarPreview revoke any previous URL (if avatarPreview) using
URL.revokeObjectURL(avatarPreview), and after the upload completes or fails
ensure you revoke the created blob URL when you clear avatarPreview (in both the
catch and finally paths) so that neither successful nor failed uploads leave
unreleased blob URLs; update the logic around avatarPreview in the upload flow
(the code that calls uploadFile and updateProfile — e.g., the handleAvatarUpload
/ upload handler where avatarPreview, uploadingAvatar, uploadFile, updateProfile
are used) to perform these revocations.
- Around line 72-86: The banner upload flow creates a blob URL via
URL.createObjectURL(file) and never revokes it, leaking blobs; update the
handler around the uploadFile/updateProfile logic (the code that sets
bannerPreview and calls uploadFile and updateProfile) to call
URL.revokeObjectURL(bannerPreview) when the blob URL is no longer needed (both
on success and on error/finally) while ensuring you don't revoke if
bannerPreview is null; reference the bannerPreview variable and use
URL.revokeObjectURL alongside the existing uploadFile and updateProfile calls so
the preview is cleaned up in all paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9652a7c-3b96-4a2e-9ab7-5145524efd59
📒 Files selected for processing (12)
platforms/file-manager/api/src/index.tsplatforms/profile-editor/api/src/controllers/ProfileController.tsplatforms/profile-editor/api/src/controllers/WebhookController.tsplatforms/profile-editor/api/src/index.tsplatforms/profile-editor/api/src/services/EVaultProfileService.tsplatforms/profile-editor/api/src/services/EVaultSyncService.tsplatforms/profile-editor/api/src/services/UserSearchService.tsplatforms/profile-editor/api/src/utils/file-proxy.tsplatforms/profile-editor/client/src/lib/components/profile/ProfileHeader.svelteplatforms/profile-editor/client/src/lib/stores/discovery.tsplatforms/profile-editor/client/src/lib/stores/profile.tsplatforms/profile-editor/client/src/lib/utils/file-manager.ts
✅ Files skipped from review due to trivial changes (3)
- platforms/profile-editor/client/src/lib/utils/file-manager.ts
- platforms/profile-editor/client/src/lib/stores/profile.ts
- platforms/profile-editor/api/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- platforms/file-manager/api/src/index.ts
- platforms/profile-editor/client/src/lib/stores/discovery.ts
- platforms/profile-editor/api/src/services/UserSearchService.ts
- platforms/profile-editor/api/src/controllers/ProfileController.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
platforms/profile-editor/api/src/utils/file-proxy.ts (3)
22-66: Good SSRF mitigation, but DNS rebinding window remains.The validation logic is comprehensive for blocking private IP ranges and localhost. However, there's a time-of-check-to-time-of-use (TOCTOU) gap: DNS is resolved during validation, but the actual
axios.getcall resolves DNS again. An attacker controlling their DNS server could return a public IP during validation, then rebind to a private IP before the fetch.Consider using a custom Axios
httpAgent/httpsAgentwith alookupoverride that re-validates the resolved IP immediately before connection.💡 Hardening suggestion (optional)
import { Agent as HttpAgent } from "http"; import { Agent as HttpsAgent } from "https"; function createSafeAgent(isHttps: boolean) { const AgentClass = isHttps ? HttpsAgent : HttpAgent; return new AgentClass({ lookup: (hostname, options, callback) => { dns.lookup(hostname, { all: true }, (err, addresses) => { if (err) return callback(err, "", 0); for (const addr of addresses) { if (isPrivateIP(addr.address)) { return callback(new Error("SSRF: private IP"), "", 0); } } callback(null, addresses[0].address, addresses[0].family); }); }, }); }Then pass
httpAgent/httpsAgentto the Axios request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/utils/file-proxy.ts` around lines 22 - 66, The current isUrlAllowed resolves DNS once, leaving a TOCTOU/DNS rebinding window; create a helper (e.g., createSafeAgent(isHttps: boolean)) that returns an http/https Agent with a custom lookup which re-resolves the hostname and rejects any private/local addresses, then use that agent when making the actual outbound request (pass as httpAgent/httpsAgent to the axios request in the code path that calls axios.get/fetch in this file) so the IP is re-validated at connect time; keep the existing isUrlAllowed checks but enforce the final check via the agent's lookup to prevent rebinding.
95-105: Consider adding a response size limit to prevent memory exhaustion.The download fetches the entire response into memory without size limits. A malicious URL pointing to a multi-gigabyte file could exhaust server memory before the file-manager's upload limits kick in.
🛡️ Proposed fix
+const MAX_DOWNLOAD_BYTES = 10 * 1024 * 1024; // 10 MB } else { const response = await axios.get(url, { responseType: "arraybuffer", timeout: 15_000, maxRedirects: 3, + maxContentLength: MAX_DOWNLOAD_BYTES, }); + if (response.data.byteLength > MAX_DOWNLOAD_BYTES) { + console.warn("Downloaded file exceeds size limit:", url); + return null; + } buffer = Buffer.from(response.data);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/utils/file-proxy.ts` around lines 95 - 105, The code fetches the whole file into memory via axios.get(..., responseType: "arraybuffer") which can exhaust memory; change the download to use responseType: "stream" (axios.get(..., responseType: "stream")), add a MAX_DOWNLOAD_BYTES constant (e.g. 50 * 1024 * 1024 or environment-configurable), read the stream into a temporary accumulator while counting bytes and immediately abort/destroy the stream and throw if the byte counter exceeds MAX_DOWNLOAD_BYTES, then convert the collected chunks into Buffer (assigning to buffer) and keep using response.headers["content-type"] to derive mimeType/filename; ensure you handle stream errors and close the request to avoid leaks.
15-16: Imports placed mid-file.The
dnsandnetimports are added after function definitions. Consider moving them to the top of the file with other imports for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/utils/file-proxy.ts` around lines 15 - 16, The dns and net imports (dns from "dns/promises" and net from "net") are declared mid-file; move these import statements up to join the other top-of-file imports in platforms/profile-editor/api/src/utils/file-proxy.ts so all imports are consolidated at the top of the module, keeping the existing import names (dns, net) unchanged and ensuring no duplicate or shadowed imports elsewhere in the file.platforms/profile-editor/api/src/services/EVaultProfileService.ts (1)
506-508: Silent failure may hide sync issues.Errors are logged but swallowed, so callers won't know if the User ontology sync failed. Consider adding metrics or structured logging to track sync failure rates in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts` around lines 506 - 508, The catch block in EVaultProfileService that currently does console.error("Failed to sync avatar/banner to User ontology:", e) should record a production metric and emit structured logs, then propagate failure to callers; update that catch to call your metrics/telemetry client (e.g., metrics.increment or telemetry.record with a key like "evault.user_sync_failed"), log structured context via your logger (include userId/profileId, operation name and the error object), and either rethrow the error or return a clear failure result so callers of the EVaultProfileService sync routine know the operation failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts`:
- Around line 476-498: The ACL preservation is broken because userNode lacks the
acl field from the META_ENVELOPES_QUERY and MetaEnvelopeNode type; update the
GraphQL query (META_ENVELOPES_QUERY) to request acl for meta envelopes and
extend the MetaEnvelopeNode type to include acl so that the existingAcl read in
EVaultProfileService (userNode -> existingAcl) returns the real ACL; after that
the UPDATE_MUTATION call using existingAcl (or fallback) in the payload will
preserve the existing ACL instead of always defaulting to ["*"].
- Around line 499-505: The CREATE branch currently sets patch.ename/displayName
but must populate required User ontology fields and surface validation errors:
set patch.username (not ename), generate and set patch.id (UUID) and
patch.createdAt (ISO timestamp), and ensure patch.displayName defaults to
profile.displayName or patch.username; call
client.request<CreateResult>(CREATE_MUTATION, { input: { ontology:
USER_ONTOLOGY, payload: patch, acl: ["*"] } }) and capture the result, then
check result.createMetaEnvelope.errors (and log/throw if present) instead of
swallowing generic exceptions; update references to CREATE_MUTATION,
USER_ONTOLOGY, CreateResult, createMetaEnvelope.errors, and the local patch
variable to implement these changes.
---
Nitpick comments:
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts`:
- Around line 506-508: The catch block in EVaultProfileService that currently
does console.error("Failed to sync avatar/banner to User ontology:", e) should
record a production metric and emit structured logs, then propagate failure to
callers; update that catch to call your metrics/telemetry client (e.g.,
metrics.increment or telemetry.record with a key like
"evault.user_sync_failed"), log structured context via your logger (include
userId/profileId, operation name and the error object), and either rethrow the
error or return a clear failure result so callers of the EVaultProfileService
sync routine know the operation failed.
In `@platforms/profile-editor/api/src/utils/file-proxy.ts`:
- Around line 22-66: The current isUrlAllowed resolves DNS once, leaving a
TOCTOU/DNS rebinding window; create a helper (e.g., createSafeAgent(isHttps:
boolean)) that returns an http/https Agent with a custom lookup which
re-resolves the hostname and rejects any private/local addresses, then use that
agent when making the actual outbound request (pass as httpAgent/httpsAgent to
the axios request in the code path that calls axios.get/fetch in this file) so
the IP is re-validated at connect time; keep the existing isUrlAllowed checks
but enforce the final check via the agent's lookup to prevent rebinding.
- Around line 95-105: The code fetches the whole file into memory via
axios.get(..., responseType: "arraybuffer") which can exhaust memory; change the
download to use responseType: "stream" (axios.get(..., responseType: "stream")),
add a MAX_DOWNLOAD_BYTES constant (e.g. 50 * 1024 * 1024 or
environment-configurable), read the stream into a temporary accumulator while
counting bytes and immediately abort/destroy the stream and throw if the byte
counter exceeds MAX_DOWNLOAD_BYTES, then convert the collected chunks into
Buffer (assigning to buffer) and keep using response.headers["content-type"] to
derive mimeType/filename; ensure you handle stream errors and close the request
to avoid leaks.
- Around line 15-16: The dns and net imports (dns from "dns/promises" and net
from "net") are declared mid-file; move these import statements up to join the
other top-of-file imports in
platforms/profile-editor/api/src/utils/file-proxy.ts so all imports are
consolidated at the top of the module, keeping the existing import names (dns,
net) unchanged and ensuring no duplicate or shadowed imports elsewhere in the
file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba5bc904-03ea-4ce0-afcb-3db11be14534
📒 Files selected for processing (10)
.env.exampleinfrastructure/dev-sandbox/src/routes/+page.svelteplatforms/file-manager/api/src/controllers/FileController.tsplatforms/file-manager/api/src/database/entities/File.tsplatforms/file-manager/api/src/database/migrations/1775700000000-AddIsPublicToFiles.tsplatforms/file-manager/api/src/services/FileService.tsplatforms/pictique/api/src/controllers/WebhookController.tsplatforms/profile-editor/api/src/services/EVaultProfileService.tsplatforms/profile-editor/api/src/utils/file-proxy.tsplatforms/profile-editor/client/src/lib/utils/file-manager.ts
✅ Files skipped from review due to trivial changes (1)
- infrastructure/dev-sandbox/src/routes/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (5)
- platforms/profile-editor/client/src/lib/utils/file-manager.ts
- platforms/pictique/api/src/controllers/WebhookController.ts
- .env.example
- platforms/file-manager/api/src/services/FileService.ts
- platforms/file-manager/api/src/controllers/FileController.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
platforms/profile-editor/api/src/services/EVaultProfileService.ts (2)
502-508:⚠️ Potential issue | 🟠 MajorCREATE branch still missing required User ontology fields.
The User schema requires
id,username,displayName, andcreatedAt, but this code only sets:
ename(should beusername)displayNameAdditionally, the CREATE result's
errorsarray is not checked — validation failures are silently swallowed by the outer catch block (lines 509-510).🔧 Proposed fix
} else { - patch.ename = eName; - patch.displayName = profile.displayName ?? eName; - await client.request<CreateResult>(CREATE_MUTATION, { + patch.id = crypto.randomUUID(); + patch.username = eName; + patch.displayName = profile.displayName ?? eName; + patch.createdAt = new Date().toISOString(); + const result = await client.request<CreateResult>(CREATE_MUTATION, { input: { ontology: USER_ONTOLOGY, payload: patch, acl: ["*"] }, }); + if (result.createMetaEnvelope.errors?.length) { + throw new Error(result.createMetaEnvelope.errors.map(e => e.message).join("; ")); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts` around lines 502 - 508, The CREATE branch is populating wrong/missing User fields and ignoring validation errors: when creating with CREATE_MUTATION in EVaultProfileService (see variables patch, eName, USER_ONTOLOGY and CreateResult from client.request) ensure patch includes required User ontology fields id, username (use eName), displayName, and createdAt (generate or reuse profile.createdAt), and then inspect the returned CreateResult.errors array from client.request and handle or throw on validation errors instead of silently swallowing them; update the code around the client.request call to populate these fields properly and to check result.errors, logging or raising an error if any are present.
479-501:⚠️ Potential issue | 🟠 MajorACL preservation still broken —
aclfield not queried.The
existingAclon line 482 will always beundefinedbecause:
META_ENVELOPES_QUERY(lines 21-39) doesn't request theaclfieldMetaEnvelopeNodetype doesn't includeaclThis causes line 499 to always fall back to
["*"], making the entire user envelope public on every sync — not just the avatar/banner URLs.🔧 Add acl to query and type
const META_ENVELOPES_QUERY = ` query MetaEnvelopes($filter: MetaEnvelopeFilterInput, $first: Int, $after: String) { metaEnvelopes(filter: $filter, first: $first, after: $after) { edges { cursor node { id ontology parsed + acl } } ... `; type MetaEnvelopeNode = { id: string; ontology: string; parsed: Record<string, unknown>; + acl?: string[]; };Then update line 482:
-const existingAcl = (userNode as any)?.acl; +const existingAcl = userNode?.acl;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts` around lines 479 - 501, The ACL is never read because META_ENVELOPES_QUERY and the MetaEnvelopeNode type don't include the acl field, so existingAcl (used in the update call in EVaultProfileService.sync/update block that calls findMetaEnvelopeByOntology) is always undefined and the update falls back to ["*"]; fix by updating the GraphQL query (META_ENVELOPES_QUERY used by findMetaEnvelopeByOntology) to request the acl field, add acl?: string[] to the MetaEnvelopeNode/type used to parse the query result, and then allow existingAcl = (userNode as any)?.acl to pick up that value so the UPDATE_MUTATION call (where acl: existingAcl ?? ["*"]) preserves the real ACL instead of defaulting to public.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts`:
- Around line 302-306: The current silent .catch(() => {}) on
markFilePublic(data.avatar, eName) and markFilePublic(data.banner, eName) can
leave files private and produce 403s; change these to surface errors by logging
the failure (including the file identifier, eName and the error) instead of
swallowing it—use the service's logger (e.g., this.logger or processLogger) or
import the module logger and replace .catch(() => {}) with .catch(err =>
logger.error(...)) so callers can debug why getFileManagerPublicUrl later
returns a 403.
---
Duplicate comments:
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts`:
- Around line 502-508: The CREATE branch is populating wrong/missing User fields
and ignoring validation errors: when creating with CREATE_MUTATION in
EVaultProfileService (see variables patch, eName, USER_ONTOLOGY and CreateResult
from client.request) ensure patch includes required User ontology fields id,
username (use eName), displayName, and createdAt (generate or reuse
profile.createdAt), and then inspect the returned CreateResult.errors array from
client.request and handle or throw on validation errors instead of silently
swallowing them; update the code around the client.request call to populate
these fields properly and to check result.errors, logging or raising an error if
any are present.
- Around line 479-501: The ACL is never read because META_ENVELOPES_QUERY and
the MetaEnvelopeNode type don't include the acl field, so existingAcl (used in
the update call in EVaultProfileService.sync/update block that calls
findMetaEnvelopeByOntology) is always undefined and the update falls back to
["*"]; fix by updating the GraphQL query (META_ENVELOPES_QUERY used by
findMetaEnvelopeByOntology) to request the acl field, add acl?: string[] to the
MetaEnvelopeNode/type used to parse the query result, and then allow existingAcl
= (userNode as any)?.acl to pick up that value so the UPDATE_MUTATION call
(where acl: existingAcl ?? ["*"]) preserves the real ACL instead of defaulting
to public.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c324410-c85d-467a-b567-4fcaf49f5ff3
📒 Files selected for processing (1)
platforms/profile-editor/api/src/services/EVaultProfileService.ts
Comment on lines
+302
to
+306
| // Mark new avatar/banner files as publicly accessible | ||
| const { markFilePublic } = await import("../utils/file-proxy"); | ||
| if (data.avatar) markFilePublic(data.avatar, eName).catch(() => {}); | ||
| if (data.banner) markFilePublic(data.banner, eName).catch(() => {}); | ||
| } |
Contributor
There was a problem hiding this comment.
Silent failures in markFilePublic may cause broken avatar/banner images.
If markFilePublic fails, the file remains private and getFileManagerPublicUrl will return a URL that 403s. Consider at minimum logging the failure so debugging is possible:
- if (data.avatar) markFilePublic(data.avatar, eName).catch(() => {});
- if (data.banner) markFilePublic(data.banner, eName).catch(() => {});
+ if (data.avatar) markFilePublic(data.avatar, eName).catch((e) => console.error(`Failed to mark avatar public: ${e}`));
+ if (data.banner) markFilePublic(data.banner, eName).catch((e) => console.error(`Failed to mark banner public: ${e}`));📝 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
| // Mark new avatar/banner files as publicly accessible | |
| const { markFilePublic } = await import("../utils/file-proxy"); | |
| if (data.avatar) markFilePublic(data.avatar, eName).catch(() => {}); | |
| if (data.banner) markFilePublic(data.banner, eName).catch(() => {}); | |
| } | |
| // Mark new avatar/banner files as publicly accessible | |
| const { markFilePublic } = await import("../utils/file-proxy"); | |
| if (data.avatar) markFilePublic(data.avatar, eName).catch((e) => console.error(`Failed to mark avatar public: ${e}`)); | |
| if (data.banner) markFilePublic(data.banner, eName).catch((e) => console.error(`Failed to mark banner public: ${e}`)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/profile-editor/api/src/services/EVaultProfileService.ts` around
lines 302 - 306, The current silent .catch(() => {}) on
markFilePublic(data.avatar, eName) and markFilePublic(data.banner, eName) can
leave files private and produce 403s; change these to surface errors by logging
the failure (including the file identifier, eName and the error) instead of
swallowing it—use the service's logger (e.g., this.logger or processLogger) or
import the module logger and replace .catch(() => {}) with .catch(err =>
logger.error(...)) so callers can debug why getFileManagerPublicUrl later
returns a 403.
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.
Description of change
makes it so that professional profiles share avatar and banner with the rest of the social platforms, deprecates avatarfileid and bannerfileid from professional user ontologies/mappings
PER USER MIGRATION/NOTIFICATION PROBABLY NEEDED (if you have a better idea lemme know)
also file manager and profile editor db would need migrations
adds profile editor to marketplace
adds professional profile to ontologies service
allows for certain file manager uploads to be publicly accessible
BONUS: show "REAL NAME" in sandbox
Issue Number
Type of change
How the change has been tested
manual (PLEASE TEST ON PROD BEFORE DEMO)
BLABSY NOT TESTED DUE TO GOD KNOWS FIREBASE SETUP
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores