Add profile asset storage baseline#150
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Storybook Image DiffOutcome: success Changed Storybook baselines: none in this PR. This check compares design-system component screenshots against committed baselines. Inline images show only added or modified Storybook baseline PNGs. |
Storybook Component Screenshot PreviewOutcome: success Screenshots: primitive component stories captured on desktop and mobile. This lane is separate from full-route Playwright screenshots and focuses on design-system component regressions. |
Playwright Hosted Data-FlowOutcome: success This optional check runs the mutation-backed profile flow against a configured hosted dev/staging target with isolated E2E test data. |
Playwright Data-Flow PreviewOutcome: success Captured flow:
Artifacts include screenshots, traces, and recorded video for the flow run. |
Playwright Image DiffOutcome: success Changed screenshot baselines: none in this PR. This check compares public route screenshots against committed baselines. Inline images show only added or modified baseline PNGs. |
Playwright Public Screenshot PreviewOutcome: success Screenshots: all public route checks passed on desktop and mobile. Full screenshot set is available in the artifact. Pixel diff baselines are handled by the separate Playwright Image Diff check. |
Vercel Preview DeploymentPreview: https://vr-dex-by14gudkx-basicbit.vercel.app |
Greptile SummaryThis PR introduces a
Confidence Score: 4/5Safe to merge; the bootstrap gate prevents the stack from applying before state-mgmt permissions are in place, and the OIDC trust policy is correctly scoped. The change is well-structured with appropriate security controls on both the Terraform and application sides. The main items worth resolving before or shortly after merge are: the AWS_ROLE_ARN fallback in vercelOidcRoleArn() which could silently pick up an unintended role during bootstrap, the hardcoded staging environment ID as a module variable default, and the absent s3:DeleteObject in the runtime role that will need to be added before any deletion path lands. apps/web/src/lib/server/profile-asset-storage.ts (credential fallback logic) and infra/terraform/profile-assets/variables.tf (staging environment ID default) are the two spots worth a second look before enabling the stack. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Vercel as Vercel Function
participant OIDC as @vercel/oidc-aws-credentials-provider
participant STS as AWS STS
participant S3 as AWS S3 (private bucket)
Vercel->>OIDC: "awsCredentialsProvider({ roleArn })"
OIDC->>STS: AssumeRoleWithWebIdentity(VERCEL_OIDC_TOKEN, roleArn)
STS-->>OIDC: Temporary credentials
OIDC-->>Vercel: Credentials provider
Vercel->>S3: PutObject / GetObject (signed with temp creds)
S3-->>Vercel: Response
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Vercel as Vercel Function
participant OIDC as @vercel/oidc-aws-credentials-provider
participant STS as AWS STS
participant S3 as AWS S3 (private bucket)
Vercel->>OIDC: "awsCredentialsProvider({ roleArn })"
OIDC->>STS: AssumeRoleWithWebIdentity(VERCEL_OIDC_TOKEN, roleArn)
STS-->>OIDC: Temporary credentials
OIDC-->>Vercel: Credentials provider
Vercel->>S3: PutObject / GetObject (signed with temp creds)
S3-->>Vercel: Response
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/web/src/lib/server/profile-asset-storage.ts:22-25
**Silent fallback to unrelated `AWS_ROLE_ARN`**
When `VRDEX_PROFILE_ASSET_ROLE_ARN` is absent and `isVercelRuntime()` is true, `AWS_ROLE_ARN` is used as the OIDC role. Vercel's own AWS integration also writes `AWS_ROLE_ARN` for its native AWS connectivity feature. If that integration is active, S3 profile-asset requests would silently use an unintended role — either failing confusingly with AccessDenied (if the role lacks S3 access) or, worse, succeeding under a broader role. The Terraform stack always populates `VRDEX_PROFILE_ASSET_ROLE_ARN`, so this fallback only fires during the bootstrap gap or in environments where the var was not set; returning `undefined` in that case and letting the SDK default credential chain handle it would be safer than reaching for an ambient env var that belongs to a different integration.
### Issue 2 of 3
infra/terraform/profile-assets/variables.tf:49-53
**Hardcoded hosted-deployment environment ID as module default**
`staging_custom_environment_ids` defaults to a specific BASIC-BIT Vercel custom environment ID. Any operator who applies this stack without a `tfvars` file (or who forks the module) would create Vercel environment variables bound to an environment they don't own and cannot access. An empty default is safer — the `terraform.tfvars.example` already documents the correct hosted value, so the module default doesn't need to carry it.
```suggestion
variable "staging_custom_environment_ids" {
description = "Vercel custom environment IDs for staging-like environments that should receive profile asset env vars. Empty leaves custom environments unmanaged."
type = set(string)
default = []
}
```
### Issue 3 of 3
infra/terraform/profile-assets/main.tf:143-153
**Runtime role missing `s3:DeleteObject`**
The `ReadAndWriteProfileAssets` policy grants only `s3:GetObject` and `s3:PutObject`. The docs list "lifecycle rules and deletion/retention policy" as follow-on work; if a future account-deletion or moderation path calls `DeleteObject` without first updating this policy, the operation will return `AccessDenied` at runtime. Adding `s3:DeleteObject` now (scoped to the same prefix ARN) keeps the policy aligned with the stated intent of full read/write asset management.
Reviews (1): Last reviewed commit: "Add profile asset storage baseline" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ccb56a0fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Summary
profile-assetsTerraform stack for the private S3 asset bucket, Vercel OIDC runtime role, and hosted Vercel profile asset env vars.state-mgmtbootstrap role has been applied.VRDEX_PROFILE_ASSET_ROLE_ARNis configured.Rollout
After merge, apply the updated
state-mgmtstack, setTERRAFORM_PROFILE_ASSETS_ENABLED=true, apply theprofile-assetsstack, redeploy Vercel, then re-probe the profile asset upload route for a non-501response.Validation
Local validation passed:
pnpm verify:web,pnpm lint:markdown,terraform fmt -check -recursive infra/terraform, andterraform validateforprofile-assetsandstate-mgmt.