feat(storage): consolidate GCS uploads into @acme/storage package#469
feat(storage): consolidate GCS uploads into @acme/storage package#469mcmxcdev wants to merge 10 commits into
Conversation
📝 WalkthroughWalkthroughIntroduces ChangesGCS Public Image Storage Refactor
Sequence Diagram(s)sequenceDiagram
participant Browser
participant UploadRoute as POST /api/upload-logo
participant AppStorage as lib/storage.ts (app)
participant PkgFactory as createPublicImageStorage
participant GCSEmulator as GCS Emulator
participant GCSProd as Google Cloud Storage
Browser->>UploadRoute: multipart/form-data (file, orgId, size?)
UploadRoute->>UploadRoute: validate MIME, size ≤10MB, orgId integer, parseOptionalSize
UploadRoute->>AppStorage: storage.uploadOrgLogo(orgId, buffer, {size})
AppStorage->>PkgFactory: prepareImageForStorage → square JPEG
alt GCS_EMULATOR_HOST set
PkgFactory->>GCSEmulator: HTTP PUT /f3-public-images-staging/org-logos/{id}.jpg
GCSEmulator-->>PkgFactory: 2xx + emulator URL
else production
PkgFactory->>GCSProd: GCS SDK file.save (contentType, cacheControl)
GCSProd-->>PkgFactory: public URL
end
PkgFactory-->>AppStorage: canonical URL string
AppStorage-->>UploadRoute: URL string
UploadRoute-->>Browser: JSON { url }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
mcmxcdev
requested review from
BigGillyStyle,
dnishiyama,
evanpetzoldt,
pstaylor-patrick and
taterhead247
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/map/src/app/_components/forms/location-event-form.tsx (2)
153-154:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove
console.log— use@acme/loggerinstead.Same issue as above — debug logging should use the shared logger or be removed.
🧹 Proposed fix
- console.log("eventTypes", eventTypes, field.value);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/map/src/app/_components/forms/location-event-form.tsx` around lines 153 - 154, Remove the console.log statement that logs "eventTypes", eventTypes, and field.value in the location-event-form.tsx file. If debug logging is actually needed for this section, replace the console.log with appropriate logging from the `@acme/logger` package instead, as the codebase uses a shared logger for consistency rather than direct console statements.Source: Coding guidelines
35-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove
console.log— use@acme/loggerinstead.Per coding guidelines, logging must go through the shared
@acme/loggerpackage. This debug statement should be removed or replaced with the appropriate log function from~/lib/logging.🧹 Proposed fix
- console.log("form eventTypeIds", form.getValues().eventTypeIds);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/map/src/app/_components/forms/location-event-form.tsx` at line 35, Remove the console.log statement that logs "form eventTypeIds" and form.getValues().eventTypeIds in location-event-form.tsx. If this debug logging is necessary, replace it with the appropriate logging function from the `@acme/logger` package imported from ~/lib/logging instead of using console.log directly, as per the coding guidelines for centralized logging.Source: Coding guidelines
apps/map/src/app/api/upload-logo/route.ts (1)
23-71:⚠️ Potential issue | 🔴 CriticalMissing authentication — any caller can overwrite any organization's logo.
The map route lacks the
requireAccessToken()check present in the admin route (apps/admin/src/app/api/upload-logo/route.ts, line 26). Combined with the storage layer accepting anyorgIdwithout authorization validation, this allows unauthenticated users to overwrite logos for any organization by supplying an arbitraryorgId. The form passesformRegionIddirectly from user input with no permission validation.Add authentication and authorization checks to validate the caller is authorized to upload for the specified organization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/map/src/app/api/upload-logo/route.ts` around lines 23 - 71, The POST function in the upload-logo route lacks authentication and authorization checks, allowing unauthenticated users to overwrite logos for any organization by providing an arbitrary orgId. Add a requireAccessToken() check at the beginning of the POST function to verify the caller is authenticated, then add authorization validation to ensure the authenticated user has permission to upload a logo for the specified orgId before proceeding with the file validation and upload logic. This should mirror the security pattern used in the admin route upload-logo endpoint.apps/map/scripts/cloud-run-env.sh (1)
117-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
readlink -fwith a macOS-compatible absolute path expansion.
readlink -fis GNU-only and will fail on macOS; withset -e, this can abort the script before deployment steps run.Suggested fix
-echo "Env file: $(readlink -f "$ENV_FILE")" +ENV_FILE_ABS="$(cd "$(dirname "$ENV_FILE")" && pwd -P)/$(basename "$ENV_FILE")" +echo "Env file: $ENV_FILE_ABS"As per coding guidelines,
**/*.shscripts must be tested on macOS and WSL 2 and avoid platform-specific commands without a Linux fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/map/scripts/cloud-run-env.sh` at line 117, The echo statement at line 117 uses readlink -f which is a GNU-only command that fails on macOS and will abort the script due to set -e. Replace the readlink -f "$ENV_FILE" call with a macOS-compatible absolute path expansion method. Consider using a combination of cd and pwd or another POSIX-compatible approach that works across both macOS and Linux environments to resolve the absolute path of the ENV_FILE variable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/map/src/app/api/upload-logo/route.ts`:
- Around line 14-21: The parseOptionalSize function is duplicated across
multiple route files and should be extracted to a shared utility module. Move
the parseOptionalSize function from this file to a shared utility location in
packages/storage or another appropriate shared package, then import and use it
in both the apps/map and apps/admin upload-logo route files, removing the
duplicate function definitions from both locations.
In `@apps/me/docs/SECURITY.md`:
- Line 124: The F3_CHANNEL environment variable description in the security
documentation is misleading as it doesn't accurately reflect the actual behavior
of the deriveStorageChannel function. Update the description for the F3_CHANNEL
entry in the documentation table to clarify that while three values exist
(local, staging, and prod), the local and staging values both map to the staging
bucket, while prod maps to the production bucket. Replace the current
description with clearer language that explains this bucket selection behavior.
In `@apps/me/README.md`:
- Line 125: Update the description for the F3_CHANNEL environment variable in
the README to clarify the actual channel behavior. The current description
misleadingly suggests that local is a standalone channel, when in fact both
local and staging map to the staging bucket, while only prod maps to the
production bucket. Replace the description with text that explicitly states this
mapping, such as indicating that local/staging values both use the staging
storage bucket and prod uses the production bucket, to make the behavior clear
to developers.
In `@apps/me/src/app/api/profile/avatar/route.ts`:
- Around line 27-32: The error message condition in the avatar route handler is
checking for "gcs_credentials is not set" but this text does not match the
actual error messages thrown by the upstream GCS credential validation in
public-images.ts. Update the if statement that checks the lower cased error
message to instead match one of the actual error messages: either "invalid
gcs_credentials payload" or "gcs_credentials is missing required service account
fields". This will ensure the condition properly catches GCS credential
configuration errors instead of falling through to the generic 500 response.
In `@docs/LOCAL_DEV_DOCKER.md`:
- Around line 379-380: The GCS emulator documentation section has inconsistent
bucket name references. Lines 379-380 correctly specify that local uploads use
the `f3-public-images-staging` bucket, but subsequent examples at lines 390,
409, and 494 still reference `f3-public-images` instead. Update all bucket name
references throughout the GCS emulator section to consistently use
`f3-public-images-staging` to match the initial guidance, or if both bucket
names are intentionally used for different scenarios, add explicit documentation
clarifying when each bucket should be used in local development.
In `@packages/storage/src/public-images.test.ts`:
- Around line 82-84: Replace all direct globalThis.fetch property assignments
with vi.stubGlobal("fetch", ...) throughout the test file to ensure proper
cleanup by Vitest. Specifically, change the pattern at lines 82–84, 97–99,
112–114, 166–175, 205–207, 217–219, 246–248, and 259–268 from globalThis.fetch =
vi.fn(...) to vi.stubGlobal("fetch", ...). Additionally, add
vi.unstubAllGlobals() in all afterEach hooks to properly clean up the stubbed
globals and prevent test pollution and flakiness across test runs.
In `@packages/storage/src/public-images.ts`:
- Around line 134-157: Add validation at the start of the uploadOrgLogo,
deleteOrgLogo, uploadUserAvatar, and deleteUserAvatar methods to ensure orgId
and userId parameters are positive integers. Reject invalid inputs such as zero,
negative numbers, non-integers, or NaN by throwing an appropriate error before
these IDs are used to construct the bucket paths. This enforces the contract
that only valid positive integer IDs are accepted at the storage API boundary.
---
Outside diff comments:
In `@apps/map/scripts/cloud-run-env.sh`:
- Line 117: The echo statement at line 117 uses readlink -f which is a GNU-only
command that fails on macOS and will abort the script due to set -e. Replace the
readlink -f "$ENV_FILE" call with a macOS-compatible absolute path expansion
method. Consider using a combination of cd and pwd or another POSIX-compatible
approach that works across both macOS and Linux environments to resolve the
absolute path of the ENV_FILE variable.
In `@apps/map/src/app/_components/forms/location-event-form.tsx`:
- Around line 153-154: Remove the console.log statement that logs "eventTypes",
eventTypes, and field.value in the location-event-form.tsx file. If debug
logging is actually needed for this section, replace the console.log with
appropriate logging from the `@acme/logger` package instead, as the codebase uses
a shared logger for consistency rather than direct console statements.
- Line 35: Remove the console.log statement that logs "form eventTypeIds" and
form.getValues().eventTypeIds in location-event-form.tsx. If this debug logging
is necessary, replace it with the appropriate logging function from the
`@acme/logger` package imported from ~/lib/logging instead of using console.log
directly, as per the coding guidelines for centralized logging.
In `@apps/map/src/app/api/upload-logo/route.ts`:
- Around line 23-71: The POST function in the upload-logo route lacks
authentication and authorization checks, allowing unauthenticated users to
overwrite logos for any organization by providing an arbitrary orgId. Add a
requireAccessToken() check at the beginning of the POST function to verify the
caller is authenticated, then add authorization validation to ensure the
authenticated user has permission to upload a logo for the specified orgId
before proceeding with the file validation and upload logic. This should mirror
the security pattern used in the admin route upload-logo endpoint.
🪄 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 Plus
Run ID: f7e8f620-b3b2-41e1-a3ab-11974c07db22
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
.github/workflows/ci.ymlapps/admin/.env.cloud-run.exampleapps/admin/.env.exampleapps/admin/scripts/cloud-run-env.shapps/admin/src/app/api/upload-logo/route.tsapps/admin/src/env.tsapps/admin/src/lib/storage.tsapps/api/vitest.config.tsapps/map/.env.cloud-run.exampleapps/map/.env.exampleapps/map/package.jsonapps/map/scripts/cloud-run-env.shapps/map/src/app/_components/forms/location-event-form.tsxapps/map/src/app/api/upload-logo/route.tsapps/map/src/env.tsapps/map/src/lib/logging.tsapps/map/src/lib/storage.tsapps/map/src/utils/image/upload-logo.tsapps/me/.env.cloud-run.exampleapps/me/.env.exampleapps/me/README.mdapps/me/docs/SECURITY.mdapps/me/scripts/cloud-run-env.shapps/me/src/app/api/profile/avatar/route.tsapps/me/src/app/api/profile/route.tsapps/me/src/env.tsapps/me/src/lib/gcs.tsapps/me/src/lib/storage.tsapps/me/vitest.config.tsdocs/LOCAL_DEV_DOCKER.mdpackages/env/src/index.tspackages/storage/src/env.tspackages/storage/src/index.tspackages/storage/src/public-images.test.tspackages/storage/src/public-images.tsscripts/local-setup.sh
💤 Files with no reviewable changes (2)
- packages/env/src/index.ts
- apps/admin/scripts/cloud-run-env.sh
| function parseOptionalSize( | ||
| sizeRaw: FormDataEntryValue | null, | ||
| ): number | undefined | "invalid" { | ||
| if (!sizeRaw) return undefined; | ||
| const parsed = Number(sizeRaw); | ||
| if (!Number.isFinite(parsed) || parsed <= 0) return "invalid"; | ||
| return parsed; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider extracting parseOptionalSize to a shared utility.
This helper is duplicated verbatim in apps/admin/src/app/api/upload-logo/route.ts (lines 16-23). Moving it to packages/storage or a shared utility would reduce duplication.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/map/src/app/api/upload-logo/route.ts` around lines 14 - 21, The
parseOptionalSize function is duplicated across multiple route files and should
be extracted to a shared utility module. Move the parseOptionalSize function
from this file to a shared utility location in packages/storage or another
appropriate shared package, then import and use it in both the apps/map and
apps/admin upload-logo route files, removing the duplicate function definitions
from both locations.
Recently published dependenciesFound 1 package published in the last 3 days:
Publish-time check healthAll package publish-time checks completed successfully. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/map/__tests__/utils/image/upload-logo.test.ts`:
- Around line 5-7: The afterEach hook in the test file currently only calls
vi.restoreAllMocks(), which does not clean up stubbed globals such as the fetch
global that is likely being stubbed in the test cases. Add vi.unstubAllGlobals()
to the afterEach function alongside the existing vi.restoreAllMocks() call to
ensure all stubbed globals are properly cleaned up after each test, preventing
test leakage and pollution to subsequent tests.
🪄 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 Plus
Run ID: a22dff57-c423-46f2-9edb-fe3fb4eac5cf
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.github/workflows/ci.ymlapps/map/__tests__/lib/storage.test.tsapps/map/__tests__/utils/image/upload-logo.test.tsapps/map/package.jsonapps/map/src/app/_components/forms/location-event-form.tsxapps/me/README.mdapps/me/__tests__/api/profile.test.tsapps/me/__tests__/lib/auth/tokens.test.tsapps/me/__tests__/lib/gcs.test.tsapps/me/__tests__/lib/storage.test.tsapps/me/docs/SECURITY.mdapps/me/src/app/api/profile/avatar/route.tsapps/me/vitest.config.tsdocs/LOCAL_DEV_DOCKER.mdpackages/storage/src/public-images.test.tspackages/storage/src/public-images.tspackages/storage/src/storage.test.ts
💤 Files with no reviewable changes (1)
- apps/map/src/app/_components/forms/location-event-form.tsx
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether stubbed globals are being reverted globally or per-test.
rg -n --type=ts '\bvi\.stubGlobal\s*\('
rg -n --type=ts '\bvi\.unstubAllGlobals\s*\('
rg -n --type=ts 'unstubGlobals\s*:\s*true'Repository: F3-Nation/f3-nation
Length of output: 1260
🏁 Script executed:
cat -n apps/map/__tests__/utils/image/upload-logo.test.tsRepository: F3-Nation/f3-nation
Length of output: 2367
🏁 Script executed:
# Find vitest config files
fd -e 'config.ts' -e 'vitest.config.ts' -e 'vitest.config.js'Repository: F3-Nation/f3-nation
Length of output: 662
🏁 Script executed:
cat -n apps/map/vitest.config.tsRepository: F3-Nation/f3-nation
Length of output: 1998
🏁 Script executed:
fd -type f -name 'vitest-config' packages/ && find packages -name '*vitest*' -type f | head -20Repository: F3-Nation/f3-nation
Length of output: 234
🏁 Script executed:
find packages -name '*vitest*' | head -20Repository: F3-Nation/f3-nation
Length of output: 349
🏁 Script executed:
rg -l '`@acme/vitest-config`' packages/Repository: F3-Nation/f3-nation
Length of output: 45
🏁 Script executed:
rg '`@acme/vitest-config`' --type ts --type tsxRepository: F3-Nation/f3-nation
Length of output: 91
🏁 Script executed:
rg '`@acme/vitest-config`' --type tsRepository: F3-Nation/f3-nation
Length of output: 305
🏁 Script executed:
find . -path ./node_modules -prune -o -name 'package.json' -type f -exec grep -l '`@acme/vitest-config`' {} \;Repository: F3-Nation/f3-nation
Length of output: 164
🏁 Script executed:
cat -n tooling/vitest/package.json && echo "---" && ls -la tooling/vitest/ && echo "---" && find tooling/vitest -type f -name '*.ts' -o -name 'index.js'Repository: F3-Nation/f3-nation
Length of output: 1404
🏁 Script executed:
cat -n tooling/vitest/coverage.tsRepository: F3-Nation/f3-nation
Length of output: 1285
Add vi.unstubAllGlobals() to afterEach to prevent test leakage.
vi.restoreAllMocks() does not unstub globals set via vi.stubGlobal. The suite must explicitly call vi.unstubAllGlobals() in the afterEach cleanup (or enable unstubGlobals: true in vitest config) to prevent stubbed fetch globals from leaking into other tests.
Affected test cases
- Lines 10-32: "returns url on successful upload"
- Lines 34-53: "throws API-provided error message on failed upload"
- Lines 55-74: "throws fallback error when failed upload body is unreadable"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/map/__tests__/utils/image/upload-logo.test.ts` around lines 5 - 7, The
afterEach hook in the test file currently only calls vi.restoreAllMocks(), which
does not clean up stubbed globals such as the fetch global that is likely being
stubbed in the test cases. Add vi.unstubAllGlobals() to the afterEach function
alongside the existing vi.restoreAllMocks() call to ensure all stubbed globals
are properly cleaned up after each test, preventing test leakage and pollution
to subsequent tests.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
taterhead247
force-pushed
the
feat/storage-consolidation-462
branch
from
b04bc6c to
1a367c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/map/src/app/_components/forms/location-event-form.tsx (1)
425-438:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle AO logo upload failures in the async change handler.
At Line 433, a rejected
uploadLogopromise will bubble out of the event handler, leading to an unhandled rejection and no user-visible error. Catch and surface the failure.Suggested fix
onChange={async (e) => { if (formRegionId == null) { toast.error("Please select a region first"); return; } const file = e.target.files?.[0]; if (!file) return; - - const url = await uploadLogo({ - file, - orgId: formRegionId, - }); - onChange(url); + try { + const url = await uploadLogo({ + file, + orgId: formRegionId, + }); + onChange(url); + } catch (err) { + toast.error( + err instanceof Error + ? err.message + : "Failed to upload logo", + ); + } }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/map/src/app/_components/forms/location-event-form.tsx` around lines 425 - 438, The async onChange handler in the location-event-form does not handle failures from the uploadLogo function call, which will result in an unhandled promise rejection. Wrap the uploadLogo call and the subsequent onChange call in a try-catch block, and use toast.error to display an error message to the user when the upload fails, similar to how the region validation error is already being handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/map/src/utils/image/upload-logo.ts`:
- Around line 24-25: The code at lines 24-25 in the upload-logo.ts file uses a
TypeScript type assertion without runtime validation, meaning a malformed 2xx
response with a missing or invalid url property will silently pass through.
After parsing the JSON response in the section handling (await response.json())
as { url: string }, add explicit runtime validation to check that data.url
exists and is a valid string before returning it. If the validation fails, throw
a descriptive error to provide clear feedback to callers instead of allowing
undefined to propagate silently.
In `@apps/me/src/env.ts`:
- Line 18: The GCS_CREDENTIALS validation in the environment schema only checks
for a non-empty string but does not validate that it is base64-encoded JSON,
causing malformed credentials to pass at startup and fail later during upload
operations. Enhance the GCS_CREDENTIALS schema definition to add validation for
base64-encoded JSON format using a refine method, regex pattern, or custom
validator combined with z.string().min(1) to ensure the validation fails fast at
environment parse time. Apply the same enhanced validation to the
GCS_CREDENTIALS field in all three environment configuration files.
In `@packages/storage/src/public-images.test.ts`:
- Around line 71-130: Add regression test cases to the uploadOrgLogo test suite
to verify that the function properly rejects invalid orgId inputs. Within the
describe block for uploadOrgLogo (emulator mode), add test cases that verify the
function throws an error when passed non-positive or non-integer ID values such
as 0, negative numbers, floats like 1.5, and NaN. Each test case should call
storage.uploadOrgLogo with one of these invalid ID values and assert that it
rejects with an appropriate error message. Additionally, apply the same
regression tests to any other similar upload methods mentioned in the file
(around lines 205-300) that accept userId or orgId parameters.
In `@packages/storage/src/public-images.ts`:
- Around line 141-147: Add validation for the options?.size parameter in the
uploadOrgLogo method to ensure it is a positive integer before passing it to
prepareImageForStorage. Reject invalid values such as zero, negative numbers,
non-integers, NaN, and undefined by throwing a deterministic contract error
(e.g., using an assertion utility like assertPositiveIntegerId if applicable, or
creating a similar validation). Apply the same validation to the other related
methods mentioned at lines 156-163 that also accept options?.size as a dimension
parameter.
---
Outside diff comments:
In `@apps/map/src/app/_components/forms/location-event-form.tsx`:
- Around line 425-438: The async onChange handler in the location-event-form
does not handle failures from the uploadLogo function call, which will result in
an unhandled promise rejection. Wrap the uploadLogo call and the subsequent
onChange call in a try-catch block, and use toast.error to display an error
message to the user when the upload fails, similar to how the region validation
error is already being handled.
🪄 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 Plus
Run ID: e5b086b7-1d5e-4f86-8045-96a0ba88f856
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (43)
.github/workflows/ci.ymlapps/admin/.env.cloud-run.exampleapps/admin/.env.exampleapps/admin/scripts/cloud-run-env.shapps/admin/src/app/api/upload-logo/route.tsapps/admin/src/env.tsapps/admin/src/lib/storage.tsapps/map/.env.cloud-run.exampleapps/map/.env.exampleapps/map/__tests__/lib/storage.test.tsapps/map/__tests__/utils/image/upload-logo.test.tsapps/map/package.jsonapps/map/scripts/cloud-run-env.shapps/map/src/app/_components/forms/location-event-form.tsxapps/map/src/app/api/upload-logo/route.tsapps/map/src/env.tsapps/map/src/lib/logging.tsapps/map/src/lib/storage.tsapps/map/src/utils/image/upload-logo.tsapps/me/.env.cloud-run.exampleapps/me/.env.exampleapps/me/README.mdapps/me/__tests__/api/profile.test.tsapps/me/__tests__/lib/auth/tokens.test.tsapps/me/__tests__/lib/gcs.test.tsapps/me/__tests__/lib/storage.test.tsapps/me/docs/SECURITY.mdapps/me/scripts/cloud-run-env.shapps/me/src/app/api/profile/avatar/route.tsapps/me/src/app/api/profile/route.tsapps/me/src/env.tsapps/me/src/lib/gcs.tsapps/me/src/lib/storage.tsapps/me/vitest.config.tsdocs/LOCAL_DEV_DOCKER.mdpackages/env/src/index.tspackages/storage/src/env.tspackages/storage/src/index.tspackages/storage/src/public-images.test.tspackages/storage/src/public-images.tspackages/storage/src/storage.test.tspnpm-workspace.yamlscripts/local-setup.sh
💤 Files with no reviewable changes (2)
- apps/admin/scripts/cloud-run-env.sh
- packages/env/src/index.ts
| const data = (await response.json()) as { url: string }; | ||
| return data.url; |
There was a problem hiding this comment.
Guard the success payload shape before returning url.
Line 24-25 trusts a type assertion; a malformed 2xx response can return undefined and silently break consumers. Validate data.url and throw a deterministic error when missing.
Suggested fix
- const data = (await response.json()) as { url: string };
- return data.url;
+ const data = (await response.json()) as { url?: unknown };
+ if (typeof data.url !== "string" || data.url.length === 0) {
+ throw new Error("Failed to upload logo");
+ }
+ return data.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.
| const data = (await response.json()) as { url: string }; | |
| return data.url; | |
| const data = (await response.json()) as { url?: unknown }; | |
| if (typeof data.url !== "string" || data.url.length === 0) { | |
| throw new Error("Failed to upload logo"); | |
| } | |
| return data.url; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/map/src/utils/image/upload-logo.ts` around lines 24 - 25, The code at
lines 24-25 in the upload-logo.ts file uses a TypeScript type assertion without
runtime validation, meaning a malformed 2xx response with a missing or invalid
url property will silently pass through. After parsing the JSON response in the
section handling (await response.json()) as { url: string }, add explicit
runtime validation to check that data.url exists and is a valid string before
returning it. If the validation fails, throw a descriptive error to provide
clear feedback to callers instead of allowing undefined to propagate silently.
| OAUTH_REDIRECT_URI: z.string().url(), | ||
| F3_CHANNEL: z.enum(["local", "ci", "branch", "dev", "staging", "prod"]), | ||
| // Base64-encoded service-account JSON for GCS public-image uploads. | ||
| GCS_CREDENTIALS: z.string().min(1), |
There was a problem hiding this comment.
Validate GCS_CREDENTIALS format at env-parse time.
Line 18 currently accepts any non-empty string, so malformed credentials pass startup and fail later in upload paths. Enforce “base64-encoded JSON” in schema parsing to fail fast and consistently (and mirror this in apps/admin/src/env.ts Line 22 and apps/map/src/env.ts Line 34).
Suggested fix
- GCS_CREDENTIALS: z.string().min(1),
+ GCS_CREDENTIALS: z
+ .string()
+ .min(1)
+ .refine((v) => {
+ try {
+ const decoded = Buffer.from(v, "base64").toString("utf8");
+ JSON.parse(decoded);
+ return true;
+ } catch {
+ return false;
+ }
+ }, "GCS_CREDENTIALS must be base64-encoded JSON"),📝 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.
| GCS_CREDENTIALS: z.string().min(1), | |
| GCS_CREDENTIALS: z | |
| .string() | |
| .min(1) | |
| .refine((v) => { | |
| try { | |
| const decoded = Buffer.from(v, "base64").toString("utf8"); | |
| JSON.parse(decoded); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| }, "GCS_CREDENTIALS must be base64-encoded JSON"), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/me/src/env.ts` at line 18, The GCS_CREDENTIALS validation in the
environment schema only checks for a non-empty string but does not validate that
it is base64-encoded JSON, causing malformed credentials to pass at startup and
fail later during upload operations. Enhance the GCS_CREDENTIALS schema
definition to add validation for base64-encoded JSON format using a refine
method, regex pattern, or custom validator combined with z.string().min(1) to
ensure the validation fails fast at environment parse time. Apply the same
enhanced validation to the GCS_CREDENTIALS field in all three environment
configuration files.
| describe("uploadOrgLogo (emulator mode)", () => { | ||
| beforeEach(() => { | ||
| process.env.GCS_EMULATOR_HOST = EMULATOR_HOST; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| delete process.env.GCS_EMULATOR_HOST; | ||
| vi.restoreAllMocks(); | ||
| vi.unstubAllGlobals(); | ||
| }); | ||
|
|
||
| it("uploads to prod bucket and returns canonical URL", async () => { | ||
| vi.stubGlobal( | ||
| "fetch", | ||
| vi.fn(() => Promise.resolve(new Response("{}", { status: 200 }))), | ||
| ); | ||
|
|
||
| const storage = createPublicImageStorage({ | ||
| channel: "prod", | ||
| credentials: FAKE_CREDENTIALS, | ||
| }); | ||
| const url = await storage.uploadOrgLogo(123, Buffer.from("img")); | ||
| expect(url).toBe( | ||
| `http://${EMULATOR_HOST}/f3-public-images/org-logos/123.jpg`, | ||
| ); | ||
| }); | ||
|
|
||
| it("uploads to staging bucket and returns canonical URL", async () => { | ||
| vi.stubGlobal( | ||
| "fetch", | ||
| vi.fn(() => Promise.resolve(new Response("{}", { status: 200 }))), | ||
| ); | ||
|
|
||
| const storage = createPublicImageStorage({ | ||
| channel: "staging", | ||
| credentials: FAKE_CREDENTIALS, | ||
| }); | ||
| const url = await storage.uploadOrgLogo(42, Buffer.from("img")); | ||
| expect(url).toBe( | ||
| `http://${EMULATOR_HOST}/f3-public-images-staging/org-logos/42.jpg`, | ||
| ); | ||
| }); | ||
|
|
||
| it("throws when emulator returns non-2xx", async () => { | ||
| vi.stubGlobal( | ||
| "fetch", | ||
| vi.fn(() => | ||
| Promise.resolve(new Response("bucket not found", { status: 404 })), | ||
| ), | ||
| ); | ||
|
|
||
| const storage = createPublicImageStorage({ | ||
| channel: "staging", | ||
| credentials: FAKE_CREDENTIALS, | ||
| }); | ||
| await expect(storage.uploadOrgLogo(1, Buffer.from("img"))).rejects.toThrow( | ||
| "GCS emulator upload failed: HTTP 404 bucket not found", | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add regression tests for invalid orgId/userId inputs.
The runtime contract now rejects non-positive/non-integer IDs, but this suite only asserts happy-path IDs. Add explicit 0/negative/float/NaN cases to lock this behavior.
Suggested test block
+describe("id validation", () => {
+ const storage = createPublicImageStorage({
+ channel: "staging",
+ credentials: FAKE_CREDENTIALS,
+ });
+
+ it.each([0, -1, 1.5, Number.NaN])(
+ "rejects invalid orgId %p",
+ async (orgId) => {
+ await expect(
+ storage.uploadOrgLogo(orgId as number, Buffer.from("img")),
+ ).rejects.toThrow("orgId must be a positive integer");
+ await expect(storage.deleteOrgLogo(orgId as number)).rejects.toThrow(
+ "orgId must be a positive integer",
+ );
+ },
+ );
+
+ it.each([0, -1, 2.25, Number.NaN])(
+ "rejects invalid userId %p",
+ async (userId) => {
+ await expect(
+ storage.uploadUserAvatar(userId as number, Buffer.from("img")),
+ ).rejects.toThrow("userId must be a positive integer");
+ await expect(storage.deleteUserAvatar(userId as number)).rejects.toThrow(
+ "userId must be a positive integer",
+ );
+ },
+ );
+});Also applies to: 205-300
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/storage/src/public-images.test.ts` around lines 71 - 130, Add
regression test cases to the uploadOrgLogo test suite to verify that the
function properly rejects invalid orgId inputs. Within the describe block for
uploadOrgLogo (emulator mode), add test cases that verify the function throws an
error when passed non-positive or non-integer ID values such as 0, negative
numbers, floats like 1.5, and NaN. Each test case should call
storage.uploadOrgLogo with one of these invalid ID values and assert that it
rejects with an appropriate error message. Additionally, apply the same
regression tests to any other similar upload methods mentioned in the file
(around lines 205-300) that accept userId or orgId parameters.
| async uploadOrgLogo(orgId, file, options) { | ||
| assertPositiveIntegerId("orgId", orgId); | ||
| const size = options?.size ?? 640; | ||
| const jpg = await prepareImageForStorage(file, { | ||
| width: size, | ||
| height: size, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Validate options.size at the storage boundary.
options?.size is used directly for Sharp dimensions. Invalid values (e.g., 0, negatives, floats, NaN) currently fail deep in image processing instead of returning a deterministic contract error from this API.
Suggested patch
+ function assertPositiveIntegerSize(
+ value: number | undefined,
+ fallback: number,
+ ): number {
+ const size = value ?? fallback;
+ if (!Number.isInteger(size) || size <= 0) {
+ throw new Error("size must be a positive integer");
+ }
+ return size;
+ }
+
return {
async uploadOrgLogo(orgId, file, options) {
assertPositiveIntegerId("orgId", orgId);
- const size = options?.size ?? 640;
+ const size = assertPositiveIntegerSize(options?.size, 640);
const jpg = await prepareImageForStorage(file, {
width: size,
height: size,
});
return uploadToBucket(`org-logos/${orgId}.jpg`, jpg, "image/jpeg");
@@
async uploadUserAvatar(userId, file, options) {
assertPositiveIntegerId("userId", userId);
- const size = options?.size ?? 512;
+ const size = assertPositiveIntegerSize(options?.size, 512);
const jpg = await prepareImageForStorage(file, {
width: size,
height: size,
});
return uploadToBucket(`user-avatars/${userId}.jpg`, jpg, "image/jpeg");Also applies to: 156-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/storage/src/public-images.ts` around lines 141 - 147, Add validation
for the options?.size parameter in the uploadOrgLogo method to ensure it is a
positive integer before passing it to prepareImageForStorage. Reject invalid
values such as zero, negative numbers, non-integers, NaN, and undefined by
throwing a deterministic contract error (e.g., using an assertion utility like
assertPositiveIntegerId if applicable, or creating a similar validation). Apply
the same validation to the other related methods mentioned at lines 156-163 that
also accept options?.size as a dimension parameter.
BigGillyStyle
left a comment
BigGillyStyle
left a comment
There was a problem hiding this comment.
Thorough, well-tested consolidation — nice work removing the per-app GoogleAuth boilerplate and centralizing the URL allow-list. Most comments are suggestions/cleanups; the one to weigh before merge is the unauthenticated map upload-logo route writing to a now-predictable shared path (see inline). Posted via Claude Code review.
| return parsed; | ||
| } | ||
|
|
||
| export async function POST(request: Request) { |
There was a problem hiding this comment.
[security] This route is unauthenticated (map middleware.ts has an empty matcher and there's no requireAccessToken() here, unlike the admin equivalent). Combined with the now-deterministic object path org-logos/{orgId}.jpg, any anonymous caller can POST with an arbitrary positive orgId and overwrite that region's live logo — orgId is validated as a positive integer but ownership is never checked. The missing auth pre-dates this PR, but the predictable shared path makes targeted overwrite trivial. Recommend adding an auth/ownership guard, or confirming the endpoint is intentionally public.
There was a problem hiding this comment.
This reminds me of a use case that @dnishiyama brought up that I forgot. Any user (authenticated, but not admin/editor), can submit a change request. That request could include a region logo image. The request has to be approved by an admin/editor. So the question was where does that logo live while it's in the requested state? Maybe that's related to this comment?
| function assertPositiveIntegerId(label: "orgId" | "userId", value: number) { | ||
| if (!Number.isInteger(value) || value <= 0) { | ||
| throw new Error(`${label} must be a positive integer`); | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
[tests] This guard (the orgId=0 / non-positive fix highlighted in the PR description) has no direct test, and there's no route-level test for the upload-logo routes covering orgId<=0, bad type, oversize, or parseOptionalSize. Worth adding so the documented bugfix is locked in.
| function isAllowedPublicImageUrl(url: string): boolean { | ||
| if ( | ||
| url.startsWith(`https://storage.googleapis.com/${BUCKETS.prod}/`) || | ||
| url.startsWith(`https://storage.googleapis.com/${BUCKETS.staging}/`) | ||
| ) { | ||
| return true; | ||
| } | ||
| const emulatorHost = getEmulatorHost(); | ||
| if (emulatorHost) { | ||
| return ( | ||
| url.startsWith(`http://${emulatorHost}/${BUCKETS.prod}/`) || | ||
| url.startsWith(`http://${emulatorHost}/${BUCKETS.staging}/`) | ||
| ); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[tests] The https cases are well covered (incl. the prefix-trick test 👍), but the emulator branch (http://${host}/...) — which is exactly what the allow-list bugfix added — isn't tested. A case with GCS_EMULATOR_HOST set would cover it.
| GOOGLE_LOGO_BUCKET_PRIVATE_KEY: mock-private-key | ||
| GOOGLE_LOGO_BUCKET_CLIENT_EMAIL: mock@example.iam.gserviceaccount.com | ||
| GOOGLE_LOGO_BUCKET_BUCKET_NAME: mock-bucket | ||
| GCS_CREDENTIALS: eyJjbGllbnRfZW1haWwiOiJtb2NrQGV4YW1wbGUuaWFtLmdzZXJ2aWNlYWNjb3VudC5jb20iLCJwcml2YXRlX2tleSI6Ii0tLS0tQkVHSU4gUlNBIFBSSVZBVEUgS0VZLS0tLS1cbmZha2Vcbi0tLS0tRU5EIFJTQSBQUklWQVRFIEtFWS0tLS0tIn0= |
There was a problem hiding this comment.
[cleanup] The three GOOGLE_LOGO_BUCKET_* vars just above are now removed from packages/env and every app env.ts, so they're dead config here. Safe to delete lines 36–38.
| function deriveStorageChannel(channel: string): "staging" | "prod" { | ||
| return channel === "prod" ? "prod" : "staging"; | ||
| } | ||
|
|
||
| export const storage = createPublicImageStorage({ | ||
| channel: deriveStorageChannel(env.F3_CHANNEL), |
There was a problem hiding this comment.
[dry] This helper is duplicated verbatim in the admin and me wrappers too. Consider exporting it (and the F3_CHANNEL literal type) from @acme/storage so the "only prod → prod bucket" rule lives in one place. Also confirming intent: dev/branch/local/ci all map to the staging bucket — fine if there's no long-lived prod-data dev env.
| return NextResponse.json({ url: publicUrl }); | ||
| } catch (error) { | ||
| console.error("Error uploading file:", error); | ||
| return NextResponse.json({ url }); |
There was a problem hiding this comment.
[ux] Objects now live at a stable path with cacheControl: public, max-age=300, so a re-uploaded logo can show stale for ~5 min. The me avatar route returns a cache-busted ?v=Date.now() URL; consider doing the same here (or noting why logos don't need it).
| vi.doMock("@acme/storage", () => ({ | ||
| createPublicImageStorage, | ||
| })); | ||
|
|
There was a problem hiding this comment.
[tests] "preview" isn't a member of the real enum (local|ci|branch|dev|staging|prod). The mock bypasses validation so it passes, but using a real non-prod value like "dev" keeps the test honest.
| GCS_BUCKET: z.string().min(1), | ||
| // Used only by the low-level uploadFile/deleteFile helpers. Apps using | ||
| // createPublicImageStorage derive the bucket from channel and do not set this. | ||
| GCS_BUCKET: z.string().min(1).optional(), |
There was a problem hiding this comment.
[cleanup] GCS_BUCKET and the low-level uploadFile/deleteFile helpers (upload.ts/delete.ts/client.ts) now appear unused by any app — only this comment references them. Recommend removing them (env var, helpers, exports in index.ts, and storage.test.ts cases) so there's a single storage entry point and no drift between the two paths.
3 participants
Summary
GoogleAuth+ rawfetchGCS calls with a sharedcreatePublicImageStoragefactory inpackages/storage/src/public-images.tsGOOGLE_LOGO_BUCKET_CLIENT_EMAIL,GOOGLE_LOGO_BUCKET_PRIVATE_KEY,GOOGLE_LOGO_BUCKET_BUCKET_NAME,GOOGLE_LOGO_BUCKET_PROJECT_IDinto a single base64-encodedGCS_CREDENTIALSenv var (consistent withapps/me)F3_CHANNELtoapps/mapandapps/adminto derive the correct bucket (f3-public-imagesvsf3-public-images-staging) at runtimescaleAndCropImagepre-processing from the upload flow; resizing now happens server-side insideprepareImageForStorageBug fixes (found during review)
isAllowedPublicImageUrlrejected emulator URLs — local-dev avatar saves always 400'd because emulator returnshttp://URLs but validation only checkedhttps://storage.googleapis.com/…. Now acceptshttp://${GCS_EMULATOR_HOST}/…when the emulator is active.orgIdvalidation accepted"0"—isNaN(Number("0"))isfalse, so org 0 was silently accepted in both upload-logo routes. Replaced withNumber.isInteger(n) && n > 0.Test plan
GCS_CREDENTIALSsecret populated,F3_CHANNEL=staging→ uploads land inf3-public-images-stagingorgId=0to/api/upload-logo→ expect 400 Invalid orgIdCloses #462
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests