Skip to content

fix(storage): sign presigned URLs for CDN_PUBLIC_ENDPOINT, not the internal CDN_ENDPOINT - #1873

Closed
pyramation wants to merge 1 commit into
mainfrom
devin/1791070671-cdn-public-endpoint
Closed

pyramation wants to merge 1 commit into
mainfrom
devin/1791070671-cdn-public-endpoint

Conversation

@pyramation

Copy link
Copy Markdown
Contributor

Summary

Fixes constructive-io/constructive-planning#2136.

What's broken: in Kubernetes, CDN_ENDPOINT is the in-cluster storage host (e.g. http://rustfs.constructive-infra.svc.cluster.local:9000). Server-side S3 calls need it, but the presigned PUT/GET URLs from uploadFiles / uploadPlatformFiles / downloadUrl were signed with the same client. Browsers and Desktop run outside the cluster and can't resolve that host, so every upload failed.

What this changes: a new optional setting, CDN_PUBLIC_ENDPOINT → cdn.publicEndpoint. When it's set, the server still talks to storage over CDN_ENDPOINT, but presigned URLs are signed by a second S3 client built for the public endpoint. When it's unset, nothing changes: URLs are signed with endpoint, which is all single-endpoint local dev needs.

// graphile-settings/src/presigned-url-resolver.ts
s3Config = {
  client: connect(endpoint),                       // server ↔ storage
  ...(publicEndpoint ? { presignClient: connect(publicEndpoint), publicEndpoint } : {}),
};

// graphile-presigned-url-plugin/src/s3-signer.ts
getSignedUrl(s3Config.presignClient ?? s3Config.client, command, { expiresIn })

All client-facing presigned URLs go through generatePresignedPutUrl / generatePresignedGetUrl: the requestUploadUrl / upload mutations in plugin.ts and the downloadUrl field. resolveS3ForDatabase spreads the global config, so per-database bucket configs keep presignClient. Server-side calls (headObject, copyS3Object, deleteS3Object, readObjectPrefix, the streaming upload lane, the bucket provisioner) still use client, which points at the internal endpoint.

Decisions

  • Sign with a second client, don't rewrite the host. SigV4 signs the Host header, so replacing the host after signing makes the signature invalid. The new test shows this.
  • A separate setting, not a reuse of CDN_PUBLIC_URL_PREFIX. That prefix builds unsigned public-object URLs and can be a CDN domain that doesn't speak the S3 API. A signing endpoint has to be the S3 API host.
  • presignClient is optional on S3Config, so plugin users with a single endpoint don't need to change anything.
  • constructive-cloud: no change needed. dev, staging and base all set CDN_ENDPOINT=https://s3.us-east-1.amazonaws.com, which is already public. The legacy k8s/overlays/local MinIO is ClusterIP-only, so it has no public host to point at.
  • constructive-db compute/.env.k8s (the local kind stack from the issue) can now set CDN_PUBLIC_ENDPOINT to the host-side RustFS port-forward. That's a follow-up in that repo; this PR doesn't touch it.

Tests

  • graphile-presigned-url-plugin/__tests__/s3-signer.test.ts (new) uses real S3Clients with fixed credentials and a fixed clock. It checks that PUT and GET URLs use the internal host when no public endpoint is configured and the public host when one is. An independent SigV4 verifier (recomputing the signature with crypto) confirms the signature validates for the host in the URL and fails once the host is rewritten.
  • graphile-settings/__tests__/presigned-url-resolver.test.ts: presignClient is built from publicEndpoint only when that is set.
  • pgpm/env/__tests__/merge.test.ts: CDN_PUBLIC_ENDPOINT maps to cdn.publicEndpoint, which is unset by default.

Without the fix, the new signer suite fails: S3Config has no presignClient.

Rollout

Any deployment whose CDN_ENDPOINT is internal-only should set CDN_PUBLIC_ENDPOINT to its client-reachable storage host.

Link to Devin session: https://app.devin.ai/sessions/abe5c10fecb54c08aa88f3e3760dd577
Open in Devin Desktop: https://app.devin.ai/desktop/session/abe5c10fecb54c08aa88f3e3760dd577?variant=devin
Requested by: @pyramation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@tenki-reviewer

tenki-reviewer Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review complete. No blocking issues — approved ✅; 1 nitpick below.

🧹 Nitpicks (1) — 🟢 1 low
  • 🟢 Make presignTarget pair presignClient/publicEndpoint symmetrically (s3-signer.ts:17) — presignTarget selects the signing client with presignClient ?? client but the endpoint with presignClient ? publicEndpoint : endpoint (graphile/graphile-presigned-url-plugin/src/s3-signer.ts:20), while S3Config declares the two fields independently optional (graphile/graphile-presigned-url-plugin/src/types.ts:196).

This PR introduces a dual-endpoint S3 configuration: an internal endpoint for data operations and a public endpoint used solely for presigning, so the SigV4 signature is computed for the host the browser will hit rather than the internal one. The presigned-URL resolver builds a second S3Client when a public endpoint is configured, and the env layer maps CDN_PUBLIC_ENDPOINT into the new CDNOptions.publicEndpoint with tests covering both merge cases and both resolver paths. One low-severity gap remains around hand-built S3Config objects, noted in the findings.

Files Change
graphile/graphile-presigned-url-plugin/src/s3-signer.ts, src/types.ts Add presignTarget() helper and presignClient/publicEndpoint config fields for split signing.
graphile/graphile-settings/src/presigned-url-resolver.ts Construct a dedicated presign S3Client when publicEndpoint is set.
pgpm/env/src/env.ts, pgpm/types/src/pgpm.ts Map CDN_PUBLIC_ENDPOINT env var into CDNOptions.publicEndpoint.
__tests__/s3-signer.test.ts, __tests__/presigned-url-resolver.test.ts, __tests__/merge.test.ts, README.md Tests for single- and dual-endpoint paths, env merge coverage, and docs.

Reviewed commit: d91cbd5

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Superseded by combined #1876 (CI green), which contains this branch unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant