Uh oh!
There was an error while loading. Please reload this page.
feat(expenditures): presigned S3 uploads for expenditure receipts - #306
Closed
nourshoreibah wants to merge 2 commits into
Closed
feat(expenditures): presigned S3 uploads for expenditure receipts#306nourshoreibah wants to merge 2 commits into
nourshoreibah wants to merge 2 commits into
Conversation
The Add Expense form requires a PDF receipt, but there was nowhere to put one:
the expenditures lambda had no upload route, so FileUpload animated a fake
progress bar over a setInterval and the file was dropped on submit.
POST /expenditures has always accepted a receiptUrl; nothing could produce one.
GET /expenditures/upload-url mints a presigned PUT for a PDF and returns the
objectUrl to pass back as receiptUrl, mirroring the reports lambda. It is
gated on the same project roles as creating an expenditure, and sanitises the
browser-supplied filename so it cannot escape the project's key prefix.
The route is registered above GET /expenditures/{id}, whose /^\/[^\/]+$/
matcher would otherwise read "upload-url" as an id.
Infra: a receipts bucket (private, unlike the public-read reports bucket, with
a CORS rule for the browser's direct PUT), the RECEIPTS_BUCKET_NAME env var,
and s3:PutObject on the lambda role -- a presigned PUT carries the signer's
permissions, so without it the browser's upload would 403 at S3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>- Auto-formatted .tf files with terraform fmt - Updated README.md with terraform-docs Co-authored-by: nourshoreibah <nourshoreibah@users.noreply.github.com>
Contributor
Terraform Plan 📖 |
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 freeto 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.
ℹ️ Issue
Prerequisite for replacing the last of the frontend's simulated data. Split out per "if the API is missing, put up a separate PR to add it".
📝 Description
AddExpenseModalrequires a PDF receipt before it will submit, andFileUploadshows a progress bar for it — but there was nowhere to put the file. The expenditures lambda has no upload route, soFileUpload.simulateUploadanimates a fake progress bar over asetInterval(with a standingTODO: update to use real progress) and theFileis then dropped on submit.POST /expenditureshas always accepted areceiptUrl; nothing in the system could produce one.GET /expenditures/upload-url?fileName=&projectId=mints a presigned PUT and returns{ uploadUrl, objectUrl }, mirroring the existingGET /reports/upload-url. The browser PUTs the file straight to S3, then passesobjectUrlback asreceiptUrlonPOST /expenditures.[A-Za-z0-9._-]before it becomes part of the key. Without that, afileNameof../../etc/passwd.pdfwrites outside the project's prefix. (GET /reports/upload-urlinterpolates the raw name; not touching it here, but worth a follow-up.)GET /expenditures/{id}, whose/^\/[^\/]+$/matcher would otherwise readupload-urlas an id and 400.RECEIPTS_BUCKET_NAMEreturns 500 rather than silently signing against"".Infra (
infrastructure/aws/):PUT— the browser uploads directly to S3, so API Gateway is not in that request's path and S3 must answer the preflight itself.RECEIPTS_BUCKET_NAMEadded to the authoritativeenvironmentblock inlambda.tf.s3:PutObjecton the lambda role. A presigned URL carries the signing principal's permissions, so without this the URL mints fine and then the browser's upload 403s at S3. Note the role currently has no S3 permissions at all, which suggests the reports lambda's S3 writes may be failing in prod too — flagging, not fixing here.✔️ Verification
In
apps/backend/lambdas/expenditures:npx jest— 117 passed, 3 suites (10 new)npx tsc --noEmit— cleanterraform fmt -checkininfrastructure/aws— cleanNew
test/receipt-upload.unit.test.tscovers: the happy path (asserting the signed command'sBucket/ContentTypeand the returnedobjectUrlshape), a member with a spending role, 403 for a member without one and for a non-member, 401, 404 for an unknown project, 400 for a non-PDF and for each invalidprojectId, that a traversing filename stays inside the project prefix, that the route isn't swallowed byGET /expenditures/{id}, and the unconfigured-bucket 500.🏕️ (Optional) Future Work / Notes
FileUploadreport real upload progress and wiresreceiptUrlthroughAddExpenseModal. It is stacked on this branch.terraform-planis worth a read on this one.🤖 Generated with Claude Code