feat(security): add HSTS header to all REST and MCP HTTP responses by fedorov · Pull Request #21 · ImagingDataCommons/IDC-REST-MCP

fedorov · 2026-07-13T17:54:52Z

Why

Per NCI security policy, all our sites must serve HSTS, and it is handled by the application: Cloud Run terminates TLS but injects no security headers, so the app must add Strict-Transport-Security to every response. This was flagged by the hosting admin.

What

A shared pure-ASGI HSTSMiddleware (src/idc_api/http_headers.py) adds Strict-Transport-Security: max-age=<n>; includeSubDomains to every response of both hosted surfaces — the REST FastAPI app and the MCP streamable-http app — including 404s, redirects, and CORS preflights. It lives beside settings.py because both adapters use it and neither may import the other.
New setting IDC_API_HSTS_MAX_AGE (default 31536000 = 1 year, the policy/prod value; 0 disables).
The deploy workflow sets 3600 on dev/test (so a misconfigured deploy can't lock browsers out of the domain for a year) and the full year on prod; overridable per tier via an HSTS_MAX_AGE GitHub Environment variable.
The post-deploy verify step now curls the live service and fails the deploy if the header is missing.
Tests for the header on REST (success/redirect/404) and on the MCP /mcp endpoint; docs updated (user-guide config table, deployment.md, CHANGELOG under Unreleased).

Notes

The currently deployed services won't send the header until rebuilt/redeployed.
All 71 tests pass; actionlint clean on the workflow change.

🤖 Generated with Claude Code

NCI security policy requires Strict-Transport-Security on every response, and it is the application's job: Cloud Run terminates TLS but injects no security headers. A shared pure-ASGI middleware now adds the header to every response of both hosted surfaces (REST FastAPI app and the MCP streamable-http app). Max-age is configurable via IDC_API_HSTS_MAX_AGE: default one year (the policy/prod value), while the deploy workflow sets 3600 on dev/test so a misconfigured deploy can't lock browsers out of the domain for a year. The deploy verify step now fails if the header is missing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Adds application-level HSTS enforcement so both hosted HTTP surfaces (REST FastAPI and MCP streamable-http) emit Strict-Transport-Security on every response, meeting NCI security policy requirements even when the hosting layer (Cloud Run/LB) does not inject security headers.

Changes:

Introduces a shared pure-ASGI HSTSMiddleware and wires it into both the REST and hosted MCP HTTP apps.
Adds a new IDC_API_HSTS_MAX_AGE setting (default 1 year; 0 disables) and documents the configuration.
Updates deployment workflow to set tier-specific HSTS max-age and adds a deploy-time header verification (currently REST-only).

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/test_rest.py	Adds assertions that REST responses include the HSTS header.
tests/test_mcp.py	Adds assertion that the hosted MCP endpoint includes the HSTS header.
src/idc_api/settings.py	Adds `hsts_max_age` setting (env-driven via `IDC_API_` prefix).
src/idc_api/rest/app.py	Installs HSTS middleware in the REST app middleware stack.
src/idc_api/mcp/server.py	Installs HSTS middleware in the hosted MCP HTTP app.
src/idc_api/http_headers.py	New shared ASGI middleware implementing the HSTS header injection.
docs/user-guide.md	Documents the new `HSTS_MAX_AGE` configuration entry.
dev/deployment.md	Documents HSTS policy rationale and tier-specific max-age behavior.
CHANGELOG.md	Notes the addition of HSTS support under Unreleased.
.github/workflows/deploy.yml	Sets `IDC_API_HSTS_MAX_AGE` per environment and verifies HSTS header (REST).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

fedorov · 2026-07-13T18:10:38Z

+    # Added after CORS so it wraps it (last-added middleware is outermost): the HSTS header
+    # then lands on every response, including CORS preflights that short-circuit here.


Good catch — reworded in b94701c: the comment now says HSTS must stay outside CORS for preflights, and notes explicitly that the audit middleware added below ends up outermost (harmless — it only logs). A preflight test now guards the ordering.

Drafted with Claude Code on behalf of @fedorov.

fedorov · 2026-07-13T18:10:39Z

+    expected = "max-age=31536000; includeSubDomains"
+    assert client.get("/v3/health").headers["strict-transport-security"] == expected
+    r = client.get("/", follow_redirects=False)
+    assert r.headers["strict-transport-security"] == expected
+    assert client.get("/zzz").headers["strict-transport-security"] == expected


Added in b94701c: the test now sends an OPTIONS preflight (Origin + Access-Control-Request-Method) and asserts the HSTS header on the short-circuited response, so moving HSTSMiddleware inside CORSMiddleware fails the suite.

Drafted with Claude Code on behalf of @fedorov.

fedorov · 2026-07-13T18:10:42Z

+          # NCI policy requires HSTS on every response; fail the deploy if the header is gone.
+          curl -sfI "$URL/v3/health" | grep -i '^strict-transport-security:'


Added in b94701c: the MCP verify step now probes HEAD /mcp (answers 405 — not a valid MCP request — but the middleware adds the header to every response) and greps for Strict-Transport-Security, failing the deploy if it's missing, same as the REST check.

Drafted with Claude Code on behalf of @fedorov.

…ordering comment - Clarify the middleware-ordering comment in rest/app.py: HSTS sits outside CORS (required for preflights); the audit middleware ends up outermost, which is harmless. - Assert the HSTS header on a CORS preflight response in test_rest.py, so moving HSTSMiddleware inside CORS fails the suite. - Verify HSTS on the hosted MCP service in the deploy workflow, matching the REST check (HEAD /mcp answers 405 but still carries the header). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

fedorov · 2026-07-13T18:20:19Z

+        async def send_with_hsts(message) -> None:
+            if message["type"] == "http.response.start":
+                message["headers"] = [*message.get("headers", []), self._header]
+            await send(message)


Fixed in c1e9a44: the middleware now filters out any existing Strict-Transport-Security entries (case-insensitive) before adding the configured one — replace, never append alongside. Covered by a new direct middleware test in tests/test_http_headers.py.

Drafted with Claude Code on behalf of @fedorov.

fedorov · 2026-07-13T18:20:21Z

+    # NCI security policy: HSTS on all responses — the app injects it (Cloud Run doesn't), so
+    # it must land on success, redirect, and 404 alike. Default max-age is the prod/policy
+    # value; dev/test deploys override via IDC_API_HSTS_MAX_AGE.
+    expected = "max-age=31536000; includeSubDomains"


Fixed in c1e9a44: the expected value is now derived from get_settings().hsts_max_age, so an environment override of IDC_API_HSTS_MAX_AGE no longer fails the test.

Drafted with Claude Code on behalf of @fedorov.

fedorov · 2026-07-13T18:20:23Z

+            # NCI policy: HSTS on every response of the hosted transport, same as REST.
+            assert r.headers["strict-transport-security"] == "max-age=31536000; includeSubDomains"


Fixed in c1e9a44, same as the REST test: the assertion now computes the expected header from get_settings().hsts_max_age.

Drafted with Claude Code on behalf of @fedorov.

…ived test expectations - HSTSMiddleware now replaces any existing Strict-Transport-Security header instead of appending a duplicate (ambiguous to clients); covered by a new direct middleware test. - REST and MCP tests derive the expected max-age from get_settings() instead of hard-coding the default, so an environment override doesn't fail them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

fedorov · 2026-07-13T18:30:05Z

+    expected = f"max-age={get_settings().hsts_max_age}; includeSubDomains"
+    assert client.get("/v3/health").headers["strict-transport-security"] == expected
+    r = client.get("/", follow_redirects=False)
+    assert r.headers["strict-transport-security"] == expected
+    assert client.get("/zzz").headers["strict-transport-security"] == expected
+    # CORS preflights are answered by CORSMiddleware without calling inward, so this only
+    # passes while HSTSMiddleware stays outside CORS (see the ordering comment in app.py).
+    preflight = client.options(
+        "/v3/sql",
+        headers={"Origin": "https://example.com", "Access-Control-Request-Method": "POST"},
+    )
+    assert preflight.status_code == 200
+    assert preflight.headers["strict-transport-security"] == expected


Fixed in 51c8214: the test now expects the header to be absent when hsts_max_age is 0 (.get() compared against None), matching the documented toggle.

Drafted with Claude Code on behalf of @fedorov.

fedorov · 2026-07-13T18:30:06Z

+            # NCI policy: HSTS on every response of the hosted transport, same as REST. The
+            # expected max-age comes from settings, which the environment may override.
+            expected = f"max-age={get_settings().hsts_max_age}; includeSubDomains"
+            assert r.headers["strict-transport-security"] == expected


Fixed in 51c8214, same as the REST test: expects absence when the toggle is 0.

Drafted with Claude Code on behalf of @fedorov.

fedorov · 2026-07-13T18:30:08Z

+    # NCI security policy, and the application's job since Cloud Run terminates TLS without
+    # injecting the header. Defaults to one year (the prod/policy value); dev and test deploys
+    # set 3600 so a misconfigured deploy can't lock browsers out of the domain for a year.
+    # 0 disables the header entirely.
+    hsts_max_age: int = 31536000


Fixed in 51c8214: hsts_max_age is now Field(default=31536000, ge=0), so a negative value fails at startup instead of silently disabling HSTS.

Drafted with Claude Code on behalf of @fedorov.

fedorov · 2026-07-13T18:30:09Z

+def _run(app, scope):
+    """Drive an ASGI app for one request; return the response-start messages."""


Fixed in 51c8214 — docstring now says it returns every ASGI message.

Drafted with Claude Code on behalf of @fedorov.

…isables toggle in tests - hsts_max_age is now Field(ge=0): a typo like -1 fails at startup instead of silently disabling HSTS in prod. - REST/MCP header tests expect absence (not KeyError) when the environment sets IDC_API_HSTS_MAX_AGE=0. - Fix a stale docstring in the middleware test helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

…agged Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

DavidPotCanuck · 2026-07-13T20:24:35Z

From Andrey, and for our records:

@David Pot (GDIT) last week Suzanne and I iterated on the api/mcp deploy in dev/test, and today it went live in prod: https://api.imaging.datacommons.cancer.gov/mcp and https://api.imaging.datacommons.cancer.gov/v3/docs!

Copilot AI review requested due to automatic review settings July 13, 2026 17:54

Copilot started reviewing on behalf of fedorov July 13, 2026 17:55 View session

style: ruff format for the MCP HSTS assertion

f61e917

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI reviewed Jul 13, 2026

View reviewed changes

fedorov requested a review from Copilot July 13, 2026 18:11

Copilot started reviewing on behalf of fedorov July 13, 2026 18:12 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

fedorov requested a review from Copilot July 13, 2026 18:21

Copilot started reviewing on behalf of fedorov July 13, 2026 18:21 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

fedorov requested a review from Copilot July 13, 2026 18:30

Copilot started reviewing on behalf of fedorov July 13, 2026 18:30 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

docs(changelog): fold the HSTS entry into 3.0.0b1, which is not yet t…

4166735

…agged Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

fedorov merged commit f79a64b into main Jul 13, 2026
4 checks passed

fedorov mentioned this pull request Jul 13, 2026

fix(ci): drop -f from the REST HSTS deploy probe #22

Merged

		# Added after CORS so it wraps it (last-added middleware is outermost): the HSTS header
		# then lands on every response, including CORS preflights that short-circuit here.

		# NCI policy requires HSTS on every response; fail the deploy if the header is gone.
		curl -sfI "$URL/v3/health" \| grep -i '^strict-transport-security:'

		# NCI policy: HSTS on every response of the hosted transport, same as REST.
		assert r.headers["strict-transport-security"] == "max-age=31536000; includeSubDomains"

		def _run(app, scope):
		"""Drive an ASGI app for one request; return the response-start messages."""

Uh oh!

Conversation

fedorov commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

DavidPotCanuck commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fedorov commented Jul 13, 2026 •

edited

Loading