Bulk Download URL & View#4871
Conversation
|
Terraform plan for meta No changes. Your infrastructure matches the configuration.✅ Plan applied in Deploy to Development and Management Environment #978 |
|
Terraform plan for dev Plan: 1 to add, 0 to change, 1 to destroy.Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
-/+ destroy and then create replacement
Terraform will perform the following actions:
# module.dev.module.cors.null_resource.cors_header must be replaced
-/+ resource "null_resource" "cors_header" {
!~ id = "*******************" -> (known after apply)
!~ triggers = { # forces replacement
!~ "always_run" = "2025-04-10T13:38:47Z" -> (known after apply)
}
}
Plan: 1 to add, 0 to change, 1 to destroy.✅ Plan applied in Deploy to Development and Management Environment #978 |
|
|
||
| Ex. Given 'historic/2022.zip', attempt to serve the file located at 'BUCKET/public-data/historic/2022.zip'. | ||
| """ | ||
| relative_path = f"/public-data/{relative_path}" |
There was a problem hiding this comment.
Do we want to process that value at all? E.g.
- make sure that the only values in it are [a-zA-Z0-9.-_]
- make sure it starts with [a-zA-Z]
I mostly don't want it to start with .., and... not sure what else we want to do here.
There was a problem hiding this comment.
I believe we're protected from this, since the value comes from the URL? Maybe not, that's a good callout.
Django itself doesn't do any processing on the string, so I think it's just relying on it being a valid URL. https://github.com/django/django/blob/71a19a0e475165dbc14c1fe02f552013ee670e4c/django/urls/converters.py#L39-L41
I'll add a more restrictive converter.
There was a problem hiding this comment.
Done! I used
[A-Za-z][A-Za-z0-9\/\-.\\_]+
One character, followed by any number of either characters, numbers, or "/", "-", ".", "", or "_".
I could have been less explicit, but this seemed okay since we know the actual names of all served files and this covers it.
There was a problem hiding this comment.
Excellent. Thank you. That was the spirit of what I was aiming for. If we ever expand this to cover other things, we'll know really quickly that this is the source of the problem. It assuages my paranoia, if nothing else. :D
|
3 participants
Bulk Download URL & View
Issue: #4862
Changes:
Just the URL and view. Which is essentially a passthrough for users to download files in the
public-datafolder of our S3 bucket. No user feedback (unless there's a 404), and no file perusal. The intended access point is outside links (the static site).Some little tests for verification.
Also, a local linting fix. Not sure why that doesn't get hit in the PR checks or in the container.
How to test:
a. The default path is http://localhost:9002
b. The default login is
minioadminfor both the user and passc. Add some files to a folder named
public-dataa. For example,
http://localhost:8000/dissemination/public-data/FILE_RELATIVE_PATH.EXTb. If you bring in data from GDrive, try something like http://localhost:8000/dissemination/public-data/historic/2022.zip
c. Attempts should succeed whether the file is directly in the folder or in a subfolder, like above
Recording:
Screen.Recording.2025-04-08.at.3.06.08.PM.mov
PR Checklist: Submitter
maininto your branch shortly before creating the PR. (You should also be mergingmaininto your branch regularly during development.)git status | grep migrations. If there are any results, you probably need to add them to the branch for the PR. Your PR should have only one new migration file for each of the component apps, except in rare circumstances; you may need to delete some and re-runpython manage.py makemigrationsto reduce the number to one. (Also, unless in exceptional circumstances, your PR should not delete any migration files.)PR Checklist: Reviewer
make docker-clean; make docker-first-run && docker compose up; then rundocker compose exec web /bin/bash -c "python manage.py test"The larger the PR, the stricter we should be about these points.
Pre Merge Checklist: Merger
-/+ resource "null_resource" "cors_header"should be destroying and recreating its self and~ resource "cloudfoundry_app" "clamav_api"might be updating itssha256for thefac-file-scannerandfac-av-${ENV}by default.main.