Replace S3 presigned urls in favor of Cloudfront

[GianniCarlo]

No description provided.

[Copilot]

Pull request overview

This PR introduces CloudFront signed URLs for download (GET) access to library objects (replacing S3 presigned URLs) behind a global configs toggle and a per-user allowlist, while keeping uploads (PUT) on S3 presigned URLs.

Changes:

• Add CloudFrontService for issuing signed CloudFront URLs and wire it into LibraryService download URL generation with S3 fallback.
• Add ConfigService + ConfigDB to read a new use_cloudfront_downloads config flag (Redis read-through cached).
• Add migration + deployment/env updates, plus unit tests for the new flagging/signing behavior.

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
package.json	Adds @aws-sdk/cloudfront-signer dependency.
yarn.lock	Locks new CloudFront signer + smithy deps.
src/services/LibraryService.ts	Routes download URL resolution through CloudFront signer with S3 fallback, plus allowlist/global-flag logic.
src/services/CloudFrontService.ts	New service to generate CloudFront signed URLs with configurable TTL.
src/services/ConfigService.ts	New config flag reader with Redis read-through caching and safe fallbacks.
src/services/db/ConfigDB.ts	New DB access layer for configs table reads.
src/database/migrations/20260607120000_add_use_cloudfront_downloads_config.ts	Adds new enum value and seeds use_cloudfront_downloads config row.
src/config/envs.ts	Adds CloudFront-related env validation.
docker/ecs/task-definition.json	Adds CloudFront env + secrets to ECS task definition.
development.env.template	Adds CloudFront env placeholders for local development.
src/__tests__/services/LibraryServiceDownloadUrl.test.ts	Tests CloudFront vs S3 decision logic + fallback behavior.
src/__tests__/services/ConfigService.test.ts	Tests config caching, type mismatch handling, and fail-safe behavior.
src/__tests__/services/CloudFrontService.test.ts	Tests signing output shape, path encoding, and TTL behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

🟡 WARN — CLOUDFRONT_EXPIRY_SECONDS=120 in the production task definition overrides CloudFrontService.DEFAULT_EXPIRY_SECONDS (86400) for every signed URL. That directly contradicts the rationale documented in CloudFrontService ("AVURLAsset re-fetches ranges with the same URL, so a shorter expiry would stall playback mid-listen"), and combined with CLOUDFRONT_ALLOWLIST=29 on line 47 it means the canary user gets download/artwork URLs that die after 2 minutes — 403s mid-playback and broken artwork. Note the library-listing path doesn't return expires_in at all, so the client can't even know to refresh.

Fix: drop this env var so the 24h default applies (or set it to the value you actually intend to ship, e.g. 86400). If 120s was only meant for a temporary experiment, land it in a separate, clearly-labelled change.

[github-actions]

🟡 WARN — getBoolean is called once per generated URL, i.e. twice per library item inside the getLibrary loop (lines 214 and 221). For a 500-item library that's up to 1000 extra Redis round-trips per /v1/library/all, and when Redis is unavailable or the key is cold, RedisService.getObject returns null and every one of those calls becomes a configs SELECT — 1000 DB queries for a single listing. The allowlist short-circuit only helps allowlisted users.

Fix: resolve the flag once per request and thread it down, e.g. compute const useCloudFront = this.isCloudFrontAllowlisted(user) || await this._config.getBoolean(ConfigKey.UseCloudFrontDownloads) before the loop and change getDownloadUrl(key, user) to getDownloadUrl(key, useCloudFront). An in-process memo with a short TTL in ConfigService would work too.

[github-actions]

🟡 WARN — The three new CLOUDFRONT_* props are .required(), so envSchema throws at boot if any is missing. That makes the deploy order fragile: the ECS task will crash-loop unless CLOUDFRONT_KEY_PAIR_ID / CLOUDFRONT_PRIVATE_KEY already exist in Secrets Manager, and any other environment (CI, staging, a dev machine on an older .development.env) fails to start — even though the feature ships flag-off and CloudFrontService.getSignedUrl already degrades gracefully to S3 when the config is absent.

Fix: make these optional (S.string() without .required()) and rely on the existing runtime guard + S3 fallback, or gate the requirement on the flag being enabled.

[github-actions]

🔵 INFO — A missing row is never negatively cached: when getConfig returns null (row absent, or the migration hasn't run in this environment yet) the code returns the fallback without a setObject, so every subsequent call hits Postgres again. With the per-item call pattern in LibraryService.getLibrary that's one query per library item per request.

Fix: cache a sentinel (e.g. { value: 'false', value_type: 'boolean' } or a short-TTL null marker) on a miss so the DB isn't re-queried on every call. Also consider moving the value_type check above the setObject so a malformed row isn't written into the cache.

[github-actions]

🔵 INFO — Two small things on this log call:

1. PII: key starts with the storage prefix, which for legacy users is their email address (your own test covers user@icloud.com/library/...). sanitizeData only redacts password/token/secret/authorization, so the email lands in the log verbatim. Log the last path segment or a hash of the key instead.
2. Level: this logs at the default info, and prod runs LOG_LEVEL=warn, so a signing misconfiguration (bad PEM, wrong key-pair id) is invisible here. Pass 'error' as the second argument, consistent with the fallback log in LibraryService.getDownloadUrl.

[github-actions]

🔵 INFO — This branch assigns url but never sets libObj.expires_in, so clients on the legacy listing path get no expiry hint. Previously that was mostly harmless (S3 presigns here live 7 days — S3Service.getPresignedUrl uses 3600 * 24 * 7); CloudFront URLs default to 24h (and 120s with the current prod task definition), so a client that caches a listing URL can start getting 403s with no way to know it should refresh. Consider setting libObj.expires_in from getDownloadUrl here as the other two call sites do.

[github-actions]

🟡 Claude PR Review — WARN

Adds a CloudFront signed-URL path for library downloads behind a DB config flag (use_cloudfront_downloads) plus a CLOUDFRONT_ALLOWLIST canary, with a new CloudFrontService, ConfigService/ConfigDB, an additive migration, and tests.

Authorization: no regression — every signed key is still derived server-side from StoragePrefixService.getPrefix(user) and a row owned by the user, and the canned policy binds the signature to that exact object, so the per-user scoping of the old S3 presigns is preserved. No routes or middleware changed, and the PRO-tier gate on the affected branches is untouched. Migration is additive and idempotent; no secrets are committed.

Main risks are operational, not authz: the prod task definition pins CLOUDFRONT_EXPIRY_SECONDS=120, which contradicts the service's own 24h rationale and will expire URLs mid-playback for the canary user; the flag is read once per URL inside the library loop (N Redis/DB round-trips per listing); and the three new CLOUDFRONT_* envs are .required(), so a deploy without the Secrets Manager entries fails to boot even though the feature is off by default. One infra prerequisite worth confirming outside this diff: the library.bookplayer.app distribution must actually require signed URLs via a trusted key group, otherwise the new distribution exposes the private bucket unauthenticated.

Findings: 3 warn · 3 info

_{Model claude-opus-5 · run log · 6 new · 0 carried over · 0 resolved · advisory (a human should still review). Findings are de-duplicated across pushes; an earlier finding closes only when the verification pass judges it against the current code — fixed, no longer applicable, accepted by a maintainer, or a duplicate of a finding reported on this push.}