WIP: Add storage infra module + working document upload

[sean-navapbc]

Summary

• Add infra/modules/storage/ — Azure Blob Storage module with security hardening (StorageV2, HTTPS-only, TLS 1.2, no shared keys, OAuth auth, blob versioning, 30-day delete retention, Azure Monitor integration)
• Wire storage into the service layer with Storage Blob Data Contributor RBAC role assignment on the app's managed identity
• Add private endpoint support for blob storage with DNS zone integration in the network layer
• Implement storage.py with Azure Blob Storage SDK using DefaultAzureCredential
• Update /document-upload route to server-side upload (Flask receives file, uploads to blob)
• Add document-upload endpoint test in Terratest

Design decisions

• Server-side upload — Azure doesn't have a direct equivalent to AWS presigned POST. Flask receives the file and uploads to blob storage. SAS-based client-side upload can be added later.
• No CMK encryption — Azure encrypts all blob data at rest with Microsoft-managed keys by default. CMK support can be added as a follow-up.
• storage_vars pattern — Follows the existing db_vars nullable object pattern for consistency. Service module owns the env var names.

Test plan

• terraform fmt passes
• terraform validate passes on storage module
• CI static analysis (terraform validate, checkov, tfsec)
• Terratest: document-upload endpoint returns 200 with upload form
• Manual: deploy to dev, upload a file via browser, verify blob in Azure Portal
• Integration: re-render template in platform-test-azure, verify plan + apply

Closes #4

[sean-navapbc]

Testing Status

Completed

• terraform fmt — passed locally and in CI
• terraform validate — passed on storage module locally and all modules in CI
• tfsec security scan — passed in CI
• checkov security scan — passed (after adding CKV2_AZURE_1 skip for customer managed key, matching existing pattern in terraform-backend-azure)
• actionlint / shellcheck / markdown lint — passed in CI
• Docker build — passed in CI

Remaining

• Terratest — needs to run against a deployed Azure environment. The test now includes a check for the /document-upload endpoint returning 200 with an upload form.
• Manual verification — deploy to dev environment, upload a file via browser, verify blob appears in Azure Portal storage container
• Integration test in platform-test-azure — re-render template, run terraform plan on networks + service layers, apply, and verify end-to-end upload flow

Manual Verification Checklist (for reviewer)

1. terraform plan on networks layer shows new blob DNS zone
2. terraform plan on service layer shows: storage account, container, role assignment, private endpoint, env vars
3. Azure Portal: confirm storage account hardening (HTTPS-only, TLS 1.2, shared key disabled, OAuth default)
4. Azure Portal: confirm RBAC role assignment (Storage Blob Data Contributor) on app identity
5. Browser: navigate to /document-upload, upload a file, confirm success
6. Azure Portal: verify blob exists under documents/uploads/<date>/<filename>

[sean-navapbc]

Manual Verification & Code Review

Test Results (Local Testing)

I cloned the branch, installed dependencies, and ran manual tests against the Flask app and storage module. Here are the results:

Test	Result	Details
Python syntax check (app.py, storage.py)	PASS	Both files compile cleanly
GET /document-upload	PASS	Returns form with multipart/form-data enctype
POST /document-upload (no file)	PASS	Returns 400 "No file part"
POST /document-upload (empty filename)	PASS	Returns 400 "No selected file"
POST /document-upload (valid file, mocked storage)	PASS	Uploads to uploads/<date>/<filename> path
GET /health	PASS	Returns healthy status with version
storage.create_upload_url()	PASS	Returns ("/document-upload", {})
storage._get_container_name() default	PASS	Returns "documents"
storage._get_container_name() env override	PASS	Respects AZURE_STORAGE_CONTAINER_NAME
Terraform HCL brace balancing	PASS	All .tf files have balanced braces
Path traversal (malicious filename)	FAIL	../../../etc/passwd is NOT sanitized — passes through to blob path
XSS via filename in response	FAIL	<script>alert(1)</script>.txt reflected unsanitized in HTML response
File size limit	WARNING	No MAX_CONTENT_LENGTH set — accepts arbitrarily large uploads
File type validation	WARNING	No file type restrictions — .exe, .sh, etc. accepted

CI Status

• 1 failing check: tfsec compliance scan
• 7 passing checks: markdown lint, checkov, terraform format, terraform validate, etc.

---

Code Review Findings

Security Issues (Should Fix Before Merge)

1. Path Traversal Vulnerability in app.py:69

path = f"uploads/{datetime.now().date()}/{file.filename}"

file.filename is user-controlled and not sanitized. A filename like ../../../etc/passwd results in the blob path uploads/2026-04-15/../../../etc/passwd. While Azure Blob Storage handles paths differently than a filesystem (blob names are flat strings), this is still a security concern:

