-
Notifications
You must be signed in to change notification settings - Fork 49
Fix API Gateway pdf uploads for alpha/applications #7758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| third_level_endpoints = [ | ||
| {}, | ||
| { | ||
| "alpha/applications/{application_id}/attachments" = [{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this for more than just this endpoint, the PUT /alpha/forms/:form_id/form_instructions/:form_instruction_id endpoint is also currently broken.
Any way to generically define some of this so we just need a list of paths? Guessing that might be tricky with the request params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add the PUT in the next commit. As for generically defining things, the current setup is as generic as I can make it without making it difficult down the line to update things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forms endpoint is now live, let me know if it doesn't work
| binary_media_types = [ | ||
| "application/pdf", | ||
| "application/octet-stream", | ||
| "multipart/form-data", | ||
| ] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the file is something else like a word doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really difficult to get a non-AI generated list of allowed types, but I believe most file types that aren't images, videos, or PDFs will fall under the multipart/form-data or application/octet-stream buckets. We can test that on the attachments endpoint to double check me though. Worst case, I can allow all, but I don't feel like that should be our first option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it allow wildcards? Can you do image/* for both image/png and image/jpeg?
For reference, I went to grants.gov's DB to see what mimetypes they have recorded for application attachments over the past 2 years, there were 77 distinct values. The top 20 or so:
application/pdf
application/vnd.openxmlformats-officedocument.wordprocessingml.document
application/vnd.openxmlformats-officedocument.spreadsheetml.sheet
image/jpeg
image/png
application/msword
application/unknown
application/x-zip-compressed
application/vnd.ms-excel
text/plain
application/vnd.openxmlformats-officedocument.presentationml.presentation
application/vnd.ms-excel.sheet.binary.macroEnabled.12
application/vnd.google-earth.kmz
application/vnd.ms-excel.sheet.macroEnabled.12
application/zip
application/vnd.oasis.opendocument.text
application/x-iwork-pages-sffpages
application/vnd.google-earth.kml+xml
text/csv
image/tiff
text/html
video/mp4
Will update PR once confirmed to be working as expected
From testing, we only need multipart so far
Summary
This PR fixes the issue for the alpha application attachments not being useable downstream
Fixes / Work for #7647
Changes proposed
/alpha/applications/{app_id}/attachmentsContext for reviewers
API Gateway encodes all request bodies by default, which was causing the attachments file to "work", but we couldn't open the uploads. This PR introduces and implements an infra side change to allow attachments, and not encode them, so downstream systems can use them as intended
Validation steps
This PR's changes have been deployed so we can allow the reviewers to test