• It breaks the intended uploads/<date>/ organizational scheme
• It could lead to unexpected blob name collisions or overwrites

Suggested fix: Use werkzeug.utils.secure_filename():

from werkzeug.utils import secure_filename
path = f"uploads/{datetime.now().date()}/{secure_filename(file.filename)}"

2. Reflected XSS in app.py:72

return f"<p>File uploaded successfully to {path}</p>"

The filename is embedded directly into the HTML response without escaping. A filename containing <script> tags will execute in the user's browser.

Suggested fix: Use Flask's escape() or Markup:

from markupsafe import escape
return f"<p>File uploaded successfully to {escape(path)}</p>"

3. No Upload Size Limit
There's no MAX_CONTENT_LENGTH set on the Flask app, allowing arbitrarily large file uploads that could exhaust server memory.

Suggested fix:

app.config['MAX_CONTENT_LENGTH'] = 16 * 1024 * 1024  # 16 MB limit

Non-Security Issues

4. Pre-existing bug in app.py:45 (not introduced by this PR)

row = cur.fetchone  # Missing parentheses — assigns method, not result

Then line 49 calls cur.fetchone()[0] which would fetch a second row. This was pre-existing but worth noting.

5. create_upload_url() in storage.py:26-28 is unused/vestigial
The function returns ("/document-upload", {}) but is never called by app.py anymore (the old presigned-URL pattern was removed). The path parameter is also ignored. Consider removing this dead code or adding a comment about future use.

6. storage_endpoint module may error when has_blob_storage is false
In {{app_name}}/service/storage.tf:34:

resource_id = module.app_config.has_blob_storage ? module.storage[0].storage_account_id : ""

Passing an empty string to resource_id when storage is disabled could cause issues depending on how the private-endpoint module validates inputs. If the enable = false guard short-circuits properly this is fine, but worth verifying.

7. skip_service_principal_aad_check in storage_access.tf:8

skip_service_principal_aad_check = var.is_temporary

This is fine for test environments but make sure this flag is well-documented. Skipping AAD checks in production would be a security concern.

Infrastructure Review (Looks Good)

• Storage module security hardening is solid: HTTPS-only, TLS 1.2, shared keys disabled, OAuth default, versioning enabled, 30-day delete retention
• GRS replication is appropriate for production data
• Private endpoint + DNS zone integration follows existing patterns well
• RBAC with "Storage Blob Data Contributor" is the right role for read/write blob access
• Monitor integration is correctly conditional on non-temporary environments
• The storage_account_name generation with substr(replace(...)) correctly handles Azure's naming constraints

Recommendations

1. Must fix: Sanitize filenames with secure_filename() and escape HTML output before merging
2. Should fix: Add MAX_CONTENT_LENGTH to Flask config
3. Nice to have: Add file type validation (allowlist of permitted extensions)
4. Cleanup: Remove or document the unused create_upload_url() function
5. CI: Investigate the failing tfsec check

[sean-navapbc]

Fixes Applied & Updated Test Results

Following up on my earlier review, I've applied fixes for all security issues identified. Here's a summary of changes and the updated test results.

Changes Made

app.py:

1. Path traversal fix — Added werkzeug.utils.secure_filename() to sanitize filenames before constructing the blob path. Malicious filenames like ../../../etc/passwd are now sanitized to etc_passwd.
2. XSS fix — Added markupsafe.escape() to the upload success response so filenames with <script> tags are HTML-escaped instead of reflected raw.
3. Upload size limit — Added app.config["MAX_CONTENT_LENGTH"] = 16 * 1024 * 1024 (16 MB). Files exceeding this limit now return 413.
4. Invalid filename guard — After secure_filename(), if the result is empty (e.g., filename was all special chars), returns 400 "Invalid filename".
5. fetchone bug fix (pre-existing) — Changed cur.fetchone → cur.fetchone() on line 48 and cur.fetchone()[0] → row[0] on line 52 so the migrations endpoint works correctly.

storage.py:
6. Dead code removal — Removed unused create_upload_url() function (no longer called after server-side upload refactor).

Full Test Results (24 tests)

#	Test	Result	Details
1	storage._get_container_name() default	✅ PASS	Returns "documents"
2	storage._get_container_name() env override	✅ PASS	Respects AZURE_STORAGE_CONTAINER_NAME
3	create_upload_url() removed	✅ PASS	Dead code cleaned up
4	GET /health	✅ PASS	Returns 200 with healthy status
5	GET /document-upload	✅ PASS	Returns upload form with multipart/form-data
6	POST /document-upload (no file)	✅ PASS	Returns 400 "No file part"
7	POST /document-upload (empty filename)	✅ PASS	Returns 400 "No selected file"
8	POST /document-upload (valid file)	✅ PASS	Uploads to uploads/2026-04-15/test.txt
9	GET /migrations (fetchone fix)	✅ PASS	cur.fetchone() called correctly
10	Path traversal (../../../etc/passwd)	✅ PASS	Sanitized to uploads/<date>/etc_passwd
11	Path traversal (URL-encoded)	✅ PASS	Sanitized by secure_filename()
12	XSS via <script> in filename	✅ PASS	Payload escaped in response
13	XSS via event handler filename	✅ PASS	Event handler sanitized
14	MAX_CONTENT_LENGTH configured	✅ PASS	16 MB limit set
15	Oversized file rejected (17MB)	✅ PASS	Returns 413 Request Entity Too Large
16	Dangerous file type (.exe)	⚠️ WARNING	Accepted (no file type allowlist — acceptable for template)
17	Special characters in filename	✅ PASS	Sanitized to file_with_spaces__special.txt
18	Null byte in filename	✅ PASS	Sanitized to file.txt
19	Terraform braces: main.tf	✅ PASS	7 pairs balanced
20	Terraform braces: variables.tf	✅ PASS	6 pairs balanced
21	Terraform braces: outputs.tf	✅ PASS	3 pairs balanced
22	Terraform braces: providers.tf	✅ PASS	3 pairs balanced
23	Terraform braces: storage_access.tf	✅ PASS	1 pair balanced
24	Terraform braces: storage.tf	✅ PASS	6 pairs balanced

Summary: 23 passed, 0 failed, 1 warning

Diff

# app.py changes:
+from markupsafe import escape
+from werkzeug.utils import secure_filename
+app.config["MAX_CONTENT_LENGTH"] = 16 * 1024 * 1024  # 16 MB upload limit
-    row = cur.fetchone
+    row = cur.fetchone()
-        last_migration_date = cur.fetchone()[0]
+        last_migration_date = row[0]
-        path = f"uploads/{datetime.now().date()}/{file.filename}"
+        filename = secure_filename(file.filename)
+        if not filename:
+            return "Invalid filename", 400
+        path = f"uploads/{datetime.now().date()}/{filename}"
-            f"<p>File uploaded successfully to {path}</p>"
+            f"<p>File uploaded successfully to {escape(path)}</p>"

# storage.py changes:
-def create_upload_url(path):
-    """Return the local upload endpoint for server-side upload."""
-    return "/document-upload", {}

Remaining Items (from Testing Status)

• Terratest — needs Azure environment deployment. The test checks /document-upload returns 200 with multipart/form-data (verified locally).
• Manual verification — deploy to dev, upload file via browser, verify blob in Azure Portal at documents/uploads/<date>/<filename>
• Integration test — re-render template in platform-test-azure, run terraform plan + apply on networks + service layers
• CI — 1 failing check: tfsec compliance scan (investigate)

[sean-navapbc]

Remaining Test Status Update

CI — tfsec compliance scan: ✅ RESOLVED

The tfsec failure on commit 1747f92 was not a code issue — it was a transient GitHub API rate limit error:

wget: server returned error: HTTP/1.1 403 rate limit exceeded
wget: bad address ''

The tfsec action tried to download the latest tfsec binary from the GitHub Releases API, hit a 403 rate limit, got an empty URL, and failed. After our two fix commits (3690648 and f0bc66f), the CI re-ran successfully. All 8 checks now pass, including tfsec. The 5 tfsec findings are all in files outside this PR (keyvault modules) and were correctly ignored by the PR commenter.

Terratest: ⏳ Requires Azure environment

The Go test in infra/test/infra_test.go deploys the full Terraform infrastructure, waits for service stability, then verifies:

• /health returns HTTP 200
• /document-upload returns HTTP 200 with multipart/form-data in the response

Cannot be run from this environment — requires an Azure subscription with credentials configured. Should be run in a CI/CD pipeline or from a developer workstation with az login configured.

Manual verification: ⏳ Requires deployed environment

Steps to verify manually:

1. Deploy to dev environment
2. Navigate to /document-upload in browser
3. Upload a test file via the form
4. Verify blob appears in Azure Portal under documents/uploads/<date>/<filename>

Integration test: ⏳ Requires platform-test-azure

Steps:

1. Re-render template in platform-test-azure
2. Run terraform plan + apply on networks + service layers
3. Verify end-to-end upload flow works

Summary

Test	Status	Notes
CI — tfsec	✅ Pass	Was a transient rate limit issue, now passing
CI — checkov	✅ Pass	All checks green
CI — terraform fmt	✅ Pass
CI — terraform validate	✅ Pass
CI — lint scripts	✅ Pass
CI — lint workflows	✅ Pass
Terratest	⏳ Pending	Needs Azure environment
Manual verification	⏳ Pending	Needs deployed dev environment
Integration test	⏳ Pending	Needs platform-test-azure